Deprecate passing a column to type_cast

The type information for type casting is entirely separated to type object, so if anyone does passing a column to `type_cast` in Rails 6, they are likely doing something wrong. See the comment for more details: https://github.com/rails/rails/blob/28d815b89487ce4001a3f6f0ab684e6f9c017ed0/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb#L33-L42 This also deprecates passing legacy binds (an array of `[column, value]` which is 4.2 style format) to query methods on connection. That legacy format was kept for backward compatibility, instead of that, I've supported casted binds format (an array of casted values), it is easier to construct binds than existing two binds format.
rails · May 1, 2020 · 92360e9 · 92360e9
1 parent 7669dc2
commit 92360e9
Show file tree

Hide file tree

Showing 6 changed files with 72 additions and 33 deletions.
diff --git a/activerecord/CHANGELOG.md b/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*

diff --git a/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb b/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb
@@ -21,7 +21,11 @@ def type_cast(value, column = nil)
         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 @@ def type_cast(value, column = nil)
       # 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 @@ def column_name_with_order_matcher # :nodoc:
 
       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
 

diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb b/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb
@@ -67,8 +67,8 @@ def quote_default_expression(value, column) # :nodoc:
           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

diff --git a/activerecord/lib/active_record/log_subscriber.rb b/activerecord/lib/active_record/log_subscriber.rb
@@ -55,13 +55,18 @@ def type_casted_binds(casted_binds)
       end
 
       def render_bind(attr, value)
-        if attr.is_a?(Array)
+        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
-        elsif attr.type.binary? && attr.value
-          value = "<#{attr.value_for_database.to_s.bytesize} bytes of binary data>"
+        else
+          attr = nil
         end
 
-        [attr && attr.name, value]
+        [attr&.name, value]
       end
 
       def colorize_payload_name(name, payload_name)

diff --git a/activerecord/test/cases/adapter_test.rb b/activerecord/test/cases/adapter_test.rb
@@ -312,41 +312,72 @@ def test_select_all_always_return_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(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
+
+        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_insert_update_delete_with_legacy_binds
-        binds = [[nil, 1]]
+      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

diff --git a/activerecord/test/cases/bind_parameter_test.rb b/activerecord/test/cases/bind_parameter_test.rb
@@ -159,7 +159,9 @@ def test_logs_binds_after_type_cast
 
       def test_logs_legacy_binds_after_type_cast
         binds = [[@pk, "10"]]
-        assert_logs_binds(binds)
+        assert_deprecated do
+          assert_logs_binds(binds)
+        end
       end
 
       private