From a9012af6883284a35bc5e6166507d429fe10482d Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Tue, 28 Jul 2020 11:57:36 +0200 Subject: [PATCH 01/40] Automatically set Link header for each stylesheet and script elements[0] can be serialized in `Link` headers[1] to allow the browser to preload them before it parsed the HTML body. It is particularly useful for scripts included at the bottom of the document. [0] https://developer.mozilla.org/en-US/docs/Web/HTML/Preloading_content [1] https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Link --- .../action_view/helpers/asset_tag_helper.rb | 32 ++++++++++------ actionview/test/abstract_unit.rb | 2 +- .../test/template/asset_tag_helper_test.rb | 38 ++++++++++++++----- 3 files changed, 50 insertions(+), 22 deletions(-) diff --git a/actionview/lib/action_view/helpers/asset_tag_helper.rb b/actionview/lib/action_view/helpers/asset_tag_helper.rb index 91794fe4e4..cbf64867aa 100644 --- a/actionview/lib/action_view/helpers/asset_tag_helper.rb +++ b/actionview/lib/action_view/helpers/asset_tag_helper.rb @@ -86,11 +86,11 @@ module ActionView def javascript_include_tag(*sources) options = sources.extract_options!.stringify_keys path_options = options.extract!("protocol", "extname", "host", "skip_pipeline").symbolize_keys - early_hints_links = [] + preload_links = [] sources_tags = sources.uniq.map { |source| href = path_to_javascript(source, path_options) - early_hints_links << "<#{href}>; rel=preload; as=script" + preload_links << "<#{href}>; rel=preload; as=script" tag_options = { "src" => href }.merge!(options) @@ -100,7 +100,7 @@ module ActionView content_tag("script", "", tag_options) }.join("\n").html_safe - request.send_early_hints("Link" => early_hints_links.join("\n")) if respond_to?(:request) && request + send_preload_links_header(preload_links) sources_tags end @@ -136,11 +136,11 @@ module ActionView def stylesheet_link_tag(*sources) options = sources.extract_options!.stringify_keys path_options = options.extract!("protocol", "host", "skip_pipeline").symbolize_keys - early_hints_links = [] + preload_links = [] sources_tags = sources.uniq.map { |source| href = path_to_stylesheet(source, path_options) - early_hints_links << "<#{href}>; rel=preload; as=style" + preload_links << "<#{href}>; rel=preload; as=style" tag_options = { "rel" => "stylesheet", "media" => "screen", @@ -149,7 +149,7 @@ module ActionView tag(:link, tag_options) }.join("\n").html_safe - request.send_early_hints("Link" => early_hints_links.join("\n")) if respond_to?(:request) && request + send_preload_links_header(preload_links) sources_tags end @@ -281,12 +281,12 @@ module ActionView crossorigin: crossorigin }.merge!(options.symbolize_keys)) - early_hints_link = "<#{href}>; rel=preload; as=#{as_type}" - early_hints_link += "; type=#{mime_type}" if mime_type - early_hints_link += "; crossorigin=#{crossorigin}" if crossorigin - early_hints_link += "; nopush" if nopush + preload_link = "<#{href}>; rel=preload; as=#{as_type}" + preload_link += "; type=#{mime_type}" if mime_type + preload_link += "; crossorigin=#{crossorigin}" if crossorigin + preload_link += "; nopush" if nopush - request.send_early_hints("Link" => early_hints_link) if respond_to?(:request) && request + send_preload_links_header([preload_link]) link_tag end @@ -482,6 +482,16 @@ module ActionView type end end + + def send_preload_links_header(preload_links) + if respond_to?(:request) && request + request.send_early_hints("Link" => preload_links.join("\n")) + end + + if respond_to?(:response) && response + response.headers["Link"] = [response.headers["Link"].presence, *preload_links].compact.join(",") + end + end end end end diff --git a/actionview/test/abstract_unit.rb b/actionview/test/abstract_unit.rb index ddd34da7bd..c7e3f0bc02 100644 --- a/actionview/test/abstract_unit.rb +++ b/actionview/test/abstract_unit.rb @@ -84,7 +84,7 @@ class RoutedRackApp end class BasicController - attr_accessor :request + attr_accessor :request, :response def config @config ||= ActiveSupport::InheritableOptions.new(ActionController::Base.config).tap do |config| diff --git a/actionview/test/template/asset_tag_helper_test.rb b/actionview/test/template/asset_tag_helper_test.rb index efef74ca00..a59929a2ea 100644 --- a/actionview/test/template/asset_tag_helper_test.rb +++ b/actionview/test/template/asset_tag_helper_test.rb @@ -9,23 +9,33 @@ ActionView::Template::Types.delegate_to Mime class AssetTagHelperTest < ActionView::TestCase tests ActionView::Helpers::AssetTagHelper - attr_reader :request + attr_reader :request, :response + + class FakeRequest + attr_accessor :script_name + def protocol() "http://" end + def ssl?() false end + def host_with_port() "localhost" end + def base_url() "http://www.example.com" end + def send_early_hints(links) end + end + + class FakeResponse + def headers + @headers ||= {} + end + end def setup super @controller = BasicController.new - @request = Class.new do - attr_accessor :script_name - def protocol() "http://" end - def ssl?() false end - def host_with_port() "localhost" end - def base_url() "http://www.example.com" end - def send_early_hints(links) end - end.new - + @request = FakeRequest.new @controller.request = @request + + @response = FakeResponse.new + @controller.response = @response end def url_for(*args) @@ -499,6 +509,14 @@ class AssetTagHelperTest < ActionView::TestCase assert_dom_equal %(), javascript_include_tag("foo.js") end + + def test_should_set_preload_links + stylesheet_link_tag("http://example.com/style.css") + javascript_include_tag("http://example.com/all.js") + expected = "; rel=preload; as=style,; rel=preload; as=script" + assert_equal expected, @response.headers["Link"] + end + def test_image_path ImagePathToTag.each { |method, tag| assert_dom_equal(tag, eval(method)) } end From 531f978319b85b9bdb5dba449afc9b08d9be9a85 Mon Sep 17 00:00:00 2001 From: lanzhiheng Date: Mon, 20 Jul 2020 19:01:58 +0800 Subject: [PATCH 02/40] [ci skip] Append some description about advance constraints in block form. --- guides/source/routing.md | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/guides/source/routing.md b/guides/source/routing.md index 93847f1736..00ec2ed845 100644 --- a/guides/source/routing.md +++ b/guides/source/routing.md @@ -751,6 +751,35 @@ end Both the `matches?` method and the lambda gets the `request` object as an argument. +#### Constraints in a block form + +You can specify constraints in a block form. This is useful for when you need to apply the same rule to several routes. For example + +``` +class RestrictedListConstraint + # ...Same as the example above +end + +Rails.application.routes.draw do + constraints(RestrictedListConstraint.new) do + get '*path', to: 'restricted_list#index', + get '*other-path', to: 'other_restricted_list#index', + end +end +``` + +You also use a `lambda`: + +``` +Rails.application.routes.draw do + constraints(lambda { |request| RestrictedList.retrieve_ips.include?(request.remote_ip) }) do + get '*path', to: 'restricted_list#index', + get '*other-path', to: 'other_restricted_list#index', + end +end +``` + + ### Route Globbing and Wildcard Segments Route globbing is a way to specify that a particular parameter should be matched to all the remaining parts of a route. For example: From 3eb48a2148ddf7abadc25112d344691f9fcddf26 Mon Sep 17 00:00:00 2001 From: Victor Perez Rodriguez Date: Wed, 29 Jul 2020 16:02:32 -0500 Subject: [PATCH 03/40] fix misleading variant test This commit fixes the "resized variation of BMP blob" test. By default `create_file_blob` use "image/jpeg" as content type, since this test does not specify the correct `content_type` for a `*.bmp` image ("image/bmp") the `ActiveStorage::Variant#specification` consider the blob as a web image which causes the variant to return a `*.bmp` URL and a "BMP" type, this is an incorrect behavior since if you upload a `*.bmp` image the variant will have a PNG format with "image/png" as content type. After this change the test will cover the current activestorage behavior. Changes: * Specify correct `content_type` on "resized variation of BMP blob" test. * Change asserts to cover the current activestorage behavior. --- activestorage/test/models/variant_test.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/activestorage/test/models/variant_test.rb b/activestorage/test/models/variant_test.rb index 63c74f48ea..c4779fdf8d 100644 --- a/activestorage/test/models/variant_test.rb +++ b/activestorage/test/models/variant_test.rb @@ -162,12 +162,12 @@ class ActiveStorage::VariantTest < ActiveSupport::TestCase end test "resized variation of BMP blob" do - blob = create_file_blob(filename: "colors.bmp") + blob = create_file_blob(filename: "colors.bmp", content_type: "image/bmp") variant = blob.variant(resize: "15x15").processed - assert_match(/colors\.bmp/, variant.url) + assert_match(/colors\.png/, variant.url) image = read_image(variant) - assert_equal "BMP", image.type + assert_equal "PNG", image.type assert_equal 15, image.width assert_equal 8, image.height end From ff881137a8ceab951211a66afa2389ae599b2ce7 Mon Sep 17 00:00:00 2001 From: Jonathan Hefner Date: Sat, 1 Aug 2020 11:25:21 -0500 Subject: [PATCH 04/40] Add baseline defaults section [ci skip] Mentioning the baseline default for a config option can be confusing when that default is overridden by `config.load_defaults`. To avoid that confusion, this commit relocates such baseline defaults from their explanatory paragraphs to a "Baseline defaults" section that flows with the other `config.load_defaults` sections. Ideally, when we override other baseline defaults in the future, we will relocate mention of them as done here. Closes #39387. --- guides/source/configuring.md | 57 +++++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 21 deletions(-) diff --git a/guides/source/configuring.md b/guides/source/configuring.md index d1fd64ae83..572a65c380 100644 --- a/guides/source/configuring.md +++ b/guides/source/configuring.md @@ -161,7 +161,7 @@ defaults to `:debug` for all environments. The available log levels are: `:debug * `config.time_zone` sets the default time zone for the application and enables time zone awareness for Active Record. -* `config.autoloader` sets the autoloading mode. This option defaults to `:zeitwerk` if `6.0` is specified in `config.load_defaults`. Applications can still use the classic autoloader by setting this value to `:classic` after loading the framework defaults: +* `config.autoloader` sets the autoloading mode. This option defaults to `:zeitwerk` when `config.load_defaults` is called with `6.0` or greater. Applications can still use the classic autoloader by setting this value to `:classic` after loading the framework defaults: ```ruby config.load_defaults 6.0 @@ -263,7 +263,7 @@ Every Rails application comes with a standard set of middleware which it uses in # `beta1.product.com`. Rails.application.config.hosts << /.*\.product\.com/ ``` - + The provided regexp will be wrapped with both anchors (`\A` and `\z`) so it must match the entire hostname. `/product.com/`, for example, once anchored, would fail to match `www.product.com`. @@ -460,10 +460,9 @@ in controllers and views. This defaults to `false`. to be reused when the object being cached of type `ActiveRecord::Relation` changes by moving the volatile information (max updated at and count) of the relation's cache key into the cache version to support recycling cache key. - Defaults to `false`. * `config.active_record.has_many_inversing` enables setting the inverse record - when traversing `belongs_to` to `has_many` associations. Defaults to `false`. + when traversing `belongs_to` to `has_many` associations. The MySQL adapter adds one additional configuration option: @@ -509,9 +508,9 @@ The schema dumper adds two additional configuration options: * `config.action_controller.per_form_csrf_tokens` configures whether CSRF tokens are only valid for the method/action they were generated for. -* `config.action_controller.default_protect_from_forgery` determines whether forgery protection is added on `ActionController::Base`. This is false by default. +* `config.action_controller.default_protect_from_forgery` determines whether forgery protection is added on `ActionController::Base`. -* `config.action_controller.urlsafe_csrf_tokens` configures whether generated CSRF tokens are URL-safe. Defaults to `false`. +* `config.action_controller.urlsafe_csrf_tokens` configures whether generated CSRF tokens are URL-safe. * `config.action_controller.relative_url_root` can be used to tell Rails that you are [deploying to a subdirectory](configuring.html#deploy-to-a-subdirectory-relative-url-root). The default is `ENV['RAILS_RELATIVE_URL_ROOT']`. @@ -629,8 +628,8 @@ Defaults to `'signed cookie'`. header without modification. Defaults to `false`. * `config.action_dispatch.cookies_same_site_protection` configures the default - value of the `SameSite` attribute when setting cookies. Defaults to `nil`, - which means the `SameSite` attribute is not added. + value of the `SameSite` attribute when setting cookies. When set to `nil`, the + `SameSite` attribute is not added. * `config.action_dispatch.ssl_default_redirect_status` configures the default HTTP status code used when redirecting non-GET/HEAD requests from HTTP to HTTPS @@ -688,7 +687,7 @@ Defaults to `'signed cookie'`. * `config.action_view.form_with_generates_remote_forms` determines whether `form_with` generates remote forms or not. This defaults to `true`. -* `config.action_view.form_with_generates_ids` determines whether `form_with` generates ids on inputs. This defaults to `false`. +* `config.action_view.form_with_generates_ids` determines whether `form_with` generates ids on inputs. * `config.action_view.default_enforce_utf8` determines whether forms are generated with a hidden tag that forces older versions of Internet Explorer to submit forms encoded in UTF-8. This defaults to `false`. @@ -795,7 +794,7 @@ There are a number of settings available on `config.action_mailer`: * `config.action_mailer.perform_caching` specifies whether the mailer templates should perform fragment caching or not. If it's not specified, the default will be `true`. -* `config.action_mailer.delivery_job` specifies delivery job for mail. Defaults to `ActionMailer::DeliveryJob`. +* `config.action_mailer.delivery_job` specifies delivery job for mail. ### Configuring Active Support @@ -812,9 +811,9 @@ There are a few configuration options available in Active Support: * `config.active_support.time_precision` sets the precision of JSON encoded time values. Defaults to `3`. -* `config.active_support.use_sha1_digests` specifies whether to use SHA-1 instead of MD5 to generate non-sensitive digests, such as the ETag header. Defaults to false. +* `config.active_support.use_sha1_digests` specifies whether to use SHA-1 instead of MD5 to generate non-sensitive digests, such as the ETag header. -* `config.active_support.use_authenticated_message_encryption` specifies whether to use AES-256-GCM authenticated encryption as the default cipher for encrypting messages instead of AES-256-CBC. This is false by default. +* `config.active_support.use_authenticated_message_encryption` specifies whether to use AES-256-GCM authenticated encryption as the default cipher for encrypting messages instead of AES-256-CBC. * `ActiveSupport::Logger.silencer` is set to `false` to disable the ability to silence logging in a block. The default is `true`. @@ -832,7 +831,7 @@ There are a few configuration options available in Active Support: * `ActiveSupport.utc_to_local_returns_utc_offset_times` configures `ActiveSupport::TimeZone.utc_to_local` to return a time with a UTC offset - instead of a UTC time incorporating that offset. Defaults to `false`. + instead of a UTC time incorporating that offset. ### Configuring Active Job @@ -889,15 +888,15 @@ There are a few configuration options available in Active Support: * `config.active_job.custom_serializers` allows to set custom argument serializers. Defaults to `[]`. -* `config.active_job.return_false_on_aborted_enqueue` change the return value of `#enqueue` to false instead of the job instance when the enqueuing is aborted. Defaults to `false`. +* `config.active_job.return_false_on_aborted_enqueue` change the return value of `#enqueue` to false instead of the job instance when the enqueuing is aborted. * `config.active_job.log_arguments` controls if the arguments of a job are logged. Defaults to `true`. -* `config.active_job.retry_jitter` controls the amount of "jitter" (random variation) applied to the delay time calculated when retrying failed jobs. Defaults to `0.0`. +* `config.active_job.retry_jitter` controls the amount of "jitter" (random variation) applied to the delay time calculated when retrying failed jobs. * `config.active_job.skip_after_callbacks_if_terminated` controls whether `after_enqueue` / `after_perform` callbacks run when a `before_enqueue` / - `before_perform` callback halts with `throw :abort`. Defaults to `false`. + `before_perform` callback halts with `throw :abort`. ### Configuring Action Cable @@ -999,7 +998,7 @@ text/javascript image/svg+xml application/postscript application/x-shockwave-fla `config.load_defaults` sets new defaults up to and including the version passed. Such that passing, say, '6.0' also gets the new defaults from every version before it. -#### For '6.1', new defaults from previous versions below and: +#### For '6.1', defaults from previous versions below and: - `config.active_record.has_many_inversing`: `true` - `config.active_storage.track_variants`: `true` @@ -1010,7 +1009,7 @@ text/javascript image/svg+xml application/postscript application/x-shockwave-fla - `ActiveSupport.utc_to_local_returns_utc_offset_times`: `true` - `config.action_controller.urlsafe_csrf_tokens`: `true` -#### For '6.0', new defaults from previous versions below and: +#### For '6.0', defaults from previous versions below and: - `config.autoloader`: `:zeitwerk` - `config.action_view.default_enforce_utf8`: `false` @@ -1023,7 +1022,7 @@ text/javascript image/svg+xml application/postscript application/x-shockwave-fla - `config.active_storage.replace_on_assign_to_many`: `true` - `config.active_record.collection_cache_versioning`: `true` -#### For '5.2', new defaults from previous versions below and: +#### For '5.2', defaults from previous versions below and: - `config.active_record.cache_versioning`: `true` - `config.action_dispatch.use_authenticated_cookie_encryption`: `true` @@ -1032,12 +1031,12 @@ text/javascript image/svg+xml application/postscript application/x-shockwave-fla - `config.action_controller.default_protect_from_forgery`: `true` - `config.action_view.form_with_generates_ids`: `true` -#### For '5.1', new defaults from previous versions below and: +#### For '5.1', defaults from previous versions below and: - `config.assets.unknown_asset_fallback`: `false` - `config.action_view.form_with_generates_remote_forms`: `true` -#### For '5.0': +#### For '5.0', baseline defaults from below and: - `config.action_controller.per_form_csrf_tokens`: `true` - `config.action_controller.forgery_protection_origin_check`: `true` @@ -1045,6 +1044,22 @@ text/javascript image/svg+xml application/postscript application/x-shockwave-fla - `config.active_record.belongs_to_required_by_default`: `true` - `config.ssl_options`: `{ hsts: { subdomains: true } }` +#### Baseline defaults: + +- `config.action_controller.default_protect_from_forgery`: `false` +- `config.action_controller.urlsafe_csrf_tokens`: `false` +- `config.action_dispatch.cookies_same_site_protection`: `nil` +- `config.action_mailer.delivery_job`: `ActionMailer::DeliveryJob` +- `config.action_view.form_with_generates_ids`: `false` +- `config.active_job.retry_jitter`: `0.0` +- `config.active_job.return_false_on_aborted_enqueue`: `false` +- `config.active_job.skip_after_callbacks_if_terminated`: `false` +- `config.active_record.collection_cache_versioning`: `false` +- `config.active_record.has_many_inversing`: `false` +- `config.active_support.use_authenticated_message_encryption`: `false` +- `config.active_support.use_sha1_digests`: `false` +- `ActiveSupport.utc_to_local_returns_utc_offset_times`: `false` + ### Configuring a Database Just about every Rails application will interact with a database. You can connect to the database by setting an environment variable `ENV['DATABASE_URL']` or by using a configuration file called `config/database.yml`. From b39ea7cc3662345ad8efbe496db5ae594449f85f Mon Sep 17 00:00:00 2001 From: Eugene Kenny Date: Mon, 3 Aug 2020 21:51:07 +0100 Subject: [PATCH 05/40] Add tiff and bmp to content_types_allowed_inline [ci skip] These formats were added in c329d323fc2cc9ba1c486932556afd8fbd9333cc and bcf370d689673031073ba2ac5588afe41cc315c9, respectively. --- guides/source/configuring.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/guides/source/configuring.md b/guides/source/configuring.md index d1fd64ae83..5ce9e67015 100644 --- a/guides/source/configuring.md +++ b/guides/source/configuring.md @@ -940,7 +940,7 @@ You can find more detailed configuration options in the * `config.active_storage.content_types_to_serve_as_binary` accepts an array of strings indicating the content types that Active Storage will always serve as an attachment, rather than inline. The default is `%w(text/html text/javascript image/svg+xml application/postscript application/x-shockwave-flash text/xml application/xml application/xhtml+xml application/mathml+xml text/cache-manifest)`. -* `config.active_storage.content_types_allowed_inline` accepts an array of strings indicating the content types that Active Storage allows to serve as inline. The default is `%w(image/png image/gif image/jpg image/jpeg image/vnd.adobe.photoshop image/vnd.microsoft.icon application/pdf)`. +* `config.active_storage.content_types_allowed_inline` accepts an array of strings indicating the content types that Active Storage allows to serve as inline. The default is `%w(image/png image/gif image/jpg image/jpeg image/tiff image/bmp image/vnd.adobe.photoshop image/vnd.microsoft.icon application/pdf)`. * `config.active_storage.queues.analysis` accepts a symbol indicating the Active Job queue to use for analysis jobs. When this option is `nil`, analysis jobs are sent to the default Active Job queue (see `config.active_job.default_queue_name`). From 45add344781b3e5305f85ec10cd4056934cb99aa Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Tue, 30 Jun 2020 16:26:47 +0800 Subject: [PATCH 06/40] Move advisory locks to own connection handler. Removes the use of `ActiveRecord::AdvisoryLockBase` since it inherits from `ActiveRecord::Base` and hence share module attributes that are defined in `ActiveRecord::Base`. This is problematic because establishing connections through `ActiveRecord::AdvisoryLockBase` can end up changing state of the default connection handler of `ActiveRecord::Base` leading to unexpected behaviors in a Rails application. In the case of https://github.com/rails/rails/issues/39157, Running migrations with `rails db:migrate:primary_shard_one` was not working as the application itself defined the following ``` class ApplicationRecord < ActiveRecord::Base self.abstract_class = true connects_to shards: { default: { writing: :primary }, shard_one: { writing: :primary_shard_one } } end ``` In the database migrate rake task, the default connection was established with the database config of `primary_shard_one`. However, the default connection was altered to that of `primary` because `ActiveRecord::AdvisoryLockBase.establish_connection` ended up loading `ApplicationRecord` which calls `connects_to shards:`. Since all we really need here is just a normal database connection, we can avoid accidentally altering the default connection handler state during the migration by creating a custom connection handler used for retrieving a connection. --- activerecord/lib/active_record.rb | 1 - .../lib/active_record/advisory_lock_base.rb | 18 ----------- activerecord/lib/active_record/migration.rb | 31 ++++++++++++------- activerecord/test/cases/migration_test.rb | 6 ++-- 4 files changed, 24 insertions(+), 32 deletions(-) delete mode 100644 activerecord/lib/active_record/advisory_lock_base.rb diff --git a/activerecord/lib/active_record.rb b/activerecord/lib/active_record.rb index e9a7623f40..4d524623a3 100644 --- a/activerecord/lib/active_record.rb +++ b/activerecord/lib/active_record.rb @@ -36,7 +36,6 @@ require "active_record/errors" module ActiveRecord extend ActiveSupport::Autoload - autoload :AdvisoryLockBase autoload :Base autoload :Callbacks autoload :Core diff --git a/activerecord/lib/active_record/advisory_lock_base.rb b/activerecord/lib/active_record/advisory_lock_base.rb deleted file mode 100644 index 7110a1f132..0000000000 --- a/activerecord/lib/active_record/advisory_lock_base.rb +++ /dev/null @@ -1,18 +0,0 @@ -# frozen_string_literal: true - -module ActiveRecord - # This class is used to create a connection that we can use for advisory - # locks. This will take out a "global" lock that can't be accidentally - # removed if a new connection is established during a migration. - class AdvisoryLockBase < ActiveRecord::Base # :nodoc: - self.abstract_class = true - - self.connection_specification_name = "AdvisoryLockBase" - - class << self - def _internal? - true - end - end - end -end diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index 88e8c583bf..56c7495226 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -1376,20 +1376,29 @@ module ActiveRecord def with_advisory_lock lock_id = generate_migrator_advisory_lock_id - AdvisoryLockBase.establish_connection(ActiveRecord::Base.connection_db_config) unless AdvisoryLockBase.connected? - connection = AdvisoryLockBase.connection - got_lock = connection.get_advisory_lock(lock_id) - raise ConcurrentMigrationError unless got_lock - load_migrated # reload schema_migrations to be sure it wasn't changed by another process before we got the lock - yield - ensure - if got_lock && !connection.release_advisory_lock(lock_id) - raise ConcurrentMigrationError.new( - ConcurrentMigrationError::RELEASE_LOCK_FAILED_MESSAGE - ) + + with_advisory_lock_connection do |connection| + got_lock = connection.get_advisory_lock(lock_id) + raise ConcurrentMigrationError unless got_lock + load_migrated # reload schema_migrations to be sure it wasn't changed by another process before we got the lock + yield + ensure + if got_lock && !connection.release_advisory_lock(lock_id) + raise ConcurrentMigrationError.new( + ConcurrentMigrationError::RELEASE_LOCK_FAILED_MESSAGE + ) + end end end + def with_advisory_lock_connection + pool = ActiveRecord::ConnectionAdapters::ConnectionHandler.new.establish_connection( + ActiveRecord::Base.connection_db_config + ) + + pool.with_connection { |connection| yield(connection) } + end + MIGRATOR_SALT = 2053462845 def generate_migrator_advisory_lock_id db_name_hash = Zlib.crc32(Base.connection.current_database) diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb index 0d65dd7f9c..8946e0afb9 100644 --- a/activerecord/test/cases/migration_test.rb +++ b/activerecord/test/cases/migration_test.rb @@ -940,8 +940,10 @@ class MigrationTest < ActiveRecord::TestCase e = assert_raises(ActiveRecord::ConcurrentMigrationError) do silence_stream($stderr) do - migrator.send(:with_advisory_lock) do - ActiveRecord::AdvisoryLockBase.connection.release_advisory_lock(lock_id) + migrator.stub(:with_advisory_lock_connection, ->(&block) { block.call(ActiveRecord::Base.connection) }) do + migrator.send(:with_advisory_lock) do + ActiveRecord::Base.connection.release_advisory_lock(lock_id) + end end end end From 1f2d0120c03345d0a880115b33fdfb72d883a7e2 Mon Sep 17 00:00:00 2001 From: piecehealth Date: Tue, 4 Aug 2020 10:47:24 +0800 Subject: [PATCH 07/40] --skip-action-mailer and --skip-active_job option doesn't work for `rails plugin new project_name --full` or `rails plugin new project_name --mountable` --- .../rails/plugin/plugin_generator.rb | 6 +++++- .../test/generators/plugin_generator_test.rb | 21 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/railties/lib/rails/generators/rails/plugin/plugin_generator.rb b/railties/lib/rails/generators/rails/plugin/plugin_generator.rb index 2445752689..cb747a67ab 100644 --- a/railties/lib/rails/generators/rails/plugin/plugin_generator.rb +++ b/railties/lib/rails/generators/rails/plugin/plugin_generator.rb @@ -24,10 +24,14 @@ module Rails directory "app" empty_directory_with_keep_file "app/assets/images/#{namespaced_name}" end + + remove_dir "app/mailers" if options[:skip_action_mailer] + remove_dir "app/jobs" if options[:skip_active_job] elsif full? empty_directory_with_keep_file "app/models" empty_directory_with_keep_file "app/controllers" - empty_directory_with_keep_file "app/mailers" + empty_directory_with_keep_file "app/mailers" unless options[:skip_action_mailer] + empty_directory_with_keep_file "app/jobs" unless options[:skip_active_job] unless api? empty_directory_with_keep_file "app/assets/images/#{namespaced_name}" diff --git a/railties/test/generators/plugin_generator_test.rb b/railties/test/generators/plugin_generator_test.rb index 1e8092666a..632dc3c6a6 100644 --- a/railties/test/generators/plugin_generator_test.rb +++ b/railties/test/generators/plugin_generator_test.rb @@ -226,6 +226,24 @@ class PluginGeneratorTest < Rails::Generators::TestCase end end + def test_skip_action_mailer_and_skip_active_job_with_mountable + run_generator [destination_root, "--mountable", "--skip-action-mailer", "--skip-active-job"] + assert_no_directory "app/mailers" + assert_no_directory "app/jobs" + end + + def test_skip_action_mailer_and_skip_active_job_with_api_and_mountable + run_generator [destination_root, "--api", "--mountable", "--skip-action-mailer", "--skip-active-job"] + assert_no_directory "app/mailers" + assert_no_directory "app/jobs" + end + + def test_skip_action_mailer_and_skip_active_job_with_full + run_generator [destination_root, "--full", "--skip-action-mailer", "--skip-active-job"] + assert_no_directory "app/mailers" + assert_no_directory "app/jobs" + end + def test_template_from_dir_pwd FileUtils.cd(Rails.root) assert_match(/It works from file!/, run_generator([destination_root, "-m", "lib/template.rb"])) @@ -262,6 +280,7 @@ class PluginGeneratorTest < Rails::Generators::TestCase assert_file "app/views" assert_file "app/helpers" assert_file "app/mailers" + assert_file "app/jobs" assert_file "bin/rails", /\s+require\s+["']rails\/all["']/ assert_file "config/routes.rb", /Rails.application.routes.draw do/ assert_file "lib/bukkits/engine.rb", /module Bukkits\n class Engine < ::Rails::Engine\n end\nend/ @@ -278,6 +297,7 @@ class PluginGeneratorTest < Rails::Generators::TestCase assert_file "hyphenated-name/app/views" assert_file "hyphenated-name/app/helpers" assert_file "hyphenated-name/app/mailers" + assert_file "hyphenated-name/app/jobs" assert_file "hyphenated-name/bin/rails" assert_file "hyphenated-name/config/routes.rb", /Rails.application.routes.draw do/ assert_file "hyphenated-name/lib/hyphenated/name/engine.rb", /module Hyphenated\n module Name\n class Engine < ::Rails::Engine\n end\n end\nend/ @@ -295,6 +315,7 @@ class PluginGeneratorTest < Rails::Generators::TestCase assert_file "my_hyphenated-name/app/views" assert_file "my_hyphenated-name/app/helpers" assert_file "my_hyphenated-name/app/mailers" + assert_file "my_hyphenated-name/app/jobs" assert_file "my_hyphenated-name/bin/rails" assert_file "my_hyphenated-name/config/routes.rb", /Rails\.application\.routes\.draw do/ assert_file "my_hyphenated-name/lib/my_hyphenated/name/engine.rb", /module MyHyphenated\n module Name\n class Engine < ::Rails::Engine\n end\n end\nend/ From eb5f5ed0be0c8bb1d136135c8ad6a22d4c5b7bd2 Mon Sep 17 00:00:00 2001 From: Lawrence Chou Date: Tue, 4 Aug 2020 12:26:27 +0800 Subject: [PATCH 08/40] Fix attaching blobs via nested attributes Closes #37411. --- .../lib/active_storage/attached/changes/create_one.rb | 2 +- activestorage/test/models/attached/one_test.rb | 7 +++++++ activestorage/test/test_helper.rb | 2 ++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/activestorage/lib/active_storage/attached/changes/create_one.rb b/activestorage/lib/active_storage/attached/changes/create_one.rb index b61d3b078c..942b493798 100644 --- a/activestorage/lib/active_storage/attached/changes/create_one.rb +++ b/activestorage/lib/active_storage/attached/changes/create_one.rb @@ -65,7 +65,7 @@ module ActiveStorage **attachable.reverse_merge( record: record, service_name: attachment_service_name - ) + ).symbolize_keys ) when String ActiveStorage::Blob.find_signed!(attachable, record: record) diff --git a/activestorage/test/models/attached/one_test.rb b/activestorage/test/models/attached/one_test.rb index c03102fcb1..dbdf547995 100644 --- a/activestorage/test/models/attached/one_test.rb +++ b/activestorage/test/models/attached/one_test.rb @@ -319,6 +319,13 @@ class ActiveStorage::OneAttachedTest < ActiveSupport::TestCase assert_equal 2736, @user.avatar.metadata[:height] end + test "creating an attachment as part of an autosave association through nested attributes" do + group = Group.create!(users_attributes: [{ name: "John", avatar: { io: StringIO.new("STUFF"), filename: "town.jpg", content_type: "image/jpg" } }]) + group.save! + new_user = User.find_by(name: "John") + assert new_user.avatar.attached? + end + test "updating an attachment as part of an autosave association" do group = Group.create!(users: [@user]) @user.avatar = fixture_file_upload("racecar.jpg") diff --git a/activestorage/test/test_helper.rb b/activestorage/test/test_helper.rb index 58dc21950b..c83f47b571 100644 --- a/activestorage/test/test_helper.rb +++ b/activestorage/test/test_helper.rb @@ -127,6 +127,8 @@ end class Group < ActiveRecord::Base has_one_attached :avatar has_many :users, autosave: true + + accepts_nested_attributes_for :users end require_relative "../../tools/test_common" From 7f11ffbcfb80af607db3bc6f39e85884d80ff3be Mon Sep 17 00:00:00 2001 From: Michael Hagar Date: Tue, 14 Jul 2020 14:11:34 -0500 Subject: [PATCH 09/40] Fix noun-verb agreement in ASt guide --- guides/source/active_storage_overview.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/guides/source/active_storage_overview.md b/guides/source/active_storage_overview.md index 0b630f9901..7e4ffaf1c0 100644 --- a/guides/source/active_storage_overview.md +++ b/guides/source/active_storage_overview.md @@ -500,7 +500,7 @@ message.video.open do |file| end ``` -It's important to know that the file are not yet available in the `after_create` callback but in the `after_create_commit` only. +It's important to know that the file is not yet available in the `after_create` callback but in the `after_create_commit` only. Analyzing Files --------------- From 631925454eec203254dc571545fd9e541803a4bd Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Wed, 5 Aug 2020 14:16:53 +0200 Subject: [PATCH 10/40] Get rid of the unused RuntimeRegistry.connection_handler It's no longer used since https://github.com/rails/rails/commit/5ce3e022ef136324d288fb493e0938e76a74981a --- activerecord/lib/active_record/runtime_registry.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/activerecord/lib/active_record/runtime_registry.rb b/activerecord/lib/active_record/runtime_registry.rb index 4975cb8967..7363d2d7a0 100644 --- a/activerecord/lib/active_record/runtime_registry.rb +++ b/activerecord/lib/active_record/runtime_registry.rb @@ -14,9 +14,9 @@ module ActiveRecord class RuntimeRegistry # :nodoc: extend ActiveSupport::PerThreadRegistry - attr_accessor :connection_handler, :sql_runtime + attr_accessor :sql_runtime - [:connection_handler, :sql_runtime].each do |val| + [:sql_runtime].each do |val| class_eval %{ def self.#{val}; instance.#{val}; end }, __FILE__, __LINE__ class_eval %{ def self.#{val}=(x); instance.#{val}=x; end }, __FILE__, __LINE__ end From 0f4de21b57a32c551172f1563c9d89450f2c52d8 Mon Sep 17 00:00:00 2001 From: assain Date: Thu, 6 Aug 2020 20:40:49 +0530 Subject: [PATCH 11/40] Update GitHub gist api link and fix documentation in api_app page [ci skip] The GitHub gist API page is out of date. This commit replaces it with the new link. Also, removed unnecessary commas, added missing fullstop and fixed a ruby snippet which wasn't rendered correctly before. --- guides/source/api_app.md | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/guides/source/api_app.md b/guides/source/api_app.md index 19ba7c5dbd..10573b64c2 100644 --- a/guides/source/api_app.md +++ b/guides/source/api_app.md @@ -93,7 +93,7 @@ Handled at the Action Pack layer: means not having to spend time thinking about how to model your API in terms of HTTP. - URL Generation: The flip side of routing is URL generation. A good API based - on HTTP includes URLs (see [the GitHub Gist API](https://developer.github.com/v3/gists/) + on HTTP includes URLs (see [the GitHub Gist API](https://docs.github.com/en/rest/reference/gists) for an example). - Header and Redirection Responses: `head :no_content` and `redirect_to user_url(current_user)` come in handy. Sure, you could manually @@ -350,8 +350,12 @@ Instead of the initializer, you'll have to set the relevant options somewhere be built (like `config/application.rb`) and pass them to your preferred middleware, like this: ```ruby -config.session_store :cookie_store, key: '_interslice_session' # <-- this also configures session_options for use below -config.middleware.use ActionDispatch::Cookies # Required for all session management (regardless of session_store) +# This also configures session_options for use below +config.session_store :cookie_store, key: '_interslice_session' + +# Required for all session management (regardless of session_store) +config.middleware.use ActionDispatch::Cookies + config.middleware.use config.session_store, config.session_options ``` @@ -405,7 +409,7 @@ controller modules by default: more information regarding this). - `ActionController::ParamsWrapper`: Wraps the parameters hash into a nested hash, so that you don't have to specify root elements sending POST requests for instance. -- `ActionController::Head`: Support for returning a response with no content, only headers +- `ActionController::Head`: Support for returning a response with no content, only headers. Other plugins may add additional modules. You can get a list of all modules included into `ActionController::API` in the rails console: @@ -433,21 +437,22 @@ Some common modules you might want to add: - `AbstractController::Translation`: Support for the `l` and `t` localization and translation methods. - Support for basic, digest, or token HTTP authentication: - * `ActionController::HttpAuthentication::Basic::ControllerMethods`, - * `ActionController::HttpAuthentication::Digest::ControllerMethods`, + * `ActionController::HttpAuthentication::Basic::ControllerMethods` + * `ActionController::HttpAuthentication::Digest::ControllerMethods` * `ActionController::HttpAuthentication::Token::ControllerMethods` - `ActionView::Layouts`: Support for layouts when rendering. - `ActionController::MimeResponds`: Support for `respond_to`. - `ActionController::Cookies`: Support for `cookies`, which includes support for signed and encrypted cookies. This requires the cookies middleware. -- `ActionController::Caching`: Support view caching for the API controller. Please notice that - you will need to manually specify cache store inside the controller like: - ```ruby - class ApplicationController < ActionController::API - include ::ActionController::Caching - self.cache_store = :mem_cache_store - end - ``` +- `ActionController::Caching`: Support view caching for the API controller. Please note + that you will need to manually specify the cache store inside the controller like this: + + ```ruby + class ApplicationController < ActionController::API + include ::ActionController::Caching + self.cache_store = :mem_cache_store + end + ``` Rails does *not* pass this configuration automatically. The best place to add a module is in your `ApplicationController`, but you can From 80f59eadb497b8a7a8b86f6377e2fc325c682679 Mon Sep 17 00:00:00 2001 From: Alexey Savin Date: Thu, 6 Aug 2020 21:22:20 +0300 Subject: [PATCH 12/40] Make `Enumerable.pluck` faster for single key --- activesupport/lib/active_support/core_ext/enumerable.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/activesupport/lib/active_support/core_ext/enumerable.rb b/activesupport/lib/active_support/core_ext/enumerable.rb index 6ba68ababa..97c918a71f 100644 --- a/activesupport/lib/active_support/core_ext/enumerable.rb +++ b/activesupport/lib/active_support/core_ext/enumerable.rb @@ -153,7 +153,8 @@ module Enumerable if keys.many? map { |element| keys.map { |key| element[key] } } else - map { |element| element[keys.first] } + key = keys.first + map { |element| element[key] } end end From e8cf45dc4a42818da7094b91547bab7c9efe37c3 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Fri, 7 Aug 2020 03:57:34 +0900 Subject: [PATCH 13/40] Don't use arel factory methods for creating join nodes Directly `klass.new` is clear enough than factory methods. --- .../lib/active_record/associations/association_scope.rb | 2 +- .../associations/join_dependency/join_association.rb | 4 ++-- activerecord/lib/active_record/relation/query_methods.rb | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/activerecord/lib/active_record/associations/association_scope.rb b/activerecord/lib/active_record/associations/association_scope.rb index b0eaec66a1..8f2c6f269c 100644 --- a/activerecord/lib/active_record/associations/association_scope.rb +++ b/activerecord/lib/active_record/associations/association_scope.rb @@ -52,7 +52,7 @@ module ActiveRecord attr_reader :value_transformation def join(table, constraint) - table.create_join(table, table.create_on(constraint), Arel::Nodes::LeadingJoin) + Arel::Nodes::LeadingJoin.new(table, Arel::Nodes::On.new(constraint)) end def last_chain_scope(scope, reflection, owner) 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 1f1e1ba6b5..98b18e53b6 100644 --- a/activerecord/lib/active_record/associations/join_dependency/join_association.rb +++ b/activerecord/lib/active_record/associations/join_dependency/join_association.rb @@ -50,7 +50,7 @@ module ActiveRecord end end - joins << table.create_join(table, table.create_on(nodes), join_type) + joins << join_type.new(table, Arel::Nodes::On.new(nodes)) if others && !others.empty? joins.concat arel.join_sources @@ -79,7 +79,7 @@ module ActiveRecord private def append_constraints(join, constraints) if join.is_a?(Arel::Nodes::StringJoin) - join_string = table.create_and(constraints.unshift(join.left)) + join_string = Arel::Nodes::And.new(constraints.unshift join.left) join.left = Arel.sql(base_klass.connection.visitor.compile(join_string)) else right = join.right diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index 005707f94a..bd1f438e57 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -1252,7 +1252,7 @@ module ActiveRecord end joins.each_with_index do |join, i| - joins[i] = table.create_string_join(Arel.sql(join.strip)) if join.is_a?(String) + joins[i] = Arel::Nodes::StringJoin.new(Arel.sql(join.strip)) if join.is_a?(String) end while joins.first.is_a?(Arel::Nodes::Join) From afeb756828853d7df59f776df9ae1f09227bfca1 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Fri, 7 Aug 2020 04:33:21 +0900 Subject: [PATCH 14/40] Fix deserializing enum mapping nil Follow up to #38086. User assigned nil is type casted by #38086, but loaded nil from database does not yet, this fixes that. --- activerecord/lib/active_record/enum.rb | 1 - activerecord/test/cases/enum_test.rb | 4 ++++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/activerecord/lib/active_record/enum.rb b/activerecord/lib/active_record/enum.rb index f7aebc5ecb..c243bcc44d 100644 --- a/activerecord/lib/active_record/enum.rb +++ b/activerecord/lib/active_record/enum.rb @@ -136,7 +136,6 @@ module ActiveRecord end def deserialize(value) - return if value.nil? mapping.key(subtype.deserialize(value)) end diff --git a/activerecord/test/cases/enum_test.rb b/activerecord/test/cases/enum_test.rb index 27ec1a5a62..394ca511a4 100644 --- a/activerecord/test/cases/enum_test.rb +++ b/activerecord/test/cases/enum_test.rb @@ -236,6 +236,10 @@ class EnumTest < ActiveRecord::TestCase assert_nil @book.reload.status end + test "deserialize nil value to enum which defines nil value to hash" do + assert_equal "forgotten", books(:ddd).last_read + end + test "assign nil value" do @book.status = nil assert_nil @book.status From 10b36e81a357f8d7fa3665630c4d41c057fe59d9 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Tue, 26 May 2020 01:29:10 +0900 Subject: [PATCH 15/40] Fix incorrect result when eager loading with duplicated through association with join scope I had found the issue while working on fixing #33525. That is if duplicated association has a scope which has `where` with explicit table name condition (e.g. `where("categories.name": "General")`), that condition in all duplicated associations will filter the first one only, other all duplicated associations are not filtered, since duplicated joins will be aliased except the first one (e.g. `INNER JOIN "categories" "categories_categorizations"`). ```ruby class Author < ActiveRecord::Base has_many :general_categorizations, -> { joins(:category).where("categories.name": "General") }, class_name: "Categorization" has_many :general_posts, through: :general_categorizations, source: :post end authors = Author.eager_load(:general_categorizations, :general_posts).to_a ``` Generated eager loading query: ```sql SELECT "authors"."id" AS t0_r0, ... FROM "authors" -- `has_many :general_categorizations, -> { joins(:category).where("categories.name": "General") }` LEFT OUTER JOIN "categorizations" ON "categorizations"."author_id" = "authors"."id" INNER JOIN "categories" ON "categories"."id" = "categorizations"."category_id" AND "categories"."name" = ? -- `has_many :general_posts, through: :general_categorizations, source: :post` ---- duplicated `through: :general_categorizations` part LEFT OUTER JOIN "categorizations" "general_categorizations_authors_join" ON "general_categorizations_authors_join"."author_id" = "authors"."id" INNER JOIN "categories" "categories_categorizations" ON "categories_categorizations"."id" = "general_categorizations_authors_join"."category_id" AND "categories"."name" = ? -- <-- filtering `"categories"."name" = ?` won't work ---- `source: :post` part LEFT OUTER JOIN "posts" ON "posts"."id" = "general_categorizations_authors_join"."post_id" ``` Originally eager loading with join scope didn't work before Rails 5.2 (#29413), and duplicated through association with join scope raised a duplicated alias error before alias tracking is improved in 590b045. But now it will potentially be got incorrect result instead of an error, it is worse than an error. To fix the issue, it makes eager loading to deduplicate / re-use duplicated through association if possible, like as `preload`. ```sql SELECT "authors"."id" AS t0_r0, ... FROM "authors" -- `has_many :general_categorizations, -> { joins(:category).where("categories.name": "General") }` LEFT OUTER JOIN "categorizations" ON "categorizations"."author_id" = "authors"."id" INNER JOIN "categories" ON "categories"."id" = "categorizations"."category_id" AND "categories"."name" = ? -- `has_many :general_posts, through: :general_categorizations, source: :post` ---- `through: :general_categorizations` part is deduplicated / re-used LEFT OUTER JOIN "posts" ON "posts"."id" = "categorizations"."post_id" ``` Fixes #32819. --- .../associations/join_dependency.rb | 10 +++++++++- .../join_dependency/join_association.rb | 18 ++++++++++++++---- .../has_many_through_associations_test.rb | 8 ++++++++ activerecord/test/models/author.rb | 3 +++ 4 files changed, 34 insertions(+), 5 deletions(-) diff --git a/activerecord/lib/active_record/associations/join_dependency.rb b/activerecord/lib/active_record/associations/join_dependency.rb index eb3fb0b74f..459ed193be 100644 --- a/activerecord/lib/active_record/associations/join_dependency.rb +++ b/activerecord/lib/active_record/associations/join_dependency.rb @@ -80,6 +80,7 @@ module ActiveRecord def join_constraints(joins_to_add, alias_tracker) @alias_tracker = alias_tracker + @joined_tables = {} joins = make_join_constraints(join_root, join_type) @@ -173,9 +174,16 @@ module ActiveRecord foreign_table = parent.table foreign_klass = parent.base_klass child.join_constraints(foreign_table, foreign_klass, join_type, alias_tracker) do |reflection| - alias_tracker.aliased_table_for(reflection.klass.arel_table) do + table = @joined_tables[reflection] + + next table, true if table && reflection != child.reflection + + table = alias_tracker.aliased_table_for(reflection.klass.arel_table) do table_alias_for(reflection, parent, reflection != child.reflection) end + + @joined_tables[reflection] ||= table if join_type == Arel::Nodes::OuterJoin + table end.concat child.children.flat_map { |c| make_constraints(child, c, join_type) } end 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 98b18e53b6..74817ec7fa 100644 --- a/activerecord/lib/active_record/associations/join_dependency/join_association.rb +++ b/activerecord/lib/active_record/associations/join_dependency/join_association.rb @@ -23,13 +23,23 @@ module ActiveRecord def join_constraints(foreign_table, foreign_klass, join_type, alias_tracker, &block) joins = [] - tables = reflection.chain.map(&block) - @table = tables.first + chain = [] + + reflection.chain.each do |reflection| + table, terminated = yield reflection + + if terminated + foreign_table, foreign_klass = table, reflection.klass + break + end + + @table ||= table + chain << [reflection, table] + end # The chain starts with the target table, but we want to end with it here (makes # more sense in this context), so we reverse - reflection.chain.reverse_each.with_index(1) do |reflection, i| - table = tables[-i] + chain.reverse_each do |reflection, table| klass = reflection.klass join_scope = reflection.join_scope(table, foreign_table, foreign_klass) diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb index 284075fb5c..10f4a7c8a3 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -1069,6 +1069,14 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase assert_equal expected, Author.eager_load(:lazy_readers_skimmers_or_not_2).last.lazy_readers_skimmers_or_not_2 end + def test_duplicated_has_many_through_with_join_scope + Categorization.create!(author: authors(:david), post: posts(:thinking), category: categories(:technology)) + + expected = [posts(:welcome)] + assert_equal expected, Author.preload(:general_categorizations, :general_posts).first.general_posts + assert_equal expected, Author.eager_load(:general_categorizations, :general_posts).first.general_posts + end + def test_has_many_through_polymorphic_with_rewhere post = TaggedPost.create!(title: "Tagged", body: "Post") tag = post.tags.create!(name: "Tag") diff --git a/activerecord/test/models/author.rb b/activerecord/test/models/author.rb index 373f6d2af6..a19890a0a5 100644 --- a/activerecord/test/models/author.rb +++ b/activerecord/test/models/author.rb @@ -93,6 +93,9 @@ class Author < ActiveRecord::Base has_many :special_categories, through: :special_categorizations, source: :category has_one :special_category, through: :special_categorizations, source: :category + has_many :general_categorizations, -> { joins(:category).where("categories.name": "General") }, class_name: "Categorization" + has_many :general_posts, through: :general_categorizations, source: :post + has_many :special_categories_with_conditions, -> { where(categorizations: { special: true }) }, through: :categorizations, source: :category has_many :nonspecial_categories_with_conditions, -> { where(categorizations: { special: false }) }, through: :categorizations, source: :category From 9289232bc96d692973b4fb6df5b5457df721344e Mon Sep 17 00:00:00 2001 From: eileencodes Date: Fri, 7 Aug 2020 13:36:03 -0400 Subject: [PATCH 16/40] Remove unnecessary with_temporary_connection_pool calls While debugging a different problem I'm working on I realized that this method `with_temporary_connection_pool` isn't necessary in most of the cases we're using it for. Anywhere we establish new connections inside the block won't throw away those new connections. I also removed this from places that can use the existing connection and don't need a new temporary pool. I'm not sure if this file was using it in many places because of copy / paste or real issues that are no longer present. --- activerecord/test/cases/query_cache_test.rb | 112 +++++++++----------- 1 file changed, 51 insertions(+), 61 deletions(-) diff --git a/activerecord/test/cases/query_cache_test.rb b/activerecord/test/cases/query_cache_test.rb index 4d5e0adafe..d4934d289e 100644 --- a/activerecord/test/cases/query_cache_test.rb +++ b/activerecord/test/cases/query_cache_test.rb @@ -445,17 +445,15 @@ class QueryCacheTest < ActiveRecord::TestCase def test_cache_is_available_when_using_a_not_connected_connection skip "In-Memory DB can't test for using a not connected connection" if in_memory_db? - with_temporary_connection_pool do - db_config = ActiveRecord::Base.configurations.configs_for(env_name: "arunit", name: "primary").dup - db_config.owner_name = "test2" - ActiveRecord::Base.connection_handler.establish_connection(db_config) - assert_not_predicate Task, :connected? + db_config = ActiveRecord::Base.configurations.configs_for(env_name: "arunit", name: "primary").dup + db_config.owner_name = "test2" + ActiveRecord::Base.connection_handler.establish_connection(db_config) + assert_not_predicate Task, :connected? - Task.cache do - assert_queries(1) { Task.find(1); Task.find(1) } - ensure - ActiveRecord::Base.connection_handler.remove_connection_pool(db_config.owner_name) - end + Task.cache do + assert_queries(1) { Task.find(1); Task.find(1) } + ensure + ActiveRecord::Base.connection_handler.remove_connection_pool(db_config.owner_name) end end @@ -532,44 +530,38 @@ class QueryCacheTest < ActiveRecord::TestCase end def test_query_cache_does_not_establish_connection_if_unconnected - with_temporary_connection_pool do - ActiveRecord::Base.clear_active_connections! - assert_not ActiveRecord::Base.connection_handler.active_connections? # sanity check + ActiveRecord::Base.clear_active_connections! + assert_not ActiveRecord::Base.connection_handler.active_connections? # sanity check - middleware { - assert_not ActiveRecord::Base.connection_handler.active_connections?, "QueryCache forced ActiveRecord::Base to establish a connection in setup" - }.call({}) + middleware { + assert_not ActiveRecord::Base.connection_handler.active_connections?, "QueryCache forced ActiveRecord::Base to establish a connection in setup" + }.call({}) - assert_not ActiveRecord::Base.connection_handler.active_connections?, "QueryCache forced ActiveRecord::Base to establish a connection in cleanup" - end + assert_not ActiveRecord::Base.connection_handler.active_connections?, "QueryCache forced ActiveRecord::Base to establish a connection in cleanup" end def test_query_cache_is_enabled_on_connections_established_after_middleware_runs - with_temporary_connection_pool do - ActiveRecord::Base.clear_active_connections! - assert_not ActiveRecord::Base.connection_handler.active_connections? # sanity check + ActiveRecord::Base.clear_active_connections! + assert_not ActiveRecord::Base.connection_handler.active_connections? # sanity check - middleware { - assert_predicate ActiveRecord::Base.connection, :query_cache_enabled - }.call({}) - assert_not_predicate ActiveRecord::Base.connection, :query_cache_enabled - end + middleware { + assert_predicate ActiveRecord::Base.connection, :query_cache_enabled + }.call({}) + assert_not_predicate ActiveRecord::Base.connection, :query_cache_enabled end def test_query_caching_is_local_to_the_current_thread - with_temporary_connection_pool do - ActiveRecord::Base.clear_active_connections! + ActiveRecord::Base.clear_active_connections! - middleware { - assert ActiveRecord::Base.connection_pool.query_cache_enabled - assert ActiveRecord::Base.connection.query_cache_enabled + middleware { + assert ActiveRecord::Base.connection_pool.query_cache_enabled + assert ActiveRecord::Base.connection.query_cache_enabled - Thread.new { - assert_not ActiveRecord::Base.connection_pool.query_cache_enabled - assert_not ActiveRecord::Base.connection.query_cache_enabled - }.join - }.call({}) - end + Thread.new { + assert_not ActiveRecord::Base.connection_pool.query_cache_enabled + assert_not ActiveRecord::Base.connection.query_cache_enabled + }.join + }.call({}) end def test_query_cache_is_enabled_on_all_connection_pools @@ -583,39 +575,37 @@ class QueryCacheTest < ActiveRecord::TestCase def test_clear_query_cache_is_called_on_all_connections skip "with in memory db, reading role won't be able to see database on writing role" if in_memory_db? - with_temporary_connection_pool do - ActiveRecord::Base.connection_handlers = { - writing: ActiveRecord::Base.default_connection_handler, - reading: ActiveRecord::ConnectionAdapters::ConnectionHandler.new - } + ActiveRecord::Base.connection_handlers = { + writing: ActiveRecord::Base.default_connection_handler, + reading: ActiveRecord::ConnectionAdapters::ConnectionHandler.new + } + ActiveRecord::Base.connected_to(role: :reading) do + db_config = ActiveRecord::Base.configurations.configs_for(env_name: "arunit", name: "primary") + ActiveRecord::Base.establish_connection(db_config) + end + + mw = middleware { |env| ActiveRecord::Base.connected_to(role: :reading) do - db_config = ActiveRecord::Base.configurations.configs_for(env_name: "arunit", name: "primary") - ActiveRecord::Base.establish_connection(db_config) + @topic = Topic.first end - mw = middleware { |env| - ActiveRecord::Base.connected_to(role: :reading) do - @topic = Topic.first - end + assert @topic - assert @topic + ActiveRecord::Base.connected_to(role: :writing) do + @topic.title = "It doesn't have to be crazy at work" + @topic.save! + end - ActiveRecord::Base.connected_to(role: :writing) do - @topic.title = "It doesn't have to be crazy at work" - @topic.save! - end + assert_equal "It doesn't have to be crazy at work", @topic.title + ActiveRecord::Base.connected_to(role: :reading) do + @topic = Topic.first assert_equal "It doesn't have to be crazy at work", @topic.title + end + } - ActiveRecord::Base.connected_to(role: :reading) do - @topic = Topic.first - assert_equal "It doesn't have to be crazy at work", @topic.title - end - } - - mw.call({}) - end + mw.call({}) ensure ActiveRecord::Base.connection_handlers = { writing: ActiveRecord::Base.default_connection_handler } end From 85ef219621def512954dbb71f0c720f24e2d0862 Mon Sep 17 00:00:00 2001 From: eileencodes Date: Fri, 7 Aug 2020 13:43:18 -0400 Subject: [PATCH 17/40] Rename `pool_key` to `shard` This change renames the following: * `current_pool_key` -> `current_shard` * `default_pool_key` -> `default_shard` * `pool_key` -> `shard` Originally we had intended to name the `shard` as `pool_key` because when we implemented the internal private API we weren't sure how it was going to be used for sharding and wanted to implement behavior without promising a public API. Now that we have a public API for sharding it's better to use the same name everywhere rather than have one name for private APIs and one name for public APIs. This should make contributions and tracking down bugs easier in the future. This PR doesn't require any deprecations because the sharding API is unreleased and so is all the internal code that was using `pool_key`. Co-authored-by: John Crepezzi --- .../abstract/connection_pool.rb | 30 ++++++++--------- .../lib/active_record/connection_handling.rb | 32 +++++++++---------- activerecord/lib/active_record/core.rb | 12 +++---- 3 files changed, 37 insertions(+), 37 deletions(-) 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 f86b7dadfc..6e884a769a 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb @@ -1031,7 +1031,7 @@ module ActiveRecord end alias :connection_pools :connection_pool_list - def establish_connection(config, pool_key = Base.default_pool_key, owner_name = Base.name) + def establish_connection(config, shard = Base.default_shard, owner_name = Base.name) owner_name = config.to_s if config.is_a?(Symbol) pool_config = resolve_pool_config(config, owner_name) @@ -1040,7 +1040,7 @@ module ActiveRecord # Protects the connection named `ActiveRecord::Base` from being removed # if the user calls `establish_connection :primary`. if owner_to_pool_manager.key?(pool_config.connection_specification_name) - remove_connection_pool(pool_config.connection_specification_name, pool_key) + remove_connection_pool(pool_config.connection_specification_name, shard) end message_bus = ActiveSupport::Notifications.instrumenter @@ -1052,7 +1052,7 @@ module ActiveRecord owner_to_pool_manager[pool_config.connection_specification_name] ||= PoolManager.new pool_manager = get_pool_manager(pool_config.connection_specification_name) - pool_manager.set_pool_config(pool_key, pool_config) + pool_manager.set_pool_config(shard, pool_config) message_bus.instrument("!connection.active_record", payload) do pool_config.pool @@ -1094,12 +1094,12 @@ module ActiveRecord # active or defined connection: if it is the latter, it will be # opened and set as the active connection for the class it was defined # for (not necessarily the current class). - def retrieve_connection(spec_name, pool_key = ActiveRecord::Base.default_pool_key) # :nodoc: - pool = retrieve_connection_pool(spec_name, pool_key) + def retrieve_connection(spec_name, shard = ActiveRecord::Base.default_shard) # :nodoc: + pool = retrieve_connection_pool(spec_name, shard) unless pool - if pool_key != ActiveRecord::Base.default_pool_key - message = "No connection pool for '#{spec_name}' found for the '#{pool_key}' shard." + if shard != ActiveRecord::Base.default_shard + message = "No connection pool for '#{spec_name}' found for the '#{shard}' shard." elsif ActiveRecord::Base.connection_handler != ActiveRecord::Base.default_connection_handler message = "No connection pool for '#{spec_name}' found for the '#{ActiveRecord::Base.current_role}' role." else @@ -1114,8 +1114,8 @@ module ActiveRecord # Returns true if a connection that's accessible to this class has # already been opened. - def connected?(spec_name, pool_key = ActiveRecord::Base.default_pool_key) - pool = retrieve_connection_pool(spec_name, pool_key) + def connected?(spec_name, shard = ActiveRecord::Base.default_shard) + pool = retrieve_connection_pool(spec_name, shard) pool && pool.connected? end @@ -1123,14 +1123,14 @@ module ActiveRecord # connection and the defined connection (if they exist). The result # can be used as an argument for #establish_connection, for easily # re-establishing the connection. - def remove_connection(owner, pool_key = ActiveRecord::Base.default_pool_key) - remove_connection_pool(owner, pool_key)&.configuration_hash + def remove_connection(owner, shard = ActiveRecord::Base.default_shard) + remove_connection_pool(owner, shard)&.configuration_hash end deprecate remove_connection: "Use #remove_connection_pool, which now returns a DatabaseConfig object instead of a Hash" - def remove_connection_pool(owner, pool_key = ActiveRecord::Base.default_pool_key) + def remove_connection_pool(owner, shard = ActiveRecord::Base.default_shard) if pool_manager = get_pool_manager(owner) - pool_config = pool_manager.remove_pool_config(pool_key) + pool_config = pool_manager.remove_pool_config(shard) if pool_config pool_config.disconnect! @@ -1142,8 +1142,8 @@ module ActiveRecord # Retrieving the connection pool happens a lot, so we cache it in @owner_to_pool_manager. # This makes retrieving the connection pool O(1) once the process is warm. # When a connection is established or removed, we invalidate the cache. - def retrieve_connection_pool(owner, pool_key = ActiveRecord::Base.default_pool_key) - pool_config = get_pool_manager(owner)&.get_pool_config(pool_key) + def retrieve_connection_pool(owner, shard = ActiveRecord::Base.default_shard) + pool_config = get_pool_manager(owner)&.get_pool_config(shard) pool_config&.pool end diff --git a/activerecord/lib/active_record/connection_handling.rb b/activerecord/lib/active_record/connection_handling.rb index 19094b6ccd..ffae5f6610 100644 --- a/activerecord/lib/active_record/connection_handling.rb +++ b/activerecord/lib/active_record/connection_handling.rb @@ -49,7 +49,7 @@ module ActiveRecord def establish_connection(config_or_env = nil) config_or_env ||= DEFAULT_ENV.call.to_sym db_config, owner_name = resolve_config_for_connection(config_or_env) - connection_handler.establish_connection(db_config, current_pool_key, owner_name) + connection_handler.establish_connection(db_config, current_shard, owner_name) end # Connects a model to the databases specified. The +database+ keyword @@ -89,15 +89,15 @@ module ActiveRecord db_config, owner_name = resolve_config_for_connection(database_key) handler = lookup_connection_handler(role.to_sym) - connections << handler.establish_connection(db_config, default_pool_key, owner_name) + connections << handler.establish_connection(db_config, default_shard, owner_name) end - shards.each do |pool_key, database_keys| + shards.each do |shard, database_keys| database_keys.each do |role, database_key| db_config, owner_name = resolve_config_for_connection(database_key) handler = lookup_connection_handler(role.to_sym) - connections << handler.establish_connection(db_config, pool_key.to_sym, owner_name) + connections << handler.establish_connection(db_config, shard.to_sym, owner_name) end end @@ -154,7 +154,7 @@ module ActiveRecord db_config, owner_name = resolve_config_for_connection(database) handler = lookup_connection_handler(role) - handler.establish_connection(db_config, default_pool_key, owner_name) + handler.establish_connection(db_config, default_shard, owner_name) with_handler(role, &blk) elsif shard @@ -172,8 +172,8 @@ module ActiveRecord # ActiveRecord::Base.connected_to?(role: :writing) #=> true # ActiveRecord::Base.connected_to?(role: :reading) #=> false # end - def connected_to?(role:, shard: ActiveRecord::Base.default_pool_key) - current_role == role.to_sym && current_pool_key == shard.to_sym + def connected_to?(role:, shard: ActiveRecord::Base.default_shard) + current_role == role.to_sym && current_shard == shard.to_sym end # Returns the symbol representing the current connected role. @@ -247,16 +247,16 @@ module ActiveRecord end def connection_pool - connection_handler.retrieve_connection_pool(connection_specification_name, current_pool_key) || raise(ConnectionNotEstablished) + connection_handler.retrieve_connection_pool(connection_specification_name, current_shard) || raise(ConnectionNotEstablished) end def retrieve_connection - connection_handler.retrieve_connection(connection_specification_name, current_pool_key) + connection_handler.retrieve_connection(connection_specification_name, current_shard) end # Returns +true+ if Active Record is connected. def connected? - connection_handler.connected?(connection_specification_name, current_pool_key) + connection_handler.connected?(connection_specification_name, current_shard) end def remove_connection(name = nil) @@ -264,11 +264,11 @@ module ActiveRecord # if removing a connection that has a pool, we reset the # connection_specification_name so it will use the parent # pool. - if connection_handler.retrieve_connection_pool(name, current_pool_key) + if connection_handler.retrieve_connection_pool(name, current_shard) self.connection_specification_name = nil end - connection_handler.remove_connection_pool(name, current_pool_key) + connection_handler.remove_connection_pool(name, current_shard) end def clear_cache! # :nodoc: @@ -302,15 +302,15 @@ module ActiveRecord end end - def with_shard(pool_key, role, prevent_writes) - old_pool_key = current_pool_key + def with_shard(shard, role, prevent_writes) + old_shard = current_shard with_role(role, prevent_writes) do - self.current_pool_key = pool_key + self.current_shard = shard yield end ensure - self.current_pool_key = old_pool_key + self.current_shard = old_shard end def swap_connection_handler(handler, &blk) # :nodoc: diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb index 696e5bfcb7..dd6b53788e 100644 --- a/activerecord/lib/active_record/core.rb +++ b/activerecord/lib/active_record/core.rb @@ -135,7 +135,7 @@ module ActiveRecord class_attribute :default_connection_handler, instance_writer: false - class_attribute :default_pool_key, instance_writer: false + class_attribute :default_shard, instance_writer: false self.filter_attributes = [] @@ -147,16 +147,16 @@ module ActiveRecord Thread.current.thread_variable_set(:ar_connection_handler, handler) end - def self.current_pool_key - Thread.current.thread_variable_get(:ar_pool_key) || default_pool_key + def self.current_shard + Thread.current.thread_variable_get(:ar_shard) || default_shard end - def self.current_pool_key=(pool_key) - Thread.current.thread_variable_set(:ar_pool_key, pool_key) + def self.current_shard=(shard) + Thread.current.thread_variable_set(:ar_shard, shard) end self.default_connection_handler = ConnectionAdapters::ConnectionHandler.new - self.default_pool_key = :default + self.default_shard = :default end module ClassMethods From d3061cdab2ce5b0730cb0047ef169ce89eff979a Mon Sep 17 00:00:00 2001 From: eileencodes Date: Fri, 7 Aug 2020 14:06:09 -0400 Subject: [PATCH 18/40] Update connection methods to use kwargs This change ensures that the connection methods are using kwargs instead of positional arguments. This change may look unnecessary but we're working on refactoring connection management to make it more robust and flexible so the method signatures of the methods changed here will continue to evolve and change. This commit does not change any released public APIs. The `shard` and `owner_name` arguments were added in 6.1 which is not released yet. Using kwargs will allow these methods to be more flexible and not get super ugly as we change their underlying behavior. The kwargs let us support multiple non-positional arguments with default. Co-authored-by: John Crepezzi --- .../abstract/connection_pool.rb | 20 +++++++++---------- .../lib/active_record/connection_handling.rb | 16 +++++++-------- ...nection_handlers_multi_pool_config_test.rb | 16 +++++++-------- .../connection_handlers_sharding_db_test.rb | 16 +++++++-------- 4 files changed, 34 insertions(+), 34 deletions(-) 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 6e884a769a..4e36fc176d 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb @@ -1031,7 +1031,7 @@ module ActiveRecord end alias :connection_pools :connection_pool_list - def establish_connection(config, shard = Base.default_shard, owner_name = Base.name) + def establish_connection(config, owner_name: Base.name, shard: Base.default_shard) owner_name = config.to_s if config.is_a?(Symbol) pool_config = resolve_pool_config(config, owner_name) @@ -1040,7 +1040,7 @@ module ActiveRecord # Protects the connection named `ActiveRecord::Base` from being removed # if the user calls `establish_connection :primary`. if owner_to_pool_manager.key?(pool_config.connection_specification_name) - remove_connection_pool(pool_config.connection_specification_name, shard) + remove_connection_pool(pool_config.connection_specification_name, shard: shard) end message_bus = ActiveSupport::Notifications.instrumenter @@ -1094,8 +1094,8 @@ module ActiveRecord # active or defined connection: if it is the latter, it will be # opened and set as the active connection for the class it was defined # for (not necessarily the current class). - def retrieve_connection(spec_name, shard = ActiveRecord::Base.default_shard) # :nodoc: - pool = retrieve_connection_pool(spec_name, shard) + def retrieve_connection(spec_name, shard: ActiveRecord::Base.default_shard) # :nodoc: + pool = retrieve_connection_pool(spec_name, shard: shard) unless pool if shard != ActiveRecord::Base.default_shard @@ -1114,8 +1114,8 @@ module ActiveRecord # Returns true if a connection that's accessible to this class has # already been opened. - def connected?(spec_name, shard = ActiveRecord::Base.default_shard) - pool = retrieve_connection_pool(spec_name, shard) + def connected?(spec_name, shard: ActiveRecord::Base.default_shard) + pool = retrieve_connection_pool(spec_name, shard: shard) pool && pool.connected? end @@ -1123,12 +1123,12 @@ module ActiveRecord # connection and the defined connection (if they exist). The result # can be used as an argument for #establish_connection, for easily # re-establishing the connection. - def remove_connection(owner, shard = ActiveRecord::Base.default_shard) - remove_connection_pool(owner, shard)&.configuration_hash + def remove_connection(owner, shard: ActiveRecord::Base.default_shard) + remove_connection_pool(owner, shard: shard)&.configuration_hash end deprecate remove_connection: "Use #remove_connection_pool, which now returns a DatabaseConfig object instead of a Hash" - def remove_connection_pool(owner, shard = ActiveRecord::Base.default_shard) + def remove_connection_pool(owner, shard: ActiveRecord::Base.default_shard) if pool_manager = get_pool_manager(owner) pool_config = pool_manager.remove_pool_config(shard) @@ -1142,7 +1142,7 @@ module ActiveRecord # Retrieving the connection pool happens a lot, so we cache it in @owner_to_pool_manager. # This makes retrieving the connection pool O(1) once the process is warm. # When a connection is established or removed, we invalidate the cache. - def retrieve_connection_pool(owner, shard = ActiveRecord::Base.default_shard) + def retrieve_connection_pool(owner, shard: ActiveRecord::Base.default_shard) pool_config = get_pool_manager(owner)&.get_pool_config(shard) pool_config&.pool end diff --git a/activerecord/lib/active_record/connection_handling.rb b/activerecord/lib/active_record/connection_handling.rb index ffae5f6610..e14f5907ae 100644 --- a/activerecord/lib/active_record/connection_handling.rb +++ b/activerecord/lib/active_record/connection_handling.rb @@ -49,7 +49,7 @@ module ActiveRecord def establish_connection(config_or_env = nil) config_or_env ||= DEFAULT_ENV.call.to_sym db_config, owner_name = resolve_config_for_connection(config_or_env) - connection_handler.establish_connection(db_config, current_shard, owner_name) + connection_handler.establish_connection(db_config, owner_name: owner_name, shard: current_shard) end # Connects a model to the databases specified. The +database+ keyword @@ -89,7 +89,7 @@ module ActiveRecord db_config, owner_name = resolve_config_for_connection(database_key) handler = lookup_connection_handler(role.to_sym) - connections << handler.establish_connection(db_config, default_shard, owner_name) + connections << handler.establish_connection(db_config, owner_name: owner_name, shard: default_shard) end shards.each do |shard, database_keys| @@ -97,7 +97,7 @@ module ActiveRecord db_config, owner_name = resolve_config_for_connection(database_key) handler = lookup_connection_handler(role.to_sym) - connections << handler.establish_connection(db_config, shard.to_sym, owner_name) + connections << handler.establish_connection(db_config, owner_name: owner_name, shard: shard.to_sym) end end @@ -247,16 +247,16 @@ module ActiveRecord end def connection_pool - connection_handler.retrieve_connection_pool(connection_specification_name, current_shard) || raise(ConnectionNotEstablished) + connection_handler.retrieve_connection_pool(connection_specification_name, shard: current_shard) || raise(ConnectionNotEstablished) end def retrieve_connection - connection_handler.retrieve_connection(connection_specification_name, current_shard) + connection_handler.retrieve_connection(connection_specification_name, shard: current_shard) end # Returns +true+ if Active Record is connected. def connected? - connection_handler.connected?(connection_specification_name, current_shard) + connection_handler.connected?(connection_specification_name, shard: current_shard) end def remove_connection(name = nil) @@ -264,11 +264,11 @@ module ActiveRecord # if removing a connection that has a pool, we reset the # connection_specification_name so it will use the parent # pool. - if connection_handler.retrieve_connection_pool(name, current_shard) + if connection_handler.retrieve_connection_pool(name, shard: current_shard) self.connection_specification_name = nil end - connection_handler.remove_connection_pool(name, current_shard) + connection_handler.remove_connection_pool(name, shard: current_shard) end def clear_cache! # :nodoc: diff --git a/activerecord/test/cases/connection_adapters/connection_handlers_multi_pool_config_test.rb b/activerecord/test/cases/connection_adapters/connection_handlers_multi_pool_config_test.rb index 18a593a2a5..1b39a825da 100644 --- a/activerecord/test/cases/connection_adapters/connection_handlers_multi_pool_config_test.rb +++ b/activerecord/test/cases/connection_adapters/connection_handlers_multi_pool_config_test.rb @@ -31,10 +31,10 @@ module ActiveRecord @prev_configs, ActiveRecord::Base.configurations = ActiveRecord::Base.configurations, config @writing_handler.establish_connection(:primary) - @writing_handler.establish_connection(:primary, :pool_config_two) + @writing_handler.establish_connection(:primary, shard: :pool_config_two) - default_pool = @writing_handler.retrieve_connection_pool("primary", :default) - other_pool = @writing_handler.retrieve_connection_pool("primary", :pool_config_two) + default_pool = @writing_handler.retrieve_connection_pool("primary", shard: :default) + other_pool = @writing_handler.retrieve_connection_pool("primary", shard: :pool_config_two) assert_not_nil default_pool assert_not_equal default_pool, other_pool @@ -59,13 +59,13 @@ module ActiveRecord @prev_configs, ActiveRecord::Base.configurations = ActiveRecord::Base.configurations, config @writing_handler.establish_connection(:primary) - @writing_handler.establish_connection(:primary, :pool_config_two) + @writing_handler.establish_connection(:primary, shard: :pool_config_two) # remove default @writing_handler.remove_connection_pool("primary") assert_nil @writing_handler.retrieve_connection_pool("primary") - assert_not_nil @writing_handler.retrieve_connection_pool("primary", :pool_config_two) + assert_not_nil @writing_handler.retrieve_connection_pool("primary", shard: :pool_config_two) ensure ActiveRecord::Base.configurations = @prev_configs ActiveRecord::Base.establish_connection(:arunit) @@ -84,14 +84,14 @@ module ActiveRecord @prev_configs, ActiveRecord::Base.configurations = ActiveRecord::Base.configurations, config @writing_handler.establish_connection(:primary) - @writing_handler.establish_connection(:primary, :pool_config_two) + @writing_handler.establish_connection(:primary, shard: :pool_config_two) # connect to default @writing_handler.connection_pool_list.first.checkout assert @writing_handler.connected?("primary") - assert @writing_handler.connected?("primary", :default) - assert_not @writing_handler.connected?("primary", :pool_config_two) + assert @writing_handler.connected?("primary", shard: :default) + assert_not @writing_handler.connected?("primary", shard: :pool_config_two) ensure ActiveRecord::Base.configurations = @prev_configs ActiveRecord::Base.establish_connection(:arunit) diff --git a/activerecord/test/cases/connection_adapters/connection_handlers_sharding_db_test.rb b/activerecord/test/cases/connection_adapters/connection_handlers_sharding_db_test.rb index 92beac0cfb..89ddc11f3f 100644 --- a/activerecord/test/cases/connection_adapters/connection_handlers_sharding_db_test.rb +++ b/activerecord/test/cases/connection_adapters/connection_handlers_sharding_db_test.rb @@ -43,13 +43,13 @@ module ActiveRecord }) base_pool = ActiveRecord::Base.connection_handlers[:writing].retrieve_connection_pool("ActiveRecord::Base") - default_pool = ActiveRecord::Base.connection_handlers[:writing].retrieve_connection_pool("ActiveRecord::Base", :default) + default_pool = ActiveRecord::Base.connection_handlers[:writing].retrieve_connection_pool("ActiveRecord::Base", shard: :default) assert_equal base_pool, default_pool assert_equal "test/db/primary.sqlite3", default_pool.db_config.database assert_equal "primary", default_pool.db_config.name - assert_not_nil pool = ActiveRecord::Base.connection_handlers[:writing].retrieve_connection_pool("ActiveRecord::Base", :shard_one) + assert_not_nil pool = ActiveRecord::Base.connection_handlers[:writing].retrieve_connection_pool("ActiveRecord::Base", shard: :shard_one) assert_equal "test/db/primary_shard_one.sqlite3", pool.db_config.database assert_equal "primary_shard_one", pool.db_config.name ensure @@ -77,23 +77,23 @@ module ActiveRecord shard_one: { writing: :primary_shard_one, reading: :primary_shard_one_replica } }) - default_writing_pool = ActiveRecord::Base.connection_handlers[:writing].retrieve_connection_pool("ActiveRecord::Base", :default) + default_writing_pool = ActiveRecord::Base.connection_handlers[:writing].retrieve_connection_pool("ActiveRecord::Base", shard: :default) base_writing_pool = ActiveRecord::Base.connection_handlers[:writing].retrieve_connection_pool("ActiveRecord::Base") assert_equal base_writing_pool, default_writing_pool assert_equal "test/db/primary.sqlite3", default_writing_pool.db_config.database assert_equal "primary", default_writing_pool.db_config.name - default_reading_pool = ActiveRecord::Base.connection_handlers[:reading].retrieve_connection_pool("ActiveRecord::Base", :default) + default_reading_pool = ActiveRecord::Base.connection_handlers[:reading].retrieve_connection_pool("ActiveRecord::Base", shard: :default) base_reading_pool = ActiveRecord::Base.connection_handlers[:reading].retrieve_connection_pool("ActiveRecord::Base") assert_equal base_reading_pool, default_reading_pool assert_equal "test/db/primary.sqlite3", default_reading_pool.db_config.database assert_equal "primary_replica", default_reading_pool.db_config.name - assert_not_nil pool = ActiveRecord::Base.connection_handlers[:writing].retrieve_connection_pool("ActiveRecord::Base", :shard_one) + assert_not_nil pool = ActiveRecord::Base.connection_handlers[:writing].retrieve_connection_pool("ActiveRecord::Base", shard: :shard_one) assert_equal "test/db/primary_shard_one.sqlite3", pool.db_config.database assert_equal "primary_shard_one", pool.db_config.name - assert_not_nil pool = ActiveRecord::Base.connection_handlers[:reading].retrieve_connection_pool("ActiveRecord::Base", :shard_one) + assert_not_nil pool = ActiveRecord::Base.connection_handlers[:reading].retrieve_connection_pool("ActiveRecord::Base", shard: :shard_one) assert_equal "test/db/primary_shard_one.sqlite3", pool.db_config.database assert_equal "primary_shard_one_replica", pool.db_config.name ensure @@ -233,10 +233,10 @@ module ActiveRecord def test_retrieve_connection_pool_with_invalid_shard assert_not_nil @rw_handler.retrieve_connection_pool("ActiveRecord::Base") - assert_nil @rw_handler.retrieve_connection_pool("ActiveRecord::Base", :foo) + assert_nil @rw_handler.retrieve_connection_pool("ActiveRecord::Base", shard: :foo) assert_not_nil @ro_handler.retrieve_connection_pool("ActiveRecord::Base") - assert_nil @ro_handler.retrieve_connection_pool("ActiveRecord::Base", :foo) + assert_nil @ro_handler.retrieve_connection_pool("ActiveRecord::Base", shard: :foo) end def test_calling_connected_to_on_a_non_existent_shard_raises From 3c500511e9e822fb9491f215ac6815272ae1b3c1 Mon Sep 17 00:00:00 2001 From: Corey Smith Date: Sat, 8 Aug 2020 04:41:34 -0500 Subject: [PATCH 19/40] Remove the unnessecary default: 'gen_random_uuid()' (#40012) --- guides/source/active_record_postgresql.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/guides/source/active_record_postgresql.md b/guides/source/active_record_postgresql.md index 57864e002c..ece990ed50 100644 --- a/guides/source/active_record_postgresql.md +++ b/guides/source/active_record_postgresql.md @@ -318,9 +318,9 @@ You can use `uuid` type to define references in migrations: ```ruby # db/migrate/20150418012400_create_blog.rb enable_extension 'pgcrypto' unless extension_enabled?('pgcrypto') -create_table :posts, id: :uuid, default: 'gen_random_uuid()' +create_table :posts, id: :uuid -create_table :comments, id: :uuid, default: 'gen_random_uuid()' do |t| +create_table :comments, id: :uuid do |t| # t.belongs_to :post, type: :uuid t.references :post, type: :uuid end @@ -414,7 +414,7 @@ extension to generate random UUIDs. ```ruby # db/migrate/20131220144913_create_devices.rb enable_extension 'pgcrypto' unless extension_enabled?('pgcrypto') -create_table :devices, id: :uuid, default: 'gen_random_uuid()' do |t| +create_table :devices, id: :uuid do |t| t.string :kind end From 61ab15431621ee44287612024e9f2fff53d3d4c9 Mon Sep 17 00:00:00 2001 From: eileencodes Date: Mon, 10 Aug 2020 09:51:43 -0400 Subject: [PATCH 20/40] Add helper method for resetting connection handlers in tests This change makes a helper method for resetting connection handlers in the Active Record tests. The change here is relatively small and may seem unnecessary. The reason we're pushing this change is for upcoming refactoring to connection management. This change will mean that we can update one location instead of 9+ files to reset connections. It will reduce the diff of our refactoring and make reusing this code easier in the future. The method name chosen is purposefully `clean_up_connection_handler` over `clean_up_connection_handlers` because in the future there will only be one handler. Co-authored-by: John Crepezzi --- activerecord/test/cases/base_test.rb | 2 +- .../connection_handlers_multi_db_test.rb | 6 ++---- .../connection_handlers_multi_pool_config_test.rb | 2 +- .../connection_handlers_sharding_db_test.rb | 2 +- activerecord/test/cases/database_selector_test.rb | 2 +- activerecord/test/cases/fixtures_test.rb | 3 +-- activerecord/test/cases/helper.rb | 4 ++++ activerecord/test/cases/query_cache_test.rb | 6 +++--- activerecord/test/cases/tasks/database_tasks_test.rb | 2 +- activerecord/test/cases/test_fixtures_test.rb | 3 +-- 10 files changed, 16 insertions(+), 16 deletions(-) diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index 6f5e4062c7..1f3f2b28ad 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -1719,7 +1719,7 @@ class BasicsTest < ActiveRecord::TestCase assert_match %r/\AWrite query attempted while in readonly mode: INSERT /, conn2_error.message ensure - ActiveRecord::Base.connection_handlers = { writing: ActiveRecord::Base.default_connection_handler } + clean_up_connection_handler ActiveRecord::Base.establish_connection(:arunit) end end diff --git a/activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb b/activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb index 6f0d5fbb68..9f7d96591b 100644 --- a/activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb +++ b/activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb @@ -21,7 +21,7 @@ module ActiveRecord end def teardown - ActiveRecord::Base.connection_handlers = { writing: ActiveRecord::Base.default_connection_handler } + clean_up_connection_handler end class MultiConnectionTestModel < ActiveRecord::Base @@ -369,8 +369,6 @@ module ActiveRecord end def test_connection_handlers_are_per_thread_and_not_per_fiber - original_handlers = ActiveRecord::Base.connection_handlers - ActiveRecord::Base.connection_handlers = { writing: ActiveRecord::Base.default_connection_handler, reading: ActiveRecord::ConnectionAdapters::ConnectionHandler.new } reading_handler = ActiveRecord::Base.connection_handlers[:reading] @@ -382,7 +380,7 @@ module ActiveRecord assert_not_equal reading, ActiveRecord::Base.connection_handler assert_equal reading, reading_handler ensure - ActiveRecord::Base.connection_handlers = original_handlers + clean_up_connection_handler end def test_connection_handlers_swapping_connections_in_fiber diff --git a/activerecord/test/cases/connection_adapters/connection_handlers_multi_pool_config_test.rb b/activerecord/test/cases/connection_adapters/connection_handlers_multi_pool_config_test.rb index 1b39a825da..5103796754 100644 --- a/activerecord/test/cases/connection_adapters/connection_handlers_multi_pool_config_test.rb +++ b/activerecord/test/cases/connection_adapters/connection_handlers_multi_pool_config_test.rb @@ -15,7 +15,7 @@ module ActiveRecord end def teardown - ActiveRecord::Base.connection_handlers = { writing: ActiveRecord::Base.default_connection_handler } + clean_up_connection_handler end unless in_memory_db? diff --git a/activerecord/test/cases/connection_adapters/connection_handlers_sharding_db_test.rb b/activerecord/test/cases/connection_adapters/connection_handlers_sharding_db_test.rb index 89ddc11f3f..5536d51acc 100644 --- a/activerecord/test/cases/connection_adapters/connection_handlers_sharding_db_test.rb +++ b/activerecord/test/cases/connection_adapters/connection_handlers_sharding_db_test.rb @@ -21,7 +21,7 @@ module ActiveRecord end def teardown - ActiveRecord::Base.connection_handlers = { writing: ActiveRecord::Base.default_connection_handler } + clean_up_connection_handler end unless in_memory_db? diff --git a/activerecord/test/cases/database_selector_test.rb b/activerecord/test/cases/database_selector_test.rb index 9742d34aef..0438adf7a3 100644 --- a/activerecord/test/cases/database_selector_test.rb +++ b/activerecord/test/cases/database_selector_test.rb @@ -12,7 +12,7 @@ module ActiveRecord end teardown do - ActiveRecord::Base.connection_handlers = { writing: ActiveRecord::Base.default_connection_handler } + clean_up_connection_handler end def test_empty_session diff --git a/activerecord/test/cases/fixtures_test.rb b/activerecord/test/cases/fixtures_test.rb index ed64d0bda2..7f7daf91e3 100644 --- a/activerecord/test/cases/fixtures_test.rb +++ b/activerecord/test/cases/fixtures_test.rb @@ -1399,7 +1399,6 @@ if current_adapter?(:SQLite3Adapter) && !in_memory_db? def setup @old_handler = ActiveRecord::Base.connection_handler - @old_handlers = ActiveRecord::Base.connection_handlers @prev_configs, ActiveRecord::Base.configurations = ActiveRecord::Base.configurations, config db_config = ActiveRecord::DatabaseConfigurations::HashConfig.new(ENV["RAILS_ENV"], "readonly", readonly_config) @@ -1413,7 +1412,7 @@ if current_adapter?(:SQLite3Adapter) && !in_memory_db? def teardown ActiveRecord::Base.configurations = @prev_configs ActiveRecord::Base.connection_handler = @old_handler - ActiveRecord::Base.connection_handlers = @old_handlers + clean_up_connection_handler end def test_uses_writing_connection_for_fixtures diff --git a/activerecord/test/cases/helper.rb b/activerecord/test/cases/helper.rb index 3c8209c103..4fe0523dda 100644 --- a/activerecord/test/cases/helper.rb +++ b/activerecord/test/cases/helper.rb @@ -149,6 +149,10 @@ def disable_extension!(extension, connection) connection.reconnect! end +def clean_up_connection_handler + ActiveRecord::Base.connection_handlers = { ActiveRecord::Base.writing_role => ActiveRecord::Base.default_connection_handler } +end + def load_schema # silence verbose schema loading original_stdout = $stdout diff --git a/activerecord/test/cases/query_cache_test.rb b/activerecord/test/cases/query_cache_test.rb index d4934d289e..46fbeefe80 100644 --- a/activerecord/test/cases/query_cache_test.rb +++ b/activerecord/test/cases/query_cache_test.rb @@ -93,7 +93,7 @@ class QueryCacheTest < ActiveRecord::TestCase mw.call({}) ensure - ActiveRecord::Base.connection_handlers = { writing: ActiveRecord::Base.default_connection_handler } + clean_up_connection_handler end @@ -157,7 +157,7 @@ class QueryCacheTest < ActiveRecord::TestCase rd.close ensure - ActiveRecord::Base.connection_handlers = { writing: ActiveRecord::Base.default_connection_handler } + clean_up_connection_handler end end @@ -607,7 +607,7 @@ class QueryCacheTest < ActiveRecord::TestCase mw.call({}) ensure - ActiveRecord::Base.connection_handlers = { writing: ActiveRecord::Base.default_connection_handler } + clean_up_connection_handler end test "query cache is enabled in threads with shared connection" do diff --git a/activerecord/test/cases/tasks/database_tasks_test.rb b/activerecord/test/cases/tasks/database_tasks_test.rb index a1acf19565..2c1504179b 100644 --- a/activerecord/test/cases/tasks/database_tasks_test.rb +++ b/activerecord/test/cases/tasks/database_tasks_test.rb @@ -1112,7 +1112,7 @@ module ActiveRecord def teardown SchemaMigration.delete_all InternalMetadata.delete_all - ActiveRecord::Base.connection_handlers = { writing: ActiveRecord::Base.default_connection_handler } + clean_up_connection_handler end def test_truncate_tables diff --git a/activerecord/test/cases/test_fixtures_test.rb b/activerecord/test/cases/test_fixtures_test.rb index ca65d5b133..cba6107b9a 100644 --- a/activerecord/test/cases/test_fixtures_test.rb +++ b/activerecord/test/cases/test_fixtures_test.rb @@ -43,7 +43,6 @@ class TestFixturesTest < ActiveRecord::TestCase end end - old_handlers = ActiveRecord::Base.connection_handlers old_handler = ActiveRecord::Base.connection_handler ActiveRecord::Base.connection_handler = ActiveRecord::ConnectionAdapters::ConnectionHandler.new ActiveRecord::Base.connection_handlers = {} @@ -53,7 +52,7 @@ class TestFixturesTest < ActiveRecord::TestCase assert_predicate(test_result, :passed?) ensure ActiveRecord::Base.connection_handler = old_handler - ActiveRecord::Base.connection_handlers = old_handlers + clean_up_connection_handler FileUtils.rm_r(tmp_dir) end end From 919eb6dca980ee7390d70f991d34d1e1c6b00f25 Mon Sep 17 00:00:00 2001 From: eileencodes Date: Mon, 10 Aug 2020 15:52:55 -0400 Subject: [PATCH 21/40] Fix missed establish_connection In `connected_to` one of the deprecated arguments wasn't well tested so the incorrect methods signature wasn't caught by the tests. This change updates the caller when `connected_to` uses the database key. I've also cleaned up a few arguments that weren't necessary. Since the handler methods set defaults for the `shard` key, we don't need to pass that in `establish_connection` when not using the sharding API. --- activerecord/lib/active_record/connection_handling.rb | 6 +++--- .../connection_handlers_multi_db_test.rb | 8 ++++++++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/activerecord/lib/active_record/connection_handling.rb b/activerecord/lib/active_record/connection_handling.rb index e14f5907ae..cb9ca865af 100644 --- a/activerecord/lib/active_record/connection_handling.rb +++ b/activerecord/lib/active_record/connection_handling.rb @@ -49,7 +49,7 @@ module ActiveRecord def establish_connection(config_or_env = nil) config_or_env ||= DEFAULT_ENV.call.to_sym db_config, owner_name = resolve_config_for_connection(config_or_env) - connection_handler.establish_connection(db_config, owner_name: owner_name, shard: current_shard) + connection_handler.establish_connection(db_config, owner_name: owner_name) end # Connects a model to the databases specified. The +database+ keyword @@ -89,7 +89,7 @@ module ActiveRecord db_config, owner_name = resolve_config_for_connection(database_key) handler = lookup_connection_handler(role.to_sym) - connections << handler.establish_connection(db_config, owner_name: owner_name, shard: default_shard) + connections << handler.establish_connection(db_config, owner_name: owner_name) end shards.each do |shard, database_keys| @@ -154,7 +154,7 @@ module ActiveRecord db_config, owner_name = resolve_config_for_connection(database) handler = lookup_connection_handler(role) - handler.establish_connection(db_config, default_shard, owner_name) + handler.establish_connection(db_config, owner_name: owner_name) with_handler(role, &blk) elsif shard diff --git a/activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb b/activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb index 9f7d96591b..d184af3b7a 100644 --- a/activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb +++ b/activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb @@ -217,6 +217,14 @@ module ActiveRecord assert_equal "`connected_to` cannot accept a `database` argument with any other arguments.", error.message end + def test_database_argument_is_deprecated + assert_deprecated do + ActiveRecord::Base.connected_to(database: { writing: { adapter: "sqlite3", database: "test/db/primary.sqlite3" } }) { } + end + ensure + ActiveRecord::Base.establish_connection(:arunit) + end + def test_switching_connections_without_database_and_role_raises error = assert_raises(ArgumentError) do ActiveRecord::Base.connected_to { } From 8d4d0f37014dddb4bd24833cf7ec1ec0da6102e5 Mon Sep 17 00:00:00 2001 From: Gannon McGibbon Date: Tue, 4 Aug 2020 18:28:30 -0400 Subject: [PATCH 22/40] Fix `assert_recognizes` on mounted root routes. Allow `assert_recognizes` routing assertions to work on mounted root routes. --- actionpack/CHANGELOG.md | 4 ++++ .../lib/action_dispatch/journey/router.rb | 3 ++- .../test/dispatch/routing_assertions_test.rb | 17 +++++++++++++++++ 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 2b6582df78..3a4b52cc58 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,7 @@ +* Allow `assert_recognizes` routing assertions to work on mounted root routes. + + *Gannon McGibbon* + * Change default redirection status code for non-GET/HEAD requests to 308 Permanent Redirect for `ActionDispatch::SSL`. *Alan Tan*, *Oz Ben-David* diff --git a/actionpack/lib/action_dispatch/journey/router.rb b/actionpack/lib/action_dispatch/journey/router.rb index 5679dde71d..b6b71720db 100644 --- a/actionpack/lib/action_dispatch/journey/router.rb +++ b/actionpack/lib/action_dispatch/journey/router.rb @@ -66,7 +66,8 @@ module ActionDispatch find_routes(rails_req).each do |match, parameters, route| unless route.path.anchored rails_req.script_name = match.to_s - rails_req.path_info = match.post_match.sub(/^([^\/])/, '/\1') + rails_req.path_info = match.post_match + rails_req.path_info = "/" + rails_req.path_info unless rails_req.path_info.start_with? "/" end parameters = route.defaults.merge parameters diff --git a/actionpack/test/dispatch/routing_assertions_test.rb b/actionpack/test/dispatch/routing_assertions_test.rb index 009b6d9bc3..760392d9df 100644 --- a/actionpack/test/dispatch/routing_assertions_test.rb +++ b/actionpack/test/dispatch/routing_assertions_test.rb @@ -14,11 +14,22 @@ class QueryBooksController < BooksController; end class RoutingAssertionsTest < ActionController::TestCase def setup + root_engine = Class.new(Rails::Engine) do + def self.name + "root_engine" + end + end + + root_engine.routes.draw do + root to: "books#index" + end + engine = Class.new(Rails::Engine) do def self.name "blog_engine" end end + engine.routes.draw do resources :books @@ -53,6 +64,8 @@ class RoutingAssertionsTest < ActionController::TestCase mount engine => "/shelf" + mount root_engine => "/" + get "/shelf/foo", controller: "query_articles", action: "index" end end @@ -118,6 +131,10 @@ class RoutingAssertionsTest < ActionController::TestCase assert_recognizes({ controller: "books", action: "show", id: "1" }, "/shelf/books/1") end + def test_assert_recognizes_with_engine_at_root + assert_recognizes({ controller: "books", action: "index" }, "/") + end + def test_assert_recognizes_with_engine_and_extras assert_recognizes({ controller: "books", action: "index", page: "1" }, "/shelf/books", page: "1") end From dabece17ad92e0fc32d6706f596099bdb97c7aff Mon Sep 17 00:00:00 2001 From: KapilSachdev Date: Tue, 11 Aug 2020 20:37:27 +0530 Subject: [PATCH 23/40] fix: warning: instance variable @controller not initialized Fixes #39937 --- actionpack/lib/action_dispatch/testing/assertions/routing.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/actionpack/lib/action_dispatch/testing/assertions/routing.rb b/actionpack/lib/action_dispatch/testing/assertions/routing.rb index 071097af1a..35288de822 100644 --- a/actionpack/lib/action_dispatch/testing/assertions/routing.rb +++ b/actionpack/lib/action_dispatch/testing/assertions/routing.rb @@ -199,7 +199,8 @@ module ActionDispatch method = :get end - request = ActionController::TestRequest.create @controller.class + controller = @controller if defined?(@controller) + request = ActionController::TestRequest.create controller&.class if %r{://}.match?(path) fail_on(URI::InvalidURIError, msg) do From d747b531728230bab73bd38b695f8bde1d95f704 Mon Sep 17 00:00:00 2001 From: Abhay Nikam Date: Tue, 11 Aug 2020 21:37:37 +0530 Subject: [PATCH 24/40] Document the --database/--db option for multiple database Rails application [skip ci] --- guides/source/command_line.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/guides/source/command_line.md b/guides/source/command_line.md index d079375764..595de9035a 100644 --- a/guides/source/command_line.md +++ b/guides/source/command_line.md @@ -395,6 +395,12 @@ With the `helper` method it is possible to access Rails and your application's h INFO: You can also use the alias "db" to invoke the dbconsole: `bin/rails db`. +If you are using multiple databases, `bin/rails dbconsole` will connect to the primary database by default. You can specify which database to connect to using `--database` or `--db`: + +```bash +$ bin/rails dbconsole --database=animals +``` + ### `bin/rails runner` `runner` runs Ruby code in the context of Rails non-interactively. For instance: From 6aa26c30e28e599a3a40b3382cc0a504e9beee81 Mon Sep 17 00:00:00 2001 From: Jonathan Hefner Date: Tue, 11 Aug 2020 17:08:09 -0500 Subject: [PATCH 25/40] Identify directly-uploaded blobs before saving the associated record An Active Storage `Blob` must be identified before it can be reliably validated. For direct uploads, a `Blob` is identified when it is attached, rather than when it is created. Before this commit, the sequence of events when attaching a `Blob` was: 1. Find the `Blob`. 2. Assign the `Blob` to an `Attachment`. 3. Save the owner record. 4. Save the `Attachment`. 5. Identify the `Blob`'s true `content_type` from its file. 6. Save the `Blob`. This meant that the owner record's validations might not see the `Blob`'s true `content_type`. After this commit, the sequence of events will be: 1. Find the `Blob`. 2. Identify the `Blob`'s true `content_type` from its file. 3. Assign the `Blob` to an `Attachment`. 4. Save the owner record. 5. Save the `Attachment`. 6. Save the `Blob`. Thus the `Blob`'s true `content_type` will be available when running the owner record's validations. --- .../app/models/active_storage/attachment.rb | 8 +-- .../app/models/active_storage/blob.rb | 6 ++ .../active_storage/blob/identifiable.rb | 13 ++-- .../attached/changes/create_many.rb | 1 + .../attached/changes/create_one.rb | 1 + .../lib/active_storage/attached/many.rb | 3 +- .../lib/active_storage/attached/one.rb | 3 +- activestorage/test/models/attachment_test.rb | 62 +++++++++++++++++++ activestorage/test/models/blob_test.rb | 10 +++ 9 files changed, 93 insertions(+), 14 deletions(-) diff --git a/activestorage/app/models/active_storage/attachment.rb b/activestorage/app/models/active_storage/attachment.rb index f7a8ff8922..9b626be82d 100644 --- a/activestorage/app/models/active_storage/attachment.rb +++ b/activestorage/app/models/active_storage/attachment.rb @@ -11,12 +11,12 @@ class ActiveStorage::Attachment < ActiveRecord::Base self.table_name = "active_storage_attachments" belongs_to :record, polymorphic: true, touch: true - belongs_to :blob, class_name: "ActiveStorage::Blob" + belongs_to :blob, class_name: "ActiveStorage::Blob", autosave: true delegate_missing_to :blob delegate :signed_id, to: :blob - after_create_commit :mirror_blob_later, :analyze_blob_later, :identify_blob + after_create_commit :mirror_blob_later, :analyze_blob_later after_destroy_commit :purge_dependent_blob_later # Synchronously deletes the attachment and {purges the blob}[rdoc-ref:ActiveStorage::Blob#purge]. @@ -38,10 +38,6 @@ class ActiveStorage::Attachment < ActiveRecord::Base end private - def identify_blob - blob.identify - end - def analyze_blob_later blob.analyze_later unless blob.analyzed? end diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb index 163f633502..dac9ddc509 100644 --- a/activestorage/app/models/active_storage/blob.rb +++ b/activestorage/app/models/active_storage/blob.rb @@ -53,6 +53,8 @@ class ActiveStorage::Blob < ActiveRecord::Base self.service_name ||= self.class.service.name end + after_commit :update_service_metadata, if: :content_type_previously_changed? + before_destroy(prepend: true) do raise ActiveRecord::InvalidForeignKey if attachments.exists? end @@ -326,6 +328,10 @@ class ActiveStorage::Blob < ActiveRecord::Base { content_type: content_type } end end + + def update_service_metadata + service.update_metadata key, **service_metadata if service_metadata.any? + end end ActiveSupport.run_load_hooks :active_storage_blob, ActiveStorage::Blob diff --git a/activestorage/app/models/active_storage/blob/identifiable.rb b/activestorage/app/models/active_storage/blob/identifiable.rb index c8d9569c5d..f2a07a634d 100644 --- a/activestorage/app/models/active_storage/blob/identifiable.rb +++ b/activestorage/app/models/active_storage/blob/identifiable.rb @@ -2,9 +2,14 @@ module ActiveStorage::Blob::Identifiable def identify + identify_without_saving + save! + end + + def identify_without_saving unless identified? - update! content_type: identify_content_type, identified: true - update_service_metadata + self.content_type = identify_content_type + self.identified = true end end @@ -24,8 +29,4 @@ module ActiveStorage::Blob::Identifiable "" end end - - def update_service_metadata - service.update_metadata key, **service_metadata if service_metadata.any? - end end diff --git a/activestorage/lib/active_storage/attached/changes/create_many.rb b/activestorage/lib/active_storage/attached/changes/create_many.rb index a7a8553e0f..4afc3d3831 100644 --- a/activestorage/lib/active_storage/attached/changes/create_many.rb +++ b/activestorage/lib/active_storage/attached/changes/create_many.rb @@ -6,6 +6,7 @@ module ActiveStorage def initialize(name, record, attachables) @name, @record, @attachables = name, record, Array(attachables) + blobs.each(&:identify_without_saving) end def attachments diff --git a/activestorage/lib/active_storage/attached/changes/create_one.rb b/activestorage/lib/active_storage/attached/changes/create_one.rb index 942b493798..c22b147d1e 100644 --- a/activestorage/lib/active_storage/attached/changes/create_one.rb +++ b/activestorage/lib/active_storage/attached/changes/create_one.rb @@ -9,6 +9,7 @@ module ActiveStorage def initialize(name, record, attachable) @name, @record, @attachable = name, record, attachable + blob.identify_without_saving end def attachment diff --git a/activestorage/lib/active_storage/attached/many.rb b/activestorage/lib/active_storage/attached/many.rb index 0a876a0065..913b534f52 100644 --- a/activestorage/lib/active_storage/attached/many.rb +++ b/activestorage/lib/active_storage/attached/many.rb @@ -29,7 +29,8 @@ module ActiveStorage # document.images.attach([ first_blob, second_blob ]) def attach(*attachables) if record.persisted? && !record.changed? - record.update(name => blobs + attachables.flatten) + record.public_send("#{name}=", blobs + attachables.flatten) + record.save else record.public_send("#{name}=", (change&.attachables || blobs) + attachables.flatten) end diff --git a/activestorage/lib/active_storage/attached/one.rb b/activestorage/lib/active_storage/attached/one.rb index 2b122b5ba2..2e8b2a91cf 100644 --- a/activestorage/lib/active_storage/attached/one.rb +++ b/activestorage/lib/active_storage/attached/one.rb @@ -29,7 +29,8 @@ module ActiveStorage # person.avatar.attach(avatar_blob) # ActiveStorage::Blob object def attach(attachable) if record.persisted? && !record.changed? - record.update(name => attachable) + record.public_send("#{name}=", attachable) + record.save else record.public_send("#{name}=", attachable) end diff --git a/activestorage/test/models/attachment_test.rb b/activestorage/test/models/attachment_test.rb index ebcb837e86..3cd9ee3a07 100644 --- a/activestorage/test/models/attachment_test.rb +++ b/activestorage/test/models/attachment_test.rb @@ -2,6 +2,7 @@ require "test_helper" require "database/setup" +require "active_support/testing/method_call_assertions" class ActiveStorage::AttachmentTest < ActiveSupport::TestCase include ActiveJob::TestHelper @@ -50,6 +51,38 @@ class ActiveStorage::AttachmentTest < ActiveSupport::TestCase end end + test "directly-uploaded blob identification for one attached occurs before validation" do + blob = directly_upload_file_blob(filename: "racecar.jpg", content_type: "application/octet-stream") + + assert_blob_identified_before_owner_validated(@user, blob, "image/jpeg") do + @user.avatar.attach(blob) + end + end + + test "directly-uploaded blob identification for many attached occurs before validation" do + blob = directly_upload_file_blob(filename: "racecar.jpg", content_type: "application/octet-stream") + + assert_blob_identified_before_owner_validated(@user, blob, "image/jpeg") do + @user.highlights.attach(blob) + end + end + + test "directly-uploaded blob identification for one attached occurs outside transaction" do + blob = directly_upload_file_blob(filename: "racecar.jpg") + + assert_blob_identified_outside_transaction(blob) do + @user.avatar.attach(blob) + end + end + + test "directly-uploaded blob identification for many attached occurs outside transaction" do + blob = directly_upload_file_blob(filename: "racecar.jpg") + + assert_blob_identified_outside_transaction(blob) do + @user.highlights.attach(blob) + end + end + test "getting a signed blob ID from an attachment" do blob = create_blob @user.avatar.attach(blob) @@ -65,4 +98,33 @@ class ActiveStorage::AttachmentTest < ActiveSupport::TestCase signed_id_generated_old_way = ActiveStorage.verifier.generate(@user.avatar.id, purpose: :blob_id) assert_equal blob, ActiveStorage::Blob.find_signed!(signed_id_generated_old_way) end + + private + def assert_blob_identified_before_owner_validated(owner, blob, content_type) + validated_content_type = nil + + owner.class.validate do + validated_content_type ||= blob.content_type + end + + yield + + assert_equal content_type, validated_content_type + assert_equal content_type, blob.reload.content_type + end + + def assert_blob_identified_outside_transaction(blob) + baseline_transaction_depth = ActiveRecord::Base.connection.open_transactions + max_transaction_depth = -1 + + track_transaction_depth = ->(*) do + max_transaction_depth = [ActiveRecord::Base.connection.open_transactions, max_transaction_depth].max + end + + blob.stub(:identify_without_saving, track_transaction_depth) do + yield + end + + assert_equal 0, (max_transaction_depth - baseline_transaction_depth) + end end diff --git a/activestorage/test/models/blob_test.rb b/activestorage/test/models/blob_test.rb index 532896cb27..10a4c69478 100644 --- a/activestorage/test/models/blob_test.rb +++ b/activestorage/test/models/blob_test.rb @@ -260,6 +260,16 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase assert_equal ["is invalid"], blob.errors[:service_name] end + test "updating the content_type updates service metadata" do + blob = directly_upload_file_blob(filename: "racecar.jpg", content_type: "application/octet-stream") + + expected_arguments = [blob.key, content_type: "image/jpeg"] + + assert_called_with(blob.service, :update_metadata, expected_arguments) do + blob.update!(content_type: "image/jpeg") + end + end + private def expected_url_for(blob, disposition: :attachment, filename: nil, content_type: nil, service_name: :local) filename ||= blob.filename From 28c5eca9a14752adde0656a8702805cf26e3ed41 Mon Sep 17 00:00:00 2001 From: Yu Ming Date: Thu, 30 Jul 2020 15:12:25 +0800 Subject: [PATCH 26/40] Updated security.md to include default_protect_from_forgery Update security.md to note that CSRF security token is included in requests automatically when `config.action_controller.default_protect_from_forgery` is set to `true`, which is the default for newly created Rails applications. --- guides/source/security.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/guides/source/security.md b/guides/source/security.md index b15c7a14db..3a3bad270e 100644 --- a/guides/source/security.md +++ b/guides/source/security.md @@ -292,13 +292,13 @@ There are many other possibilities, like using a `