Merge pull request #39882 from kamipo/simplify_attribute_type_decoration

Simplify attribute type decoration

activerecord/lib/active_record/attribute_decorators.rb

@@ -1,88 +0,0 @@
-# frozen_string_literal: true
-
-module ActiveRecord
-  module AttributeDecorators # :nodoc:
-    extend ActiveSupport::Concern
-
-    included do
-      class_attribute :attribute_type_decorations, instance_accessor: false, default: TypeDecorator.new # :internal:
-    end
-
-    module ClassMethods # :nodoc:
-      # This method is an internal API used to create class macros such as
-      # +serialize+, and features like time zone aware attributes.
-      #
-      # Used to wrap the type of an attribute in a new type.
-      # When the schema for a model is loaded, attributes with the same name as
-      # +column_name+ will have their type yielded to the given block. The
-      # return value of that block will be used instead.
-      #
-      # Subsequent calls where +column_name+ and +decorator_name+ are the same
-      # will override the previous decorator, not decorate twice. This can be
-      # used to create idempotent class macros like +serialize+
-      def decorate_attribute_type(column_name, decorator_name, &block)
-        matcher = ->(name, _) { name == column_name.to_s }
-        key = "_#{column_name}_#{decorator_name}"
-        decorate_matching_attribute_types(matcher, key, &block)
-      end
-
-      # This method is an internal API used to create higher level features like
-      # time zone aware attributes.
-      #
-      # When the schema for a model is loaded, +matcher+ will be called for each
-      # attribute with its name and type. If the matcher returns a truthy value,
-      # the type will then be yielded to the given block, and the return value
-      # of that block will replace the type.
-      #
-      # Subsequent calls to this method with the same value for +decorator_name+
-      # will replace the previous decorator, not decorate twice. This can be
-      # used to ensure that class macros are idempotent.
-      def decorate_matching_attribute_types(matcher, decorator_name, &block)
-        reload_schema_from_cache
-        decorator_name = decorator_name.to_s
-
-        # Create new hashes so we don't modify parent classes
-        self.attribute_type_decorations = attribute_type_decorations.merge(decorator_name => [matcher, block])
-      end
-
-      private
-        def load_schema!
-          super
-          attribute_types.each do |name, type|
-            decorated_type = attribute_type_decorations.apply(name, type)
-            define_attribute(name, decorated_type)
-          end
-        end
-    end
-
-    class TypeDecorator # :nodoc:
-      delegate :clear, to: :@decorations
-
-      def initialize(decorations = {})
-        @decorations = decorations
-      end
-
-      def merge(*args)
-        TypeDecorator.new(@decorations.merge(*args))
-      end
-
-      def apply(name, type)
-        decorations = decorators_for(name, type)
-        decorations.inject(type) do |new_type, block|
-          block.call(new_type)
-        end
-      end
-
-      private
-        def decorators_for(name, type)
-          matching(name, type).map(&:last)
-        end
-
-        def matching(name, type)
-          @decorations.values.select do |(matcher, _)|
-            matcher.call(name, type)
-          end
-        end
-    end
-  end
-end

activerecord/lib/active_record/attribute_methods/serialization.rb

@@ -69,12 +69,19 @@ module ActiveRecord
             Coders::YAMLColumn.new(attr_name, class_name_or_coder)
           end
 
-          decorate_attribute_type(attr_name, :serialize) do |type|
+          attr_name = attr_name.to_s
-            if type_incompatible_with_serialize?(type, class_name_or_coder)
+          type, options = attributes_to_define_after_schema_loads[attr_name]
-              raise ColumnNotSerializableError.new(attr_name, type)
+
+          attribute(attr_name) do |cast_type|
+            if type && !type.is_a?(Proc)
+              cast_type = _lookup_cast_type(attr_name, type, options)
             end
 
-            Type::Serialized.new(type, coder)
+            if type_incompatible_with_serialize?(cast_type, class_name_or_coder)
+              raise ColumnNotSerializableError.new(attr_name, cast_type)
+            end
+
+            Type::Serialized.new(cast_type, coder)
           end
         end
 

activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb

@@ -6,6 +6,10 @@ module ActiveRecord
   module AttributeMethods
     module TimeZoneConversion
       class TimeZoneConverter < DelegateClass(Type::Value) # :nodoc:
+        def self.new(subtype)
+          self === subtype ? subtype : super
+        end
+
         def deserialize(value)
           convert_time_to_time_zone(super)
         end
@@ -64,21 +68,14 @@ module ActiveRecord
       end
 
       module ClassMethods # :nodoc:
-        private
+        def define_attribute(name, cast_type, **)
-          def inherited(subclass)
+          if create_time_zone_conversion_attribute?(name, cast_type)
-            super
+            cast_type = TimeZoneConverter.new(cast_type)
-            # We need to apply this decorator here, rather than on module inclusion. The closure
-            # created by the matcher would otherwise evaluate for `ActiveRecord::Base`, not the
-            # sub class being decorated. As such, changes to `time_zone_aware_attributes`, or
-            # `skip_time_zone_conversion_for_attributes` would not be picked up.
-            subclass.class_eval do
-              matcher = ->(name, type) { create_time_zone_conversion_attribute?(name, type) }
-              decorate_matching_attribute_types(matcher, "_time_zone_conversion") do |type|
-                TimeZoneConverter.new(type)
-              end
-            end
           end
+          super
+        end
 
+        private
           def create_time_zone_conversion_attribute?(name, cast_type)
             enabled_for_column = time_zone_aware_attributes &&
               !skip_time_zone_conversion_for_attributes.include?(name.to_sym)

activerecord/lib/active_record/attributes.rb

@@ -246,17 +246,7 @@ module ActiveRecord
       def load_schema! # :nodoc:
         super
         attributes_to_define_after_schema_loads.each do |name, (type, options)|
-          case type
+          define_attribute(name, _lookup_cast_type(name, type, options), **options.slice(:default))
-          when Symbol
-            adapter_name = ActiveRecord::Type.adapter_name_from(self)
-            type = ActiveRecord::Type.lookup(type, **options.except(:default), adapter: adapter_name)
-          when Proc
-            type = type[type_for_attribute(name)]
-          else
-            type ||= type_for_attribute(name)
-          end
-
-          define_attribute(name, type, **options.slice(:default))
         end
       end
 
@@ -279,6 +269,18 @@ module ActiveRecord
           end
           _default_attributes[name] = default_attribute
         end
+
+        def _lookup_cast_type(name, type, options)
+          case type
+          when Symbol
+            adapter_name = ActiveRecord::Type.adapter_name_from(self)
+            ActiveRecord::Type.lookup(type, **options.except(:default), adapter: adapter_name)
+          when Proc
+            type[type_for_attribute(name)]
+          else
+            type || type_for_attribute(name)
+          end
+        end
     end
   end
 end

activerecord/lib/active_record/base.rb

@@ -5,7 +5,6 @@ require "active_support/dependencies"
 require "active_support/descendants_tracker"
 require "active_support/time"
 require "active_support/core_ext/class/subclasses"
-require "active_record/attribute_decorators"
 require "active_record/log_subscriber"
 require "active_record/explain_subscriber"
 require "active_record/relation/delegation"
@@ -293,7 +292,6 @@ module ActiveRecord #:nodoc:
     include Validations
     include CounterCache
     include Attributes
-    include AttributeDecorators
     include Locking::Optimistic
     include Locking::Pessimistic
     include AttributeMethods

activerecord/lib/active_record/locking/optimistic.rb

@@ -165,20 +165,12 @@ module ActiveRecord
             super
           end
 
