Ensure relationship renders independent of fields

[richmolj]

Explained https://github.com/jsonapi-rb/jsonapi-serializable/pull/102

[klobuczek]

@richmolj why did you decide to make this change against the specification? This has broken our app. I get your point that the spec is somewhat imperfect regarding conflicting fields and include, but if you really wanted to go ahead without waiting for the spec to adjust then the better way would've been to take the super set of fields and include here rather than all relationships. Our clients are complaining now that they cannot restrict the returned relationships. When using graph database this has not only esthetic but as well performance implications as the ids are not stored on the primary objects but the related objects which requires additional traversal.

[richmolj]

why did you decide to make this change

Because it was breaking other clients, who (rightly) thought they should not have to duplicate the relationship name in fields.

the better way would've been to take the super set of fields and include here rather than all relationships.

I'd happily merge a PR.

cannot restrict the returned relationships

This is unclear to me. Graph database or not, relationships are restricted via include in the URL. If you don't want a relationship, don't request a relationship? I'm sure there is something I am missing but I do not understand your point.

[lshimokawa]

The spec says:

A resource object’s attributes and its relationships are collectively called its “fields”.

So when using sparse field sets you should be able to return only specific attributes or relationships.

# Temporary fix until fixed upstream
# https://github.com/jsonapi-rb/jsonapi-serializable/pull/102
JSONAPI::Serializable::Resource.class_eval do
  def requested_relationships(fields)
    @_relationships
  end
end

This change breaks the spec because you can no longer define the requested relationships to be returned in the payload, it's returning all of them ignoring the fields parameter to fetch sparse fieldsets.

[richmolj]

@lshimokawa yes, this is noted in the linked issue, we have tried to raise the issue but haven't gotten much traction.

you can no longer define the requested relationships to be returned in the payload

Can you not do this with include for some reason?

[klobuczek]

@richmolj the major issue here is that even if you do not request any relationships via include and do not specify any in sparse fields you still get all the relationship objects. Please note the difference between relationship and resource objects. With sparse fields you are supposed to be able to restrict the relationship objects while include controls resource objects. I do not understand the use case in the specification for having included resources without the corresponding relationship objects, but I would still follow the spec rather than picking just the parts that I like and agree with. Following the spec makes life easier for everyone involved and it is the very reason for having a specification in the first place.

What you are calling breaking for other clients is hardly breaking. It has not worked differently before and it is just merely inconvenience to specify things twice as per specification while not having a workaround for previous functionality without forking is a breaking change.

[richmolj]

I think the real issue here is the 0.x version we are on. We've explicitly kept this version for exactly this type of reason. The specification has much room for improvement, and we've never had (or have tried to have) 100% compliance. For instance, we have never rejected requests without the application/vdn.api+json header, and I think we're better for it.

What we really need is a 1.0 version, that commits to no backwards-incompatible changes and a changelog describing the changes we are making. This is why we are moving to Graphiti, the 1.0 version of JSONAPI Suite. Graphiti is just ending its beta stage, and we'll enter rc1 which will commit to a changelog and backwards-compatible guarantee.

Regarding this specific issue, Graphiti will not render relationships if not present via the include parameter. This is a change from jsonapi_compliable, and yes I mean the relationships not resource objects. From your description, this accommodates your use case...but I have never been against a PR adding "strict mode" or an elsewise compatible solution. Per my original comment, I am also not against a PR to this repo if you cannot upgrade to Graphiti.

I've been working on this issue for years and from everyone I talked to, the only reason the spec reads this way is because relationships can be rendered by default. Graphiti will never do this, which means the reasoning behind the spec does not apply. I stand by that logic, but agree it is a problem if we are introducing backwards-incompatible solutions without a changelog. This is why I am devoting my efforts to a backwards-compatible 1.0 solution in Graphiti.