Recurse associations by default

[brauliobo]

Given these two serializers

class TeamsPlugin::ContextSerializer < ActiveModel::Serializer
  has_many :teams, serializer: TeamsPlugin::TeamSerializer
end
class TeamsPlugin::TeamSerializer < ActiveModel::Serializer
  has_many :members, serializer: TeamsPlugin::MemberSerializer
end

If I call

ActiveModel::SerializableResource.new(@article, serializer: TeamsPlugin::ContextSerializer)

It will serialize teams, but not members. For members I would have to specify include: {teams: :members} option for it to work.

But this seems very strange to me. Intuitively, by default it should go recursively.

[beauby]

You are right that the current default behavior of AMS with the Json/Attributes adapters is to embed only the first level associations. I am personally in favor of making the default embedding policy **, i.e. embed all levels, although we would need to add a check for infinite recursion in case the data graph has cycles. Thoughts @bf4, @NullVoxPopuli, @joaomdmoura?
As a workaround, you can specify include: '**' when serializing.

[brauliobo]

@beauby this workaround is quite handy, I vote to be a default :)

[bf4]

I'd rather make deep nesting an intentional oractice, rather than easy, and so therefore like that more than two levels requires explicitness.

[beauby]

@bf4 I don't feel strongly either way, but it should be made clear in the docs so that people don't get surprised by this behavior (intuitively, at least I, would expect all relationships to be serialized for the Json/Attributes adapters).

[brauliobo]

@bf4 @beauby to make sense that the top association (teams) is going to get serialized, an option include: true on has_many :teams is necessary. otherwise, this option could be true recursively, by default.

[NullVoxPopuli]

I'm against having ** by default, as it could lead to an infinite loop of including, and redundant data. :-\

I think it's one step of magic that would cause a lot of confusion among many.

[brauliobo]

@NullVoxPopuli so what do you think about associations explicity saying they need to be included?

[beauby]

@NullVoxPopuli I agree this would happen, although it would just be the consequence of bad design (Json adapter + cyclic data graph).

[NullVoxPopuli]

The trend right now is to only have the model you ask for, and the ids of the asnociations for that model. I know ember prefers things to be sideloaded, so, this would align with that. And there is a push for async relationships as well. But depending on what other people want to do, and what their workflow is, maybe we need a configuration option?

I don't know what is 'best', but having an AMS option that makes you specify what associations you want every time could be pretty helpful. Of course, you can do it right now just by passing empty string to the include option. When serializing

[beauby]

@NullVoxPopuli If you start doing sideloading and async relationships, you might as well go with JSON API. The goal of the Json adapter should not be to define a poor man's version of JSON API.

[NullVoxPopuli]

Agreed

On Sun, Oct 4, 2015, 10:30 AM Lucas Hosseini [email protected]
wrote:

@NullVoxPopuli https://github.com/NullVoxPopuli If you start doing
sideloading and async relationships, you might as well go with JSON API.
The goal of the Json adapter should not be to define a poor man's version
of JSON API.

—
Reply to this email directly or view it on GitHub
#1205 (comment)
.

[NullVoxPopuli]

We should maybe say that in the docs though, so people don't try it

On Sun, Oct 4, 2015, 10:31 AM L. Preston Sego III [email protected] wrote:

Agreed

On Sun, Oct 4, 2015, 10:30 AM Lucas Hosseini [email protected]
wrote:

@NullVoxPopuli https://github.com/NullVoxPopuli If you start doing
sideloading and async relationships, you might as well go with JSON API.
The goal of the Json adapter should not be to define a poor man's version
of JSON API.

—
Reply to this email directly or view it on GitHub
#1205 (comment)
.

[passalini]

I think that specify what associations you want is pretty helpful too and when I looked to the code I saw that adding 3 lines we can do it!!!

If I'm right (please correct me if I'm wrong) changing #relationship_value_for from the adapter will solve the problem. In my tests I created a new adapter...

module ActiveModel
  class Serializer
    module Adapter
      class CustomAttributes < Attributes
        def serializable_hash_for_collection(options)
          serializer.map { |s| CustomAttributes.new(s, instance_options).serializable_hash(options) }
        end

        def relationship_value_for(association, options)
          return association.options[:virtual_value] if association.options[:virtual_value]
          return unless association.serializer && association.serializer.object

          include_tree_hash        = @include_tree[association.key].instance_variable_get(:@hash)
          include_association_hash = IncludeTree.from_include_args(association.options[:include]).instance_variable_get(:@hash)
          include_tree = IncludeTree.from_include_args(include_association_hash.merge include_tree_hash)

          opts = instance_options.merge(include: include_tree)
          CustomAttributes.new(association.serializer, opts).serializable_hash(options)
        end
      end
    end
  end
end

... now I can use include at the association's call just like in controllers and serializers...

class UserSerializer < ApplicationSerializer
  has_many :accounts, serializer: UserScope::AccountSerializer, include: :roles
end

ps: the include from the controller still works and if you want you can overwrite the include from the association with it :)

[sandstrom]

Many client-side applications are architected in a way that make it easy to retrieve additional resources as needed. Also, with HTTP/2 there is little network overhead.

Thus, I'd vote for having it non-recursive by default (i.e. first-level only).

[NullVoxPopuli]

I feel like at this point, everyone wants the json adapter to fit their specific use case -- it's not really capable of doing that.

I think the json adapter should be an example for how to make a basic adapter that aids in rendering basic json documents, such that users can implement whatever they need as they please.

JSON API is the future.

[prusswan]

Recurse was the default behavior in 0-9-stable. To avoid confusion, the docs for 0.10 should mention this

[beauby]

@prusswan You are right. Would you mind issuing a PR to update the docs? That would really help.

[prusswan]

There is some inconsistency when using include '**' with associated models without explicit serializers (which I believe should default to the <SomeModel>Serializer in the same namespace). The result is also affected by config.eager_load = true|false, which is probably a bug.

[lserman]

I think recursing by default is fine because that was the behavior in 0.9.x (and 0.8.x afaik). What I would like though is a way to easily override the default serializer for associations via the adapter. In our case, we want to load only the IDs of the associated record unless explicitly included using the "include" parameter. Defaulting to one-level deep does not make sense for us.

Unless I am missing something, this requires a lot of rewriting and hacks because the serializer is already determined when it reaches the adapter. I was hoping to be able to just check the include tree, override the serializer: key in an options hash to an IdSerializer, and be done with it. The current adapter has options and instance_options, neither of which handle the serializer key.

[beauby]

So, the initial issue here is whether the Attributes/Json adapters should serialize all associations recursively by default.

Pros:

• It is intuitive.
• It was the behavior of 0.8 and 0.9.

Cons:

• It crashes in case of cycles in the data graph.

The current consensus is that the default behavior will remain including only one level of relationships, although this should be explicitly stated in the docs.

For other issues mentioned here, please open separate GH issues.

Closing this issue for now, but feel free to keep commenting/making a case for it here.