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

Prevent possible duplicated attributes in serializer #914

Merged
merged 1 commit into from
May 18, 2015

Conversation

groyoh
Copy link
Member

@groyoh groyoh commented May 18, 2015

This PR is based on #904 .

The issue was that calling ActiveModel::Serializer.attributes or ActiveModel::Serializer.attribute methods multiple times with the same attribute would add this attribute again and again to the @_attributes variable. As the attribute method is called by the FragmentCache multiple times, this might be a important issue.

Calling ActiveModel::Serializer.attributes or ActiveModel::Serializer.attribute
methods multiple times won't create duplicated attributes anymore.
@joaomdmoura
Copy link
Member

Indeed this a important issue! I'm deeply interest on this.
Tests falling, I'm restarting it.

@joaomdmoura joaomdmoura added this to the 0.10 milestone May 18, 2015
joaomdmoura added a commit that referenced this pull request May 18, 2015
Prevent possible duplicated attributes in serializer
@joaomdmoura joaomdmoura merged commit a59cc4c into rails-api:master May 18, 2015
@@ -31,6 +31,7 @@ def self.inherited(base)
def self.attributes(*attrs)
attrs = attrs.first if attrs.first.class == Array
@_attributes.concat attrs
@_attributes.uniq!
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we need to worry about the uniq! here

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I can see 3 use cases:

  • a user calls attributes :title, :title
  • a user calls:
attributes :title
attributes :title
  • a future feature to call attributes in a similar way the fragment cache does

The first two are not really serious, but the last one might go unseen like the fragment cache.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants