Merge pull request #39219 from kamipo/bind_param

Should not substitute binds when `prepared_statements: true`

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

@@ -185,12 +185,16 @@ def column_name_with_order_matcher # :nodoc:
       private
         def type_casted_binds(binds)
           case binds.first
-          when ActiveModel::Attribute
-            binds.map { |attr| type_cast(attr.value_for_database) }
           when Array
             binds.map { |column, value| type_cast(value, column) }
           else
-            binds.map { |value| type_cast(value) }
+            binds.map do |value|
+              if ActiveModel::Attribute === value
+                type_cast(value.value_for_database)
+              else
+                type_cast(value)
+              end
+            end
           end
         end
 

activerecord/lib/active_record/explain.rb

@@ -37,13 +37,18 @@ def str.inspect
 
     private
       def render_bind(attr)
-        value = if attr.type.binary? && attr.value
-          "<#{attr.value_for_database.to_s.bytesize} bytes of binary data>"
+        if ActiveModel::Attribute === attr
+          value = if attr.type.binary? && attr.value
+            "<#{attr.value_for_database.to_s.bytesize} bytes of binary data>"
+          else
+            connection.type_cast(attr.value_for_database)
+          end
         else
-          connection.type_cast(attr.value_for_database)
+          value = connection.type_cast(attr)
+          attr  = nil
         end
 
-        [attr.name, value]
+        [attr&.name, value]
       end
   end
 end

activerecord/lib/active_record/statement_cache.rb

@@ -50,8 +50,13 @@ def initialize(values)
 
       def sql_for(binds, connection)
         val = @values.dup
-        casted_binds = binds.map(&:value_for_database)
-        @indexes.each { |i| val[i] = connection.quote(casted_binds.shift) }
+        @indexes.each do |i|
+          value = binds.shift
+          if ActiveModel::Attribute === value
+            value = value.value_for_database
+          end
+          val[i] = connection.quote(value)
+        end
         val.join
       end
     end
@@ -75,6 +80,15 @@ def add_bind(obj)
         self
       end
 
+      def add_binds(binds)
+        @binds.concat binds
+        binds.size.times do |i|
+          @parts << ", " unless i == 0
+          @parts << Substitute.new
+        end
+        self
+      end
+
       def value
         [@parts, @binds]
       end
@@ -102,7 +116,7 @@ def initialize(bound_attributes)
         @bound_attributes = bound_attributes
 
         bound_attributes.each_with_index do |attr, i|
-          if Substitute === attr.value
+          if ActiveModel::Attribute === attr && Substitute === attr.value
             @indexes << i
           end
         end

activerecord/lib/arel/collectors/bind.rb

@@ -16,6 +16,11 @@ def add_bind(bind)
         self
       end
 
+      def add_binds(binds)
+        @binds.concat binds
+        self
+      end
+
       def value
         @binds
       end

activerecord/lib/arel/collectors/composite.rb

@@ -22,6 +22,12 @@ def add_bind(bind, &block)
         self
       end
 
+      def add_binds(binds, &block)
+        left.add_binds(binds, &block)
+        right.add_binds(binds, &block)
+        self
+      end
+
       def value
         [left.value, right.value]
       end

activerecord/lib/arel/collectors/sql_string.rb

@@ -17,6 +17,11 @@ def add_bind(bind)
         @bind_index += 1
         self
       end
+
+      def add_binds(binds, &block)
+        self << (@bind_index...@bind_index += binds.size).map(&block).join(", ")
+        self
+      end
     end
   end
 end

activerecord/lib/arel/collectors/substitute_binds.rb

@@ -20,6 +20,10 @@ def add_bind(bind)
         self << quoter.quote(bind)
       end
 
+      def add_binds(binds)
+        self << binds.map { |bind| quoter.quote(bind) }.join(", ")
+      end
+
       def value
         delegate.value
       end

activerecord/lib/arel/visitors/postgresql.rb

@@ -41,10 +41,6 @@ def visit_Arel_Nodes_DistinctOn(o, collector)
           visit(o.expr, collector) << " )"
         end
 
-        def visit_Arel_Nodes_BindParam(o, collector)
-          collector.add_bind(o.value) { |i| "$#{i}" }
-        end
-
         def visit_Arel_Nodes_GroupingElement(o, collector)
           collector << "( "
           visit(o.expr, collector) << " )"