-          private
+          def define_attribute(name, cast_type, **) # :nodoc:
-            # We need to apply this decorator here, rather than on module inclusion. The closure
+            if lock_optimistically && name == locking_column
-            # created by the matcher would otherwise evaluate for `ActiveRecord::Base`, not the
+              cast_type = LockingType.new(cast_type)
-            # sub class being decorated. As such, changes to `lock_optimistically`, or
-            # `locking_column` would not be picked up.
-            def inherited(subclass)
-              subclass.class_eval do
-                is_lock_column = ->(name, _) { lock_optimistically && name == locking_column }
-                decorate_matching_attribute_types(is_lock_column, "_optimistic_locking") do |type|
-                  LockingType.new(type)
-                end
-              end
-              super
             end
+            super
+          end
         end
     end
 
@@ -186,6 +178,10 @@ module ActiveRecord
     # `nil` values to `lock_version`, and not result in `ActiveRecord::StaleObjectError`
     # during update record.
     class LockingType < DelegateClass(Type::Value) # :nodoc:
+      def self.new(subtype)
+        self === subtype ? subtype : super
+      end
+
       def deserialize(value)
         super.to_i
       end

activerecord/test/cases/attribute_decorators_test.rb

@@ -1,127 +0,0 @@
-# frozen_string_literal: true
-
-require "cases/helper"
-
-module ActiveRecord
-  class AttributeDecoratorsTest < ActiveRecord::TestCase
-    class Model < ActiveRecord::Base
-      self.table_name = "attribute_decorators_model"
-    end
-
-    class StringDecorator < SimpleDelegator
-      def initialize(delegate, decoration = "decorated!")
-        @decoration = decoration
-        super(delegate)
-      end
-
-      def cast(value)
-        "#{super} #{@decoration}"
-      end
-
-      alias deserialize cast
-    end
-
-    setup do
-      @connection = ActiveRecord::Base.connection
-      @connection.create_table :attribute_decorators_model, force: true do |t|
-        t.string :a_string
-      end
-    end
-
-    teardown do
-      return unless @connection
-      @connection.drop_table "attribute_decorators_model", if_exists: true
-      Model.attribute_type_decorations.clear
-      Model.reset_column_information
-    end
-
-    test "attributes can be decorated" do
-      model = Model.new(a_string: "Hello")
-      assert_equal "Hello", model.a_string
-
-      Model.decorate_attribute_type(:a_string, :test) { |t| StringDecorator.new(t) }
-
-      model = Model.new(a_string: "Hello")
-      assert_equal "Hello decorated!", model.a_string
-    end
-
-    test "decoration does not eagerly load existing columns" do
-      Model.reset_column_information
-      assert_no_queries do
-        Model.decorate_attribute_type(:a_string, :test) { |t| StringDecorator.new(t) }
-      end
-    end
-
-    test "undecorated columns are not touched" do
-      Model.attribute :another_string, :string, default: "something or other"
-      Model.decorate_attribute_type(:a_string, :test) { |t| StringDecorator.new(t) }
-
-      assert_equal "something or other", Model.new.another_string
-    end
-
-    test "decorators can be chained" do
-      Model.decorate_attribute_type(:a_string, :test) { |t| StringDecorator.new(t) }
-      Model.decorate_attribute_type(:a_string, :other) { |t| StringDecorator.new(t) }
-
-      model = Model.new(a_string: "Hello!")
-
-      assert_equal "Hello! decorated! decorated!", model.a_string
-    end
-
-    test "decoration of the same type multiple times is idempotent" do
-      Model.decorate_attribute_type(:a_string, :test) { |t| StringDecorator.new(t) }
-      Model.decorate_attribute_type(:a_string, :test) { |t| StringDecorator.new(t) }
-
-      model = Model.new(a_string: "Hello")
-      assert_equal "Hello decorated!", model.a_string
-    end
-
-    test "decorations occur in order of declaration" do
-      Model.decorate_attribute_type(:a_string, :test) { |t| StringDecorator.new(t) }
-      Model.decorate_attribute_type(:a_string, :other) do |type|
-        StringDecorator.new(type, "decorated again!")
-      end
-
-      model = Model.new(a_string: "Hello!")
-
-      assert_equal "Hello! decorated! decorated again!", model.a_string
-    end
-
-    test "decorating attributes does not modify parent classes" do
-      Model.attribute :another_string, :string, default: "whatever"
-      Model.decorate_attribute_type(:a_string, :test) { |t| StringDecorator.new(t) }
-      child_class = Class.new(Model)
-      child_class.decorate_attribute_type(:another_string, :test) { |t| StringDecorator.new(t) }
-      child_class.decorate_attribute_type(:a_string, :other) { |t| StringDecorator.new(t) }
-
-      model = Model.new(a_string: "Hello!")
-      child = child_class.new(a_string: "Hello!")
-
-      assert_equal "Hello! decorated!", model.a_string
-      assert_equal "whatever", model.another_string
-      assert_equal "Hello! decorated! decorated!", child.a_string
-      assert_equal "whatever decorated!", child.another_string
-    end
-
-    class Multiplier < SimpleDelegator
-      def cast(value)
-        return if value.nil?
-        value * 2
-      end
-      alias deserialize cast
-    end
-
-    test "decorating with a proc" do
-      Model.attribute :an_int, :integer
-      type_is_integer = proc { |_, type| type.type == :integer }
-      Model.decorate_matching_attribute_types type_is_integer, :multiplier do |type|
-        Multiplier.new(type)
-      end
-
-      model = Model.new(a_string: "whatever", an_int: 1)
-
-      assert_equal "whatever", model.a_string
-      assert_equal 2, model.an_int
-    end
-  end
-end

