Attribute filtering for 0.10.x

[lautis]

This is something I've (ab)used in 0.8.x quite frequently. Adds a method named filter that by default returns all fields. Actual serialised implementations can override this method to dynamically filter the list of attributes to be serialized.

Implementation based on #1058.

[NullVoxPopuli]

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

[NullVoxPopuli]

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

[lautis]

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

[lautis]

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

[NullVoxPopuli]

Because you wrote the method fields, you should test how that plays with various seriolizers. This test is testing the overridden method, which wouldn't touch the new code you wrote.

[lautis]

I don't think I'm quite following you. Granted, I'm not very familiar with the code base so I could be missing something.

There are tests verify the behavior of the introduced method method fields. This is pretty much covered by existing tests. Also the output of attributes without overwritten method and options is tested. I'm quite sure that mutations would be catched by the tests.

Is there serializer classes that would overwrite the default behaviour? Or some cases where you'd think an integration test would be needed?

[NullVoxPopuli]

I just mean that your test isn't testing https://github.com/rails-api/active_model_serializers/pull/1141/files#diff-5624f9cf35be4dc254412d979fd293c4R129 or https://github.com/rails-api/active_model_serializers/pull/1141/files#diff-5624f9cf35be4dc254412d979fd293c4R140 but is testing only https://github.com/rails-api/active_model_serializers/pull/1141/files#diff-be102247b73584d61072b4ac379b6e5fR32

But I see what you're saying. I'm going to leave it up to someone else to decide whether or not this is ready for merge.

[beauby]

As far as I am concerned, I would only care about thoroughly testing the output of attributes depending on the definition of fields. Once that's ensured, there is no difference on the rest of the pipeline.

[beauby]

It might be worth making it possible to specify the fields when calling render json: post, fields: myfields,
and adding a helper method in ActiveModel::Serializer::Utils to parse a JSONAPI fields query parameter and feed it directly into the render call.
I'd like to make sure this PR at least allows for adding that in the future.

[lautis]

It might be worth making it possible to specify the fields when calling render json: post, fields: myfields,
and adding a helper method in ActiveModel::Serializer::Utils to parse a JSONAPI fields query parameter and feed it directly into the render call.
I'd like to make sure this PR at least allows for adding that in the future.

@beauby, supporting render son: post, fields: myfields is possible by applying the filtering used for options[:filter] also to @options[:filter]. This would work with the proposed implementation, except that you have more chances to write conflicting code.

[NullVoxPopuli]

I think I'm good with this. Anybody else good with this? :-)

[jfelchner]

👍

[jfelchner]

However I agree with @beauby on all of his points so if we think that's something that should be done, I think we should tackle it in this PR.

[bf4]

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

[bf4]

If you want to select which attributes to use, it really needs to be when the serializer is instantiated, because you don't have control of its rendering from the controller.

I'm also against merging this until the fields instance method is removed, and until some documentation is added explaining how to use it, with an example.

Also, see my comment above #1141 (comment)

[bf4]

I'd also rather use the rails convention of only and except as the names and add code and tests to ensure it doesn't process non-existant attributes

[lautis]

If fields is removed, how could the serialisation be customised?

In a simple case, the same functionality could be achieved by implementing class methods and using those in controller (fields, only or except all do the trick). However, I'm a bit confused how that would work in a case where there are embedded objects with has_many/has_one.

[bf4]

fields is already sent to the adapter, and used by jsonapi in ActiveModel::Serializer::Fieldset, does that help?

        def resource_object_for(serializer, options = {})
          options[:fields] = fieldset && fieldset.fields_for(serializer)
          attributes = serializer.attributes(options).except(:id)

and in the serializer

    def attributes(options = {})
      attributes =
        if options[:fields]
          self.class._attributes & options[:fields]
        else
          self.class._attributes.dup
        end

so, basically, this is already available

[lautis]

That is a very good start, but I was more concerned how to make that manageable. My use case is that sometimes – likely in few different places – it is necessary to return a subset of the full JSON representation. This would likely involve multiple serializers with relations. With include_attribute? this has been relatively easy to implement reliably in 0.8.

There seems to be some recent work to make that easy in #1158. #1157 might also help. While it is the responsibility of controller to pass the full list of included/excluded attributes, it isn't very different to have render json: object, filter: allowed_attributes_method instead of render json: object, option_to_control_rendering: value (which I've used in 0.8).