@@ -92,6 +88,11 @@ def visit_Arel_Nodes_NullsLast(o, collector)
           collector << " NULLS LAST"
         end
 
+        BIND_BLOCK = proc { |i| "$#{i}" }
+        private_constant :BIND_BLOCK
+
+        def bind_block; BIND_BLOCK; end
+
         # Used by Lateral visitor to enclose select queries in parentheses
         def grouping_parentheses(o, collector)
           if o.expr.is_a? Nodes::SelectStatement

activerecord/lib/arel/visitors/to_sql.rb

@@ -332,15 +332,14 @@ def visit_Arel_Nodes_HomogeneousIn(o, collector)
             collector << " NOT IN ("
           end
 
-          values = o.casted_values.map { |v| @connection.quote(v) }
+          values = o.casted_values
 
-          expr = if values.empty?
-            @connection.quote(nil)
+          if values.empty?
+            collector << @connection.quote(nil)
           else
-            values.join(",")
+            collector.add_binds(values, &bind_block)
           end
 
-          collector << expr
           collector << ")"
           collector
         end
@@ -691,8 +690,13 @@ def visit_Arel_Attributes_Attribute(o, collector)
           collector << quote_table_name(join_name) << "." << quote_column_name(o.name)
         end
 
+        BIND_BLOCK = proc { "?" }
+        private_constant :BIND_BLOCK
+
+        def bind_block; BIND_BLOCK; end
+
         def visit_Arel_Nodes_BindParam(o, collector)
-          collector.add_bind(o.value) { "?" }
+          collector.add_bind(o.value, &bind_block)
         end
 
         def visit_Arel_Nodes_SqlLiteral(o, collector)

activerecord/test/cases/bind_parameter_test.rb

@@ -164,7 +164,57 @@ def test_logs_legacy_binds_after_type_cast
         end
       end
 
+      def test_bind_params_to_sql_with_prepared_statements
+        assert_bind_params_to_sql
+      end
+
+      def test_bind_params_to_sql_with_unprepared_statements
+        @connection.unprepared_statement do
+          assert_bind_params_to_sql
+        end
+      end
+
       private
+        def assert_bind_params_to_sql
+          table = Author.quoted_table_name
+          pk = "#{table}.#{Author.quoted_primary_key}"
+
+          # prepared_statements: true
+          #
+          #   SELECT `authors`.* FROM `authors` WHERE (`authors`.`id` IN (?, ?, ?) OR `authors`.`id` IS NULL)
+          #
+          # prepared_statements: false
+          #
+          #   SELECT `authors`.* FROM `authors` WHERE (`authors`.`id` IN (1, 2, 3) OR `authors`.`id` IS NULL)
+          #
+          sql = "SELECT #{table}.* FROM #{table} WHERE (#{pk} IN (#{bind_params(1..3)}) OR #{pk} IS NULL)"
+
+          authors = Author.where(id: [1, 2, 3, nil])
+          assert_equal sql, @connection.to_sql(authors.arel)
+          assert_sql(sql) { assert_equal 3, authors.length }
+
+          # prepared_statements: true
+          #
+          #   SELECT `authors`.* FROM `authors` WHERE `authors`.`id` IN (?, ?, ?)
+          #
+          # prepared_statements: false
+          #
+          #   SELECT `authors`.* FROM `authors` WHERE `authors`.`id` IN (1, 2, 3)
+          #
+          sql = "SELECT #{table}.* FROM #{table} WHERE #{pk} IN (#{bind_params(1..3)})"
+
+          authors = Author.where(id: [1, 2, 3, 9223372036854775808])
+          assert_equal sql, @connection.to_sql(authors.arel)
+          assert_sql(sql) { assert_equal 3, authors.length }
+        end
+
+        def bind_params(ids)
+          collector = @connection.send(:collector)
+          bind_params = ids.map { |i| Arel::Nodes::BindParam.new(i) }
+          sql, _ = @connection.visitor.compile(bind_params, collector)
+          sql
+        end
+
         def to_sql_key(arel)
           sql = @connection.to_sql(arel)
           @connection.respond_to?(:sql_key, true) ? @connection.send(:sql_key, sql) : sql