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

ActiveRecord::Aggregations#composed_of splats Hash values instead of assigning Hash #25978

Closed
olivierlacan opened this Issue Jul 28, 2016 · 8 comments

Comments

Projects
None yet
3 participants
@olivierlacan
Contributor

olivierlacan commented Jul 28, 2016

Steps to reproduce

  • define composed_of on a model with a preferences text column with a mapping set to a method that returns a Hash instance.
  • point the class_name argument to a class (e.g. User::Preference) whose initializer expects a single argument
  • assign `model.preferences = { thing: false, stuff: true }
  • ArgumentError 2 for 1

See the repro app here: https://github.com/olivierlacan/rails-4-2-composed-of-hash-values-splat

This "bug" (debatable) was introduced by: 36e9be8

It wasn't resolved by 0d5d859 which resolved a different issue caused by the above commit.

I promise I really like @sgrif, it's just very surprising behavior and I don't think composed_of should do that. Furthermore, there was no deprecation policy announcing this breaking change.

Expected behavior

I would expect composed_of's dynamic writer to assign my damn Hash instance to my damn text column. 😄

Actual behavior

ArgumentError because composed_of's writer_method is splatting the Hash#values and sending them to klass.new (in my example User::Preferences.new) instead of sending the full Hash.

System configuration

Rails version: 4.2.7

Ruby version: 2.3.1

@olivierlacan

This comment has been minimized.

Show comment
Hide comment
@olivierlacan

olivierlacan Jul 28, 2016

Contributor

I feel like I'm being devil's advocate here. I will definitely spray composed_of with fire in our codebase and replace it with ActiveRecord::Store. It's just that we used composed_ofs class_name argument to define a class with default values. Something I basically have to rebuild as a specific coder with store.

I wouldn't cry too much if you classified this as a "won't fix", it's just a hurdle to the upgrade path I really didn't expect.

Contributor

olivierlacan commented Jul 28, 2016

I feel like I'm being devil's advocate here. I will definitely spray composed_of with fire in our codebase and replace it with ActiveRecord::Store. It's just that we used composed_ofs class_name argument to define a class with default values. Something I basically have to rebuild as a specific coder with store.

I wouldn't cry too much if you classified this as a "won't fix", it's just a hurdle to the upgrade path I really didn't expect.

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Jul 28, 2016

Member

I promise I really like @sgrif

Suuuuuuuuuuuuuuuuuuuure you do.

Member

sgrif commented Jul 28, 2016

I promise I really like @sgrif

Suuuuuuuuuuuuuuuuuuuure you do.

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Jul 28, 2016

Member

Just to be clear, your example worked in 4.1, correct?

Member

sgrif commented Jul 28, 2016

Just to be clear, your example worked in 4.1, correct?

@sgrif sgrif self-assigned this Jul 28, 2016

@olivierlacan

This comment has been minimized.

Show comment
Hide comment
@olivierlacan

olivierlacan Jul 28, 2016

Contributor

@sgrif It did, yes. But I can do a 4.1 repro app if you want. :-)

Contributor

olivierlacan commented Jul 28, 2016

@sgrif It did, yes. But I can do a 4.1 repro app if you want. :-)

@olivierlacan

This comment has been minimized.

Show comment
Hide comment
@olivierlacan

olivierlacan Jul 28, 2016

Contributor

@sgrif Confirmed without a repro app since the Hash check is absent in 4-1-stable:

def writer_method(name, class_name, mapping, allow_nil, converter)
define_method("#{name}=") do |part|
klass = class_name.constantize
unless part.is_a?(klass) || converter.nil? || part.nil?
part = converter.respond_to?(:call) ? converter.call(part) : klass.send(converter, part)
end
if part.nil? && allow_nil
mapping.each { |pair| self[pair.first] = nil }
@aggregation_cache[name] = nil
else
mapping.each { |pair| self[pair.first] = part.send(pair.last) }
@aggregation_cache[name] = part.freeze
end
end
end

Contributor

olivierlacan commented Jul 28, 2016

@sgrif Confirmed without a repro app since the Hash check is absent in 4-1-stable:

def writer_method(name, class_name, mapping, allow_nil, converter)
define_method("#{name}=") do |part|
klass = class_name.constantize
unless part.is_a?(klass) || converter.nil? || part.nil?
part = converter.respond_to?(:call) ? converter.call(part) : klass.send(converter, part)
end
if part.nil? && allow_nil
mapping.each { |pair| self[pair.first] = nil }
@aggregation_cache[name] = nil
else
mapping.each { |pair| self[pair.first] = part.send(pair.last) }
@aggregation_cache[name] = part.freeze
end
end
end

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Jul 28, 2016

Member

Thanks. I'll look into it tomorrow.

Member

sgrif commented Jul 28, 2016

Thanks. I'll look into it tomorrow.

sgrif added a commit to sgrif/rails that referenced this issue Aug 5, 2016

Don't assume all hashes are from multiparameter assignment in `compos…
…ed_of`

So this bug is kinda funky. The code path is basically "if we weren't passed an
instance of the class we compose to, and we have a converter, call that".
Ignoring the hash case for a moment, everything after that was roughly intended
to be the "else" clause, meaning that we are expected to have an instance of
the class we compose to. Really, we should be blowing up in that case, as we
can give a much better error message than what they user will likely get (e.g.
`NameError: No method first for String` or something). Still, Ruby is duck
typed, so if the object you're assigning responds to the same methods as the
type you compose to, knock yourself out.

The hash case was added in 36e9be8 to remove a bunch of special cased code from
multiparameter assignment. I wrongly assumed that the only time we'd get a hash
there is in that case. Multiparameter assignment will construct a very specific
hash though, where the keys are integers, and we will have a set of keys
covering `1..part.size` exactly. I'm pretty sure this could actually be passed
around as an array, but that's a different story. Really I should convert this
to something like `class MultiParameterAssignment < Hash; end`, which I might
do soon. However for a change that I'm willing to backport to 4-2-stable, this
is what I want to go with for the time being.

Fixes #25978

@sgrif sgrif closed this in b63d532 Aug 5, 2016

sgrif added a commit to sgrif/rails that referenced this issue Aug 5, 2016

Don't assume all hashes are from multiparameter assignment in `compos…
…ed_of`

So this bug is kinda funky. The code path is basically "if we weren't passed an
instance of the class we compose to, and we have a converter, call that".
Ignoring the hash case for a moment, everything after that was roughly intended
to be the "else" clause, meaning that we are expected to have an instance of
the class we compose to. Really, we should be blowing up in that case, as we
can give a much better error message than what they user will likely get (e.g.
`NameError: No method first for String` or something). Still, Ruby is duck
typed, so if the object you're assigning responds to the same methods as the
type you compose to, knock yourself out.

The hash case was added in 36e9be8 to remove a bunch of special cased code from
multiparameter assignment. I wrongly assumed that the only time we'd get a hash
there is in that case. Multiparameter assignment will construct a very specific
hash though, where the keys are integers, and we will have a set of keys
covering `1..part.size` exactly. I'm pretty sure this could actually be passed
around as an array, but that's a different story. Really I should convert this
to something like `class MultiParameterAssignment < Hash; end`, which I might
do soon. However for a change that I'm willing to backport to 4-2-stable, this
is what I want to go with for the time being.

Fixes #25978
@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Aug 5, 2016

Member

Backported to 4.2 in #26062

Member

sgrif commented Aug 5, 2016

Backported to 4.2 in #26062

sgrif added a commit to sgrif/rails that referenced this issue Aug 5, 2016

Don't assume all hashes are from multiparameter assignment in `compos…
…ed_of`

So this bug is kinda funky. The code path is basically "if we weren't passed an
instance of the class we compose to, and we have a converter, call that".
Ignoring the hash case for a moment, everything after that was roughly intended
to be the "else" clause, meaning that we are expected to have an instance of
the class we compose to. Really, we should be blowing up in that case, as we
can give a much better error message than what they user will likely get (e.g.
`NameError: No method first for String` or something). Still, Ruby is duck
typed, so if the object you're assigning responds to the same methods as the
type you compose to, knock yourself out.

The hash case was added in 36e9be8 to remove a bunch of special cased code from
multiparameter assignment. I wrongly assumed that the only time we'd get a hash
there is in that case. Multiparameter assignment will construct a very specific
hash though, where the keys are integers, and we will have a set of keys
covering `1..part.size` exactly. I'm pretty sure this could actually be passed
around as an array, but that's a different story. Really I should convert this
to something like `class MultiParameterAssignment < Hash; end`, which I might
do soon. However for a change that I'm willing to backport to 4-2-stable, this
is what I want to go with for the time being.

Fixes #25978
@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Aug 5, 2016

Member

Backported to 5.0 in #26063

Member

sgrif commented Aug 5, 2016

Backported to 5.0 in #26063

MichaelSp added a commit to MichaelSp/rails that referenced this issue Nov 2, 2016

Don't assume all hashes are from multiparameter assignment in `compos…
…ed_of`

So this bug is kinda funky. The code path is basically "if we weren't passed an
instance of the class we compose to, and we have a converter, call that".
Ignoring the hash case for a moment, everything after that was roughly intended
to be the "else" clause, meaning that we are expected to have an instance of
the class we compose to. Really, we should be blowing up in that case, as we
can give a much better error message than what they user will likely get (e.g.
`NameError: No method first for String` or something). Still, Ruby is duck
typed, so if the object you're assigning responds to the same methods as the
type you compose to, knock yourself out.

The hash case was added in 36e9be8 to remove a bunch of special cased code from
multiparameter assignment. I wrongly assumed that the only time we'd get a hash
there is in that case. Multiparameter assignment will construct a very specific
hash though, where the keys are integers, and we will have a set of keys
covering `1..part.size` exactly. I'm pretty sure this could actually be passed
around as an array, but that's a different story. Really I should convert this
to something like `class MultiParameterAssignment < Hash; end`, which I might
do soon. However for a change that I'm willing to backport to 4-2-stable, this
is what I want to go with for the time being.

Fixes #25978
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment