5-0-stable: Fix select_all with legacy binds

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

@@ -155,6 +155,14 @@ def prepare_binds_for_database(binds) # :nodoc:
         binds.map(&:value_for_database)
       end
 
+      def type_casted_binds(binds) # :nodoc:
+        if binds.first.is_a?(Array)
+          binds.map { |column, value| type_cast(value, column) }
+        else
+          binds.map { |attr| type_cast(attr.value_for_database) }
+        end
+      end
+
       private
 
       def types_which_need_no_typecasting

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

@@ -579,14 +579,15 @@ def translate_exception_class(e, sql)
         exception
       end
 
-      def log(sql, name = "SQL", binds = [], statement_name = nil)
+      def log(sql, name = "SQL", binds = [], type_casted_binds = [], statement_name = nil)
         @instrumenter.instrument(
           "sql.active_record",
-          :sql            => sql,
-          :name           => name,
-          :connection_id  => object_id,
-          :statement_name => statement_name,
-          :binds          => binds) { yield }
+          sql:               sql,
+          name:              name,
+          binds:             binds,
+          type_casted_binds: type_casted_binds,
+          statement_name:    statement_name,
+          connection_id:     object_id) { yield }
       rescue => e
         raise translate_exception_class(e, sql)
       end

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

@@ -73,9 +73,9 @@ def exec_stmt_and_free(sql, name, binds, cache_stmt: false)
           # made since we established the connection
           @connection.query_options[:database_timezone] = ActiveRecord::Base.default_timezone
 
-          type_casted_binds = binds.map { |attr| type_cast(attr.value_for_database) }
+          type_casted_binds = type_casted_binds(binds)
 