activerecord/test/cases/attribute_methods/read_test.rb

@@ -14,7 +14,6 @@ module ActiveRecord
         @klass = Class.new(Class.new { def self.initialize_generated_modules; end }) do
           def self.superclass; Base; end
           def self.base_class?; true; end
-          def self.decorate_matching_attribute_types(*); end
 
           include ActiveRecord::AttributeMethods
 

activerecord/test/cases/attributes_test.rb

@@ -70,13 +70,36 @@ module ActiveRecord
 
     test "extra options are forwarded to the type caster constructor" do
       klass = Class.new(OverloadedType) do
-        attribute :starts_at, :datetime, precision: 3, limit: 2, scale: 1
+        attribute :starts_at, :datetime, precision: 3, limit: 2, scale: 1, default: -> { Time.now.utc }
       end
 
       starts_at_type = klass.type_for_attribute(:starts_at)
+
       assert_equal 3, starts_at_type.precision
       assert_equal 2, starts_at_type.limit
       assert_equal 1, starts_at_type.scale
+
+      assert_kind_of Type::DateTime, starts_at_type
+      assert_instance_of Time, klass.new.starts_at
+    end
+
+    test "time zone aware attribute" do
+      with_timezone_config aware_attributes: true, zone: "Pacific Time (US & Canada)" do
+        klass = Class.new(OverloadedType) do
+          attribute :starts_at, :datetime, precision: 3, default: -> { Time.now.utc }
+          attribute :ends_at, default: -> { Time.now.utc }
+        end
+
+        starts_at_type = klass.type_for_attribute(:starts_at)
+        ends_at_type   = klass.type_for_attribute(:ends_at)
+
+        assert_instance_of AttributeMethods::TimeZoneConversion::TimeZoneConverter, starts_at_type
+        assert_instance_of AttributeMethods::TimeZoneConversion::TimeZoneConverter, ends_at_type
+        assert_kind_of Type::DateTime, starts_at_type.__getobj__
+        assert_kind_of Type::DateTime, ends_at_type.__getobj__
+        assert_instance_of ActiveSupport::TimeWithZone, klass.new.starts_at
+        assert_instance_of ActiveSupport::TimeWithZone, klass.new.ends_at
+      end
     end
 
     test "nonexistent attribute" do

activerecord/test/schema/schema.rb

@@ -1127,6 +1127,7 @@ ActiveRecord::Schema.define do
     t.string :overloaded_string_with_limit, limit: 255
     t.string :string_with_default, default: "the original default"
     t.string :inferred_string, limit: 255
+    t.datetime :starts_at, :ends_at
   end
 
   create_table :users, force: true do |t|