Revert "Disable automatic write protection on replicas"

This reverts commit 951deec.

This change prevents applications from testing replicas and would
require explicitly setting `prevent_writes` when connecting to reading
roles in `connected_to`. For now we'll revert this until there's a
longer term fix in place

Author: Tonkpils

activerecord/CHANGELOG.md

@@ -16,19 +16,6 @@
 
     *Jorge Manrubia*
 
-*   Disable automatic write protection on replicas.
-
-    Write protection is no longer automatically enabled for replicas.
-    You can manually prevent writes in your app with
-    `while_preventing_writes`. To automatically disable all writes on
-    your replica, configure the database user you are using to connect
-    to your replica to prevent writes. How you configure this is
-    specific to which database adapter you are using, but it usually
-    involves only granting the database user permission to do `SELECT`
-    queries.
-
-    *Adam Hess*
-
 *   The MySQL adapter now cast numbers and booleans bind parameters to to string for safety reasons.
 
     When comparing a string and a number in a query, MySQL convert the string to a number. So for

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

@@ -131,12 +131,15 @@ def use_metadata_table?
 
       # Determines whether writes are currently being prevented.
       #
+      # Returns true if the connection is a replica.
+      #
       # If the application is using legacy handling, returns
       # true if +connection_handler.prevent_writes+ is set.
       #
       # If the application is using the new connection handling
       # will return true based on +current_preventing_writes+.
       def preventing_writes?
+        return true if replica?
         return ActiveRecord::Base.connection_handler.prevent_writes if ActiveRecord.legacy_connection_handling
         return false if connection_klass.nil?
 

activerecord/lib/active_record/connection_handling.rb

@@ -184,6 +184,8 @@ def connected_to_many(*classes, role:, shard: nil, prevent_writes: false)
         raise NotImplementedError, "connected_to_many can only be called on ActiveRecord::Base."
       end
 
+      prevent_writes = true if role == ActiveRecord.reading_role
+
       connected_to_stack << { role: role, shard: shard, prevent_writes: prevent_writes, klasses: classes }
       yield
     ensure
@@ -202,6 +204,8 @@ def connecting_to(role: default_role, shard: default_shard, prevent_writes: fals
         raise NotImplementedError, "`connecting_to` is not available with `legacy_connection_handling`."
       end
 
+      prevent_writes = true if role == ActiveRecord.reading_role
+
       self.connected_to_stack << { role: role, shard: shard, prevent_writes: prevent_writes, klasses: [self] }
     end
 
@@ -352,6 +356,8 @@ def with_handler(handler_key, &blk)
       end
 
       def with_role_and_shard(role, shard, prevent_writes)
+        prevent_writes = true if role == ActiveRecord.reading_role
+
         if ActiveRecord.legacy_connection_handling
           with_handler(role.to_sym) do
             connection_handler.while_preventing_writes(prevent_writes) do

activerecord/lib/active_record/middleware/database_selector/resolver.rb

@@ -58,7 +58,7 @@ def read_from_primary(&blk)
           end
 
           def read_from_replica(&blk)
-            ActiveRecord::Base.connected_to(role: ActiveRecord.reading_role) do
+            ActiveRecord::Base.connected_to(role: ActiveRecord.reading_role, prevent_writes: true) do
               instrumenter.instrument("database_selector.active_record.read_from_replica") do
                 yield
               end

activerecord/test/cases/base_test.rb

@@ -1721,7 +1721,7 @@ def test_protected_environments_are_stored_as_an_array_of_string
     SecondAbstractClass.connecting_to(role: :reading)
 
     assert SecondAbstractClass.connected_to?(role: :reading)
-    assert_not SecondAbstractClass.current_preventing_writes
+    assert SecondAbstractClass.current_preventing_writes
   ensure
     ActiveRecord::Base.connected_to_stack.pop
   end
@@ -1777,24 +1777,24 @@ def test_protected_environments_are_stored_as_an_array_of_string
     end
   end
 
-  test "#connected_to_many does not sets prevent_writes if role is reading" do
+  test "#connected_to_many sets prevent_writes if role is reading" do
     ActiveRecord::Base.connected_to_many([SecondAbstractClass], role: :reading) do
-      assert_not SecondAbstractClass.current_preventing_writes
+      assert SecondAbstractClass.current_preventing_writes
       assert_not ActiveRecord::Base.current_preventing_writes
     end
   end
 
-  test "#connected_to_many with a single argument for classes does not set prevent_writes" do
+  test "#connected_to_many with a single argument for classes" do
     ActiveRecord::Base.connected_to_many(SecondAbstractClass, role: :reading) do
-      assert_not SecondAbstractClass.current_preventing_writes
+      assert SecondAbstractClass.current_preventing_writes
       assert_not ActiveRecord::Base.current_preventing_writes
     end
   end
 
-  test "#connected_to_many with a multiple classes without brackets does not prevent_writes" do
+  test "#connected_to_many with a multiple classes without brackets works" do
     ActiveRecord::Base.connected_to_many(FirstAbstractClass, SecondAbstractClass, role: :reading) do
-      assert_not FirstAbstractClass.current_preventing_writes
-      assert_not SecondAbstractClass.current_preventing_writes
+      assert FirstAbstractClass.current_preventing_writes
+      assert SecondAbstractClass.current_preventing_writes
       assert_not ActiveRecord::Base.current_preventing_writes
     end
   end

activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb

@@ -130,7 +130,7 @@ def test_switching_connections_via_handler
             assert_equal :reading, ActiveRecord::Base.current_role
             assert ActiveRecord::Base.connected_to?(role: :reading)
             assert_not ActiveRecord::Base.connected_to?(role: :writing)
-            assert_not_predicate ActiveRecord::Base.connection, :preventing_writes?
+            assert_predicate ActiveRecord::Base.connection, :preventing_writes?
           end
 
           ActiveRecord::Base.connected_to(role: :writing) do

activerecord/test/cases/connection_adapters/connection_handlers_sharding_db_test.rb

@@ -135,7 +135,7 @@ def test_switching_connections_via_handler
             assert_not ActiveRecord::Base.connected_to?(role: :writing, shard: :default)
             assert_not ActiveRecord::Base.connected_to?(role: :writing, shard: :shard_one)
             assert_not ActiveRecord::Base.connected_to?(role: :reading, shard: :shard_one)
-            assert_not_predicate ActiveRecord::Base.connection, :preventing_writes?
+            assert_predicate ActiveRecord::Base.connection, :preventing_writes?
           end
 
           ActiveRecord::Base.connected_to(role: :writing, shard: :default) do
@@ -153,7 +153,7 @@ def test_switching_connections_via_handler
             assert_not ActiveRecord::Base.connected_to?(role: :writing, shard: :shard_one)
             assert_not ActiveRecord::Base.connected_to?(role: :writing, shard: :default)
             assert_not ActiveRecord::Base.connected_to?(role: :reading, shard: :default)
-            assert_not_predicate ActiveRecord::Base.connection, :preventing_writes?
+            assert_predicate ActiveRecord::Base.connection, :preventing_writes?
           end
 
           ActiveRecord::Base.connected_to(role: :writing, shard: :shard_one) do

activerecord/test/cases/connection_adapters/connection_swapping_nested_test.rb

@@ -391,23 +391,23 @@ def test_prevent_writes_can_be_changed_granularly
 
             # Switch only primary to reading
             PrimaryBase.connected_to(role: :reading) do
-              assert_not_predicate PrimaryBase.connection, :preventing_writes?
+              assert_predicate PrimaryBase.connection, :preventing_writes?
               assert_not_predicate SecondaryBase.connection, :preventing_writes?
 
               # Switch global to reading
               ActiveRecord::Base.connected_to(role: :reading) do
-                assert_not_predicate PrimaryBase.connection, :preventing_writes?
-                assert_not_predicate SecondaryBase.connection, :preventing_writes?
+                assert_predicate PrimaryBase.connection, :preventing_writes?
+                assert_predicate SecondaryBase.connection, :preventing_writes?
 
                 # Switch only secondary to writing
                 SecondaryBase.connected_to(role: :writing) do
-                  assert_not_predicate PrimaryBase.connection, :preventing_writes?
+                  assert_predicate PrimaryBase.connection, :preventing_writes?
                   assert_not_predicate SecondaryBase.connection, :preventing_writes?
                 end
 
                 # Ensure restored to global reading
-                assert_not_predicate PrimaryBase.connection, :preventing_writes?
-                assert_not_predicate SecondaryBase.connection, :preventing_writes?
+                assert_predicate PrimaryBase.connection, :preventing_writes?
+                assert_predicate SecondaryBase.connection, :preventing_writes?
               end
 
               # Switch everything to writing
@@ -416,7 +416,7 @@ def test_prevent_writes_can_be_changed_granularly
                 assert_not_predicate SecondaryBase.connection, :preventing_writes?
               end
 
-              assert_not_predicate PrimaryBase.connection, :preventing_writes?
+              assert_predicate PrimaryBase.connection, :preventing_writes?
               assert_not_predicate SecondaryBase.connection, :preventing_writes?
             end
 
@@ -444,7 +444,7 @@ def test_application_record_prevent_writes_can_be_changed
             assert_not_predicate ApplicationRecord.connection, :preventing_writes?
 
             ApplicationRecord.connected_to(role: :reading) do
-              assert_not_predicate ApplicationRecord.connection, :preventing_writes?
+              assert_predicate ApplicationRecord.connection, :preventing_writes?
             end
           end
         ensure

activerecord/test/cases/connection_adapters/legacy_connection_handlers_multi_db_test.rb

@@ -180,7 +180,7 @@ def test_switching_connections_via_handler
             assert_equal :reading, ActiveRecord::Base.current_role
             assert ActiveRecord::Base.connected_to?(role: :reading)
             assert_not ActiveRecord::Base.connected_to?(role: :writing)
-            assert_not_predicate ActiveRecord::Base.connection, :preventing_writes?
+            assert_predicate ActiveRecord::Base.connection, :preventing_writes?
           end
 
           ActiveRecord::Base.connected_to(role: :writing) do

activerecord/test/cases/connection_adapters/legacy_connection_handlers_sharding_db_test.rb

@@ -150,7 +150,7 @@ def test_switching_connections_via_handler
             assert_not ActiveRecord::Base.connected_to?(role: :writing, shard: :default)
             assert_not ActiveRecord::Base.connected_to?(role: :writing, shard: :shard_one)
             assert_not ActiveRecord::Base.connected_to?(role: :reading, shard: :shard_one)
-            assert_not_predicate ActiveRecord::Base.connection, :preventing_writes?
+            assert_predicate ActiveRecord::Base.connection, :preventing_writes?
           end
 
           ActiveRecord::Base.connected_to(role: :writing, shard: :default) do
@@ -172,7 +172,7 @@ def test_switching_connections_via_handler
             assert_not ActiveRecord::Base.connected_to?(role: :writing, shard: :shard_one)
             assert_not ActiveRecord::Base.connected_to?(role: :writing, shard: :default)
             assert_not ActiveRecord::Base.connected_to?(role: :reading, shard: :default)
-            assert_not_predicate ActiveRecord::Base.connection, :preventing_writes?
+            assert_predicate ActiveRecord::Base.connection, :preventing_writes?
           end
 
           ActiveRecord::Base.connected_to(role: :writing, shard: :shard_one) do