-          log(sql, name, binds) do
+          log(sql, name, binds, type_casted_binds) do
             if cache_stmt
               cache = @statements[sql] ||= {
                 stmt: @connection.prepare(sql)

activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb

@@ -594,15 +594,15 @@ def execute_and_clear(sql, name, binds, prepare: false)
         end
 
         def exec_no_cache(sql, name, binds)
-          type_casted_binds = binds.map { |attr| type_cast(attr.value_for_database) }
-          log(sql, name, binds) { @connection.async_exec(sql, type_casted_binds) }
+          type_casted_binds = type_casted_binds(binds)
+          log(sql, name, binds, type_casted_binds) { @connection.async_exec(sql, type_casted_binds) }
         end
 
         def exec_cache(sql, name, binds)
           stmt_key = prepare_statement(sql)
-          type_casted_binds = binds.map { |attr| type_cast(attr.value_for_database) }
+          type_casted_binds = type_casted_binds(binds)
 
-          log(sql, name, binds, stmt_key) do
+          log(sql, name, binds, type_casted_binds, stmt_key) do
             @connection.exec_prepared(stmt_key, type_casted_binds)
           end
         rescue ActiveRecord::StatementInvalid => e

activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb

@@ -188,9 +188,9 @@ def explain(arel, binds = [])
       end
 
       def exec_query(sql, name = nil, binds = [], prepare: false)
-        type_casted_binds = binds.map { |attr| type_cast(attr.value_for_database) }
+        type_casted_binds = type_casted_binds(binds)
 
-        log(sql, name, binds) do
+        log(sql, name, binds, type_casted_binds) do
           # Don't cache statements if they are not prepared
           unless prepare
             stmt    = @connection.prepare(sql)
@@ -203,7 +203,6 @@ def exec_query(sql, name = nil, binds = [], prepare: false)
             ensure
               stmt.close
             end
-            stmt = records
           else
             cache = @statements[sql] ||= {
               :stmt => @connection.prepare(sql)
@@ -212,9 +211,10 @@ def exec_query(sql, name = nil, binds = [], prepare: false)
             cols = cache[:cols] ||= stmt.columns
             stmt.reset!
             stmt.bind_params(type_casted_binds)
+            records = stmt.to_a
           end
 
-          ActiveRecord::Result.new(cols, stmt.to_a)
+          ActiveRecord::Result.new(cols, records)
         end
       end
 

activerecord/lib/active_record/log_subscriber.rb

@@ -20,20 +20,6 @@ def initialize
       @odd = false
     end
 
-    def render_bind(attribute)
-      value = if attribute.type.binary? && attribute.value
-        if attribute.value.is_a?(Hash)
-          "<#{attribute.value_for_database.to_s.bytesize} bytes of binary data>"
-        else
-          "<#{attribute.value.bytesize} bytes of binary data>"
-        end
-      else
-        attribute.value_for_database
-      end
-
-      [attribute.name, value]
-    end
-
     def sql(event)
       self.class.runtime += event.duration
       return unless logger.debug?
@@ -47,7 +33,10 @@ def sql(event)
       binds = nil
 
       unless (payload[:binds] || []).empty?
-        binds = "  " + payload[:binds].map { |attr| render_bind(attr) }.inspect
+        casted_params = type_casted_binds(payload[:binds], payload[:type_casted_binds])
+        binds = "  " + payload[:binds].zip(casted_params).map { |attr, value|
+          render_bind(attr, value)
+        }.inspect
       end
 
       name = colorize_payload_name(name, payload[:name])
@@ -58,6 +47,20 @@ def sql(event)
 
     private
 
+    def type_casted_binds(binds, casted_binds)
+      casted_binds || ActiveRecord::Base.connection.type_casted_binds(binds)
+    end
+
+    def render_bind(attr, value)
+      if attr.is_a?(Array)
+        attr = attr.first
+      elsif attr.type.binary? && attr.value
+        value = "<#{attr.value_for_database.to_s.bytesize} bytes of binary data>"
+      end
+
+      [attr && attr.name, value]
+    end
+
     def colorize_payload_name(name, payload_name)
       if payload_name.blank? || payload_name == "SQL" # SQL vs Model Load/Exists
         color(name, MAGENTA, true)

activerecord/test/cases/adapter_test.rb

@@ -244,6 +244,15 @@ def test_select_all_always_return_activerecord_result
       assert result.is_a?(ActiveRecord::Result)
     end
 
+    if ActiveRecord::Base.connection.prepared_statements
+      def test_select_all_with_legacy_binds
+        post = Post.create!(title: "foo", body: "bar")
+        expected = @connection.select_all("SELECT * FROM posts WHERE id = #{post.id}")
+        result = @connection.select_all("SELECT * FROM posts WHERE id = #{Arel::Nodes::BindParam.new.to_sql}", nil, [[nil, post.id]])
+        assert_equal expected.to_hash, result.to_hash
+      end
+    end
+
     def test_select_methods_passing_a_association_relation
       author = Author.create!(name: 'john')
       Post.create!(author: author, title: 'foo', body: 'bar')

activerecord/test/cases/bind_parameter_test.rb

@@ -3,36 +3,36 @@
 require 'models/author'
 require 'models/post'
 
-module ActiveRecord
-  class BindParameterTest < ActiveRecord::TestCase
-    fixtures :topics, :authors, :posts
+if ActiveRecord::Base.connection.supports_statement_cache? &&
+   ActiveRecord::Base.connection.prepared_statements
+  module ActiveRecord
+    class BindParameterTest < ActiveRecord::TestCase
+      fixtures :topics, :authors, :posts
 
-    class LogListener
-      attr_accessor :calls
+      class LogListener
+        attr_accessor :calls
 
-      def initialize
-        @calls = []
-      end
+        def initialize
+          @calls = []
+        end
 
-      def call(*args)
-        calls << args
+        def call(*args)
+          calls << args
+        end
       end
-    end
 
-    def setup
-      super
-      @connection = ActiveRecord::Base.connection
-      @subscriber = LogListener.new
-      @pk = Topic.columns_hash[Topic.primary_key]
-      @subscription = ActiveSupport::Notifications.subscribe('sql.active_record', @subscriber)
-    end
+      def setup
+        super
+        @connection = ActiveRecord::Base.connection
+        @subscriber = LogListener.new
+        @pk = Topic.columns_hash[Topic.primary_key]
+        @subscription = ActiveSupport::Notifications.subscribe("sql.active_record", @subscriber)
+      end
 
-    teardown do
-      ActiveSupport::Notifications.unsubscribe(@subscription)
-    end
+      def teardown
+        ActiveSupport::Notifications.unsubscribe(@subscription)
+      end
 
-    if ActiveRecord::Base.connection.supports_statement_cache? &&
-       ActiveRecord::Base.connection.prepared_statements
       def test_bind_from_join_in_subquery
         subquery = Author.joins(:thinking_posts).where(name: 'David')
         scope = Author.from(subquery, 'authors').where(id: 1)
@@ -56,34 +56,48 @@ def test_find_one_uses_binds
         assert message, 'expected a message with binds'
       end
 
-      def test_logs_bind_vars_after_type_cast
-        payload = {
-          :name  => 'SQL',
-          :sql   => 'select * from topics where id = ?',
-          :binds => [Relation::QueryAttribute.new("id", "10", Type::Integer.new)]
-        }
-        event  = ActiveSupport::Notifications::Event.new(
-          'foo',
-          Time.now,
-          Time.now,
-          123,
-          payload)
-
-        logger = Class.new(ActiveRecord::LogSubscriber) {
-          attr_reader :debugs
-          def initialize
-            super
-            @debugs = []
-          end
-
-          def debug str
-            @debugs << str
-          end
-        }.new
-
-        logger.sql event
-        assert_match([[@pk.name, 10]].inspect, logger.debugs.first)
+      def test_logs_binds_after_type_cast
+        binds = [Relation::QueryAttribute.new("id", "10", Type::Integer.new)]
+        assert_logs_binds(binds)
+      end
+
+      def test_logs_legacy_binds_after_type_cast
+        binds = [[@pk, "10"]]
+        assert_logs_binds(binds)
       end
+
+      private
+        def assert_logs_binds(binds)
+          payload = {
+            name: "SQL",
+            sql: "select * from topics where id = ?",
+            binds: binds,
+            type_casted_binds: @connection.type_casted_binds(binds)
+          }
+
+          event = ActiveSupport::Notifications::Event.new(
+            "foo",
+            Time.now,
+            Time.now,
+            123,
+            payload)
+
+          logger = Class.new(ActiveRecord::LogSubscriber) {
+            attr_reader :debugs
+
+            def initialize
+              super
+              @debugs = []
+            end
+
+            def debug(str)
+              @debugs << str
+            end
+          }.new
+
+          logger.sql(event)
+          assert_match([[@pk.name, 10]].inspect, logger.debugs.first)
+        end
     end
   end
 end