Avoid double type cast when serializing attributes

jonathanhefner · jonathanhefner · commit 8e383fdad662 · 2022-09-29T11:34:29.000-05:00
Most model attribute types try to cast a given value before serializing
it.  This allows uncast values to be passed to finder methods and still
be serialized appropriately.  However, when persisting a model, this
cast is unnecessary because the value will already have been cast by
`ActiveModel::Attribute#value`.

To eliminate the overhead of a 2nd cast, this commit introduces a
`ActiveModel::Type::SerializeCastValue` module.  Types can include this
module, and their `serialize_cast_value` method will be called instead
of `serialize` when serializing an already-cast value.

To preserve existing behavior of any user types that subclass Rails'
types, `serialize_after_cast` will only be called if the type itself
(not a superclass) includes `ActiveModel::Type::SerializeCastValue`.
This also applies to type decorators implemented via `DelegateClass`.

Benchmark script:

  ```ruby
  require "active_model"
  require "benchmark/ips"

  class ActiveModel::Attribute
    alias baseline_value_for_database value_for_database
  end

  VALUES = {
    my_big_integer: "123456",
    my_boolean: "true",
    my_date: "1999-12-31",
    my_datetime: "1999-12-31 12:34:56 UTC",
    my_decimal: "123.456",
    my_float: "123.456",
    my_immutable_string: "abcdef",
    my_integer: "123456",
    my_string: "abcdef",
    my_time: "1999-12-31T12:34:56.789-10:00",
  }

  TYPES = VALUES.to_h { |name, value| [name, name.to_s.delete_prefix("my_").to_sym] }

  class MyModel
    include ActiveModel::API
    include ActiveModel::Attributes

    TYPES.each do |name, type|
      attribute name, type
    end
  end

  TYPES.each do |name, type|
    $attribute_set ||= MyModel.new(VALUES).instance_variable_get(:@attributes)
    attribute = $attribute_set[name.to_s]

    puts "=" * 72
    Benchmark.ips do |x|
      x.report("#{type} before") { attribute.baseline_value_for_database }
      x.report("#{type} after") { attribute.value_for_database }
      x.compare!
    end
  end
  ```

