Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Parameter wrapping doesn't support aliased attributes #10375

Closed
wants to merge 2 commits into from

5 participants

@vanstee

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
Collaborator
  • We need a CHANGELOG entry for this.
  • Would you mind squash your commits together as well?
@vanstee

@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
Collaborator

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

@vanstee

Thanks @robin850.

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 Owner
jeremy added a note

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy
Owner

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

@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

@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

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.

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 Owner
jeremy added a note

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

@vanstee
vanstee added a note

Ah. You're right! Fixin'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 Owner
jeremy added a note

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

@vanstee
vanstee added a note

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy
Owner

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

@vanstee

@jeremy I'm pretty happy with the name attribute_method_names :+1: 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
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 Owner
jeremy added a note

Why do we need to check any? before assigning?

@vanstee
vanstee added a note

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy
Owner

No need to start with memoization, IMO.

@vanstee vanstee commented on the diff
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 added a note

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@vanstee

@jeremy Anything else that needs fixing?

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 Owner
jeremy added a note

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 added a note

Yeah, that makes sense. Fixing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@vanstee

@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

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
@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

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

@vanstee

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

@vanstee vanstee closed this
@vanstee vanstee deleted the vanstee:wrap-aliased-parameters branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 18, 2013
  1. @vanstee

    Add method returning attribute names and aliases

    vanstee authored vanstee committed
    This new api was added to help with wrapping all parameters related to a
    model (both parameters matching attribute names and aliases).
  2. @vanstee

    Use `attribute_method_names` to wrap parameters

    vanstee authored
    We also want to slice out the names of aliased attributes when wrapping
    up params.
This page is out of date. Refresh to see the latest.
View
4 actionpack/CHANGELOG.md
@@ -35,4 +35,8 @@
*Bryan Ricker*
+* Add support for wrapping parameters matching aliased attribute names.
+
+ *Patrick Van Stee*
+
Please check [4-0-stable](https://github.com/rails/rails/blob/4-0-stable/actionpack/CHANGELOG.md) for previous changes.
View
6 actionpack/lib/action_controller/metal/params_wrapper.rb
@@ -44,7 +44,7 @@ module ActionController
#
# On ActiveRecord models with no +:include+ or +:exclude+ option set,
# it will only wrap the parameters returned by the class method
- # <tt>attribute_names</tt>.
+ # <tt>attribute_method_names</tt>.
#
# If you're going to pass the parameters to an +ActiveModel+ object (such as
# <tt>User.new(params[:user])</tt>), you might consider passing the model class to
@@ -106,8 +106,8 @@ def include
@include_set = true
unless super || exclude
- if m.respond_to?(:attribute_names) && m.attribute_names.any?
@vanstee
vanstee added a note

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- self.include = m.attribute_names
+ if m.respond_to?(:attribute_method_names) && m.attribute_method_names.any?
@jeremy Owner
jeremy added a note

Why do we need to check any? before assigning?

@vanstee
vanstee added a note

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ self.include = m.attribute_method_names
end
end
end
View
18 actionpack/test/controller/params_wrapper_test.rb
@@ -155,8 +155,8 @@ def test_nested_params
end
def test_derived_wrapped_keys_from_matching_model
- User.expects(:respond_to?).with(:attribute_names).returns(true)
- User.expects(:attribute_names).twice.returns(["username"])
+ User.expects(:respond_to?).with(:attribute_method_names).returns(true)
+ User.expects(:attribute_method_names).twice.returns(["username"])
with_default_wrapper_options do
@request.env['CONTENT_TYPE'] = 'application/json'
@@ -167,8 +167,8 @@ def test_derived_wrapped_keys_from_matching_model
def test_derived_wrapped_keys_from_specified_model
with_default_wrapper_options do
- Person.expects(:respond_to?).with(:attribute_names).returns(true)
- Person.expects(:attribute_names).twice.returns(["username"])
+ Person.expects(:respond_to?).with(:attribute_method_names).returns(true)
+ Person.expects(:attribute_method_names).twice.returns(["username"])
UsersController.wrap_parameters Person
@@ -179,8 +179,8 @@ def test_derived_wrapped_keys_from_specified_model
end
def test_not_wrapping_abstract_model
- User.expects(:respond_to?).with(:attribute_names).returns(true)
- User.expects(:attribute_names).returns([])
+ User.expects(:respond_to?).with(:attribute_method_names).returns(true)
+ User.expects(:attribute_method_names).returns([])
with_default_wrapper_options do
@request.env['CONTENT_TYPE'] = 'application/json'
@@ -209,13 +209,13 @@ def parse
end
class SampleOne
- def self.attribute_names
+ def self.attribute_method_names
["username"]
end
end
class SampleTwo
- def self.attribute_names
+ def self.attribute_method_names
["title"]
end
end
@@ -298,7 +298,7 @@ class IrregularInflectionParamsWrapperTest < ActionController::TestCase
include ParamsWrapperTestHelp
class ParamswrappernewsItem
- def self.attribute_names
+ def self.attribute_method_names
['test_attr']
end
end
View
5 activemodel/CHANGELOG.md
@@ -7,4 +7,9 @@
*Nick Sutterer*
+* Add the `ActiveModel#attribute_method_names` method, which returns an
+ array of attribute names and aliases as strings.
+
+ *Patrick Van Stee*
+
Please check [4-0-stable](https://github.com/rails/rails/blob/4-0-stable/activemodel/CHANGELOG.md) for previous changes.
View
26 activemodel/lib/active_model/attribute_methods.rb
@@ -72,9 +72,10 @@ module AttributeMethods
CALL_COMPILABLE_REGEXP = /\A[a-zA-Z_]\w*[!?]?\z/
included do
- class_attribute :attribute_aliases, :attribute_method_matchers, instance_writer: false
+ class_attribute :attribute_aliases, :attribute_method_matchers, :defined_attribute_names, instance_writer: false
self.attribute_aliases = {}
self.attribute_method_matchers = [ClassMethods::AttributeMethodMatcher.new]
+ self.defined_attribute_names = []
end
module ClassMethods
@@ -276,6 +277,10 @@ def define_attribute_methods(*attr_names)
# person.name # => "Bob"
# person.name_short? # => true
def define_attribute_method(attr_name)
+ attr_name = attr_name.to_s
+
+ self.defined_attribute_names << attr_name unless defined_attribute_names.include?(attr_name)
+
attribute_method_matchers.each do |matcher|
method_name = matcher.method_name(attr_name)
@@ -285,10 +290,11 @@ def define_attribute_method(attr_name)
if respond_to?(generate_method, true)
send(generate_method, attr_name)
else
- define_proxy_call true, generated_attribute_methods, method_name, matcher.method_missing_target, attr_name.to_s
+ define_proxy_call true, generated_attribute_methods, method_name, matcher.method_missing_target, attr_name
end
end
end
+
attribute_method_matchers_cache.clear
end
@@ -322,6 +328,22 @@ def undefine_attribute_methods
attribute_method_matchers_cache.clear
end
+ # Returns an array of attribute names and aliases as strings.
+ #
+ # class Person
+ # include ActiveModel::AttributeMethods
+ #
+ # define_attribute_method :name
+ #
+ # alias_attribute :nickname, :name
+ # end
+ #
+ # Person.attribute_method_names
+ # # => ["name", "nickname"]
+ def attribute_method_names
+ defined_attribute_names + attribute_aliases.keys
+ end
+
# Returns true if the attribute methods defined have been generated.
def generated_attribute_methods #:nodoc:
@generated_attribute_methods ||= Module.new.tap { |mod| include mod }
View
9 activemodel/test/cases/attribute_methods_test.rb
@@ -186,6 +186,15 @@ def foo
assert_equal "value of end", ModelWithRubyKeywordNamedAttributes.new.to
end
+ test '#attribute_method_names returns attribute names and attribute aliases' do
+ klass = Class.new(ModelWithAttributes) do
+ define_attribute_methods :foo
+ alias_attribute :bar, :foo
+ end
+
+ assert_equal ["foo", "bar"], klass.attribute_method_names
+ end
+
test '#undefine_attribute_methods removes attribute methods' do
ModelWithAttributes.define_attribute_methods(:foo)
ModelWithAttributes.undefine_attribute_methods
View
2  railties/test/application/configuration_test.rb
@@ -514,7 +514,7 @@ def index
app_file 'app/models/post.rb', <<-RUBY
class Post
- def self.attribute_names
+ def self.attribute_method_names
%w(title)
end
end
Something went wrong with that request. Please try again.