Add ActiveModel::Serializer#default_include

[iMacTia]

Purpose

Let's assume we have the current serializers:

class BlogSerializer < ActiveModel::Serializer
  attributes :id
  attribute :name, key: :title

  has_many :posts
end

class PostSerializer < ActiveModel::Serializer
  attributes :id, :title

  has_one :author
  has_one :category
end

class AuthorSerializer < ActiveModel::Serializer
  attributes :id, :name

  has_one :address 
end

class CategorySerializer < ActiveModel::Serializer
  attributes :id, :name
end

The only way to specify nested associations inclusion, at the moment, is through the controller's render include option, as shown in the current examples:

# blogs_controller.rb
render json: @blog, include: 'posts.category, posts.author.address'

# posts_controller.rb
render json: @posts, include: 'category, author.address'

The issue is that in this case (I admit is probably a stupid example, but stay with me), when rendering a post, we always want to render also its category, its author and its author address.

Changes

Add two new class methods to ActiveModel::Serializer:

default_include: associations that you want to include by default (if you use json/attributes adapter, this defaults to all first-level associations). This value will be overriden by the controller's render include.
always_include: associations that should always be serialized even if the object is deeply nested. This CAN'T BE OVERRIDDEN from the controller.

Both methods will accept a parameter which will be parsed by JSONAPI::IncludeDirective.
Optionally, a block can be provided that will return this value, in this case the block will be evaluated on the serialiser instance.

Related GitHub issues

#1333, #1849

Additional helpful information

Tests are currently failing since we're waiting for this PR to be merged on jsonapi gem. Hopefully @beauby will have a look at it soon :)

Discussion

@NullVoxPopuli, @bf4: what should happen if the default_include method is called many times in the same serialiser? Current implementation will override the previous value, does it make sense for you as well?

[mention-bot]

@iMacTia, thanks for your PR! By analyzing the annotation information on this pull request, we identified @bf4, @bolshakov and @beauby to be potential reviewers

[NullVoxPopuli]

I'm surprised ruby lets you name a variable include

[iMacTia]

I agree this is probably not the smartest name, will refactor it to args ;)

[iMacTia]

Actually, changed to include_args to follow JSONAPI::IncludeDirective#initialize convention 😄

[NullVoxPopuli]

what should happen if the default_include method is called many times in the same serialiser?

I think overriding the previous value is perfect!

Nice work overall, only had a couple comments :-)

[NullVoxPopuli]

also, I wonder if we should move @beauby 's jsonapi to rails-api or rails?

[iMacTia]

@NullVoxPopuli great! And thanks a lot for your comments 😄!
It's always hard to put your hands on someone else's project, and sometime you miss conventions/logics. So please feel free to ask for changes if you think they could improve the PR ;)

[beauby]

@NullVoxPopuli: The move to rails-api is planned, I just want to stabilize some stuff first.

[iMacTia]

Just pushed another commit to add always_include as well.
Now both implementations are available! 🎉 🎉 🎉 🎉
Tests are still failing, will check them carefully after I'm done with the PR on @beauby project 😄
In the meantime, @NullVoxPopuli, if you have any idea for additional tests, just let me know 😉

[NullVoxPopuli]

for both these tests and the always tests, I'd actually make sure that the correct types are rendered, just so whomever is looking at these tests in the future can very easily see what is being rendered.

[iMacTia]

Added a check for them in my latest commit 😄

[bf4]

Looks like my comment on api syntax got lost in a diff change #1845 (comment)

[iMacTia]

FYI just added a temporary monkey-patch to add the IncludeDirective#merge method and all tests are passing 🎉 🎉 🎉 🎉!
Maybe we need tougher tests 😄 Any idea?

[bf4]

@iMacTia would you mind updating the PR description with a higher level outline of the problem this PR is addressing?

