Fix decorated type with `type_for_attribute` on the serialized attribute #41139

kamipo · 2021-01-16T06:22:16Z

The regression #41138 which is caused by #39929 is happened due to using cast_type (:encrypted) with prev_decorator (serialize in store).

Since multiple decorations (:encrypted on store on the original attribute) were never officially supported, it was just a coincidence that it worked, but I don't want to break that in exchange of allowing to ommit a type on subclasses.

IMO, even if a type on subclasses can be ommited, I'd not recommend to ommit the type on subclasses for devs.

# app/models/post.rb
class Post < ActiveRecord::Base
  attribute :x, :integer
end

# app/models/special_post.rb
class SpecialPost < Post
  # :integer can be ommited, but it is less obvious.
  attribute :x, default: 42

  # I'd recommend to not ommit the :integer for devs.
  attribute :x, :integer, default: 42
end

It is a similar point of view with 94ba417#r41923157.

Fixes #41138.

cc @georgeclaghorn @kaspth @jonathanhefner

It is a regression test for rails#41138.

…riginal-attribute-type" This reverts commit 79d0c17, reversing changes made to bc828f7. Fixes rails#41138.

jonathanhefner · 2021-01-16T15:36:31Z

@kamipo If this PR is the direction you prefer, then I am fine with that.

However, to me, it seems like EncryptedType is really a decorator, and so the encrypts macro would be appropriately written as:

def encrypts(name)
  attribute(name) { |subtype| EncryptedType.new(subtype: subtype) }
end

But this would not work currently. We could make it work by implementing decorator stacking, as I mentioned in #39929 (comment). Something like:

decorator = [prev_decorator, decorator].compact.reduce(&:>>)

Also, I realize the decorator block is a private API. However, I believe the plan is to extract EncryptedType to Rails.

