Merge pull request #40219 from...

Merge pull request #40219 from eileencodes/ensure-connects_to-is-always-called-on-base-or-abstract-classes

Ensure `connects_to` can only be called on base or abstract classes

activerecord/CHANGELOG.md

+*   `connects_to` can only be called on `ActiveRecord::Base` or abstract classes.
+
+    Ensure that `connects_to` can only be called from `ActiveRecord::Base` or abstract classes. This protects the application from opening duplicate or too many connections.
+
+    *Eileen M. Uchitelle*, *John Crepezzi*
+
 *   All connection adapters `execute` now raises `ActiveRecord::ConnectionNotEstablished` rather than
     `ActiveRecord::InvalidStatement` when they encounter a connection error.
 

activerecord/lib/active_record/connection_handling.rb

@@ -79,6 +79,8 @@ def establish_connection(config_or_env = nil)
     #
     # Returns an array of database connections.
     def connects_to(database: {}, shards: {})
+      raise NotImplementedError, "connects_to can only be called on ActiveRecord::Base or abstract classes" unless self == Base || abstract_class?
+
       if database.present? && shards.present?
         raise ArgumentError, "connects_to can only accept a `database` or `shards` argument, but not both arguments."
       end

activerecord/test/cases/base_test.rb

@@ -1661,6 +1661,14 @@ def test_protected_environments_are_stored_as_an_array_of_string
     end
   end
 
+  test "cannot call connects_to on non-abstract or non-ActiveRecord::Base classes" do
+    error = assert_raises(NotImplementedError) do
+      Bird.connects_to(database: { writing: :arunit })
+    end
+
+    assert_equal "connects_to can only be called on ActiveRecord::Base or abstract classes", error.message
+  end
+
   test "cannot call connected_to on subclasses of ActiveRecord::Base" do
     error = assert_raises(NotImplementedError) do
       Bird.connected_to(role: :reading) { }

activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb

@@ -24,7 +24,11 @@ def teardown
         clean_up_connection_handler
       end
 
-      class MultiConnectionTestModel < ActiveRecord::Base
+      class SecondaryBase < ActiveRecord::Base
+        self.abstract_class = true
+      end
+
+      class MultiConnectionTestModel < SecondaryBase
       end
 
       def test_multiple_connection_handlers_works_in_a_threaded_environment
@@ -33,7 +37,7 @@ def test_multiple_connection_handlers_works_in_a_threaded_environment
 
         # 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" } }
+        SecondaryBase.connects_to database: { writing: { database: tf_writing.path, adapter: "sqlite3" }, secondary: { database: tf_reading.path, adapter: "sqlite3" } }
 
         MultiConnectionTestModel.connection.execute("CREATE TABLE `multi_connection_test_models` (connection_role VARCHAR (255))")
         MultiConnectionTestModel.connection.execute("INSERT INTO multi_connection_test_models VALUES ('writing')")
@@ -73,7 +77,7 @@ def test_multiple_connection_handlers_works_in_a_threaded_environment
       def test_loading_relations_with_multi_db_connection_handlers
         # 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: ":memory:", adapter: "sqlite3" }, secondary: { database: ":memory:", adapter: "sqlite3" } }
+        SecondaryBase.connects_to database: { writing: { database: ":memory:", adapter: "sqlite3" }, secondary: { database: ":memory:", adapter: "sqlite3" } }
 
         relation = ActiveRecord::Base.connected_to(role: :secondary) do
           MultiConnectionTestModel.connection.execute("CREATE TABLE `multi_connection_test_models` (connection_role VARCHAR (255))")

activerecord/test/cases/connection_adapters/connection_handlers_sharding_db_test.rb

@@ -264,15 +264,23 @@ def test_calling_connected_to_on_a_non_existent_shard_raises
         end
       end
 
-      class ShardConnectionTestModel < ActiveRecord::Base
+      class SecondaryBase < ActiveRecord::Base
+        self.abstract_class = true
       end
 
-      class ShardConnectionTestModelB < ActiveRecord::Base
+      class ShardConnectionTestModel < SecondaryBase
+      end
+
+      class SomeOtherBase < ActiveRecord::Base
+        self.abstract_class = true
+      end
+
+      class ShardConnectionTestModelB < SomeOtherBase
       end
 
       def test_same_shards_across_clusters
-        ShardConnectionTestModel.connects_to shards: { one: { writing: { database: ":memory:", adapter: "sqlite3" } } }
-        ShardConnectionTestModelB.connects_to shards: { one: { writing: { database: ":memory:", adapter: "sqlite3" } } }
+        SecondaryBase.connects_to shards: { one: { writing: { database: ":memory:", adapter: "sqlite3" } } }
+        SomeOtherBase.connects_to shards: { one: { writing: { database: ":memory:", adapter: "sqlite3" } } }
 
         ActiveRecord::Base.connected_to(shard: :one) do
           ShardConnectionTestModel.connection.execute("CREATE TABLE `shard_connection_test_models` (shard_key VARCHAR (255))")
@@ -287,7 +295,7 @@ def test_same_shards_across_clusters
       end
 
       def test_sharding_separation
-        ShardConnectionTestModel.connects_to shards: {
+        SecondaryBase.connects_to shards: {
           default: { writing: { database: ":memory:", adapter: "sqlite3" } },
           one: { writing: { database: ":memory:", adapter: "sqlite3" } }
         }
@@ -323,7 +331,7 @@ def test_swapping_shards_in_a_multi_threaded_environment
         tf_default = Tempfile.open "shard_key_default"
         tf_shard_one = Tempfile.open "shard_key_one"
 
-        ShardConnectionTestModel.connects_to shards: {
+        SecondaryBase.connects_to shards: {
           default: { writing: { database: tf_default.path, adapter: "sqlite3" } },
           one: { writing: { database: tf_shard_one.path, adapter: "sqlite3" } }
         }
@@ -368,7 +376,7 @@ def test_swapping_shards_and_roles_in_a_multi_threaded_environment
         tf_default_reading = Tempfile.open "shard_key_default_reading"
         tf_shard_one_reading = Tempfile.open "shard_key_one_reading"
 
-        ShardConnectionTestModel.connects_to shards: {
+        SecondaryBase.connects_to shards: {
           default: { writing: { database: tf_default.path, adapter: "sqlite3" }, secondary: { database: tf_default_reading.path, adapter: "sqlite3" } },
           one: { writing: { database: tf_shard_one.path, adapter: "sqlite3" }, secondary: { database: tf_shard_one_reading.path, adapter: "sqlite3" } }
         }