Provide a class method (ActiveModel::Serializer#default_include) to specify which serialiser associations should ALWAYS be included when that serialiser is used.
Parameters are the same as the include option available on controller's render.

Is very implementation-specific.

The problem, if I understand it, is that when using the JSON API adapter, and we have an API that has default include behavior, it can be tiresome to write render json: post, include: [:metrics, :media, :publishes, :author], fields: { media: [:title], publishers: (PublisherSerializer._attributes - :publisher_bio) } in multiple places.

The goal, if I understand it, is to add a way, when using JSON API, for the serializer to specify default included relationships and their fields, right?

and the proposed implementation of that is (?)

class PostSerializer < ActiveModel::Serializer
  always_include 'metrics,media,publishers,author'
  default_include 'metrics,media,publishers,author'
end
render json: post, fields: { media: [:title], publishers: (PublisherSerializer._attributes - :publisher_bio) }

where always_include resolves the need to specify include: [:metrics, :media, :publishes, :author]

(without carefully reading all the code, it's not clear to me the difference between always include and default include. )

It should be noted that I proposed something like

class PostSerializer
  has_many :metrics, include: :all
  has_one :media, include: [:title]
  has_many :publishers, exclude: [:publisher_bio]
end
render json: post

where include/exclude map to the JSON API include and field. They bear no relation to the AMS if/unless conditional exclusion nor only/except json serialization options in ActiveModel::Serialization

[tommiller1]

Awesome work getting this working!

My couple of cents;

The always_include method should be specified on the association itself ex. "has_one :author, always_include: true". Basically always_include should make a serializer treat the association as an attribute. Attributes are always included even for deeply nested associations. Ex: including 'posts.comments.images.uploader' in a render call will return all attributes in this entire chain but no associations. With always_include we want to change this behaviour so that all associations along this chain with always_include set to true are included.

The default_include method should be just that; default includes if there are no other includes. It should be completely ignored if we specify includes on the render call (not merged). Also currently default_includes doesnt really offer anything new, we are just moving the includes from the controller to the serializer. You should consider making default_includes an instance method so we can specify parameters in the render call that we use in the default_include method to bunch together common includes. Ex:

render json: @post, show: true

PostSerializer
    def default_includes
        if instance_options[:show]
            ['comments.images', 'images', 'dragons']
        elsif instance_options[:no_dragons]
            ['comments.images', 'images']
        elsif instance_options[:no_includes]
            []
        else
            # default adapter setting (include all top-level associations)
        end
    end

Anyway, I am happy this is getting included in some form. Thanks for taking the time to work on this, highly appreciated!

[bolshakov]

I have the similar feature implemented in our project.

    # Method may be used when you have to define an attribute which 
    # should be serialized using AMS. So you can define always included relations.
    #
    # @param [String, Symbol]
    # @return [void]
    # @example
    #   class InvoiceSerializer < ApplicationSerializer
    #     attribute :id
    #     serialized_attribute :balance
    #   end
    #
    # It will be rendered the following way without using explicit includes:
    #
    #   {
    #     "object": "invoice",
    #     "id": "3f8b90da-2a74-4f0a-8f5e-e63835348b72",
    #     "balance": {
    #       "object": "money",
    #       "currency": "BTC",
    #       "value": 664
    #     }
    #   }
    # 
    def serialized_attribute(name)
      attribute(name)

      define_method name do
        ActiveModel::SerializableResource
          .new(object.send(name))
          .serializable_hash[:data]
      end
    end

[NullVoxPopuli]

So, @iMacTia , it looks like I was wrong about the top level default_includes API, and that people want what @bf4 suggested. Do you think you have time to investigate that route?

[iMacTia]

@NullVoxPopuli @bf4
Understood, but I think this PR is too messy now, I prefer to work a new branch and open another PR/Issue to discuss about it

[iMacTia]

As discussed in #1849, I'll now finish to work on this one and hopefully we'll have default/always _include feature soon 🎉

Improvements to work on:

1. Make default_include and always_include optionally accepting a block to be evaluated at instance level.
2. Update tests

Point 1 was discussed in #1849 after the input from @tommiller1 and @NullVoxPopuli proposed a nice solution:

class PostSerializer < AM::S
  default_include do |object, serializer_instance|
    # whatever logic in here, must return a value to be parsed as JSONAPI::IncludeDirective
  end
end

We also discussed about a possible relationship-level include, but that will be implemented in a separate PR.

[iMacTia]

Updated PR description

[tommiller1]

"This value will be merged/overriden by the controller's render include."

Will it be merged or overidden? default_includes should be overridden by the includes on the render call imo. Merging them is not intuitive behaviour. always_includes should be merged.

[oyeanuj]

@iMacTia Really looking forward to your PR getting merged! :) Any updates by any chance?

[bf4]

@oyeanuj This PR is really about moving some behavior from userland to AMS, which is why we're not in a rush to add more complexity to AMS.

As stated in the PR description, this behavior is already provided by passing in (query) options.

And as stated elsewhere, you can probably get the desired default behavior by taking advantage of serializer inheritance #1845 (comment)

[oyeanuj]

@bf4 Thanks for the quick response! While I understand the complexity part of your message above, I am not sure if this behavior is already provided.

The two options - in the PR description above, the behavior is done through controller options and in the comment you linked to, through serializer inheritance.

Regd. the controller option

That doesn't make for a easy transition from 0.8.x and 0.9.x to 0.10.x as one would have to move a lot of the behavior from serializer to the controller and specify it every time the object is rendered (as mentioned in more detail in #1333).

Regd the serializable_hash option

That is an interesting option but I had two concerns with it.

1. Arguably and subjectively, that doesn't look like the clean Rails way of doing it :) I could see that reverse_merge statement get pretty nasty in a slightly larger serializer.
2. With the above code, could you do something like? On a quick test, that didn't work out but I can spend more time on it.

render json: post, except: [:publishers]

Also, my aim with the comment isn't to suggest which approach is better (older or newer), but to point out that there was a usage established in the earlier versions (so not necessarily moving complexity from userland per se), and that same behavior isn't possible yet. Since those versions did provide this in the AMS, my assumption was that it would still be considered as fair territory for AMS 0.10.x and 1.x in the future..

Totally understand if the default_includes or always_includes is not a direction you choose, but hope this perspective helps in making the decision.

If I am missing something basic in my understanding of the serializable_hash option, that would still allow for something like this, please let me know and I'll stop bothering you all on this thread :)

Appreciate all of your effort on this project, and PR (and hoping that didn't come off as an entitled comment)!

Thank you!

[bf4]

Great comment Anuj!

B mobile phone

On Aug 13, 2016, at 10:41 PM, Anuj notifications@github.com wrote:

@bf4 Thanks for the quick response! While I understand the complexity part of your message above, I am not sure if this behavior is already provided.

In the PR above, the behavior is done through controller options and in the comment linked through serializer inheritance.

Regd. the controller option

That doesn't make for a easy transition from 0.8.x and 0.9.x to 0.10.x as one would have to move a lot of the behavior from serializer to the controller and specify it every time the object is rendered (as mentioned in more detail in #1333).

Regd the serializable_hash option

That is an interesting option but I had two concerns with it.

Arguably and subjectively, that doesn't look like the clean Rails way of doing it :) I could see that reverse_merge statement get pretty nasty in a slightly larger serializer.

With the above code, could you do something like? On a quick test, that didn't work out but I can spend more time on it.

render json: post, except: [:publishers]
Also, my aim with the comment isn't to suggest which approach is better (older or newer), but to point out that there was a usage established in the earlier versions (so not necessarily moving complexity from userland per se), and that same behavior isn't possible yet (especially, if you are not using the JSONAPI adapter). Since those version did provide this in the AMS, my assumption was that it would still be considered as fair territory for AMS 0.10.x and 1.x in the future..

Totally understand if the default_includes or always_includes is not a direction you choose, but hope this perspective helps in making the decision.

If I am missing something basic in my understanding of the serializable_hash option, that would still allow for something like this, please let me know and I'll stop bothering you all on this thread :)

Appreciate all of your effort on this project, and PR (and hoping that didn't come off as an entitled comment)!

Thank you!

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[remear]

I'm not really convinced that default_includes/always_includes or any include logic should be in the serializers. IMO, the serializers should be concerned with how to serialize the model to which they pertain. My opinion is that includes is an option that pertains to document assembly, and should, therefore, be done via the controller. If you'd like a default include set, perhaps even different defaults for various actions, configure them and supply them in the appropriate render calls.

[NullVoxPopuli]

This is actually the consensus, thanks for touching on this again, @remear.

There are just too many differing and conflicting scenarios that everyone wants all at once.

[oyeanuj]

@remear @NullVoxPopuli Thats a bummer :( Maybe we can document it for those upgrading from 0.9 to 0.10 since it was supported in the earlier version. Speaking of, is there a guide for to help with that? (Maybe I just wasn't able to find one.)

Also, is #1865 still fair game?

@iMacTia What has been your workaround/interim solution for this? Or have you been maintaining a different fork for this?

[NullVoxPopuli]

I believe #1865 is still fair game as it's a render option, rather than a serializer option

@bf4 mentioned a work around. #1865 (comment)

[iMacTia]

@oyeanuj I was in a rush and had to refactor controllers to use the include option there. The only workaround I used to avoid too much duplication was defining constants at Serializer level to be used in the controller.
Example:

class PostSerializer
  INCLUDES = [:author, :comments]

  has_one :author
  has_many :comments
end

# and then in the controller
render @post, include: PostSerializer::INCLUDES

I DON'T LIKE this solution, but again I was in a rush to deploy the application with the updated AMS gem, so I had to...
I'm now on holiday till early next week, as soon as I'm back I'll look for a better solution. probably defining relations as attributes as most people do.

Really a shame we didn't find an agreement on this. Even though I understand @remear and @NullVoxPopuli motivation, I still think this leaves a good portion of developer in the ugly situation of writing un-DRY unmaintainable code. It is riskious if you start with 0.10 from day one, a pain if you have to refactor your app from older version as you can easily miss a controller render (especially implicit ones) and we don't all live in the ideal world where every response is checked by automated tests...

[NullVoxPopuli]

for what it's worth, I think includes should be in the controller, which managing includes there is pretty dry and maintainable.

But, skirting around a problem (lack of tests) by adding a large amount of automated functionality is suuuuuuuuuper risky and smelly.

You're more than welcome to monkey patch, and @bf4 and I have been wanting to have a showcase page of the different forks / extensions / monkey patches that people use for different situations so that if someone with teh same issue comes across the showcase page, you'll still end up helping a brotha out. :-)

[iMacTia]

@NullVoxPopuli I agree with all the above but the first sentence.
I can't remember if there'a an example that show this already, but in case we have a resource that always needs to be rendered with some subresources and that gets rendered in different controller actions, than you definitely end up copy-pasting your include directive in all those controller actions.

I'm not complaining and as I said I understand your decision, this issue probably doesn't affect the JSONAPI adapter but is more related to the Attributes one and I know your focus is on the former.
Moreover, there are workarounds to do it without this PR so I think it's OK. At least we now have an issue where this thing was discussed, decided and closed, so other devs in the future knows what they have to do.

[NullVoxPopuli]

than you definitely end up copy-pasting your include directive in all those controller actions.

You could move that include string to a constant in your controller.

:-\

[iMacTia]

In fact my temporary solution was to create a constant (in the serializer, not the controller, but it's actually the same).

However, this is still prone to errors (like adding a new action in the future and forgetting the include, or missing an implicit render of that Serializer, which actually happened during my refactoring).
So my preferred solution at this point will be to have the association defined as an attribute to mimic the default_include functionality.

Again, I'm not blaiming anyone, I understand your decision. I'm just glad this was discussed :)

[oyeanuj]

Agree with @iMacTia, glad this was discussed.

Implicit renders of the associations, and that too, across controllers is the most problematic area for me as well, and doesn't feel like a clean solution. But given that this is a breaking change, I feel like this would be a great addition as a 'recipe', 'showcase' or a 'monkey-patch' and will help out others who might upgrade in the future.

[robinsingh-bw]

# usage: always_include :image
    # the included object must be an association (not a method/attribute)!
    def self.always_include(name, options = {})
      attribute(name, options)
      define_method name do
        if obj = object.send(name)
          resource = ActiveModelSerializers::SerializableResource.new(obj)
          resource.serialization_scope = instance_options[:scope]
          resource.serialization_scope_name = instance_options[:scope_name]
          resource.serializable_hash
        end
      end
    end

[iMacTia]

That's a great monkey-patch example, thanks @robinsingh-bw!
It will be my starting point to get this done properly :)!
Thanks for sharing!

[oyeanuj]

@iMacTia Did you end up using @robinsingh-bw 's monkeypatch or another solution? Is there a gist of before -> after of your code that you could share, from 0.9 to 0.10? Or a gist of your usage?

@robinsingh-bw stupid question, but where are you including that monkeypatch? In the ApplicationSerializer?

[robinsingh-bw]

In my BaseSerializer that all serializers extend from, its not really a monkeypatch as we are not overwriting any core methods. I have updated it to support custom methods defined on the serializers (in addition to object relations):

    # usage: always_include :image
    def self.always_include(name, options = {})
      options[:key] ||= name
      method_name = name.to_s + '_serialized'
      define_method method_name do
        data = respond_to?(name) ? send(name) : object.send(name)
        if data
          options = instance_options.dup
          options.delete(:serializer)
          options.delete(:each_serializer)

          # use Attributes adapter here instead of Json as we dont want a root object
          options[:adapter] = ActiveModelSerializers::Adapter::Attributes

          resource = ActiveModelSerializers::SerializableResource.new(data, options)
          resource.serializable_hash
        end
      end

      attribute(method_name, options)
    end