…ype`

kamipo · 2021-01-17T02:54:00Z

activerecord/test/cases/serialized_attribute_test.rb

+    klass = Class.new(ActiveRecord::Base) do
+      self.table_name = Topic.table_name
+      store :content
+      attribute(:content) { |subtype| EncryptedType.new(subtype: subtype) }


I've allowed multiple decorations which can be used by such like EncryptedType in 70259f5.

jonathanhefner · 2021-01-17T17:00:15Z

activerecord/lib/active_record/attributes.rb

-        prev_cast_type, prev_options, prev_decorator = attributes_to_define_after_schema_loads[name]
+        case cast_type
+        when Symbol
+          type = cast_type
+          cast_type = -> _ { Type.lookup(type, **options, adapter: Type.adapter_name_from(self)) }
+        when nil
+          if (prev_cast_type, prev_default = attributes_to_define_after_schema_loads[name])
+            default = prev_default if default == NO_DEFAULT_PROVIDED

-        unless cast_type && prev_cast_type
-          cast_type ||= prev_cast_type
-          options = prev_options || options if options.empty?
-          decorator ||= prev_decorator
+            cast_type = if block_given?
+              -> subtype { yield Proc === prev_cast_type ? prev_cast_type[subtype] : prev_cast_type }
+            else
+              prev_cast_type
+            end
+          else
+            cast_type = block || -> subtype { subtype }
+          end
        end

-        self.attributes_to_define_after_schema_loads = attributes_to_define_after_schema_loads.merge(
-          name => [cast_type, options, decorator]
-        )
+        self.attributes_to_define_after_schema_loads =
+          attributes_to_define_after_schema_loads.merge(name => [cast_type, default])


Can we assume that attributes_to_define_after_schema_loads[name][0] will now always be either a Proc or nil? If so, what about:

prev_type_proc, prev_default = attributes_to_define_after_schema_loads[name] type_proc = if cast_type -> _ { Type.lookup(cast_type, **options, adapter: Type.adapter_name_from(self)) } else [prev_type_proc, block].compact.reduce { |f, g| -> subtype { g[f[subtype]] } } end default = prev_default if cast_type.nil? && default == NO_DEFAULT_PROVIDED self.attributes_to_define_after_schema_loads = attributes_to_define_after_schema_loads.merge(name => [type_proc, default])

The else condition could be written more neatly as [prev_type_proc, block].compact.reduce(&:>>) when we drop Ruby 2.5 support, or if we add a Proc#>> polyfill.

cast_type will always be Proc or a type object, isn't nil.

i.e. attribute :attr_name, EncryptedType.new should work as it is.

Ah, right! I forgot that case. So then perhaps:

prev_type_proc, prev_default = attributes_to_define_after_schema_loads[name] type_proc = case cast_type when nil [prev_type_proc, block].compact.reduce { |f, g| -> subtype { g[f[subtype]] } } when Symbol -> _ { Type.lookup(cast_type, **options, adapter: Type.adapter_name_from(self)) } else -> _ { cast_type } end default = prev_default if cast_type.nil? && default == NO_DEFAULT_PROVIDED self.attributes_to_define_after_schema_loads = attributes_to_define_after_schema_loads.merge(name => [type_proc, default])

default = prev_default if cast_type.nil? && default == NO_DEFAULT_PROVIDED in the above will override database default to nil accidentally, and [prev_type_proc, block].compact.reduce { |f, g| -> subtype { g[f[subtype]] } } can be nil.

Personally I'm not interested creating extra block especially when cast_type is a symbol or a type object which are the most case, just to use Proc#>> in a future.

(Wrapping a block for a symbol is to avoid bin/test testing issue. I'll fix that in a later PR.)

and [prev_type_proc, block].compact.reduce { |f, g| -> subtype { g[f[subtype]] } } can be nil.

Yes. It would require changing load_schema! from

cast_type = cast_type[type_for_attribute(name)] if Proc === cast_type

to something like

cast_type = type_for_attribute(name) cast_type = type_proc[cast_type] if type_proc

default = prev_default if cast_type.nil? && default == NO_DEFAULT_PROVIDED in the above will override database default to nil accidentally,

That could be addressed by adding a default value of [nil, NO_DEFAULT_PROVIDED] to the attributes_to_define_after_schema_loads Hash.

However, it seems like you have plans for this code, so let's do the way that you prefer! 👍

kaspth · 2021-01-19T07:00:34Z

activerecord/test/cases/serialized_attribute_test.rb

+    klass = Class.new(ActiveRecord::Base) do
+      self.table_name = Topic.table_name
+      store :content
+      attribute :content, :encrypted, subtype: type_for_attribute(:content)


Do users have to provide a subtype or can it be inferred automatically? Feels like attribute should automatically cascade the types.

Do you mean that we should provide a way to inject a subtype to the type which has subtype option?

rails/activerecord/test/cases/serialized_attribute_test.rb

Lines 395 to 400 in 70259f5

class EncryptedType < ActiveRecord::Type::Text

include ActiveModel::Type::Helpers::Mutable

attr_reader :subtype, :encryptor

def initialize(subtype: ActiveModel::Type::String.new)

rails/activerecord/test/cases/serialized_attribute_test.rb

Lines 446 to 450 in 70259f5

klass = Class.new(ActiveRecord::Base) do

self.table_name = Topic.table_name

store :content

attribute(:content) { |subtype| EncryptedType.new(subtype: subtype) }

end

I'm not sure what you mean by "automatically cascade the types".

In 6.1, there is no reliable way. If an attribute isn't decorated (no enum, serialize, store are used), attribute(:content) { |subtype| EncryptedType.new(subtype: subtype) }, and decorate_attribute_type("content") { |subtype| EncryptedType.new(subtype: subtype) } works.

In main branch, attribute(:content) { |subtype| EncryptedType.new(subtype: subtype) } works even if enum, serialize, store are used on the attribute (decorate_attribute_type is removed in the branch).

If we provide a way for decorator officially, I feel that it is better to provide the API separately (e.g. decorate_attribute etc) from the attribute.

Wups, sorry, I forgot to get back to this! I'd just like to be able to do:

store :content attribute :content, :encrypted

without needing to pass the subtype, attribute :content, :encrypted, subtype: type_for_attribute(:content).

If we provide a way for decorator officially, I feel that it is better to provide the API separately (e.g. decorate_attribute etc) from the attribute.

That also sounds good 👍

georgeclaghorn · 2021-01-19T17:54:01Z

Thank you both!

Just an implementation for rails#41139 (comment).

…_with_serialized_attribute" This reverts commit 60e979d, reversing changes made to db79da6.

kamipo added 2 commits January 16, 2021 14:40

Add a regression test for decorated type with type_for_attribute

b5ab89b

It is a regression test for rails#41138.

Revert "Merge pull request rails#39929 from jonathanhefner/decorate-o…

5fc0c4c

…riginal-attribute-type" This reverts commit 79d0c17, reversing changes made to bc828f7. Fixes rails#41138.

rails-bot bot added the activerecord label Jan 16, 2021

Allow multiple decorations which can be used by such like `EncryptedT…

