diff options
21 files changed, 164 insertions, 159 deletions
diff --git a/actionpack/lib/action_dispatch/railtie.rb b/actionpack/lib/action_dispatch/railtie.rb index efc3988bc3..5f711c7348 100644 --- a/actionpack/lib/action_dispatch/railtie.rb +++ b/actionpack/lib/action_dispatch/railtie.rb @@ -52,5 +52,11 @@ module ActionDispatch ActionDispatch.test_app = app end + + initializer "action_dispatch.system_tests" do |app| + ActiveSupport.on_load(:action_dispatch_system_test_case) do + include app.routes.url_helpers + end + end end end diff --git a/actionpack/lib/action_dispatch/system_test_case.rb b/actionpack/lib/action_dispatch/system_test_case.rb index 066daa4a12..a7fb5fa330 100644 --- a/actionpack/lib/action_dispatch/system_test_case.rb +++ b/actionpack/lib/action_dispatch/system_test_case.rb @@ -10,7 +10,6 @@ require "action_dispatch/system_testing/browser" require "action_dispatch/system_testing/server" require "action_dispatch/system_testing/test_helpers/screenshot_helper" require "action_dispatch/system_testing/test_helpers/setup_and_teardown" -require "action_dispatch/system_testing/test_helpers/undef_methods" module ActionDispatch # = System Testing @@ -110,12 +109,11 @@ module ActionDispatch # Because <tt>ActionDispatch::SystemTestCase</tt> is a shim between Capybara # and Rails, any driver that is supported by Capybara is supported by system # tests as long as you include the required gems and files. - class SystemTestCase < IntegrationTest + class SystemTestCase < ActiveSupport::TestCase include Capybara::DSL include Capybara::Minitest::Assertions include SystemTesting::TestHelpers::SetupAndTeardown include SystemTesting::TestHelpers::ScreenshotHelper - include SystemTesting::TestHelpers::UndefMethods def initialize(*) # :nodoc: super @@ -160,6 +158,10 @@ module ActionDispatch driven_by :selenium + def url_options # :nodoc: + default_url_options.merge(host: Capybara.app_host) + end + ActiveSupport.run_load_hooks(:action_dispatch_system_test_case, self) end diff --git a/actionpack/lib/action_dispatch/system_testing/test_helpers/setup_and_teardown.rb b/actionpack/lib/action_dispatch/system_testing/test_helpers/setup_and_teardown.rb index 7080dbe022..20f6a7634f 100644 --- a/actionpack/lib/action_dispatch/system_testing/test_helpers/setup_and_teardown.rb +++ b/actionpack/lib/action_dispatch/system_testing/test_helpers/setup_and_teardown.rb @@ -7,7 +7,6 @@ module ActionDispatch DEFAULT_HOST = "http://127.0.0.1" def host!(host) - super Capybara.app_host = host end diff --git a/actionpack/lib/action_dispatch/system_testing/test_helpers/undef_methods.rb b/actionpack/lib/action_dispatch/system_testing/test_helpers/undef_methods.rb deleted file mode 100644 index d64be3b3d9..0000000000 --- a/actionpack/lib/action_dispatch/system_testing/test_helpers/undef_methods.rb +++ /dev/null @@ -1,26 +0,0 @@ -# frozen_string_literal: true - -module ActionDispatch - module SystemTesting - module TestHelpers - module UndefMethods # :nodoc: - extend ActiveSupport::Concern - included do - METHODS = %i(get post put patch delete).freeze - - METHODS.each do |verb| - undef_method verb - end - - def method_missing(method, *args, &block) - if METHODS.include?(method) - raise NoMethodError, "System tests cannot make direct requests via ##{method}; use #visit and #click_on instead. See http://www.rubydoc.info/github/teamcapybara/capybara/master#The_DSL for more information." - else - super - end - end - end - end - end - end -end diff --git a/actionpack/test/dispatch/system_testing/system_test_case_test.rb b/actionpack/test/dispatch/system_testing/system_test_case_test.rb index 847b09dcfe..3319db1665 100644 --- a/actionpack/test/dispatch/system_testing/system_test_case_test.rb +++ b/actionpack/test/dispatch/system_testing/system_test_case_test.rb @@ -46,40 +46,3 @@ class SetHostTest < DrivenByRackTest assert_equal "http://example.com", Capybara.app_host end end - -class UndefMethodsTest < DrivenBySeleniumWithChrome - test "get" do - exception = assert_raise NoMethodError do - get "http://example.com" - end - assert_equal "System tests cannot make direct requests via #get; use #visit and #click_on instead. See http://www.rubydoc.info/github/teamcapybara/capybara/master#The_DSL for more information.", exception.message - end - - test "post" do - exception = assert_raise NoMethodError do - post "http://example.com" - end - assert_equal "System tests cannot make direct requests via #post; use #visit and #click_on instead. See http://www.rubydoc.info/github/teamcapybara/capybara/master#The_DSL for more information.", exception.message - end - - test "put" do - exception = assert_raise NoMethodError do - put "http://example.com" - end - assert_equal "System tests cannot make direct requests via #put; use #visit and #click_on instead. See http://www.rubydoc.info/github/teamcapybara/capybara/master#The_DSL for more information.", exception.message - end - - test "patch" do - exception = assert_raise NoMethodError do - patch "http://example.com" - end - assert_equal "System tests cannot make direct requests via #patch; use #visit and #click_on instead. See http://www.rubydoc.info/github/teamcapybara/capybara/master#The_DSL for more information.", exception.message - end - - test "delete" do - exception = assert_raise NoMethodError do - delete "http://example.com" - end - assert_equal "System tests cannot make direct requests via #delete; use #visit and #click_on instead. See http://www.rubydoc.info/github/teamcapybara/capybara/master#The_DSL for more information.", exception.message - end -end diff --git a/actiontext/app/models/action_text/rich_text.rb b/actiontext/app/models/action_text/rich_text.rb index 19fa3e030e..1a3ffdfa27 100644 --- a/actiontext/app/models/action_text/rich_text.rb +++ b/actiontext/app/models/action_text/rich_text.rb @@ -15,7 +15,7 @@ module ActionText has_many_attached :embeds before_save do - self.embeds = body.attachables.grep(ActiveStorage::Blob) if body.present? + self.embeds = body.attachables.grep(ActiveStorage::Blob).uniq if body.present? end def to_plain_text diff --git a/actiontext/test/unit/model_test.rb b/actiontext/test/unit/model_test.rb index af53f88caa..c2c3ccaaec 100644 --- a/actiontext/test/unit/model_test.rb +++ b/actiontext/test/unit/model_test.rb @@ -44,6 +44,15 @@ class ActionText::ModelTest < ActiveSupport::TestCase assert_equal [ActiveStorage::Attachment], message.content.embeds.map(&:class) end + test "embed extraction deduplicates file attachments" do + blob = create_file_blob(filename: "racecar.jpg", content_type: "image/jpg") + content = ActionText::Content.new("Hello world").append_attachables([ blob, blob ]) + + assert_nothing_raised do + Message.create!(subject: "Greetings", content: content) + end + end + test "saving content" do message = Message.create!(subject: "Greetings", content: "<h1>Hello world</h1>") assert_equal "Hello world", message.content.to_plain_text diff --git a/activerecord/lib/active_record/associations/join_dependency/join_association.rb b/activerecord/lib/active_record/associations/join_dependency/join_association.rb index ca0305abbb..6a7e92dc28 100644 --- a/activerecord/lib/active_record/associations/join_dependency/join_association.rb +++ b/activerecord/lib/active_record/associations/join_dependency/join_association.rb @@ -44,8 +44,7 @@ module ActiveRecord unless others.empty? joins.concat arel.join_sources - right = joins.last.right - right.expr.children.concat(others) + append_constraints(joins.last, others) end # The current table in this iteration becomes the foreign table in the next @@ -65,6 +64,16 @@ module ActiveRecord @readonly = reflection.scope && reflection.scope_for(base_klass.unscoped).readonly_value end + + private + def append_constraints(join, constraints) + if join.is_a?(Arel::Nodes::StringJoin) + join_string = table.create_and(constraints.unshift(join.left)) + join.left = Arel.sql(base_klass.connection.visitor.compile(join_string)) + else + join.right.expr.children.concat(constraints) + end + end end end end diff --git a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb index 68498b5dc5..4ff3cb0071 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb @@ -294,15 +294,33 @@ module ActiveRecord @frequency = frequency end + @@mutex = Mutex.new + @@pools = {} + + def self.register_pool(pool, frequency) # :nodoc: + @@mutex.synchronize do + if @@pools.key?(frequency) + @@pools[frequency] << pool + else + @@pools[frequency] = [pool] + Thread.new(frequency) do |t| + loop do + sleep t + @@mutex.synchronize do + @@pools[frequency].each do |p| + p.reap + p.flush + end + end + end + end + end + end + end + def run return unless frequency && frequency > 0 - Thread.new(frequency, pool) { |t, p| - loop do - sleep t - p.reap - p.flush - end - } + self.class.register_pool(pool, frequency) end end diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb index 49bcb1d010..9ed25ca7c2 100644 --- a/activerecord/test/cases/associations/eager_test.rb +++ b/activerecord/test/cases/associations/eager_test.rb @@ -101,6 +101,17 @@ class EagerAssociationTest < ActiveRecord::TestCase assert_equal [taggings(:normal_comment_rating)], rating.taggings_without_tag end + def test_loading_association_with_string_joins + rating = Rating.first + assert_equal [taggings(:normal_comment_rating)], rating.taggings_with_no_tag + + rating = Rating.preload(:taggings_with_no_tag).first + assert_equal [taggings(:normal_comment_rating)], rating.taggings_with_no_tag + + rating = Rating.eager_load(:taggings_with_no_tag).first + assert_equal [taggings(:normal_comment_rating)], rating.taggings_with_no_tag + end + def test_loading_with_scope_including_joins member = Member.first assert_equal members(:groucho), member diff --git a/activerecord/test/models/rating.rb b/activerecord/test/models/rating.rb index 49aa38285f..2a18ea45ac 100644 --- a/activerecord/test/models/rating.rb +++ b/activerecord/test/models/rating.rb @@ -4,4 +4,5 @@ class Rating < ActiveRecord::Base belongs_to :comment has_many :taggings, as: :taggable has_many :taggings_without_tag, -> { left_joins(:tag).where("tags.id": nil) }, as: :taggable, class_name: "Tagging" + has_many :taggings_with_no_tag, -> { joins("LEFT OUTER JOIN tags ON tags.id = taggings.tag_id").where("tags.id": nil) }, as: :taggable, class_name: "Tagging" end diff --git a/activestorage/CHANGELOG.md b/activestorage/CHANGELOG.md index 957591ec0a..956567e08a 100644 --- a/activestorage/CHANGELOG.md +++ b/activestorage/CHANGELOG.md @@ -1,3 +1,24 @@ +* The S3 service now permits uploading files larger than 5 gigabytes. + When uploading a file greater than 100 megabytes in size, the service + transparently switches to [multipart uploads](https://docs.aws.amazon.com/AmazonS3/latest/dev/mpuoverview.html) + using a part size computed from the file's total size and S3's part count limit. + + No application changes are necessary to take advantage of this feature. You + can customize the default 100 MB multipart upload threshold in your S3 + service's configuration: + + ```yaml + production: + service: s3 + access_key_id: <%= Rails.application.credentials.dig(:aws, :access_key_id) %> + secret_access_key: <%= Rails.application.credentials.dig(:aws, :secret_access_key) %> + region: us-east-1 + bucket: my-bucket + upload: + multipart_threshold: <%= 250.megabytes %> + ``` + + *George Claghorn* Please check [6-0-stable](https://github.com/rails/rails/blob/6-0-stable/activestorage/CHANGELOG.md) for previous changes. diff --git a/activestorage/app/models/active_storage/variant.rb b/activestorage/app/models/active_storage/variant.rb index bc0058967a..1859b8482e 100644 --- a/activestorage/app/models/active_storage/variant.rb +++ b/activestorage/app/models/active_storage/variant.rb @@ -96,19 +96,13 @@ class ActiveStorage::Variant end def process - blob.open do |image| - transform(image) { |output| upload(output) } + blob.open do |input| + variation.transform(input, format: format) do |output| + service.upload(key, output) + end end end - def transform(image, &block) - variation.transform(image, format: format, &block) - end - - def upload(file) - service.upload(key, file) - end - def specification @specification ||= diff --git a/activestorage/lib/active_storage/service/s3_service.rb b/activestorage/lib/active_storage/service/s3_service.rb index c7e4ec96a2..e4bd57048a 100644 --- a/activestorage/lib/active_storage/service/s3_service.rb +++ b/activestorage/lib/active_storage/service/s3_service.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +gem "aws-sdk-s3", "~> 1.14" + require "aws-sdk-s3" require "active_support/core_ext/numeric/bytes" @@ -7,20 +9,24 @@ module ActiveStorage # Wraps the Amazon Simple Storage Service (S3) as an Active Storage service. # See ActiveStorage::Service for the generic API documentation that applies to all services. class Service::S3Service < Service - attr_reader :client, :bucket, :upload_options + attr_reader :client, :bucket + attr_reader :multipart_upload_threshold, :upload_options def initialize(bucket:, upload: {}, **options) @client = Aws::S3::Resource.new(**options) @bucket = @client.bucket(bucket) + @multipart_upload_threshold = upload.fetch(:multipart_threshold, 100.megabytes) @upload_options = upload end def upload(key, io, checksum: nil, content_type: nil, **) instrument :upload, key: key, checksum: checksum do - object_for(key).put(upload_options.merge(body: io, content_md5: checksum, content_type: content_type)) - rescue Aws::S3::Errors::BadDigest - raise ActiveStorage::IntegrityError + if io.size < multipart_upload_threshold + upload_with_single_part key, io, checksum: checksum, content_type: content_type + else + upload_with_multipart key, io, content_type: content_type + end end end @@ -94,6 +100,24 @@ module ActiveStorage end private + MAXIMUM_UPLOAD_PARTS_COUNT = 10000 + MINIMUM_UPLOAD_PART_SIZE = 5.megabytes + + def upload_with_single_part(key, io, checksum: nil, content_type: nil) + object_for(key).put(body: io, content_md5: checksum, content_type: content_type, **upload_options) + rescue Aws::S3::Errors::BadDigest + raise ActiveStorage::IntegrityError + end + + def upload_with_multipart(key, io, content_type: nil) + part_size = [ io.size.fdiv(MAXIMUM_UPLOAD_PARTS_COUNT).ceil, MINIMUM_UPLOAD_PART_SIZE ].max + + object_for(key).upload_stream(content_type: content_type, part_size: part_size, **upload_options) do |out| + IO.copy_stream(io, out) + end + end + + def object_for(key) bucket.object(key) end diff --git a/activestorage/test/controllers/blobs_controller_test.rb b/activestorage/test/controllers/blobs_controller_test.rb index 9bf2641de6..9c811df895 100644 --- a/activestorage/test/controllers/blobs_controller_test.rb +++ b/activestorage/test/controllers/blobs_controller_test.rb @@ -20,28 +20,3 @@ class ActiveStorage::BlobsControllerTest < ActionDispatch::IntegrationTest assert_equal "max-age=300, private", @response.headers["Cache-Control"] end end - -if SERVICE_CONFIGURATIONS[:s3] && SERVICE_CONFIGURATIONS[:s3][:access_key_id].present? - class ActiveStorage::S3BlobsControllerTest < ActionDispatch::IntegrationTest - setup do - @old_service = ActiveStorage::Blob.service - ActiveStorage::Blob.service = ActiveStorage::Service.configure(:s3, SERVICE_CONFIGURATIONS) - end - - teardown do - ActiveStorage::Blob.service = @old_service - end - - test "allow redirection to the different host" do - blob = create_file_blob filename: "racecar.jpg" - - assert_nothing_raised { get rails_blob_url(blob) } - assert_response :redirect - assert_no_match @request.host, @response.headers["Location"] - ensure - blob.purge - end - end -else - puts "Skipping S3 redirection tests because no S3 configuration was supplied" -end diff --git a/activestorage/test/controllers/representations_controller_test.rb b/activestorage/test/controllers/representations_controller_test.rb index 4ae0ff877e..2662cc5283 100644 --- a/activestorage/test/controllers/representations_controller_test.rb +++ b/activestorage/test/controllers/representations_controller_test.rb @@ -59,33 +59,3 @@ class ActiveStorage::RepresentationsControllerWithPreviewsTest < ActionDispatch: assert_response :not_found end end - -if SERVICE_CONFIGURATIONS[:s3] && SERVICE_CONFIGURATIONS[:s3][:access_key_id].present? - class ActiveStorage::S3RepresentationsControllerWithVariantsTest < ActionDispatch::IntegrationTest - setup do - @old_service = ActiveStorage::Blob.service - ActiveStorage::Blob.service = ActiveStorage::Service.configure(:s3, SERVICE_CONFIGURATIONS) - end - - teardown do - ActiveStorage::Blob.service = @old_service - end - - test "allow redirection to the different host" do - blob = create_file_blob filename: "racecar.jpg" - - assert_nothing_raised do - get rails_blob_representation_url( - filename: blob.filename, - signed_blob_id: blob.signed_id, - variation_key: ActiveStorage::Variation.encode(resize: "100x100")) - end - assert_response :redirect - assert_no_match @request.host, @response.headers["Location"] - ensure - blob.purge - end - end -else - puts "Skipping S3 redirection tests because no S3 configuration was supplied" -end diff --git a/activestorage/test/service/s3_service_test.rb b/activestorage/test/service/s3_service_test.rb index 74c0aa0405..b9120770e6 100644 --- a/activestorage/test/service/s3_service_test.rb +++ b/activestorage/test/service/s3_service_test.rb @@ -46,8 +46,7 @@ if SERVICE_CONFIGURATIONS[:s3] end test "uploading with server-side encryption" do - config = SERVICE_CONFIGURATIONS.deep_merge(s3: { upload: { server_side_encryption: "AES256" } }) - service = ActiveStorage::Service.configure(:s3, config) + service = build_service(upload: { server_side_encryption: "AES256" }) begin key = SecureRandom.base58(24) @@ -77,6 +76,25 @@ if SERVICE_CONFIGURATIONS[:s3] ensure @service.delete key end + + test "uploading a large object in multiple parts" do + service = build_service(upload: { multipart_threshold: 5.megabytes }) + + begin + key = SecureRandom.base58(24) + data = SecureRandom.bytes(8.megabytes) + + service.upload key, StringIO.new(data), checksum: Digest::MD5.base64digest(data) + assert data == service.download(key) + ensure + service.delete key + end + end + + private + def build_service(configuration) + ActiveStorage::Service.configure :s3, SERVICE_CONFIGURATIONS.deep_merge(s3: configuration) + end end else puts "Skipping S3 Service tests because no S3 configuration was supplied" diff --git a/guides/source/association_basics.md b/guides/source/association_basics.md index 62e9270539..27387ac3ac 100644 --- a/guides/source/association_basics.md +++ b/guides/source/association_basics.md @@ -2451,8 +2451,8 @@ Extensions can refer to the internals of the association proxy using these three * `proxy_association.reflection` returns the reflection object that describes the association. * `proxy_association.target` returns the associated object for `belongs_to` or `has_one`, or the collection of associated objects for `has_many` or `has_and_belongs_to_many`. -Single Table Inheritance ------------------------- +Single Table Inheritance (STI) +------------------------------ Sometimes, you may want to share fields and behavior between different models. Let's say we have Car, Motorcycle, and Bicycle models. We will want to share diff --git a/guides/source/routing.md b/guides/source/routing.md index e3a6bbb138..4aeb9ee585 100644 --- a/guides/source/routing.md +++ b/guides/source/routing.md @@ -508,7 +508,7 @@ end This will recognize `/photos/1/preview` with GET, and route to the `preview` action of `PhotosController`, with the resource id value passed in `params[:id]`. It will also create the `preview_photo_url` and `preview_photo_path` helpers. -Within the block of member routes, each route name specifies the HTTP verb +Within the block of member routes, each route name specifies the HTTP verb that will be recognized. You can use `get`, `patch`, `put`, `post`, or `delete` here . If you don't have multiple `member` routes, you can also pass `:on` to a route, eliminating the block: diff --git a/railties/lib/rails/application/dummy_erb_compiler.rb b/railties/lib/rails/application/dummy_erb_compiler.rb index 086b9e76f4..028e790292 100644 --- a/railties/lib/rails/application/dummy_erb_compiler.rb +++ b/railties/lib/rails/application/dummy_erb_compiler.rb @@ -11,14 +11,8 @@ end class DummyCompiler < ERB::Compiler # :nodoc: def compile_content(stag, out) - case stag - when "<%=" - content = out.instance_variable_get(:@compiler).instance_variable_get(:@content) - if content.include?("?") && content.include?(":") - out.push "_erbout << 'dummy_key: dummy_value'" - else - out.push "_erbout << 'dummy_value'" - end + if stag == "<%=" + out.push "_erbout << ''" end end end diff --git a/railties/test/application/rake/dbs_test.rb b/railties/test/application/rake/dbs_test.rb index ca2d45b1c9..e08cd09abd 100644 --- a/railties/test/application/rake/dbs_test.rb +++ b/railties/test/application/rake/dbs_test.rb @@ -138,6 +138,23 @@ module ApplicationTests db_create_and_drop("db/development.sqlite3", environment_loaded: false) end + test "db:create and db:drop don't raise errors when loading YAML which contains a key's value as an ERB statement" do + app_file "config/database.yml", <<-YAML + development: + database: <%= Rails.application.config.database ? 'db/development.sqlite3' : 'db/development.sqlite3' %> + custom_option: <%= ENV['CUSTOM_OPTION'] %> + adapter: sqlite3 + YAML + + app_file "config/environments/development.rb", <<-RUBY + Rails.application.configure do + config.database = "db/development.sqlite3" + end + RUBY + + db_create_and_drop("db/development.sqlite3", environment_loaded: false) + end + def with_database_existing Dir.chdir(app_path) do set_database_url |