From 5d36b04986f8a89d0f68faa3913dd6120feb5245 Mon Sep 17 00:00:00 2001 From: eileencodes Date: Fri, 11 Sep 2020 16:36:42 -0400 Subject: [PATCH] Ensure `connects_to` can only be called on base or abstract classes `connectes_to` should only be called on `ActiveRecord::Base` or abstract classes. This is recommended in the documentation but until now was not enforced by the code. It's unsafe to open too many connections to mysql (and probably other databases), so it's safest to have 1 class for the connection and subclass from that. Co-authored-by: John Crepezzi --- activerecord/CHANGELOG.md | 6 +++++ .../lib/active_record/connection_handling.rb | 2 ++ activerecord/test/cases/base_test.rb | 8 +++++++ .../connection_handlers_multi_db_test.rb | 10 ++++++--- .../connection_handlers_sharding_db_test.rb | 22 +++++++++++++------ 5 files changed, 38 insertions(+), 10 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 94729c9db5..c7d6186493 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,9 @@ +* `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. diff --git a/activerecord/lib/active_record/connection_handling.rb b/activerecord/lib/active_record/connection_handling.rb index 8ddf8dc183..62e588f2bf 100644 --- a/activerecord/lib/active_record/connection_handling.rb +++ b/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 diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index 1f3f2b28ad..8846d9830d 100644 --- a/activerecord/test/cases/base_test.rb +++ b/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) { } 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 d184af3b7a..5cab75c703 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 @@ -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))") 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 6d0e17320a..aea3b902df 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 @@ -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" } } } -- GitLab