Benchmark results:

  ```
  ========================================================================
  Warming up --------------------------------------
    big_integer before   100.417k i/100ms
     big_integer after   260.375k i/100ms
  Calculating -------------------------------------
    big_integer before      1.005M (± 1.0%) i/s -      5.121M in   5.096498s
     big_integer after      2.630M (± 1.0%) i/s -     13.279M in   5.050387s

  Comparison:
     big_integer after:  2629583.6 i/s
    big_integer before:  1004961.2 i/s - 2.62x  (± 0.00) slower

  ========================================================================
  Warming up --------------------------------------
        boolean before   230.663k i/100ms
         boolean after   299.262k i/100ms
  Calculating -------------------------------------
        boolean before      2.313M (± 0.7%) i/s -     11.764M in   5.085925s
         boolean after      3.037M (± 0.6%) i/s -     15.262M in   5.026280s

  Comparison:
         boolean after:  3036640.8 i/s
        boolean before:  2313127.8 i/s - 1.31x  (± 0.00) slower

  ========================================================================
  Warming up --------------------------------------
           date before   148.821k i/100ms
            date after   298.939k i/100ms
  Calculating -------------------------------------
           date before      1.486M (± 0.6%) i/s -      7.441M in   5.006091s
            date after      2.963M (± 0.8%) i/s -     14.947M in   5.045651s

  Comparison:
            date after:  2962535.3 i/s
           date before:  1486459.4 i/s - 1.99x  (± 0.00) slower

  ========================================================================
  Warming up --------------------------------------
       datetime before    92.818k i/100ms
        datetime after   136.710k i/100ms
  Calculating -------------------------------------
       datetime before    920.236k (± 0.6%) i/s -      4.641M in   5.043355s
        datetime after      1.366M (± 0.8%) i/s -      6.836M in   5.003307s

  Comparison:
        datetime after:  1366294.1 i/s
       datetime before:   920236.1 i/s - 1.48x  (± 0.00) slower

  ========================================================================
  Warming up --------------------------------------
        decimal before    50.194k i/100ms
         decimal after   298.674k i/100ms
  Calculating -------------------------------------
        decimal before    494.141k (± 1.4%) i/s -      2.510M in   5.079995s
         decimal after      3.015M (± 1.0%) i/s -     15.232M in   5.052929s

  Comparison:
         decimal after:  3014901.3 i/s
        decimal before:   494141.2 i/s - 6.10x  (± 0.00) slower

  ========================================================================
  Warming up --------------------------------------
          float before   217.547k i/100ms
           float after   298.106k i/100ms
  Calculating -------------------------------------
          float before      2.157M (± 0.8%) i/s -     10.877M in   5.043292s
           float after      2.991M (± 0.6%) i/s -     15.203M in   5.082806s

  Comparison:
           float after:  2991262.8 i/s
          float before:  2156940.2 i/s - 1.39x  (± 0.00) slower

  ========================================================================
  Warming up --------------------------------------
  immutable_string before
                         163.287k i/100ms
  immutable_string after
                         298.245k i/100ms
  Calculating -------------------------------------
  immutable_string before
                            1.652M (± 0.7%) i/s -      8.328M in   5.040855s
  immutable_string after
                            3.022M (± 0.9%) i/s -     15.210M in   5.033151s

  Comparison:
  immutable_string after:  3022313.3 i/s
  immutable_string before:  1652121.7 i/s - 1.83x  (± 0.00) slower

  ========================================================================
  Warming up --------------------------------------
        integer before   115.383k i/100ms
         integer after   159.702k i/100ms
  Calculating -------------------------------------
        integer before      1.132M (± 0.8%) i/s -      5.769M in   5.095041s
         integer after      1.641M (± 0.5%) i/s -      8.305M in   5.061893s

  Comparison:
         integer after:  1640635.8 i/s
        integer before:  1132381.5 i/s - 1.45x  (± 0.00) slower

  ========================================================================
  Warming up --------------------------------------
         string before   163.061k i/100ms
          string after   299.885k i/100ms
  Calculating -------------------------------------
         string before      1.659M (± 0.7%) i/s -      8.316M in   5.012609s
          string after      2.999M (± 0.6%) i/s -     15.294M in   5.100008s

  Comparison:
          string after:  2998956.0 i/s
         string before:  1659115.6 i/s - 1.81x  (± 0.00) slower

  ========================================================================
  Warming up --------------------------------------
           time before    98.250k i/100ms
            time after   133.463k i/100ms
  Calculating -------------------------------------
           time before    987.771k (± 0.7%) i/s -      5.011M in   5.073023s
            time after      1.330M (± 0.5%) i/s -      6.673M in   5.016573s

  Comparison:
            time after:  1330253.9 i/s
           time before:   987771.0 i/s - 1.35x  (± 0.00) slower
  ```
diff --git a/activemodel/lib/active_model/attribute.rb b/activemodel/lib/active_model/attribute.rb
@@ -179,6 +179,10 @@ def type_cast(value)
           type.cast(value)
         end
 
+        def value_for_database
+          Type::SerializeCastValue.serialize(type, value)
+        end
+
         def came_from_user?
           !type.value_constructed_by_mass_assignment?(value_before_type_cast)
         end
diff --git a/activemodel/lib/active_model/type.rb b/activemodel/lib/active_model/type.rb
@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require "active_model/type/helpers"
+require "active_model/type/serialize_cast_value"
 require "active_model/type/value"
 
 require "active_model/type/big_integer"
diff --git a/activemodel/lib/active_model/type/big_integer.rb b/activemodel/lib/active_model/type/big_integer.rb
@@ -21,6 +21,12 @@ module Type
     # All casting and serialization are performed in the same way as the
     # standard ActiveModel::Type::Integer type.
     class BigInteger < Integer
