ActiveRecord::Core: improve find by cache hit rate

Prevents cases where `Model.find(1)` and `Model.find_by(id: 1)` results
in two caches entries, with the respective cache keys `"id"` and
`["id"]`.

This is achieved by treating the `find` use case similarly to find_by
with a single pair of attribute/value, namely the primary key and the
provided value.

activerecord/lib/active_record/core.rb

@@ -274,14 +274,8 @@ module ActiveRecord
 
         return super if StatementCache.unsupported_value?(id)
 
-        key = primary_key
-
-        statement = cached_find_by_statement(key) { |params|
-          where(key => params.bind).limit(1)
-        }
-
-        statement.execute([id], connection).first ||
-          raise(RecordNotFound.new("Couldn't find #{name} with '#{key}'=#{id}", name, key, id))
+        cached_find_by([primary_key], [id]) ||
+          raise(RecordNotFound.new("Couldn't find #{name} with '#{primary_key}'=#{id}", name, primary_key, id))
       end
 
       def find_by(*args) # :nodoc:
@@ -313,17 +307,7 @@ module ActiveRecord
           h[key] = value
         end
 
-        keys = hash.keys
-        statement = cached_find_by_statement(keys) { |params|
-          wheres = keys.index_with { params.bind }
-          where(wheres).limit(1)
-        }
-
-        begin
-          statement.execute(hash.values, connection).first
-        rescue TypeError
-          raise ActiveRecord::StatementInvalid
-        end
+        cached_find_by(hash.keys, hash.values)
       end
 
       def find_by!(*args) # :nodoc:
@@ -448,6 +432,19 @@ module ActiveRecord
         def table_metadata
           TableMetadata.new(self, arel_table)
         end
+
+        def cached_find_by(keys, values)
+          statement = cached_find_by_statement(keys) { |params|
+            wheres = keys.index_with { params.bind }
+            where(wheres).limit(1)
+          }
+
+          begin
+            statement.execute(values, connection).first
+          rescue TypeError
+            raise ActiveRecord::StatementInvalid
+          end
+        end
     end
 
     # New objects can be instantiated as either empty (pass no construction parameter) or pre-set with

activerecord/test/cases/bind_parameter_test.rb

@@ -63,10 +63,10 @@ if ActiveRecord::Base.connection.prepared_statements
         assert_equal 1, Topic.find(1).id
         assert_raises(RecordNotFound) { SillyReply.find(2) }
 
-        topic_sql = cached_statement(Topic, Topic.primary_key)
+        topic_sql = cached_statement(Topic, [Topic.primary_key])
         assert_includes statement_cache, to_sql_key(topic_sql)
 
-        reply_sql = cached_statement(SillyReply, SillyReply.primary_key)
+        reply_sql = cached_statement(SillyReply, [SillyReply.primary_key])
         assert_includes statement_cache, to_sql_key(reply_sql)
 
         replies = SillyReply.where(id: 2).limit(1)

activerecord/test/cases/core_test.rb

@@ -133,4 +133,17 @@ class CoreTest < ActiveRecord::TestCase
     PP.pp(topic, StringIO.new(actual))
     assert_match(/id: 1/, actual)
   end
+
+  def test_find_by_cache_does_not_duplicate_entries
+    Topic.initialize_find_by_cache
+    using_prepared_statements = Topic.connection.prepared_statements
+    topic_find_by_cache = Topic.instance_variable_get("@find_by_statement_cache")[using_prepared_statements]
+
+    assert_difference -> { topic_find_by_cache.size }, +1 do
+      Topic.find(1)
+    end
+    assert_no_difference -> { topic_find_by_cache.size } do
+      Topic.find_by(id: 1)
+    end
+  end
 end