Parameter wrapping doesn't support aliased attributes #10375

Closed
wants to merge 2 commits into
from

Projects

None yet

5 participants

@vanstee
vanstee commented Apr 29, 2013

When wrapping parameters, only the attribute_names were sliced out. I was hoping we could also support wrapping aliased attribute names as well. To support them, I just included attribute_aliases.keys in the list of keys to wrap for the model.

@sikachu
Member
sikachu commented Apr 30, 2013
  • We need a CHANGELOG entry for this.
  • Would you mind squash your commits together as well?
@vanstee
vanstee commented Apr 30, 2013

@sikachu Done!

I added the Rails 4.1.0.beta header to the CHANGELOG since it was empty. Not sure if that's correct or not.

@robin850
Member
robin850 commented May 1, 2013

@vanstee : Apparently the header on top of the CHANGELOG is now deprecated.

@vanstee
vanstee commented May 1, 2013

Thanks @robin850.

@jeremy jeremy commented on an outdated diff May 1, 2013
actionpack/lib/action_controller/metal/params_wrapper.rb
@@ -106,8 +106,12 @@ def include
@include_set = true
unless super || exclude
- if m.respond_to?(:attribute_names) && m.attribute_names.any?
- self.include = m.attribute_names
+ attribute_names_and_aliases = []
+ attribute_names_and_aliases += m.attribute_names if m.respond_to?(:attribute_names)
+ attribute_names_and_aliases += m.attribute_aliases.keys if m.respond_to?(:attribute_aliases)
@jeremy
jeremy May 1, 2013 Ruby on Rails member

Embedding these details about aliases into the params wrapper feels like it's misallocating responsibility.

Perhaps the model should return an array of attributes it responds to with a single method call. Like #attribute_names...

@jeremy
Member
jeremy commented May 1, 2013

I think the problem here isn't that the params wrapper doesn't support aliased attributes, but that the model doesn't present aliases in its list of attribute names.

@vanstee
vanstee commented May 1, 2013

@jeremy Yeah, let me see what all breaks if I include aliases in the attribute_names array. That would totally be a cleaner solution.

@vanstee
vanstee commented May 8, 2013

@jeremy Since changing attribute_names to include aliases seems to be an entirely different approach and sorta unrelated to the params wrapping problem I opened a separate PR: #10518

I'll close this one once we figure out what to do with that.

@vanstee
vanstee commented May 9, 2013

Ok, added a new attribute_names_and_aliases method which is then used in the params wrapper. I'm not too happy with the name attribute_names_and_aliases but I couldn't come up with anything better :/ Any suggestions?

Oh and should I still squash this up even though there are two separate CHANGELOG entries? Separate PRs seemed overkill I guess.

@jeremy jeremy and 1 other commented on an outdated diff May 9, 2013
activerecord/lib/active_record/attribute_methods.rb
@@ -114,6 +114,18 @@ def attribute_names
[]
end
end
+
+ # Returns an array of attribute names and aliases as strings.
+ #
+ # class Person < ActiveRecord::Base
+ # attribute_alias :nickname, :name
+ # end
+ #
+ # Person.attribute_names_and_aliases
+ # # => ["id", "created_at", "updated_at", "name", "age", "nickname"]
+ def attribute_names_and_aliases
+ @attribute_names_and_aliases ||= attribute_names + attribute_aliases.keys
@jeremy
jeremy May 9, 2013 Ruby on Rails member

Memoizing this means it'll go stale after you add an alias.

@vanstee
vanstee May 9, 2013

Ah. You're right! Fixin'

@jeremy jeremy and 1 other commented on an outdated diff May 9, 2013
activerecord/lib/active_record/attribute_methods.rb
@@ -114,6 +114,18 @@ def attribute_names
[]
end
end
+
+ # Returns an array of attribute names and aliases as strings.
+ #
+ # class Person < ActiveRecord::Base
+ # attribute_alias :nickname, :name
+ # end
+ #
+ # Person.attribute_names_and_aliases
+ # # => ["id", "created_at", "updated_at", "name", "age", "nickname"]
+ def attribute_names_and_aliases
@jeremy
jeremy May 9, 2013 Ruby on Rails member