[beauby]

@lautis To clarify the situation, #1158 and the include adapter option in general handle only relationships, not attributes. #1157 brings a convenient syntax for specifying which attributes and relationships should be serialized on nested models.

[beauby]

#1193 brings in an even cooler syntax for nested serializers.

[lxcodes]

@beauby Do you plan on implementing the example include_attributes or something like it here? Trying to get a sense of direction now after the recent conversations.

[beauby]

@al3x-edge I'm currently working on a fields adapter option to allow for JSON API style fields filtering. The other use cases should be covered by #1193, except for dynamic filtering of attributes at serializer level, which will come in an other PR in the near future (i.e. this one once its ready for merge).

[lautis]

I changed the method name to include_attribute to avoid confusion with other features.

[NullVoxPopuli]

👍

[NullVoxPopuli]

@lautis could you maybe add a little something to the readme for how to use this? thanks!

[lautis]

@NullVoxPopuli added documentation, but it's a bit inconvenient being the first mention about serializer scope / current_user. That needs documentation as well, but it could be better addressed as a separate PR.

[NullVoxPopuli]

Totally agreed - and as far as I know, it's on all our radars. :-)

[lxcodes]

All good. Wasn't sure if this was being abandoned for another approach. Thanks for the hard work from everyone and the follow-ups. 👍

[vasilakisfil]

I my projects, based on user's permission role I would return different representation of my model (higher the role, the more attributes user would get). That was really easy using only: option in 0.9.x serializers. I guess you are trying to implement the same thing here. Ideally I would like to be able to filter attributes on embedded associations too. So if I request a video which has a user model embedded, I would like to not only filter which video attributes I want but also which user attributes I want. Would that be possible ?

I am pretty sure JSONAPI specs supports and embraces that pattern.

[beauby]

@vasilakisfil JSON API does not state much about that. Currently, you could override the instance attributes method of your serializer like that:

PostSerializer < ActiveModel::Serializer
  def attributes(*)
    default_attributes = super
    ... do your stuff about filtering your attributes ...
  end
end

[vasilakisfil]

@beauby the thing is that we should opt for filtering the attributes from outside the serializers, that is, from the constructor just as it used to be in 0.9.x with the only: option. Not only due to permission issue but also exactly because JSONAPI embraces the 'fields' query in which client is able to ask specific attributes of a resource we should be able to specify which attributes should be allowed from the constructor.

Having said that, ideally you should be able to filter the attributes on the requested embedded resources. Although not explicitly clarified in the JSONAPI I think it's a best practice anyway and an implementation should take into account this potential feature.

[beauby]

There are two things here:

1. the list of available attributes should be dynamically modifiable (that is depending on the current_user and that kind of stuff)
2. the list of requested attributes (either directly by the user, or through a parameter sent by the client) should be modifiable in the constructor

[bf4]

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

[bf4]

I'm pretty 👎 on this till I'm convinced it's necessary, helpful, domain-appropriate, and doesn't introduce unnecessary complexity to AMS

[lautis]

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.

[lautis]

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

[bf4]

Example plz

[bf4]

I'm going to close this for now until we can get an implementation that I think is an improvement on what AMS already does. This isn't a knock on OP, just that, as a maintainer, I'm conservative about what to let in.

basically, the current state of this PR

• does not make a case that it is better than using fields or subclassing a serializer
• is too focused on Serializer#attributes, even though that isn't really a public interface for AMS. (SerializableResource is)

Ref:

• Attribute filtering for 0.10.x #1141 (comment)
• Attribute filtering for 0.10.x #1141 (comment)
• Attribute filtering for 0.10.x #1141 (comment)
• Attribute filtering for 0.10.x #1141 (comment)

[lxcodes]

Understand your position on this as a maintainer and haven't tried the overrides. Will the overrides become the default way to override attributes? Will changes to attributes be considered public API so we aren't surprised by a change later?

Bummed on this change and doesn't seem pragmatic.

[lautis]

The fields option provides pretty much the same functionality as the syntax for handling attributes in embedded resources has improved a lot since this PR was created. Depends on your particular use case which one is more convenient. If it's an attribtue hidden due to a feature flag, I'd rather have the logic in serialiser to not have to deal with it in every controller that might serialise the given object. More attributes in show method vs list – subclassing sounds like the way to go.

With the current fields implementation, you can have a method and use that to generate list of attributes render json: @object, fields: list_of_fields_for(user). Unfortunately, the easiest way to do this might involve using Serializer._attributes which doesn't exactly look like a public method. I'd be interested in hearing if there are any better options.

Regarding the focus on Serializers, I don't quite understand how adding dynamic attributes for serialisers could not be focused on Serializer. That is, after all, the level where the attributes are introduced in the first place. Maybe the tests could focus more on full use case instead of unit tests, but unit tests seem to be how the current tests are written.

[bf4]

The PR at the time I closed it either wouldn't work or would provide redundant features depending on what you're trying to do.

A compromise could be something like if any attributes have the value :do_not_serialize they wouldn't be included, that way the responsibility to return that value is entirely on the end-user, and the code change in AMS very narrow. (nil wouldn't work since that's a valid value).

Besides that, none of the use-cases I've seen require a change in AMS

[beauby]

My personal feeling on this is that the user should be able to provide to the adapter a Hash{Symbol => Array<Symbol>} that gives for some types of resources the allowed attributes to be serialized. That way, the JSON API fields feature is easy to implement, and the user can do their own filtering before passing that hash to AMS. Thoughts?

[bf4]

@lautis wrote:

With the current fields implementation, you can have a method and use that to generate list of attributes render json: @object, fields: list_of_fields_for(user). Unfortunately, the easiest way to do this might involve using Serializer._attributes which doesn't exactly look like a public method.

not sure what you mean. You want to supply a list of attributes and then remove some instead of just specify the ones you want? _attributes isn't exactly public, but I think you could rely on it for now. I have in my own code before I started working on this project.

Regarding the focus on Serializers,

I mean passing in arguments to attributes. That's not the way to use the library.

[lautis]

@bf4

not sure what you mean. You want to supply a list of attributes and then remove some instead of just specify the ones you want?

No. I'd hope to have a way of dynamically ignoring some attributes without having to reimplement serialiser structures. This could be accomplished at least with

• whitelisting as it's implemented currently in AMS (Attribute filtering for 0.10.x #1141), but I'm not sure how much of JSON API I'd need to reimplement – if any – to combine the internal logic with ?fields[type]=list,of,fields.
• a special value to omit attribute (:do_not_serialize or a constant such as ActiveModel::Serializer::IGNORE_ATTRIBUTE)
• include_attributes Serializer method as suggested in Bringing Filter to 0.10.x #1058 and implemented here
• 0.8 style ignore_attribute_name?

I mean passing in arguments to attributes. That's not the way to use the library.

No arguments on the use of arguments for attributes, but that is how the existing functionality is tested. I'd suggest fixing the existing tests to not have confusing examples.

[vasilakisfil]

I agree with @lautis a lot. I am not sure how it should be implemented but as a user of this gem I would like to have an option to inject from the constructor which attributes I want to allow to serialize from an object. Ideally I should be able to specify a list of attributes in the associations too. Just like how strong params work. There was an option for that in 0.8 and 0.9.

[bf4]

To be clear, we just need an example of something you can't do now.

?fields[author]=name,email is what JSON API expects

No arguments on the use of arguments for attributes, but that is how the existing functionality is tested. I'd suggest fixing the existing tests to not have confusing examples.

I would distinguish between public methods and how to use the gem. Serializer#attributes is a public method, and, as such, it is tested. However, it is only used by the Adapter which is used by SerializableResource to coordinate the serialization of the given resource and options.

Eventually I intend to port my RSpec serializer tests into this lib, and it will test attributes. BUT, calling Serializer#attributes in app code (not test code), means you're not serializing a resource, you're just decorating Model#attributes with Serializer#attributes.

Believe me that I've hacked this library, too, but please believe me that what is in the PR won't get what the PR intends, which is why I closed it.

I also want to update the docs so that how AMS works is clearer. Wanna help? :)

And again, we just need an example of something you can't do now. Then we can work backwards to an implementation.

[beauby]

I believe what's not currently doable is to dynamically specify the attributes of an associated resource (which JSON API makes possible by specifying attributes per-type).