-
Notifications
You must be signed in to change notification settings - Fork 871
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
Upgrade links to RFC8288-modelled web links #1348
Conversation
I was just digging back into JSON Schema and found some prior art WRT to using JSON Pointers as fragments: https://json-schema.org/latest/json-schema-core.html#rfc.section.5 |
94e2807
to
e4ef638
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments. Looks good overall @gabesullice
_format/1.1/index.md
Outdated
attributes. | ||
|
||
Any additional member names **MUST** be valid target attribute names as defined by | ||
[RFC8288 Section 2.2](https://tools.ietf.org/html/rfc8288#section-2.2). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unclear to me how this paragraph interacts with the previous paragraph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I combined it with the previous one. Let me know it still feels disjointed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does seem better, but still not crystal clear. How about something like:
A link parameter object MAY contain other members, all of which MUST be considered target attributes. The names of target attributes MUST conform with RFC8288 Section 2.2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this section limits the ability to ever reserve the meaning of any members beyond hreflang
, media
, title
, and type
. This makes me a bit wary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reading of this is slightly different. I think this is trying to specify that:
- all target attributes must conform to RFC8288 section 2.2
- the
hreflang
,media
,title
andtype
target attributes have a superset of requirements: they must also conform to RFC8288 section 3.4.1 (in addition to conforming to RFC8288 section 2.2)
If my interpretation is correct, I agree that this should be made more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I significantly cleaned up the section on target attributes. Very curious to know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting close! A few minor nits and a couple things to discuss.
_format/1.1/index.md
Outdated
attributes. | ||
|
||
Any additional member names **MUST** be valid target attribute names as defined by | ||
[RFC8288 Section 2.2](https://tools.ietf.org/html/rfc8288#section-2.2). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does seem better, but still not crystal clear. How about something like:
A link parameter object MAY contain other members, all of which MUST be considered target attributes. The names of target attributes MUST conform with RFC8288 Section 2.2.
_format/1.1/index.md
Outdated
attributes. | ||
|
||
Any additional member names **MUST** be valid target attribute names as defined by | ||
[RFC8288 Section 2.2](https://tools.ietf.org/html/rfc8288#section-2.2). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this section limits the ability to ever reserve the meaning of any members beyond hreflang
, media
, title
, and type
. This makes me a bit wary.
_format/1.1/index.md
Outdated
attributes. | ||
|
||
Any additional member names **MUST** be valid target attribute names as defined by | ||
[RFC8288 Section 2.2](https://tools.ietf.org/html/rfc8288#section-2.2). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reading of this is slightly different. I think this is trying to specify that:
- all target attributes must conform to RFC8288 section 2.2
- the
hreflang
,media
,title
andtype
target attributes have a superset of requirements: they must also conform to RFC8288 section 3.4.1 (in addition to conforming to RFC8288 section 2.2)
If my interpretation is correct, I agree that this should be made more clear.
eaebe79
to
fbaefa7
Compare
Co-Authored-By: Dan Gebhardt <dan@cerebris.com>
Co-Authored-By: Dan Gebhardt <dan@cerebris.com>
🎉 |
I know there has been much discussion of links in this project. This PR attempts to faithfully project RFC8288 style links onto the JSON:API specification. It is very intentionally concise and not feature rich—meaning that it completely avoids topics like hyper-schema, URI templating, inter alia.
If this proposal is not acceptable without those features, I'd ask that this PR be closed. In other words, let's not let those things derail getting a sensible start to better hypermedia support in JSON:API. Nothing here precludes those features. In essence, "a bird in hand is worth two in the bush".
This approach lifts the
link-param
idea from RFC8288 and serializes them inapplication/vnd.api+json
format under a newparams
member[1].The established link-params
rel
,anchor
andrev
have special handling, this is my reasoning:rel
: inherits from the link object key to maximize backwards compatibility. I used the SHOULD key word for this inheritance to guard against that case that a key may not be a valid link relation type.anchor
: Because JSON:API has a very well defined structure, I think anchors can be implicitly defined for theapplication/vnd.api+json
media type. In other words, that they don't have to explicitly be added as an object member.I considered adding language to define a new
anchor
member that could be placed under anymeta
object so that custom anchors could be used, but I left that out. I think that that choice makes this easier to land and to implement for clients and for servers.The biggest problem I see here is that it makes it difficult for implementors to comply with RFC3986's language about consistency of fragments between different representations. Frankly, I think that's a really pie-in-the-sky requirement to begin with and I wouldn't feel too bad about ignoring it. The
meta.anchor
idea couldalways be added later if it becomes a real problem.
@ethanresnick suggested elsewhere that a fragment should be defined by
type
andid
. That would make it easier to meet the consistency requirement, but the problem I faced with that approach is that a fragment also should be able to identify a relationships object within a resource object. A fragment syntax like{type}:{id}(:{relationship field name})?
is the naive approach, but falls down because there are no rules for validid
formats. I.e., it would break if a resource object'sid
contained a colon (:
). Perhaps there are clever ways around that?rev
: this is deprecated by RFC8288 so I tried to reinforce this with a SHOULD NOT key word.All other valid link-params are acceptable, like
hreflang
for example, in addition to any custom ones.To preclude any misunderstanding with how RFC8288 Link header link parameters are parsed, I added the language stipulating that multi-value parameters must be arrays so that implementors do not try to use space-delimited or comma-delimited parameter values like one would for a
Link
header.1: I know I said that this PR should not discuss URI templating, but I just want to step in front of any criticism of the
params
member name by saying, "adding avars
member would be a perfectly compatible way to support templating later" ;) And that's all I want to say about that :P