Fixes filter method being called multiple times.

[arenoir]

Fixes #459 by adding filtered_keys method. It combines attributes.keys with association keys and passes them to filter method memoizing result.

[spastorino]

Hey thanks for helping but can you explain a bit better what are you trying to fix?.
If it's performance that needs to come with benchs.

You are calling filter once now but you're calling & twice

[arenoir]

I am assuming the filter method is public and intended to be implemented on a per class basis. In my opinion the filter method should be called once on instantiation. Currently when associations are defined the filter is called twice. Once passing the attributes keys, and then again passing the association keys.

This is confusing from an implementation stand point. For example say I want to serialize a collection and know that all items in the collection will use the same keys based on the serialization_scope. It would make sense to do the filter logic once.

Here is a example that I think should work.

class PostSerializer < ActiveModel::Serializer

  attributes :title, :body

  has_many :comments

  def filter(_keys)
    @keys ||= authorize_keys(_keys)
  end

  private

  def authorize_keys(_keys)
     if user.lame?
       return _keys - [:comments]
     else
       return _keys
     end
  end

end 

In this example the user.lame? call is expensive and only needs to be called once. Your right about the & being called twice; perhaps it can be fixed in a more efficient way. In the meantime I think it is important the filter method is consistent.

[arenoir]

The previous example isn't great. In my application I have defined a AccessControl object. It is instantiated with current_user and params. It is responsible for authorizing/defining the serialization keys as well as the permitted params.

Here is an example. https://gist.github.com/7709521

I think another solution would be to break up the filter method into two methods. filter_associations and filter_attributes.

[tonywok]

@arenoir I noticed this as well. I was also thinking it would be better if there were separate filter_associations and filter_attributes methods.

For example, in my API, I'm supporting an ember appp (with ember-data) and a more traditional rails app (amongst other things 😄). So rather than having a serializer that sideloads in a bunch of associations and a sparse serializer for every resource, it'd be nice to allow the client to specify what associations they'd like included. To do that, I've considered supporting an API inspired by the proposed JSON api specification for inclusion of related documents.

I'm sure you're familiar, but for quick reference:

?include=foos,foos.bars,baz

In addition, I'd like to more easily support the sparse field set suggestion in the proposed spec. For reference:

?fields=field1,field2

or more specifically:

fields[foo]=field1,field2

I think adding separate methods would make implementing things like this a lot easier.

However, going forward, and noticing the JSON API spec is listed on @spastorino's todo list, would it be worthwhile to define a more interface driven approach? For example, AMS could ship with an ActiveModel::Filter class that probably doesn't do too much (if anything) by default, but would allow developers to implement things like I mentioned above more easily.

For example (just sketching), I could potentially write something like this:

# in config/initializers/active_model_serializers.rb
ActiveModel::Serializer.setup do |c|
  c.association_include_key :include  # or I've seen people use expand
  c.fieldset_include_key :field # or more specifically, fields[resource]
end

module ActiveModel
  class JSONApiFilter < Filter

    def filtered_associations
      # custom filtering logic using the params extracted using the configured include_key
    end

    def filtered_attributes
      # custom filtering logic using the params extracted using the configured fieldset_key
    end

  end
end

Again, the above is a rough sketch...

I'd be glad to whip up a PR if anyone else is interested in this? I've already been building on top of a couple existing open PRs including this one.

[arenoir]

@tonywok I like the idea of having a filter class. I think a filtering object would have to be initialized for the original serializer and then once for each association with the serialization_scope and controller params. @spastorino are we barking up the wrong tree?

[fivetanley]

@arenoir What do you think about also adding a filter_embedded_in_root_associations(keys) method? Here's my use case, and maybe I just haven't figured out how to do it with current master yet...

The reason I'd like to use filter_embedded_in_root_associations is that I'd like include the IDs for an association, without including the records through sideloading. But I'd like to be able to toggle that on and off. E.g., I might want to explicitly opt in to including my "comments" records with my own logic.

So, even though I would have the following serializer:

class PostSerializer < ActiveModel::Serializer
  attributes: :id
  embed :comments, include: true, embed: :ids
end

I would like to override the default I set with some custom business logic:

class PostSerializer
  # shortened for brevity

  def filter_embedded_in_root_associations(keys)
     if dont_include_comments_in_root?
       keys - [:comments]
     else
       keys
     end
  end
end

{
  "posts": [
    { "id": 1, "comments": [1,2,3] 
  ]
}

What do you think?

[arenoir]

@fivetanley I think that may be too much of an edge case. Looking over the jsonapi format it looks like the standard is to side load associations. I know there are cases where it makes sense to embed objects, but I think dynamically switching between embedded and included is a anti pattern.

[fivetanley]

@arenoir Sorry, I'm not proposing switching between embedding objects and including them in the response. The ids are always embedded in this case, just the comments may not be sideloaded. However, the comments will never be embedded (full JSON representation under the "comments" key) with this.

This allows your API consumers to opt in to including resources in a response. See "Inclusion of Related Documents" under the "Fetching" section on the format page. That's what I'm trying to get at.

Edit: The difference between the two ruby classes I wrote would be the following JSON responses:

{
  "posts": [
    {"id": 1, "comments": [1,2,3]}
  ],
  "comments": [
    { "id": 1 },
    { "id": 2 },
    { "id": 3}
  ]
}

and for the second class with the filter_embedded_in_root_associations method:

{
  "posts": [
    {"id": 1, "comments": [1,2,3]}
  ]
}

[arenoir]

@fivetanley okay I get it now. I am currently working on a implementation of the Inclusion of Related Documents section.
https://github.com/arenoir/active_model_serializers/tree/jsonapi

Given the current spec it appears associations are included or not included based on a include parameter. Perhaps there should be a way to specify how.

Maybe passing in a empty include parameter with the association name in the fields parameter should yield the desired result.

GET /posts?include=&fields[posts]=id,comments

[fivetanley]

Your jsonapi branch looks great!

[momelnyk]

+1 for Fixes filter method being called multiple times.

[steveklabnik]

So, is this change still desirable? It's out of date if so.

[steveklabnik]

This is still out of date and haven't heard anything. Closing.