Sparse fieldsets usage suppresses relationships

[felixbuenemann]

When using sparse fieldsets in combination with includes on 0.10.0rc4 (and master), the serialized json api document is missing the relationships top level key, but the included key is still present.

Test Case:

require 'bundler/inline'

gemfile(true) do
  source 'https://rubygems.org'
  # gem 'active_model_serializers', '0.10.0.rc4'
  gem 'active_model_serializers', github: 'rails-api/active_model_serializers'
end

require 'pp'

class Node < ActiveModelSerializers::Model
  attr_accessor :id, :name, :children
end

class NodeSerializer < ActiveModel::Serializer
  attribute :name
  has_many :children, serializer: NodeSerializer
end

root = Node.new(id: 1, name: 'root')
root.children = [
  Node.new(id: 2, name: 'child1', children: []),
  Node.new(id: 3, name: 'child2', children: [])
]

serializer = NodeSerializer.new(root)

normal = ActiveModel::Serializer::Adapter::JsonApi.new(serializer)
pp normal.as_json
# {:data=>
#  {:id=>"1",
#   :type=>"nodes",
#   :attributes=>{:name=>"root"},
#   :relationships=>{:children=>{:data=>nil}}}}
# It seems has_many doesn't work properly with poros, but the :relationships key is present

sparse = ActiveModel::Serializer::Adapter::JsonApi.new(serializer, fields: { nodes: [:name] })
pp sparse.as_json
# {:data=>{:id=>"1", :type=>"nodes", :attributes=>{:name=>"root"}}}

[groyoh]

Your example contains an error but the behavior you state is indeed a bug since excluded relationships fields should not appear in included (http://jsonapi.org/format/#document-compound-documents).

The corrected example:

require 'bundler/inline'

gemfile(true) do
  source 'https://rubygems.org'
  # gem 'active_model_serializers', '0.10.0.rc4'
  gem 'active_model_serializers', github: 'rails-api/active_model_serializers'
end

require 'pp'

class Node < ActiveModelSerializers::Model
  attr_accessor :id, :name, :children
end

class NodeSerializer < ActiveModel::Serializer
  attribute :name
  has_many :children, serializer: NodeSerializer
end

root = Node.new(
  id: 1,
  name: 'root',
  children: [
    Node.new(id: 2, name: 'child1', children: []),
    Node.new(id: 3, name: 'child2', children: [])
  ]
)
serializer = NodeSerializer.new(root)

normal = ActiveModel::Serializer::Adapter::JsonApi.new(serializer)
pp normal.as_json
# {:data=>
#  {:id=>"1",
#   :type=>"nodes",
#   :attributes=>{:name=>"root"},
#   :relationships=>
#    {:children=>
#     {:data=>[{:id=>"2", :type=>"nodes"}, {:id=>"3", :type=>"nodes"}]}}}}

sparse = ActiveModel::Serializer::Adapter::JsonApi.new(serializer, fields: { nodes: [:name] }, include: 'children')
pp sparse.as_json
# {:data=>{:id=>"1", :type=>"nodes", :attributes=>{:name=>"root"}},
#  :included=>
#  [{:id=>"2", :type=>"nodes", :attributes=>{:name=>"child1"}},
#   {:id=>"3", :type=>"nodes", :attributes=>{:name=>"child2"}}]}

[beauby]

Just skimmed through the issue, but have you noticed this note:

Note: Full linkage ensures that included resources are related to either the primary data (which could be resource objects or resource identifier objects) or to each other.

[groyoh]

@beauby Maybe I got it wrong but does the following mean that only the relationship would be removed but the included should still contain the related resource?

The only exception to the full linkage requirement is when relationship fields that would otherwise contain linkage data are excluded via sparse fieldsets.

[beauby]

@groyoh That's how I understand it, yes.

[felixbuenemann]

Hmm, reading through the spec:

If a client requests a restricted set of fields for a given resource type, an endpoint MUST NOT include additional fields in resource objects of that type in its response.

Fields are both attributes and relationships (emphasis mine):

A resource object's attributes and its relationships are collectively called its "fields".

If I understand that correctly, the relationships should be excluded when using sparse fieldsets, unless specifically included in the lists of fields.

In the above example to get the relationship included I should specify:

ActiveModel::Serializer::Adapter::JsonApi.new(serializer, fields: [:name, :children])

On AMS 0.10.0.rc4 and master with the above example I always get children relationship included, even if I did not list it under fields, which seems to be wrong according to the spec.

But my initial bug report wrongly assumed that fields should not apply to relationships and the behavior I observed in my app differs from the (fixed) example. I tested with ActiveRecord-Models in my app, so something might be working differently based on the model being serialized.

[felixbuenemann]

To summarize all of the above the spec seems to indicate that:

• When using sparse fieldsets, only attributes and relationships should be returned, that have been listed using the fields query param
• When the include query param is used with sparse fieldsets, the resources must be included, even though they may not have full linkage, due to their relationships not having been listed under fields

The spec is a bit unclear on what to do with multi-part path intermediate resources (emphasis mine):

Note: Because compound documents require full linkage (except when relationship linkage is excluded by sparse fieldsets), intermediate resources in a multi-part path must be returned along with the leaf nodes. For example, a response to a request for comments.author should include comments as well as the author of each of those comments.

This would indicate, that when using sparse fieldsets and include=comments.author, the implementation could choose to only include author, but not comments. But doing so would probably be confusing…

[groyoh]

My bad for mistaking what the spec said and what you meant. Got a bit confused by

When using sparse fieldsets in combination with includes

though. I'm closing my PR then.

[felixbuenemann]

@groyoh Should I open a new bug report for AMS including relationships that were not listed in sparse fields?

[felixbuenemann]

Forget my last comment, it's working fine.

[felixbuenemann]

Closing this issue, sorry for any confusion caused…