Skip to content

Attribute filtering for 0.10.x #1141

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Attribute filtering for 0.10.x #1141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ Features:
- [#1158](https://github.com/rails-api/active_model_serializers/pull/1158) Add support for wildcards in `include` option (@beauby)
- [#1127](https://github.com/rails-api/active_model_serializers/pull/1127) Add support for nested
associations for JSON and Attributes adapters via the `include` option (@NullVoxPopuli, @beauby).
- [#1141](https://github.com/rails-api/active_model_serializers/pull/1141) Add support for
dynamically including or excluding attributes in serializers (@lautis).

Fixes:

Expand Down
20 changes: 20 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,26 @@ class PostSerializer < ActiveModel::Serializer
end
```

### Conditionally including or excluding attributes

You can also dynamically include or exclude serialized attributes by defining a
method named include_attributes. This is typically used to customize the output
based on `current_user`. For example:

```ruby
class PostSerializer < ActiveModel::Serializer
attributes :id, :body, :author

def include_attributes(included)
if current_user.admin?
included
else
included - [:author]
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this a lot of work just to be able to

def attributes(*)
  super.tap do |attrs|
    # not sure where current_user is coming from.. 
    # is it an attribute on the object? 
    # an accessor on the serializer?
    attrs.delete(:author) unless current_user.admin? 
  end

I think the better approach would be just OO

class PostSerializer < AM::S
  attributes :id, :body
end
class AdminPostSerializer < PostSerializer
  attributes: author
end

I really just don't think this is a problem that needs to be solved in AMS

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 pretty 👎 on this till I'm convinced it's necessary, helpful, domain-appropriate, and doesn't introduce unnecessary complexity to AMS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't this a lot of work just to be able to

def attributes(*)
  super.tap do |attrs|
    # not sure where current_user is coming from.. 
    # is it an attribute on the object? 
    # an accessor on the serializer?
    attrs.delete(:author) unless current_user.admin? 
  end

The PR is different as it allows serializer to skip the computation of the attributes. Overriding attributes does not work when you're concerned about performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the OO approach, how could that be easily used with has_many? Is it possible to change the serializer for relationships dynamically?

Copy link
Member

Choose a reason for hiding this comment

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

Example plz

end
```

### Built in Adapters

#### Attributes
Expand Down
6 changes: 5 additions & 1 deletion lib/active_model/serializer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,10 @@ def json_key
root || object.class.model_name.to_s.underscore
end

def include_attributes(included)
included
end
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to narrow the Serializer interface rather than broaden it. Every method we add has the potential to interfere with an attribute on the model.

Also, with respect to this change, couldn't you just use the key option?

class FooSerializer < AM::Serializer
  attribute :name, key: title
end
FooSerializer.new(foo).attributes
FooSerializer._attributes_keys
FooSerializer._attributes


def attributes(options = {})
attributes =
if options[:fields]
Expand All @@ -151,7 +155,7 @@ def attributes(options = {})
self.class._attributes.dup
end

attributes.each_with_object({}) do |name, hash|
include_attributes(attributes).each_with_object({}) do |name, hash|
unless self.class._fragmented
hash[name] = send(name)
else
Expand Down
20 changes: 20 additions & 0 deletions test/serializers/attributes_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,31 @@ def test_attributes_definition
@profile_serializer.class._attributes)
end

def test_attributes
assert_equal({ name: 'Name 1', description: 'Description 1' },
@profile_serializer.attributes)
end

def test_attributes_with_fields_option
assert_equal({ name: 'Name 1' },
@profile_serializer.attributes(fields: [:name]))
end

def test_attributes_with_fields_method
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add a couple more tests with a couple more scenarios?
:-)

Copy link
Contributor

Choose a reason for hiding this comment

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

like, what happens when there are no fields on the serializer?, etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fields method case could be more explicitly tested. There is already a case that covers that behaviour, though (https://github.com/lautis/active_model_serializers/blob/fields-filtering/test/serializers/attributes_test.rb#L39).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added test cases to cover no overridden fields and combination of fields method and option.

@profile_serializer.define_singleton_method(:include_attributes) do |included|
included - [:name]
end
assert_equal({ description: 'Description 1' },
@profile_serializer.attributes)
end

def test_attributes_with_fields_option_and_method
@profile_serializer.define_singleton_method(:include_attributes) do |included|
included - [:name]
end
assert_equal({}, @profile_serializer.attributes(fields: [:name]))
Copy link
Member

Choose a reason for hiding this comment

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

You should pretty much never be using AMS this way 👎

Copy link
Member

Choose a reason for hiding this comment

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

Also, as I wrote earlier, #1141 (comment) fields is already available to the adapter and so this PR will only work for people getting attributes off the serializer directly which is not something this library, IMHO, supports as a primary API (outside of testing the serializer gives you what you expect).

end

def test_attributes_inheritance_definition
assert_equal([:id, :body], @serializer_klass._attributes)
end
Expand Down