From 1c98e6c0053033440c8ea5f3ee8e958d3a5e5b09 Mon Sep 17 00:00:00 2001 From: eileencodes Date: Thu, 9 Jan 2020 16:52:55 -0500 Subject: [PATCH] Ensure the reading connection always raises if we try to write Since test fixtures share connections (due to transactional tests) we end up overwriting the reading configuration so Rails doesn't recognize it as a replica connection. This change ensures that if we're using the `reading` role that connections will always have prevent writes turned on. If you need a replica connection that does not block writes, you should use a different role name other than `:reading`. The db selector test and connection handlers test have been updated to test for these changes. In the db selector test we don't always have a writing handler so I updated test fixtures to return if that's nil. Lastly one test needed to be updated to use a different handler name due to it needing to write to successfully test what it needs to test. Fixes #37765 --- activerecord/CHANGELOG.md | 6 ++++ .../lib/active_record/connection_handling.rb | 10 +++--- .../middleware/database_selector/resolver.rb | 2 +- .../lib/active_record/test_fixtures.rb | 2 ++ .../connection_handlers_multi_db_test.rb | 11 +++++-- .../test/cases/database_selector_test.rb | 33 +++++++++++-------- 6 files changed, 41 insertions(+), 23 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 1ea45c74ed..9b026e0cae 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,9 @@ +* Ensure `:reading` connections always raise if a write is attempted. + + Now Rails will raise an `ActiveRecord::ReadOnlyError` if any connection on the reading handler attempts to make a write. If your reading role needs to write you should name the role something other than `:reading`. + + *Eileen M. Uchitelle* + * Deprecate "primary" as the connection_specification_name for ActiveRecord::Base `"primary"` has been deprecated as the `connection_specification_name` for `ActiveRecord::Base` in favor of using `"ActiveRecord::Base"`. This change affects calls to `ActiveRecord::Base.connection_handler.retrieve_connection` and `ActiveRecord::Base.connection_handler.remove_connection`. If you're calling these methods with `"primary"`, please switch to `"ActiveRecord::Base"`. diff --git a/activerecord/lib/active_record/connection_handling.rb b/activerecord/lib/active_record/connection_handling.rb index 0721dbe06c..e05d91ecb6 100644 --- a/activerecord/lib/active_record/connection_handling.rb +++ b/activerecord/lib/active_record/connection_handling.rb @@ -115,12 +115,10 @@ module ActiveRecord with_handler(role, &blk) elsif role - if role == writing_role - with_handler(role.to_sym) do - connection_handler.while_preventing_writes(prevent_writes, &blk) - end - else - with_handler(role.to_sym, &blk) + prevent_writes = true if role == reading_role + + with_handler(role.to_sym) do + connection_handler.while_preventing_writes(prevent_writes, &blk) end else raise ArgumentError, "must provide a `database` or a `role`." diff --git a/activerecord/lib/active_record/middleware/database_selector/resolver.rb b/activerecord/lib/active_record/middleware/database_selector/resolver.rb index 3e9ce020bc..b19700a5f7 100644 --- a/activerecord/lib/active_record/middleware/database_selector/resolver.rb +++ b/activerecord/lib/active_record/middleware/database_selector/resolver.rb @@ -53,7 +53,7 @@ module ActiveRecord end def read_from_replica(&blk) - ActiveRecord::Base.connected_to(role: ActiveRecord::Base.reading_role) do + ActiveRecord::Base.connected_to(role: ActiveRecord::Base.reading_role, prevent_writes: true) do instrumenter.instrument("database_selector.active_record.read_from_replica") do yield end diff --git a/activerecord/lib/active_record/test_fixtures.rb b/activerecord/lib/active_record/test_fixtures.rb index 96fa79f6e8..cdd31f7f7a 100644 --- a/activerecord/lib/active_record/test_fixtures.rb +++ b/activerecord/lib/active_record/test_fixtures.rb @@ -194,6 +194,8 @@ module ActiveRecord if handler != writing_handler handler.connection_pool_names.each do |name| writing_pool_manager = writing_handler.send(:owner_to_pool_manager)[name] + return unless writing_pool_manager + writing_pool_config = writing_pool_manager.get_pool_config(:default) pool_manager = handler.send(:owner_to_pool_manager)[name] 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 1ef3f965af..95cc369d44 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 @@ -30,12 +30,14 @@ module ActiveRecord tf_writing = Tempfile.open "test_writing" tf_reading = Tempfile.open "test_reading" - MultiConnectionTestModel.connects_to database: { writing: { database: tf_writing.path, adapter: "sqlite3" }, reading: { database: tf_reading.path, adapter: "sqlite3" } } + # We need to use a role for reading not named reading, otherwise we'll prevent writes + # and won't be able to write to the second connection. + MultiConnectionTestModel.connects_to database: { writing: { database: tf_writing.path, adapter: "sqlite3" }, secondary: { database: tf_reading.path, adapter: "sqlite3" } } MultiConnectionTestModel.connection.execute("CREATE TABLE `test_1` (connection_role VARCHAR (255))") MultiConnectionTestModel.connection.execute("INSERT INTO test_1 VALUES ('writing')") - ActiveRecord::Base.connected_to(role: :reading) do + ActiveRecord::Base.connected_to(role: :secondary) do MultiConnectionTestModel.connection.execute("CREATE TABLE `test_1` (connection_role VARCHAR (255))") MultiConnectionTestModel.connection.execute("INSERT INTO test_1 VALUES ('reading')") end @@ -53,7 +55,7 @@ module ActiveRecord read_latch.count_down end - ActiveRecord::Base.connected_to(role: :reading) do + ActiveRecord::Base.connected_to(role: :secondary) do write_latch.count_down assert_equal "reading", MultiConnectionTestModel.connection.select_value("SELECT connection_role from test_1") read_latch.wait @@ -113,6 +115,7 @@ module ActiveRecord assert_equal :reading, ActiveRecord::Base.current_role assert ActiveRecord::Base.connected_to?(role: :reading) assert_not ActiveRecord::Base.connected_to?(role: :writing) + assert_predicate ActiveRecord::Base.connection, :preventing_writes? end ActiveRecord::Base.connected_to(role: :writing) do @@ -121,11 +124,13 @@ module ActiveRecord assert_equal :writing, ActiveRecord::Base.current_role assert ActiveRecord::Base.connected_to?(role: :writing) assert_not ActiveRecord::Base.connected_to?(role: :reading) + assert_not_predicate ActiveRecord::Base.connection, :preventing_writes? end ensure ActiveRecord::Base.configurations = @prev_configs ActiveRecord::Base.establish_connection(:arunit) ENV["RAILS_ENV"] = previous_env + FileUtils.rm_rf("db") end def test_establish_connection_using_3_levels_config_with_non_default_handlers diff --git a/activerecord/test/cases/database_selector_test.rb b/activerecord/test/cases/database_selector_test.rb index 930d463449..07257a9f4f 100644 --- a/activerecord/test/cases/database_selector_test.rb +++ b/activerecord/test/cases/database_selector_test.rb @@ -49,24 +49,31 @@ module ActiveRecord assert called end - def test_can_write_while_reading_from_replicas_if_explicit - @session_store[:last_write] = ActiveRecord::Middleware::DatabaseSelector::Resolver::Session.convert_time_to_timestamp(Time.now - 5.seconds) + unless in_memory_db? + def test_can_write_while_reading_from_replicas_if_explicit + @session_store[:last_write] = ActiveRecord::Middleware::DatabaseSelector::Resolver::Session.convert_time_to_timestamp(Time.now - 5.seconds) - resolver = ActiveRecord::Middleware::DatabaseSelector::Resolver.new(@session) + resolver = ActiveRecord::Middleware::DatabaseSelector::Resolver.new(@session) - called = false - resolver.read do - called = true - assert ActiveRecord::Base.connected_to?(role: :reading) + called = false + resolver.read do + ActiveRecord::Base.establish_connection :arunit - ActiveRecord::Base.connected_to(role: :writing, prevent_writes: false) do - assert ActiveRecord::Base.connected_to?(role: :writing) - assert_not_predicate ActiveRecord::Base.connection, :preventing_writes? + called = true + + assert ActiveRecord::Base.connected_to?(role: :reading) + assert_predicate ActiveRecord::Base.connection, :preventing_writes? + + ActiveRecord::Base.connected_to(role: :writing, prevent_writes: false) do + assert ActiveRecord::Base.connected_to?(role: :writing) + assert_not_predicate ActiveRecord::Base.connection, :preventing_writes? + end + + assert ActiveRecord::Base.connected_to?(role: :reading) + assert_predicate ActiveRecord::Base.connection, :preventing_writes? end - - assert ActiveRecord::Base.connected_to?(role: :reading) + assert called end - assert called end def test_read_from_primary