70259f5

…ype`

kamipo force-pushed the fix_decorated_type_with_serialized_attribute branch from 21af293 to 70259f5 Compare January 17, 2021 02:39

kamipo commented Jan 17, 2021

View reviewed changes

jonathanhefner reviewed Jan 17, 2021

View reviewed changes

kamipo merged commit 60e979d into rails:main Jan 19, 2021

kamipo deleted the fix_decorated_type_with_serialized_attribute branch January 19, 2021 06:26

kaspth reviewed Jan 19, 2021

View reviewed changes

kamipo added a commit to kamipo/rails that referenced this pull request Jan 28, 2021

Provide a way decorate_attribute for attribute type decoration

c723252

Just an implementation for rails#41139 (comment).

kamipo mentioned this pull request Jan 28, 2021

Provide a way decorate_attribute for attribute type decoration #41262

Closed

etiennebarrie added a commit to etiennebarrie/rails that referenced this pull request Feb 11, 2021

Revert "Merge pull request rails#41139 from kamipo/fix_decorated_type…

f670b4c

…_with_serialized_attribute" This reverts commit 60e979d, reversing changes made to db79da6.

kamipo mentioned this pull request Apr 19, 2024

Fix some more ignored block warnings #51608

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix decorated type with `type_for_attribute` on the serialized attribute #41139

Fix decorated type with `type_for_attribute` on the serialized attribute #41139

kamipo commented Jan 16, 2021

jonathanhefner commented Jan 16, 2021

kamipo Jan 17, 2021

jonathanhefner Jan 17, 2021

kamipo Jan 17, 2021

kamipo Jan 17, 2021

jonathanhefner Jan 18, 2021

kamipo Jan 18, 2021

jonathanhefner Jan 18, 2021

kaspth Jan 19, 2021

kamipo Jan 19, 2021

kamipo Jan 19, 2021

kaspth Jan 28, 2021

georgeclaghorn commented Jan 19, 2021

	class EncryptedType < ActiveRecord::Type::Text
	include ActiveModel::Type::Helpers::Mutable

	attr_reader :subtype, :encryptor

	def initialize(subtype: ActiveModel::Type::String.new)

	klass = Class.new(ActiveRecord::Base) do
	self.table_name = Topic.table_name
	store :content
	attribute(:content) { \|subtype\| EncryptedType.new(subtype: subtype) }
	end

Fix decorated type with type_for_attribute on the serialized attribute #41139

Fix decorated type with type_for_attribute on the serialized attribute #41139

Conversation

kamipo commented Jan 16, 2021

jonathanhefner commented Jan 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgeclaghorn commented Jan 19, 2021

Fix decorated type with `type_for_attribute` on the serialized attribute #41139

Fix decorated type with `type_for_attribute` on the serialized attribute #41139