Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix decorated type with type_for_attribute on the serialized attribute #41139

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ def serialize(attr_name, class_name_or_coder = Object, **options)
raise ColumnNotSerializableError.new(attr_name, cast_type)
end

cast_type = cast_type.subtype if Type::Serialized === cast_type
Type::Serialized.new(cast_type, coder)
end
end
Expand Down
40 changes: 21 additions & 19 deletions activerecord/lib/active_record/attributes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -208,21 +208,30 @@ module ClassMethods
# tracking is performed. The methods +changed?+ and +changed_in_place?+
# will be called from ActiveModel::Dirty. See the documentation for those
# methods in ActiveModel::Type::Value for more details.
def attribute(name, cast_type = nil, **options, &decorator)
def attribute(name, cast_type = nil, default: NO_DEFAULT_PROVIDED, **options, &block)
name = name.to_s
reload_schema_from_cache

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])
Comment on lines -215 to +234
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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])

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! 👍

end

# This is the low level API which sits beneath +attribute+. It only
Expand Down Expand Up @@ -255,16 +264,9 @@ def define_attribute(

def load_schema! # :nodoc:
super
attributes_to_define_after_schema_loads.each do |name, (type, options, decorator)|
if type.is_a?(Symbol)
type = ActiveRecord::Type.lookup(type, **options.except(:default), adapter: ActiveRecord::Type.adapter_name_from(self))
elsif type.nil?
type = type_for_attribute(name)
end

type = decorator[type] if decorator

define_attribute(name, type, **options.slice(:default))
attributes_to_define_after_schema_loads.each do |name, (cast_type, default)|
cast_type = cast_type[type_for_attribute(name)] if Proc === cast_type
define_attribute(name, cast_type, default: default)
end
end

Expand Down
5 changes: 4 additions & 1 deletion activerecord/lib/active_record/enum.rb
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,10 @@ def assert_valid_value(value)
end
end

attr_reader :subtype

private
attr_reader :name, :mapping, :subtype
attr_reader :name, :mapping
end

def enum(definitions)
Expand Down Expand Up @@ -182,6 +184,7 @@ def enum(definitions)
attr = attribute_alias?(name) ? attribute_alias(name) : name

attribute(attr, **default) do |subtype|
subtype = subtype.subtype if EnumType === subtype
EnumType.new(attr, enum_values, subtype)
end

Expand Down
62 changes: 62 additions & 0 deletions activerecord/test/cases/serialized_attribute_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,68 @@ def test_nil_is_always_persisted_as_null
assert_equal [topic], Topic.where(content: nil)
end

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

attr_reader :subtype, :encryptor

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

@subtype = subtype
@encryptor = ActiveSupport::MessageEncryptor.new("abcd" * 8)
end

def serialize(value)
subtype.serialize(value).yield_self do |cleartext|
encryptor.encrypt_and_sign(cleartext) unless cleartext.nil?
end
end

def deserialize(ciphertext)
encryptor.decrypt_and_verify(ciphertext)
.yield_self { |cleartext| subtype.deserialize(cleartext) } unless ciphertext.nil?
end

def changed_in_place?(old, new)
if old.nil?
!new.nil?
else
deserialize(old) != new
end
end
end

def test_decorated_type_with_type_for_attribute
old_registry = ActiveRecord::Type.registry
ActiveRecord::Type.registry = ActiveRecord::Type.registry.dup
ActiveRecord::Type.register :encrypted, EncryptedType

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

end

topic = klass.create!(content: { trial: true })

assert_equal({ "trial" => true }, topic.content)
ensure
ActiveRecord::Type.registry = old_registry
end

def test_decorated_type_with_decorator_block
klass = Class.new(ActiveRecord::Base) do
self.table_name = Topic.table_name
store :content
attribute(:content) { |subtype| EncryptedType.new(subtype: subtype) }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end

topic = klass.create!(content: { trial: true })

assert_equal({ "trial" => true }, topic.content)
end

def test_mutation_detection_does_not_double_serialize
coder = Object.new
def coder.dump(value)
Expand Down