Merge pull request #39106 from kamipo/deprecate_passing_column_to_type_cast

Deprecate passing a column to `type_cast`

activerecord/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Deprecate passing a column to `type_cast`.
+
+    *Ryuta Kamizono*
+
 *   Deprecate `in_clause_length` and `allowed_index_name_length` in `DatabaseLimits`.
 
     *Ryuta Kamizono*

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

@@ -21,7 +21,11 @@ module ActiveRecord
         value = id_value_for_database(value) if value.is_a?(Base)
 
         if column
-          value = type_cast_from_column(column, value)
+          ActiveSupport::Deprecation.warn(<<~MSG.squish)
+            Passing a column to `type_cast` is deprecated and will be removed in Rails 6.2.
+          MSG
+          type = lookup_cast_type_from_column(column)
+          value = type.serialize(value)
         end
 
         _type_cast(value)
@@ -39,16 +43,6 @@ module ActiveRecord
       # represent the type doesn't sufficiently reflect the differences
       # (varchar vs binary) for example. The type used to get this primitive
       # should have been provided before reaching the connection adapter.
-      def type_cast_from_column(column, value) # :nodoc:
-        if column
-          type = lookup_cast_type_from_column(column)
-          type.serialize(value)
-        else
-          value
-        end
-      end
-
-      # See docs for #type_cast_from_column
       def lookup_cast_type_from_column(column) # :nodoc:
         lookup_cast_type(column.sql_type)
       end
@@ -193,10 +187,13 @@ module ActiveRecord
 
       private
         def type_casted_binds(binds)
-          if binds.first.is_a?(Array)
+          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 { |attr| type_cast(attr.value_for_database) }
+            binds.map { |value| type_cast(value) }
           end
         end
 

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

@@ -67,8 +67,8 @@ module ActiveRecord
           elsif column.type == :uuid && value.is_a?(String) && /\(\)/.match?(value)
             value # Does not quote function default values for UUID columns
           elsif column.respond_to?(:array?)
-            value = type_cast_from_column(column, value)
-            quote(value)
+            type = lookup_cast_type_from_column(column)
+            quote(type.serialize(value))
           else
             super
           end

activerecord/lib/active_record/log_subscriber.rb

@@ -55,13 +55,18 @@ module ActiveRecord
       end
 
       def render_bind(attr, value)
-        if attr.is_a?(Array)
-          attr = attr.first
-        elsif attr.type.binary? && attr.value
+        case attr
+        when ActiveModel::Attribute
+          if attr.type.binary? && attr.value
             value = "<#{attr.value_for_database.to_s.bytesize} bytes of binary data>"
           end
+        when Array
+          attr = attr.first
+        else
+          attr = nil
+        end
 
-        [attr && attr.name, value]
+        [attr&.name, value]
       end
 
       def colorize_payload_name(name, payload_name)

activerecord/test/cases/adapter_test.rb

@@ -312,41 +312,72 @@ module ActiveRecord
     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(nil).to_sql}", nil, [[nil, post.id]])
-        assert_equal expected.to_a, result.to_a
+      def test_select_all_insert_update_delete_with_legacy_binds
+        binds = [[Event.column_for_attribute("id"), 1]]
+        bind_param = Arel::Nodes::BindParam.new(nil)
+
+        assert_deprecated do
+          id = @connection.insert("INSERT INTO events(id) VALUES (#{bind_param.to_sql})", nil, nil, nil, nil, binds)
+          assert_equal 1, id
         end
 
-      def test_insert_update_delete_with_legacy_binds
-        binds = [[nil, 1]]
+        assert_deprecated do
+          updated = @connection.update("UPDATE events SET title = 'foo' WHERE id = #{bind_param.to_sql}", nil, binds)
+          assert_equal 1, updated
+        end
+
+        assert_deprecated do
+          result = @connection.select_all("SELECT * FROM events WHERE id = #{bind_param.to_sql}", nil, binds)
+          assert_equal({ "id" => 1, "title" => "foo" }, result.first)
+        end
+
+        assert_deprecated do
+          deleted = @connection.delete("DELETE FROM events WHERE id = #{bind_param.to_sql}", nil, binds)
+          assert_equal 1, deleted
+        end
+
+        assert_deprecated do
+          result = @connection.select_all("SELECT * FROM events WHERE id = #{bind_param.to_sql}", nil, binds)
+          assert_nil result.first
+        end
+      end
+
+      def test_select_all_insert_update_delete_with_casted_binds
+        binds = [Event.type_for_attribute("id").serialize(1)]
         bind_param = Arel::Nodes::BindParam.new(nil)
 
         id = @connection.insert("INSERT INTO events(id) VALUES (#{bind_param.to_sql})", nil, nil, nil, nil, binds)
         assert_equal 1, id
 
-        @connection.update("UPDATE events SET title = 'foo' WHERE id = #{bind_param.to_sql}", nil, binds)
+        updated = @connection.update("UPDATE events SET title = 'foo' WHERE id = #{bind_param.to_sql}", nil, binds)
+        assert_equal 1, updated
+
         result = @connection.select_all("SELECT * FROM events WHERE id = #{bind_param.to_sql}", nil, binds)
         assert_equal({ "id" => 1, "title" => "foo" }, result.first)
 
-        @connection.delete("DELETE FROM events WHERE id = #{bind_param.to_sql}", nil, binds)
+        deleted = @connection.delete("DELETE FROM events WHERE id = #{bind_param.to_sql}", nil, binds)
+        assert_equal 1, deleted
+
         result = @connection.select_all("SELECT * FROM events WHERE id = #{bind_param.to_sql}", nil, binds)
         assert_nil result.first
       end
 
-      def test_insert_update_delete_with_binds
-        binds = [Relation::QueryAttribute.new("id", 1, Type.default_value)]
+      def test_select_all_insert_update_delete_with_binds
+        binds = [Relation::QueryAttribute.new("id", 1, Event.type_for_attribute("id"))]
         bind_param = Arel::Nodes::BindParam.new(nil)
 
         id = @connection.insert("INSERT INTO events(id) VALUES (#{bind_param.to_sql})", nil, nil, nil, nil, binds)
         assert_equal 1, id
 
-        @connection.update("UPDATE events SET title = 'foo' WHERE id = #{bind_param.to_sql}", nil, binds)
+        updated = @connection.update("UPDATE events SET title = 'foo' WHERE id = #{bind_param.to_sql}", nil, binds)
+        assert_equal 1, updated
+
         result = @connection.select_all("SELECT * FROM events WHERE id = #{bind_param.to_sql}", nil, binds)
         assert_equal({ "id" => 1, "title" => "foo" }, result.first)
 
-        @connection.delete("DELETE FROM events WHERE id = #{bind_param.to_sql}", nil, binds)
+        deleted = @connection.delete("DELETE FROM events WHERE id = #{bind_param.to_sql}", nil, binds)
+        assert_equal 1, deleted
+
         result = @connection.select_all("SELECT * FROM events WHERE id = #{bind_param.to_sql}", nil, binds)
         assert_nil result.first
       end

activerecord/test/cases/bind_parameter_test.rb

@@ -159,8 +159,10 @@ if ActiveRecord::Base.connection.prepared_statements
 
       def test_logs_legacy_binds_after_type_cast
         binds = [[@pk, "10"]]
+        assert_deprecated do
           assert_logs_binds(binds)
         end
+      end
 
       private
         def to_sql_key(arel)