Fix prepared statements caching to be enabled even when query caching is enabled

Related cbcdecd, 2a56b2d.

This is a regression caused by cbcdecd.

If query caching is enabled, prepared statement handles are never
re-used, since we missed that a query is preprocessed when query caching
is enabled, but doesn't keep the `preparable` flag.

We should care about that case.

activerecord/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Fix prepared statements caching to be enabled even when query caching is enabled.
+
+    *Ryuta Kamizono*
+
 *   Ensure `update_all` series cares about optimistic locking.
 
     *Ryuta Kamizono*

activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb

@@ -20,9 +20,22 @@ module ActiveRecord
             raise "Passing bind parameters with an arel AST is forbidden. " \
               "The values must be stored on the AST directly"
           end
-          sql, binds = visitor.compile(arel_or_sql_string.ast, collector)
-          [sql.freeze, binds || []]
+
+          if prepared_statements
+            sql, binds = visitor.compile(arel_or_sql_string.ast, collector)
+
+            if binds.length > bind_params_length
+              unprepared_statement do
+                sql, binds = to_sql_and_binds(arel_or_sql_string)
+                visitor.preparable = false
+              end
+            end
+          else
+            sql = visitor.compile(arel_or_sql_string.ast, collector)
+          end
+          [sql.freeze, binds]
         else
+          visitor.preparable = false if prepared_statements
           [arel_or_sql_string.dup.freeze, binds]
         end
       end
@@ -47,13 +60,8 @@ module ActiveRecord
         arel = arel_from_relation(arel)
         sql, binds = to_sql_and_binds(arel, binds)
 
-        if !prepared_statements || (arel.is_a?(String) && preparable.nil?)
-          preparable = false
-        elsif binds.length > bind_params_length
-          sql, binds = unprepared_statement { to_sql_and_binds(arel) }
-          preparable = false
-        else
-          preparable = visitor.preparable
+        if preparable.nil?
+          preparable = prepared_statements ? visitor.preparable : false
         end
 
         if prepared_statements && preparable

activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb

@@ -97,9 +97,8 @@ module ActiveRecord
           arel = arel_from_relation(arel)
           sql, binds = to_sql_and_binds(arel, binds)
 
-          if binds.length > bind_params_length
-            sql, binds = unprepared_statement { to_sql_and_binds(arel) }
-            preparable = false
+          if preparable.nil?
+            preparable = prepared_statements ? visitor.preparable : false
           end
 
           cache_sql(sql, name, binds) { super(sql, name, binds, preparable: preparable) }

activerecord/lib/active_record/connection_adapters/determine_if_preparable_visitor.rb

@@ -3,7 +3,7 @@
 module ActiveRecord
   module ConnectionAdapters
     module DetermineIfPreparableVisitor
-      attr_reader :preparable
+      attr_accessor :preparable
 
       def accept(*)
         @preparable = true

activerecord/test/cases/bind_parameter_test.rb

@@ -34,6 +34,49 @@ if ActiveRecord::Base.connection.prepared_statements
         ActiveSupport::Notifications.unsubscribe(@subscription)
       end
 
+      def test_statement_cache
+        @connection.clear_cache!
+
+        topics = Topic.where(id: 1)
+        assert_equal [1], topics.map(&:id)
+        assert_includes statement_cache, to_sql_key(topics.arel)
+      end
+
+      def test_statement_cache_with_query_cache
+        @connection.enable_query_cache!
+        @connection.clear_cache!
+
+        topics = Topic.where(id: 1)
+        assert_equal [1], topics.map(&:id)
+        assert_includes statement_cache, to_sql_key(topics.arel)
+      ensure
+        @connection.disable_query_cache!
+      end
+
+      def test_statement_cache_with_find
+        @connection.clear_cache!
+
+        topics = Topic.where(id: 1).limit(1)
+        assert_equal 1, Topic.find(1).id
+        assert_includes statement_cache, to_sql_key(topics.arel)
+      end
+
+      def test_statement_cache_with_in_clause
+        @connection.clear_cache!
+
+        topics = Topic.where(id: [1, 3])
+        assert_equal [1, 3], topics.map(&:id)
+        assert_not_includes statement_cache, to_sql_key(topics.arel)
+      end
+
+      def test_statement_cache_with_sql_string_literal
+        @connection.clear_cache!
+
+        topics = Topic.where("topics.id = ?", 1)
+        assert_equal [1], topics.map(&:id)
+        assert_not_includes statement_cache, to_sql_key(topics.arel)
+      end
+
       def test_too_many_binds
         bind_params_length = @connection.send(:bind_params_length)
 
@@ -45,7 +88,8 @@ if ActiveRecord::Base.connection.prepared_statements
       end
 
       def test_too_many_binds_with_query_cache
-        Topic.connection.enable_query_cache!
+        @connection.enable_query_cache!
+
         bind_params_length = @connection.send(:bind_params_length)
         topics = Topic.where(id: (1 .. bind_params_length + 1).to_a)
         assert_equal Topic.count, topics.count
@@ -53,7 +97,7 @@ if ActiveRecord::Base.connection.prepared_statements
         topics = Topic.where.not(id: (1 .. bind_params_length + 1).to_a)
         assert_equal 0, topics.count
       ensure
-        Topic.connection.disable_query_cache!
+        @connection.disable_query_cache!
       end
 
       def test_bind_from_join_in_subquery
@@ -90,6 +134,15 @@ if ActiveRecord::Base.connection.prepared_statements
       end
 
       private
+        def to_sql_key(arel)
+          sql = @connection.to_sql(arel)
+          @connection.respond_to?(:sql_key, true) ? @connection.send(:sql_key, sql) : sql
+        end
+
+        def statement_cache
+          @connection.instance_variable_get(:@statements).send(:cache)
+        end
+
         def assert_logs_binds(binds)
           payload = {
             name: "SQL",