+      include SerializeCastValue
+
+      def serialize_cast_value(value) # :nodoc:
+        value
+      end
+
       private
         def max_value
           ::Float::INFINITY
diff --git a/activemodel/lib/active_model/type/boolean.rb b/activemodel/lib/active_model/type/boolean.rb
@@ -10,6 +10,8 @@ module Type
     # - Empty strings are coerced to +nil+.
     # - All other values will be coerced to +true+.
     class Boolean < Value
+      include SerializeCastValue
+
       FALSE_VALUES = [
         false, 0,
         "0", :"0",
diff --git a/activemodel/lib/active_model/type/date.rb b/activemodel/lib/active_model/type/date.rb
@@ -22,6 +22,7 @@ module Type
     # String values are parsed using the ISO 8601 date format. Any other values
     # are cast using their +to_date+ method, if it exists.
     class Date < Value
+      include SerializeCastValue
       include Helpers::Timezone
       include Helpers::AcceptsMultiparameterTime.new
 
diff --git a/activemodel/lib/active_model/type/date_time.rb b/activemodel/lib/active_model/type/date_time.rb
@@ -38,6 +38,7 @@ module Type
     #     attribute :start, :datetime, precision: 4
     #   end
     class DateTime < Value
+      include SerializeCastValue
       include Helpers::Timezone
       include Helpers::TimeValue
       include Helpers::AcceptsMultiparameterTime.new(
@@ -48,6 +49,8 @@ def type
         :datetime
       end
 
+      alias :serialize_cast_value :serialize_time_value # :nodoc:
+
       private
         def cast_value(value)
           return apply_seconds_precision(value) unless value.is_a?(::String)
diff --git a/activemodel/lib/active_model/type/decimal.rb b/activemodel/lib/active_model/type/decimal.rb
@@ -32,6 +32,7 @@ module Type
     #     attribute :weight, :decimal, precision: 24
     #   end
     class Decimal < Value
+      include SerializeCastValue
       include Helpers::Numeric
       BIGDECIMAL_PRECISION = 18
 
diff --git a/activemodel/lib/active_model/type/float.rb b/activemodel/lib/active_model/type/float.rb
@@ -26,6 +26,7 @@ module Type
     # - <tt>"-Infinity"</tt> is cast to <tt>-Float::INFINITY</tt>.
     # - <tt>"NaN"</tt> is cast to <tt>Float::NAN</tt>.
     class Float < Value
+      include SerializeCastValue
       include Helpers::Numeric
 
       def type
diff --git a/activemodel/lib/active_model/type/helpers/time_value.rb b/activemodel/lib/active_model/type/helpers/time_value.rb
@@ -20,6 +20,7 @@ def serialize(value)
 
           value
         end
+        alias :serialize_time_value :serialize
 
         def apply_seconds_precision(value)
           return value unless precision && value.respond_to?(:nsec)
diff --git a/activemodel/lib/active_model/type/immutable_string.rb b/activemodel/lib/active_model/type/immutable_string.rb
@@ -33,6 +33,8 @@ module Type
     #
     #   person.active # => "aye"
     class ImmutableString < Value
+      include SerializeCastValue
+
       def initialize(**args)
         @true  = -(args.delete(:true)&.to_s  || "t")
         @false = -(args.delete(:false)&.to_s || "f")
diff --git a/activemodel/lib/active_model/type/integer.rb b/activemodel/lib/active_model/type/integer.rb
@@ -36,6 +36,7 @@ module Type
     #     attribute :age, :integer, limit: 6
     #   end
     class Integer < Value
+      include SerializeCastValue
       include Helpers::Numeric
 
       # Column storage size in bytes.
@@ -61,6 +62,10 @@ def serialize(value)
         ensure_in_range(super)
       end
 
+      def serialize_cast_value(value) # :nodoc:
+        ensure_in_range(value)
+      end
+
       def serializable?(value)
         cast_value = cast(value)
         in_range?(cast_value) || begin
diff --git a/activemodel/lib/active_model/type/serialize_cast_value.rb b/activemodel/lib/active_model/type/serialize_cast_value.rb
@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+module ActiveModel
+  module Type
+    module SerializeCastValue # :nodoc:
+      def self.included(klass)
+        unless klass.respond_to?(:included_serialize_cast_value)
+          klass.singleton_class.attr_accessor :included_serialize_cast_value
+          klass.attr_reader :itself_if_class_included_serialize_cast_value
+        end
+        klass.included_serialize_cast_value = true
+      end
+
+      def self.serialize(type, value)
+        # Verify that `type.class` explicitly included SerializeCastValue.
+        # Using `type.equal?(type.itself_if_...)` is a performant way to also
+        # ensure that `type` is not just a DelegateClass instance (e.g.
+        # ActiveRecord::Type::Serialized) unintentionally delegating
+        # SerializeCastValue methods.
+        if type.equal?((type.itself_if_class_included_serialize_cast_value rescue nil))
+          type.serialize_cast_value(value)
+        else
+          type.serialize(value)
+        end
+      end
+
+      def initialize(...)
+        @itself_if_class_included_serialize_cast_value = self if self.class.included_serialize_cast_value
+        super
+      end
+
+      def serialize_cast_value(value)
+        value
+      end
+    end
+  end
+end
diff --git a/activemodel/lib/active_model/type/string.rb b/activemodel/lib/active_model/type/string.rb
@@ -11,6 +11,8 @@ module Type
     # However, it accounts for mutable strings, so dirty tracking can properly
     # check if a string has changed.
     class String < ImmutableString
+      include SerializeCastValue
+
       def changed_in_place?(raw_old_value, new_value)
         if new_value.is_a?(::String)
           raw_old_value != new_value
diff --git a/activemodel/lib/active_model/type/time.rb b/activemodel/lib/active_model/type/time.rb
@@ -38,6 +38,7 @@ module Type
     #     attribute :start, :time, precision: 4
     #   end
     class Time < Value
+      include SerializeCastValue
       include Helpers::Timezone
       include Helpers::TimeValue
       include Helpers::AcceptsMultiparameterTime.new(
@@ -67,6 +68,8 @@ def user_input_in_time_zone(value)
         super(value)
       end
 
+      alias :serialize_cast_value :serialize_time_value # :nodoc:
+
       private
         def cast_value(value)
           return apply_seconds_precision(value) unless value.is_a?(::String)
diff --git a/activemodel/lib/active_model/type/value.rb b/activemodel/lib/active_model/type/value.rb
@@ -5,13 +5,15 @@ module Type
     # The base class for all attribute types. This class also serves as the
     # default type for attributes that do not specify a type.
     class Value
+      include SerializeCastValue
       attr_reader :precision, :scale, :limit
 
       # Initializes a type with three basic configuration settings: precision,
       # limit, and scale. The Value base class does not define behavior for
       # these settings. It uses them for equality comparison and hash key
       # generation only.
       def initialize(precision: nil, limit: nil, scale: nil)
+        super()
         @precision = precision
         @scale = scale
         @limit = limit
diff --git a/activemodel/test/cases/attribute_test.rb b/activemodel/test/cases/attribute_test.rb
@@ -82,6 +82,20 @@ def deserialize(value)
       assert_equal "serialize(cast(whatever))", attribute.value_for_database
     end
 
+    test "from_user + value_for_database uses serialize_cast_value when possible" do
+      @type = Class.new(InscribingType) do
+        include Type::SerializeCastValue
+
+        def serialize_cast_value(value)
+          "serialize_cast_value(#{value})"
+        end
+      end.new
+
+      attribute = Attribute.from_user(nil, "whatever", @type)
+
+      assert_equal "serialize_cast_value(cast(whatever))", attribute.value_for_database
+    end
+
     test "duping dups the value" do
       attribute = Attribute.from_database(nil, "a value", @type)
 
diff --git a/activemodel/test/cases/type/big_integer_test.rb b/activemodel/test/cases/type/big_integer_test.rb
@@ -20,6 +20,14 @@ def test_large_values
         type = Type::BigInteger.new
         assert_equal 9999999999999999999999999999999, type.serialize(9999999999999999999999999999999)
       end
+
+      test "serialize_cast_value is equivalent to serialize after cast" do
+        type = Type::BigInteger.new
+        value = type.cast(9999999999999999999999999999999)
+
+        assert_equal type.serialize(value), type.serialize_cast_value(value)
+        assert_equal type.serialize(-value), type.serialize_cast_value(-value)
+      end
     end
   end
 end
diff --git a/activemodel/test/cases/type/date_time_test.rb b/activemodel/test/cases/type/date_time_test.rb
@@ -37,6 +37,13 @@ def test_hash_with_wrong_keys
         assert_equal "Provided hash {:a=>1} doesn't contain necessary keys: [1, 2, 3]", error.message
       end
 
+      test "serialize_cast_value is equivalent to serialize after cast" do
+        type = Type::DateTime.new(precision: 1)
+        value = type.cast("1999-12-31 12:34:56.789 -1000")
+
+        assert_equal type.serialize(value), type.serialize_cast_value(value)
+      end
+
       private
         def with_timezone_config(default:)
           old_zone_default = ::Time.zone_default
diff --git a/activemodel/test/cases/type/integer_test.rb b/activemodel/test/cases/type/integer_test.rb
@@ -131,6 +131,18 @@ class IntegerTest < ActiveModel::TestCase
           type.serialize(9999999999999999999999999999999)
         end
       end
+
+      test "serialize_cast_value enforces range" do
+        type = Integer.new
+
+        assert_raises(ActiveModel::RangeError) do
+          type.serialize_cast_value(-2147483649)
+        end
+
+        assert_raises(ActiveModel::RangeError) do
+          type.serialize_cast_value(2147483648)
+        end
+      end
     end
   end
 end
diff --git a/activemodel/test/cases/type/serialize_cast_value_test.rb b/activemodel/test/cases/type/serialize_cast_value_test.rb
@@ -0,0 +1,57 @@
+# frozen_string_literal: true
+
+require "cases/helper"
+
+module ActiveModel
+  module Type
+    class SerializeCastValueTest < ActiveModel::TestCase
+      class DoesNotIncludeModule
+        def serialize(value)
+          "serialize(#{value})"
+        end
+
+        def serialize_cast_value(value)
+          raise "this should never be called"
+        end
+      end
+
+      class IncludesModule < DoesNotIncludeModule
+        include SerializeCastValue
+
+        def serialize_cast_value(value)
+          super("serialize_cast_value(#{value})")
+        end
+      end
+
+      test "calls #serialize when a class does not include SerializeCastValue" do
+        assert_equal "serialize(foo)", SerializeCastValue.serialize(DoesNotIncludeModule.new, "foo")
+      end
+
+      test "calls #serialize_cast_value when a class directly includes SerializeCastValue" do
+        assert_equal "serialize_cast_value(foo)", SerializeCastValue.serialize(IncludesModule.new, "foo")
+      end
+
+      test "calls #serialize when a subclass does not directly include SerializeCastValue" do
+        subclass = Class.new(IncludesModule)
+        assert_equal "serialize(foo)", SerializeCastValue.serialize(subclass.new, "foo")
+      end
+
+      test "calls #serialize_cast_value when a subclass re-includes SerializeCastValue" do
+        subclass = Class.new(IncludesModule)
+        subclass.include SerializeCastValue
+        assert_equal "serialize_cast_value(foo)", SerializeCastValue.serialize(subclass.new, "foo")
+      end
+
+      test "calls #serialize when a delegate class does not include SerializeCastValue" do
+        delegate_class = DelegateClass(IncludesModule)
+        assert_equal "serialize(foo)", SerializeCastValue.serialize(delegate_class.new(IncludesModule.new), "foo")
+      end
+
+      test "calls #serialize_cast_value when a delegate class includes SerializeCastValue" do
+        delegate_class = DelegateClass(IncludesModule)
+        delegate_class.include SerializeCastValue
+        assert_equal "serialize_cast_value(foo)", SerializeCastValue.serialize(delegate_class.new(IncludesModule.new), "foo")
+      end
+    end
+  end
+end
diff --git a/activemodel/test/cases/type/time_test.rb b/activemodel/test/cases/type/time_test.rb
@@ -35,6 +35,13 @@ def test_user_input_in_time_zone
           assert_equal offset, type.user_input_in_time_zone(time_string).formatted_offset
         end
       end
+
+      test "serialize_cast_value is equivalent to serialize after cast" do
+        type = Type::Time.new(precision: 1)
+        value = type.cast("1999-12-31T12:34:56.789-10:00")
+
+        assert_equal type.serialize(value), type.serialize_cast_value(value)
+      end
     end
   end
 end
diff --git a/activerecord/lib/active_record/type/date.rb b/activerecord/lib/active_record/type/date.rb
@@ -3,6 +3,7 @@
 module ActiveRecord
   module Type
     class Date < ActiveModel::Type::Date
+      include ActiveModel::Type::SerializeCastValue
       include Internal::Timezone
     end
   end
diff --git a/activerecord/lib/active_record/type/date_time.rb b/activerecord/lib/active_record/type/date_time.rb
@@ -3,6 +3,7 @@
 module ActiveRecord
   module Type
     class DateTime < ActiveModel::Type::DateTime
+      include ActiveModel::Type::SerializeCastValue
       include Internal::Timezone
     end
   end
diff --git a/activerecord/lib/active_record/type/decimal_without_scale.rb b/activerecord/lib/active_record/type/decimal_without_scale.rb
@@ -3,6 +3,8 @@
 module ActiveRecord
   module Type
     class DecimalWithoutScale < ActiveModel::Type::BigInteger # :nodoc:
+      include ActiveModel::Type::SerializeCastValue
+
       def type
         :decimal
       end
diff --git a/activerecord/lib/active_record/type/text.rb b/activerecord/lib/active_record/type/text.rb
@@ -3,6 +3,8 @@
 module ActiveRecord
   module Type
     class Text < ActiveModel::Type::String # :nodoc:
+      include ActiveModel::Type::SerializeCastValue
+
       def type
         :text
       end
diff --git a/activerecord/lib/active_record/type/time.rb b/activerecord/lib/active_record/type/time.rb
@@ -3,6 +3,7 @@
 module ActiveRecord
   module Type
     class Time < ActiveModel::Type::Time
+      include ActiveModel::Type::SerializeCastValue
       include Internal::Timezone
 
       class Value < DelegateClass(::Time) # :nodoc:
@@ -17,6 +18,10 @@ def serialize(value)
         end
       end
 
+      def serialize_cast_value(value) # :nodoc:
+        Value.new(super) if value
+      end
+
       private
         def cast_value(value)
           case value = super
diff --git a/activerecord/lib/active_record/type/unsigned_integer.rb b/activerecord/lib/active_record/type/unsigned_integer.rb
diff --git a/activerecord/test/cases/type/date_time_test.rb b/activerecord/test/cases/type/date_time_test.rb
diff --git a/activerecord/test/cases/type/time_test.rb b/activerecord/test/cases/type/time_test.rb
diff --git a/activerecord/test/cases/type/unsigned_integer_test.rb b/activerecord/test/cases/type/unsigned_integer_test.rb

Original file line number	Diff line number	Diff line change
`@@ -33,6 +33,8 @@ module Type`
`33`	`33`	`#`
`34`	`34`	`# person.active # => "aye"`
`35`	`35`	`class ImmutableString < Value`
	`36`	`+ include SerializeCastValue`
	`37`	`+`
`36`	`38`	`def initialize(**args)`
`37`	`39`	`@true = -(args.delete(:true)&.to_s \|\| "t")`
`38`	`40`	`@false = -(args.delete(:false)&.to_s \|\| "f")`