Passing klass to StatementCache.new

[kamipo]

Actually StatementCache#execute is always passed the same klass that
the owner klass of the connection when the statement cache is created.
So passing klass to StatementCache.new will make more DRY.

[matthewd]

It's definitely not safe to keep connection, but klass seems possible. Does it create problems around class reloading, though?

[kamipo]

Yeah, keeping klass is safe because it doesn't reach to the cache before reloaded if the klass is reloaded.

% ARCONN=sqlite3 be ruby -w -Itest test/cases/statement_cache_test.rb -n test_class_reloading
Using sqlite3
Run options: -n test_class_reloading --seed 52455

# Running:

{:object_id=>70143223977420}
{:object_id=>70143214190320}
{:object_id=>70143216979300}
{:object_id=>70143215224780}
.

Finished in 0.311839s, 3.2068 runs/s, 12.8271 assertions/s.
1 runs, 4 assertions, 0 failures, 0 errors, 0 skips

--- a/activerecord/lib/active_record/statement_cache.rb
+++ b/activerecord/lib/active_record/statement_cache.rb
@@ -106,6 +106,7 @@ def execute(params, &block)
       binds = bind_map.bind(params)
       sql = query_builder.sql_for(binds, klass.connection)
 
+      p object_id: klass.object_id
       klass.find_by_sql(sql, binds, preparable: true, &block)
     end
 
diff --git a/activerecord/test/cases/statement_cache_test.rb b/activerecord/test/cases/statement_cache_test.rb
index cbac1bdbd9..05bc059e32 100644
--- a/activerecord/test/cases/statement_cache_test.rb
+++ b/activerecord/test/cases/statement_cache_test.rb
@@ -1,7 +1,6 @@
 # frozen_string_literal: true
 
 require "cases/helper"
-require "models/book"
 require "models/liquid"
 require "models/molecule"
 require "models/electron"
@@ -12,6 +11,24 @@ def setup
       @connection = ActiveRecord::Base.connection
     end
 
+    def test_class_reloading
+      require "active_support/dependencies"
+      ActiveSupport::Dependencies.autoload_paths << "test/models"
+
+      author = Author.create(name: "foo")
+      author.books << Book.create(name: "my book")
+
+      book = Book.find_by(name: "my book")
+      assert_equal "my book", book.name
+      assert_equal "foo", book.author.name
+
+      ActiveSupport::Dependencies.clear
+
+      book = Book.find_by(name: "my book")
+      assert_equal "my book", book.name
+      assert_equal "foo", book.author.name
+    end
+

[sgrif]

Given the work being done to reduce the number of places which rely on thread-local/class level connections, I would prefer not to do that here.

[kamipo]

Current implementation in this PR is passing klass to StatementCache.create and using klass.connection in StatementCache#execute. I think that it is sufficient to change 3 lines to be able to use another connection.

--- a/activerecord/lib/active_record/statement_cache.rb
+++ b/activerecord/lib/active_record/statement_cache.rb
@@ -102,11 +102,11 @@ def initialize(query_builder, bind_map, klass)
       @klass = klass
     end
 
-    def execute(params, &block)
+    def execute(params, connection = klass.connection, &block)
       binds = bind_map.bind(params)
-      sql = query_builder.sql_for(binds, klass.connection)
+      sql = query_builder.sql_for(binds, connection)
 
-      klass.find_by_sql(sql, binds, preparable: true, &block)
+      klass.find_by_sql(sql, binds, preparable: true, connection: connection, &block)
     end

[sgrif]

I'd still prefer to have the connection explicitly passed instead of optional. Otherwise it's too easy to accidentally rely on the thread local connection when you didn't mean to.

[kamipo]

Okay, I changed to keep the connection explicitly passed, and only change to passing klass from constructed relation to StatementCache.new.

Is it okay now?

[sgrif]

Won't this cause a problem with STI? It looks like the cache is shared between the parent and subclasses. Unless I'm missing something, this would break if the statement gets cached using the child class and then executed by the parent?

[kamipo]

The statement cache is not shared between the parent and subclasses.

https://github.com/rails/rails/blob/v5.1.3.rc3/activerecord/lib/active_record/core.rb#L164

% ARCONN=sqlite3 be ruby -w -Itest test/cases/statement_cache_test.rb -n test_klass_has_dedicated_find_by_statement_cache
Using sqlite3
Run options: -n test_klass_has_dedicated_find_by_statement_cache --seed 13379

# Running:

.

Finished in 0.002533s, 394.8614 runs/s, 394.8614 assertions/s.
1 runs, 1 assertions, 0 failures, 0 errors, 0 skips

--- a/activerecord/test/cases/statement_cache_test.rb
+++ b/activerecord/test/cases/statement_cache_test.rb
@@ -5,6 +5,7 @@
 require "models/liquid"
 require "models/molecule"
 require "models/electron"
+require "models/post"
 
 module ActiveRecord
   class StatementCacheTest < ActiveRecord::TestCase
@@ -12,6 +13,13 @@ def setup
       @connection = ActiveRecord::Base.connection
     end
 
+    def test_klass_has_dedicated_find_by_statement_cache
+      post_cache = Post.instance_variable_get(:@find_by_statement_cache)
+      special_post_cache = SpecialPost.instance_variable_get(:@find_by_statement_cache)
+
+      assert_not_same post_cache, special_post_cache
+    end
+

[sgrif]

👍