Ensure valid jsonapi when blank relationship #1930

richmolj · 2016-09-16T18:35:29Z

Purpose

If you specify include_data false, and do not have any links for this
relationship, we would output something like:

{ relationships: { comments: {} } }

This is not valid jsonapi.

Changes

We will now render

{ relationships: { comments: { meta: {} } } }

Caveats

N/A

Related GitHub issues

#1797

Additional helpful information

Relevant jsonapi spec: http://jsonapi.org/format/#document-resource-object-relationships

mention-bot · 2016-09-16T18:35:31Z

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

NullVoxPopuli · 2016-09-16T18:37:35Z

lib/active_model_serializers/adapter/json_api/relationship.rb

@@ -34,6 +34,11 @@ def as_json

        private

+        def ensure_valid!(json)


is this the JSON API recommended way to deal with this problem? seems funky

We definitely need one of data, links, or meta according to the spec. We definitely don't have the first (remember empty data is incorrect here). Empty links also seems invalid, so that's a no go. The section on meta seems to indicate empty meta is valid. So If a better alternative exists, I don't know it.

fair enough

remember empty data is incorrect here

I don't remember. Why can't it be data: null, or data: []?

@remear it's the difference between "we're not saying anything about this relationship one way or the other" versus "this relationship is blank".

For instance, let's say you use ember-data and post belongs to author, both objects already loaded into memory. A subsequent GET to the API that returns author: { data: null } will blow away post.author. Omitting the data key does not.

Why does it need to be a separate endpoint? What if it's an existing one with a parameter? Why not just make an an endpoint for fetching that data? What about a relationships endpoint for that resource? The answers to those questions are better determined by the application and API design.

Why does it need to be a separate endpoint? What if it's an existing one with a parameter?

Then you'd be linking to a collection of posts, instead of a collection of comments, which is invalid (even if the comments are sideloaded via an ?include parameter).

Why not just make an an endpoint for fetching that data?

Because I'd rather not code and test a completely unused endpoint just because relationships: { comments: {} } is not valid jsonapi.

What about a relationships endpoint for that resource?

Not needed, this relationship is only fetched by my web application when sideloading.

The answers to those questions are better determined by the application and API design.

Agree. With the above answers, I am saying it is a reasonable API design to not have a separate endpoint, which means no links can be present.

The schema is the only thing we have against which to actually validate documents. The schema must be fixed before changes like this can be merged.

Merging this PR would still validate against the existing schema 😄

But I am fine with waiting, as long as we verify there are no other concerns.

We should wait. Schema ambiguity is dangerous territory.

richmolj · 2016-09-16T18:38:03Z

@kevintyll brought this up. I'm making this change independent of #1797 since the situation already exists in master.

NullVoxPopuli · 2016-09-19T14:55:36Z

In all this discussion, I've forgotten the root of the issue. @richmolj can you summarize the problem trying to be solved here?

richmolj · 2016-09-19T14:59:29Z

@NullVoxPopuli is there something missing from the PR description I could add? The problem is that AMS is outputting invalid jsonapi. No actual errors thrown or anything like that within AMS, but errors might be thrown by clients consuming this json and expecting a compliant structure.

Props for adding the word root to as many issues as possible 😆

NullVoxPopuli · 2016-09-19T15:00:31Z

Ok, cool. So this is just about validating against JSON API's schema. So, lets all revisit this when the schema gets updated. :-)

richmolj · 2016-09-19T15:11:28Z

Cool, here is a PR to fix the schema: json-api/json-api#1094

NullVoxPopuli · 2016-09-19T16:43:53Z

Ok, now that that is merged, @richmolj what does the difference between meta: {} and links: {} mean to you?

richmolj · 2016-09-19T16:58:17Z

@NullVoxPopuli I have a hunch links: {} is invalid. I've opened an issue to get clarification.

To answer your question, empty links is meaningless to me, which is why I think it is invalid. Empty meta tells me "I'm not saying anything about this relationship one way or the other", which is the intent of this PR.

If links: {} is valid, I'd be happy to accept that as a solution, though I find meta: {} more sensible, personally.

NullVoxPopuli · 2016-09-19T17:01:19Z

I guess one way to think about it that if links is empty that says: there is no way to get any information about this relationship. Meaning that the only way to even get that relationship is through the endpoint you just hit.

meta: {} just feels ambiguous to me.

idk.

@remear thoughts?

richmolj · 2016-09-19T17:04:08Z

Honestly, I think this is a jsonapi issue, not an AMS issue. the jsonapi maintainers have told us empty meta is preferable.

I don't care too much one way or the other as long as it's valid, but suggest we try to convince them if we want to go in a different direction. Seems like an area AMS and jsonapi should be on the same page.

NullVoxPopuli · 2016-09-19T17:05:09Z

well, if JSON API people say that it's preferable, then I guess that's what we go with.

NullVoxPopuli

Testing out github's review feature. Pretty handy. I know how to use it now!

NullVoxPopuli · 2016-09-19T17:06:27Z

lib/active_model_serializers/adapter/json_api/relationship.rb

@@ -25,7 +25,7 @@ def as_json
          meta = meta_for(association)
          hash[:meta] = meta if meta


Now for code review. :-)

I don't like that ensure_valid! just adds a blank meta. The method name is misleading.
Since the logic is so simple, can you add that logic to this method?

My thought was this is a placeholder for other things we might want to do, but sure no worries.

I don't want to have too many placeholders right now. Overall, we're trying to simplify AMS. :-)

I've force pushed this change. I still find the updated code less descriptive, but my salience is very low.

richmolj · 2016-09-19T17:12:22Z

@NullVoxPopuli turns out links: {} is valid FWIW.

NullVoxPopuli · 2016-09-19T17:13:20Z

I saw that. :-)

If you specify include_data false, and do not have any links for this relationship, we would output something like: `{ relationships: { comments: {} } }` This is not valid jsonapi. We will now render `{ relationships: { comments: { meta: {} } } }` Instead. Relevant jsonapi spec: http://jsonapi.org/format/#document-resource-object-relationships

richmolj force-pushed the empty_meta branch from 57b0f19 to f9410db Compare September 16, 2016 18:36

NullVoxPopuli reviewed Sep 16, 2016

View reviewed changes

NullVoxPopuli requested changes Sep 19, 2016

View reviewed changes

richmolj force-pushed the empty_meta branch from f9410db to a27b908 Compare September 19, 2016 17:13

richmolj force-pushed the empty_meta branch from a27b908 to 3a9d372 Compare September 19, 2016 17:18

NullVoxPopuli approved these changes Sep 19, 2016

View reviewed changes

NullVoxPopuli merged commit a77dfda into rails-api:master Sep 19, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure valid jsonapi when blank relationship #1930

Ensure valid jsonapi when blank relationship #1930

richmolj commented Sep 16, 2016

mention-bot commented Sep 16, 2016

NullVoxPopuli Sep 16, 2016

richmolj Sep 16, 2016

NullVoxPopuli Sep 16, 2016

remear Sep 16, 2016

richmolj Sep 16, 2016

remear Sep 19, 2016

richmolj Sep 19, 2016

remear Sep 19, 2016

richmolj Sep 19, 2016

NullVoxPopuli Sep 19, 2016

richmolj commented Sep 16, 2016

NullVoxPopuli commented Sep 19, 2016

richmolj commented Sep 19, 2016

NullVoxPopuli commented Sep 19, 2016

richmolj commented Sep 19, 2016

NullVoxPopuli commented Sep 19, 2016

richmolj commented Sep 19, 2016

NullVoxPopuli commented Sep 19, 2016

richmolj commented Sep 19, 2016 •

edited

Loading

NullVoxPopuli commented Sep 19, 2016

NullVoxPopuli left a comment

NullVoxPopuli Sep 19, 2016

richmolj Sep 19, 2016

NullVoxPopuli Sep 19, 2016

richmolj Sep 19, 2016

richmolj commented Sep 19, 2016

NullVoxPopuli commented Sep 19, 2016

		@@ -34,6 +34,11 @@ def as_json

		private

		def ensure_valid!(json)

		@@ -25,7 +25,7 @@ def as_json
		meta = meta_for(association)
		hash[:meta] = meta if meta

Ensure valid jsonapi when blank relationship #1930

Ensure valid jsonapi when blank relationship #1930

Conversation

richmolj commented Sep 16, 2016

Purpose

Changes

Caveats

Related GitHub issues

Additional helpful information

mention-bot commented Sep 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richmolj commented Sep 16, 2016

NullVoxPopuli commented Sep 19, 2016

richmolj commented Sep 19, 2016

NullVoxPopuli commented Sep 19, 2016

richmolj commented Sep 19, 2016

NullVoxPopuli commented Sep 19, 2016

richmolj commented Sep 19, 2016

NullVoxPopuli commented Sep 19, 2016

richmolj commented Sep 19, 2016 • edited Loading

NullVoxPopuli commented Sep 19, 2016

NullVoxPopuli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richmolj commented Sep 19, 2016

NullVoxPopuli commented Sep 19, 2016

richmolj commented Sep 19, 2016 •

edited

Loading