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

@@ -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"`.

activerecord/lib/active_record/connection_handling.rb

@@ -115,13 +115,11 @@ module ActiveRecord
 
         with_handler(role, &blk)
       elsif role
-        if role == writing_role
+        prevent_writes = true if role == reading_role
+
         with_handler(role.to_sym) do
           connection_handler.while_preventing_writes(prevent_writes, &blk)
         end
-        else
-          with_handler(role.to_sym, &blk)
-        end
       else
         raise ArgumentError, "must provide a `database` or a `role`."
       end

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

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]

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

activerecord/test/cases/database_selector_test.rb

@@ -49,6 +49,7 @@ module ActiveRecord
       assert called
     end
 
+    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)
 
@@ -56,8 +57,12 @@ module ActiveRecord
 
         called = false
         resolver.read do
+          ActiveRecord::Base.establish_connection :arunit
+
           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)
@@ -65,9 +70,11 @@ module ActiveRecord
           end
 
           assert ActiveRecord::Base.connected_to?(role: :reading)
+          assert_predicate ActiveRecord::Base.connection, :preventing_writes?
         end
         assert called
       end
+    end
 
     def test_read_from_primary
       @session_store[:last_write] = ActiveRecord::Middleware::DatabaseSelector::Resolver::Session.convert_time_to_timestamp(Time.now)