Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify formats for representing relationships #482

Closed
ethanresnick opened this issue Mar 16, 2015 · 16 comments
Closed

Unify formats for representing relationships #482

ethanresnick opened this issue Mar 16, 2015 · 16 comments

Comments

@ethanresnick
Copy link
Member

Right now, we have two formats for representing a relationship.

When the relationship is being displayed in a response, it gets the full link-object representation:

"author": {
  "related": "", 
  "self": "",
  "linkage": ""
  //...
}

But when the relationship is being sent by the client to the server, only the contents of the linkage key are sent. So the format becomes: "author": { "type": "people", "id": "3"}.

It makes sense that creating/updating/deleting relationships is only doable through linkage, but do we really want the two separate formats?

Almost everywhere else in the spec we've gradually switched from preferring conciseness to preferring consistency. Why not do that here too?

If we did that, when clients sent a relationship, they'd specify:

"author": {
  "linkage":  { "type": "people", "id": "3"}
}

And the spec would simply say that these client-provided link objects MUST contain a linkage key.

@bintoro
Copy link
Contributor

bintoro commented Mar 16, 2015

Yes! I had not yet realized this consequence of introducing the linkage key. Input and output representations should absolutely be symmetrical.

@hhware
Copy link
Contributor

hhware commented Mar 17, 2015

Indeed!

@bobholt
Copy link
Contributor

bobholt commented Mar 19, 2015

Just ran into this trying to implement linkage changes in endpoints. The read representation should definitely be the same as the create/update representations.

Due to the verbosity and less-elegant implementation (in the endpoints use case), I'd personally prefer to go back to this for read, create, and update:

"author": {
  "type": "people",
  "id": "3"
},
"comments": {
  "type": "comments",
  "id": ["1", "2", "3"]
}

@tkellen
Copy link
Member

tkellen commented Mar 20, 2015

@bobholt That won't work for heterogenous collections, that's why the other format was introduced. I'm in support of the symmetrical representation suggested by @ethanresnick.

@tkellen
Copy link
Member

tkellen commented Mar 20, 2015

@steveklabnik @dgeb @wycats could you weigh in here so we can continue with #480? As mentioned previously, I'm 👍 for this change.

@dgeb
Copy link
Member

dgeb commented Mar 20, 2015

I agree that symmetry and consistency are important.

There has been a minor asymmetry in request / response formats since RC3 that the introduction of linkage objects has only served to highlight. Previously, it was only apparent for heterogeneous collections. It's been bothering me all along.

I am 👍 to this change as well. The only alternative I've found was to move linkage to be parallel to links, but the duplication of relationship keys seemed unappealing.

@ethanresnick
Copy link
Member Author

Awesome! I'm going to expand #480 to include the other cases of inputing relationship info (e.g. PATCH) and then hopefully we can get that merged!

@mpazik
Copy link

mpazik commented Mar 23, 2015

Why are you introduce heterogenous collections? Do they have any important use case? I've found #237 and arguments against them looks very reasonable.

@bobholt
Copy link
Contributor

bobholt commented Mar 23, 2015

@mpazik I feel the same way, but while I have a strong philosophical bias against polymorphic collections in an API, it seems to be something people have done, and thus json-api needs to support.

I considered proposing that the base spec only support homogeneous collections and that heterogeneous collections be an extension, but I'm not sure it provides enough value.

@dgeb
Copy link
Member

dgeb commented Mar 23, 2015

Why are you introduce heterogenous collections? Do they have any important use case?

Although I personally try to avoid heterogeneous collections in APIs, many APIs embrace them. ember-data fully embraces them, which is one reason why @wycats felt strongly about including support in the RC2 rewrite.

I considered proposing that the base spec only support homogeneous collections and that heterogeneous collections be an extension, but I'm not sure it provides enough value.

I've tried exploring this path but found support for heterogeneous collections to be such a primary design consideration that it affects almost all aspects of an API's design. A heterogeneous extension might be radically different from a purely homogeneous base spec.

@steveklabnik
Copy link
Contributor

@steveklabnik @dgeb @wycats could you weigh in here so we can continue with #480?

I'm really opposed to any significant changes, as that's what we've already promised with RC3, and client libraries are already implementing it.

@bobholt
Copy link
Contributor

bobholt commented Mar 23, 2015

This issue is part of the due diligence around the change on March 10. This issue was opened before RC3 was pushed less than a week later. RC3 and its promise of a major spec freeze came as a complete surprise to me, especially given the then-recent structural changes we hadn't had the opportunity to test out. Implementing RC3 in endpoints is currently on hold pending resolution of this issue.

Consistency of the data model has been such an important part of the specification that I wholeheartedly disagree with abandoning it now.

@bintoro
Copy link
Contributor

bintoro commented Mar 23, 2015

I'd like to echo what @bobholt said. The core proposal of this issue (already implemented and ready to merge as PR #480) simply takes the recent linkage format change to completion.

Discarding the PR would mean introducing an arbitrary input/output mismatch in resource representations, i.e., you can't PATCH/PUT what you GET. I would regard that as a pretty significant defect in any RESTful API.

@tkellen
Copy link
Member

tkellen commented Mar 23, 2015

Chiming in to agree with @bobholt and @bintoro.

@dgeb
Copy link
Member

dgeb commented Mar 23, 2015

I agree with this proposal as well. I think this kind of minor correction fits with the final review process, which we added to shake out just these sorts of inconsistencies and implementation problems.

@dgeb
Copy link
Member

dgeb commented Mar 26, 2015

#480 has been merged with approval from @steveklabnik and agreement from many implementors. Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants