Merge pull request #27962 from kamipo/deprecate_quoted_id

Deprecate using `#quoted_id` in quoting / type casting

7 files changed, 111 insertions, 60 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index aeabd5b39d..a2566ae5fb 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,7 @@
+*   Deprecate using `#quoted_id` in quoting.
+
+    *Ryuta Kamizono*
+
 *   Fix `wait_timeout` to configurable for mysql2 adapter.
 
     Fixes #26556.
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb b/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb
index 7f4132accf..e5a24b2aca 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb
@@ -7,8 +7,13 @@ module ActiveRecord
       # Quotes the column value to help prevent
       # {SQL injection attacks}[http://en.wikipedia.org/wiki/SQL_injection].
       def quote(value)
-        # records are quoted as their primary key
-        return value.quoted_id if value.respond_to?(:quoted_id)
+        value = id_value_for_database(value) if value.is_a?(Base)
+
+        if value.respond_to?(:quoted_id)
+          ActiveSupport::Deprecation.warn \
+            "Using #quoted_id is deprecated and will be removed in Rails 5.2."
+          return value.quoted_id
+        end
 
         _quote(value)
       end
@@ -17,6 +22,8 @@ module ActiveRecord
       # SQLite does not understand dates, so this method will convert a Date
       # to a String.
       def type_cast(value, column = nil)
+        value = id_value_for_database(value) if value.is_a?(Base)
+
         if value.respond_to?(:quoted_id) && value.respond_to?(:id)
           return value.id
         end
@@ -151,6 +158,12 @@ module ActiveRecord
           binds.map { |attr| type_cast(attr.value_for_database) }
         end
 
+        def id_value_for_database(value)
+          if primary_key = value.class.primary_key
+            value.instance_variable_get(:@attributes)[primary_key].value_for_database
+          end
+        end
+
         def types_which_need_no_typecasting
           [nil, Numeric, String]
         end
diff --git a/activerecord/lib/active_record/sanitization.rb b/activerecord/lib/active_record/sanitization.rb
index 427c0019c6..64bda1539c 100644
--- a/activerecord/lib/active_record/sanitization.rb
+++ b/activerecord/lib/active_record/sanitization.rb
@@ -1,4 +1,3 @@
-
 module ActiveRecord
   module Sanitization
     extend ActiveSupport::Concern
@@ -207,9 +206,9 @@ module ActiveRecord
         end
     end
 
-    # TODO: Deprecate this
     def quoted_id # :nodoc:
       self.class.connection.quote(@attributes[self.class.primary_key].value_for_database)
     end
+    deprecate :quoted_id
   end
 end
diff --git a/activerecord/test/cases/adapters/postgresql/quoting_test.rb b/activerecord/test/cases/adapters/postgresql/quoting_test.rb
index 141baffa5b..a1e966b915 100644
--- a/activerecord/test/cases/adapters/postgresql/quoting_test.rb
+++ b/activerecord/test/cases/adapters/postgresql/quoting_test.rb
@@ -1,5 +1,4 @@
 require "cases/helper"
-require "ipaddr"
 
 module ActiveRecord
   module ConnectionAdapters
diff --git a/activerecord/test/cases/adapters/sqlite3/quoting_test.rb b/activerecord/test/cases/adapters/sqlite3/quoting_test.rb
index 9750840051..aefbb309e6 100644
--- a/activerecord/test/cases/adapters/sqlite3/quoting_test.rb
+++ b/activerecord/test/cases/adapters/sqlite3/quoting_test.rb
@@ -1,6 +1,5 @@
 require "cases/helper"
 require "bigdecimal"
-require "yaml"
 require "securerandom"
 
 class SQLite3QuotingTest < ActiveRecord::SQLite3TestCase
@@ -15,31 +14,6 @@ class SQLite3QuotingTest < ActiveRecord::SQLite3TestCase
     assert_equal expected, @conn.type_cast(binary)
   end
 
-  def test_type_cast_symbol
-    assert_equal "foo", @conn.type_cast(:foo)
-  end
-
-  def test_type_cast_date
-    date = Date.today
-    expected = @conn.quoted_date(date)
-    assert_equal expected, @conn.type_cast(date)
-  end
-
-  def test_type_cast_time
-    time = Time.now
-    expected = @conn.quoted_date(time)
-    assert_equal expected, @conn.type_cast(time)
-  end
-
-  def test_type_cast_numeric
-    assert_equal 10, @conn.type_cast(10)
-    assert_equal 2.2, @conn.type_cast(2.2)
-  end
-
-  def test_type_cast_nil
-    assert_nil @conn.type_cast(nil)
-  end
-
   def test_type_cast_true
     assert_equal "t", @conn.type_cast(true)
   end
@@ -53,31 +27,6 @@ class SQLite3QuotingTest < ActiveRecord::SQLite3TestCase
     assert_equal bd.to_f, @conn.type_cast(bd)
   end
 
-  def test_type_cast_unknown_should_raise_error
-    obj = Class.new.new
-    assert_raise(TypeError) { @conn.type_cast(obj) }
-  end
-
-  def test_type_cast_object_which_responds_to_quoted_id
-    quoted_id_obj = Class.new {
-      def quoted_id
-        "'zomg'"
-      end
-
-      def id
-        10
-      end
-    }.new
-    assert_equal 10, @conn.type_cast(quoted_id_obj)
-
-    quoted_id_obj = Class.new {
-      def quoted_id
-        "'zomg'"
-      end
-    }.new
-    assert_raise(TypeError) { @conn.type_cast(quoted_id_obj) }
-  end
-
   def test_quoting_binary_strings
     value = "hello".encode("ascii-8bit")
     type = ActiveRecord::Type::String.new
diff --git a/activerecord/test/cases/quoting_test.rb b/activerecord/test/cases/quoting_test.rb
index 5ff5e3c735..f260d043e4 100644
--- a/activerecord/test/cases/quoting_test.rb
+++ b/activerecord/test/cases/quoting_test.rb
@@ -82,7 +82,7 @@ module ActiveRecord
       end
 
       def test_quote_with_quoted_id
-        assert_equal 1, @quoter.quote(Struct.new(:quoted_id).new(1))
+        assert_deprecated { assert_equal 1, @quoter.quote(Struct.new(:quoted_id).new(1)) }
       end
 
       def test_quote_nil
@@ -150,6 +150,62 @@ module ActiveRecord
       end
     end
 
+    class TypeCastingTest < ActiveRecord::TestCase
+      def setup
+        @conn = ActiveRecord::Base.connection
+      end
+
+      def test_type_cast_symbol
+        assert_equal "foo", @conn.type_cast(:foo)
+      end
+
+      def test_type_cast_date
+        date = Date.today
+        expected = @conn.quoted_date(date)
+        assert_equal expected, @conn.type_cast(date)
+      end
+
+      def test_type_cast_time
+        time = Time.now
+        expected = @conn.quoted_date(time)
+        assert_equal expected, @conn.type_cast(time)
+      end
+
+      def test_type_cast_numeric
+        assert_equal 10, @conn.type_cast(10)
+        assert_equal 2.2, @conn.type_cast(2.2)
+      end
+
+      def test_type_cast_nil
+        assert_nil @conn.type_cast(nil)
+      end
+
+      def test_type_cast_unknown_should_raise_error
+        obj = Class.new.new
+        assert_raise(TypeError) { @conn.type_cast(obj) }
+      end
+
+      def test_type_cast_object_which_responds_to_quoted_id
+        quoted_id_obj = Class.new {
+          def quoted_id
+            "'zomg'"
+          end
+
+          def id
+            10
+          end
+        }.new
+        assert_equal 10, @conn.type_cast(quoted_id_obj)
+
+        quoted_id_obj = Class.new {
+          def quoted_id
+            "'zomg'"
+          end
+        }.new
+        assert_raise(TypeError) { @conn.type_cast(quoted_id_obj) }
+      end
+    end
+
     class QuoteBooleanTest < ActiveRecord::TestCase
       def setup
         @connection = ActiveRecord::Base.connection
@@ -165,5 +221,32 @@ module ActiveRecord
         assert_predicate @connection.type_cast(false), :frozen?
       end
     end
+
+    if subsecond_precision_supported?
+      class QuoteARBaseTest < ActiveRecord::TestCase
+        class DatetimePrimaryKey < ActiveRecord::Base
+        end
+
+        def setup
+          @time = ::Time.utc(2017, 2, 14, 12, 34, 56, 789999)
+          @connection = ActiveRecord::Base.connection
+          @connection.create_table :datetime_primary_keys, id: :datetime, precision: 3, force: true
+        end
+
+        def teardown
+          @connection.drop_table :datetime_primary_keys, if_exists: true
+        end
+
+        def test_quote_ar_object
+          value = DatetimePrimaryKey.new(id: @time)
+          assert_equal "'2017-02-14 12:34:56.789000'",  @connection.quote(value)
+        end
+
+        def test_type_cast_ar_object
+          value = DatetimePrimaryKey.new(id: @time)
+          assert_equal "2017-02-14 12:34:56.789000",  @connection.type_cast(value)
+        end
+      end
+    end
   end
 end
diff --git a/activerecord/test/cases/sanitize_test.rb b/activerecord/test/cases/sanitize_test.rb
index 23bcb0af1e..72f09186e2 100644
--- a/activerecord/test/cases/sanitize_test.rb
+++ b/activerecord/test/cases/sanitize_test.rb
@@ -152,11 +152,15 @@ class SanitizeTest < ActiveRecord::TestCase
   end
 
   def test_bind_record
-    o = Struct.new(:quoted_id).new(1)
-    assert_equal "1", bind("?", o)
+    o = Class.new {
+      def quoted_id
+        1
+      end
+    }.new
+    assert_deprecated { assert_equal "1", bind("?", o) }
 
     os = [o] * 3
-    assert_equal "1,1,1", bind("?", os)
+    assert_deprecated { assert_equal "1,1,1", bind("?", os) }
   end
 
   def test_named_bind_with_postgresql_type_casts