What's in common here? They're names of attribute methods. How about attribute_method_names?

@vanstee
vanstee May 9, 2013

Yeah that's not bad. I thought about attribute_accessor_names as well. I'll try out attribute_method_names and see how it feels.

@jeremy
Member
jeremy commented May 9, 2013

One PR is cool as long as the changes are two distinct commits, as you have them 😁

@vanstee
vanstee commented May 10, 2013

@jeremy I'm pretty happy with the name attribute_method_names 👍 I nuked the memorization for now. Do you think it's worth it to get that working for this PR?

@jeremy jeremy commented on the diff May 10, 2013
actionpack/lib/action_controller/metal/params_wrapper.rb
@@ -106,8 +106,8 @@ def include
@include_set = true
unless super || exclude
- if m.respond_to?(:attribute_names) && m.attribute_names.any?
- self.include = m.attribute_names
+ if m.respond_to?(:attribute_method_names) && m.attribute_method_names.any?
@jeremy
jeremy May 10, 2013 Ruby on Rails member

Why do we need to check any? before assigning?

@vanstee
vanstee May 10, 2013

I'm not actually sure. It seems like you could unintentionally set self.include to nil considering we set @include_set to true outside the conditional. Hrm. I'll try removing that.

@jeremy
Member
jeremy commented May 10, 2013

No need to start with memoization, IMO.

@vanstee vanstee commented on the diff May 10, 2013
actionpack/lib/action_controller/metal/params_wrapper.rb
@@ -106,8 +106,8 @@ def include
@include_set = true
unless super || exclude
- if m.respond_to?(:attribute_names) && m.attribute_names.any?
@vanstee
vanstee May 10, 2013

This change seems to be causing a failure here: https://github.com/rails/rails/blob/master/actionpack/test/controller/params_wrapper_test.rb#L181-L191

Seems interesting. I guess if you have an abstract model, the attribute_names array would be empty so you fallback to including everything?

@vanstee
vanstee commented May 14, 2013

@jeremy Anything else that needs fixing?

@jeremy jeremy and 1 other commented on an outdated diff May 14, 2013
activerecord/lib/active_record/attribute_methods.rb
@@ -114,6 +114,18 @@ def attribute_names
[]
end
end
+
+ # Returns an array of attribute names and aliases as strings.
+ #
+ # class Person < ActiveRecord::Base
+ # attribute_alias :nickname, :name
+ # end
+ #
+ # Person.attribute_method_names
+ # # => ["id", "created_at", "updated_at", "name", "age", "nickname"]
+ def attribute_method_names
+ attribute_names + attribute_aliases.keys
+ end
@jeremy
jeremy May 14, 2013 Ruby on Rails member

At a high level, attributes and aliases are Active Model's responsibility. It doesn't have an attribute_names method yet, but we could add it along with this method.

@vanstee
vanstee May 25, 2013

Yeah, that makes sense. Fixing!

@vanstee
vanstee commented May 25, 2013

@jeremy Added an attribute_names instance variable that adds on each attribute added with define_attribute_method and moved over the attribute_method_names method from ActiveRecord::AttributeMethods. Looking better?

@vanstee
vanstee commented May 25, 2013

Also if this gets merged in I'll probably submit a follow up PR to switch out some of the instances of column_names with attribute_names.

vanstee added some commits May 8, 2013
@vanstee vanstee Add method returning attribute names and aliases
This new api was added to help with wrapping all parameters related to a
model (both parameters matching attribute names and aliases).
217e902
@vanstee vanstee Use `attribute_method_names` to wrap parameters
We also want to slice out the names of aliased attributes when wrapping
up params.
0c4934b
@mikegee
mikegee commented May 22, 2014

This is quite stale now. Do we want to try to get it merged, or just close it?

@vanstee
vanstee commented May 22, 2014

Still seems to be a problem, but let's just close it for now.

@vanstee vanstee closed this May 22, 2014
@vanstee vanstee deleted the vanstee:wrap-aliased-parameters branch May 22, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment