-
Notifications
You must be signed in to change notification settings - Fork 855
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
Introduce Local IDs to v1.1 #1244
Conversation
👍 Thanks for writing this up. I just want to note the connection between the approach here and #1245: adding { "lid": "...." /* not id key here; maybe no type either, but that doesn't matter */ } This makes lids basically an optional feature that can hard fail if not supported, analogous to how the base spec handles unsupported optional features like Again, a new format like that may not be necessary; I'm just highlighting the dependency on #1245 in its absence. |
@dgeb Very much in favour! 🏅 This should enable implementors such as Ember Data to solve some issues around nested resources. May also be useful as an alternative to JSON pointers. |
I've updated this PR to introduce the concept of a top-level By including an identifier in this array, a client can guarantee that the server will return it, regardless of the shape of the |
@dgeb Awesome improvement! 😄 Easy to parse for clients (that need to pair up |
While the I've also reworded some of the language in this PR as per suggestions from @courajs in #1254. I think I will let the optional |
_format/1.1/index.md
Outdated
together with `type`, uniquely identifies the resource _locally_ within the | ||
document. If a [resource object][resource objects] contains a `lid` member, then | ||
every representation of that resource, including other [resource objects] and | ||
[resource identifier objects], **MUST** also contain the `lid` member with the |
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.
This [resource identifier objects]
fails to link to the correct section. Should this instead be
[resource identifier objects][resource identifier object]
_format/1.1/index.md
Outdated
A [resource object][resource objects] **MAY** contain a `lid` member that, | ||
together with `type`, uniquely identifies the resource _locally_ within the | ||
document. If a [resource object][resource objects] contains a `lid` member, then | ||
every representation of that resource, including other [resource objects] and |
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.
missing word: "...of that resource, including within other..."
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 don't think that word is missing? Because:
including other ROs and RIOs must also contain […]
("including within other ROs and RIOs must also contain" does not sound right to me)
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.
The full sentence:
If a resource object contains a lid member, then every representation of that resource, including other resource objects and [resource identifier objects], MUST also contain the lid member with the same value.
I am suggesting clarifying that the other resources objects are referring to the subject of the sentence, not that they themselves are other versions of it (which is prohibited by JSON API).
Here's another go, even more specific:
If a resource object contains a lid member, then every representation of that resource, including referential representations contained within other resource objects and [resource identifier objects], MUST also contain the lid member with the same value.
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 pointing out a few potential clarifications.
P.S.: after reading "lid
member" many times now, I can't help but point out something funny: "member" in Dutch is … "lid" 😄
_format/1.1/index.md
Outdated
A [resource object][resource objects] **MAY** contain a `lid` member that, | ||
together with `type`, uniquely identifies the resource _locally_ within the | ||
document. If a [resource object][resource objects] contains a `lid` member, then | ||
every representation of that resource, including other [resource objects] and |
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 don't think that word is missing? Because:
including other ROs and RIOs must also contain […]
("including within other ROs and RIOs must also contain" does not sound right to me)
_format/1.1/index.md
Outdated
A "resource identifier object" **MUST** contain `type` and `id` members. | ||
A "resource identifier object" **MUST** contain a `type` member. A resource | ||
identifier object **MUST** also contain an `id` member, except when it contains | ||
a `lid` that identifies it as matching a new resource to be created on the |
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.
Should this then state explicitly that lid
s can only exist in request documents, not in response documents?
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.
@wimleers lid
can exist both in the request and the response object (that's sort of their purpose)
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.
Right, to communicate to the client what the server-generated id
for a client-specified lid
is. Of course. Silly me!
_format/1.1/index.md
Outdated
|
||
A "resource identifier object" **MAY** also include a `meta` member, whose value is a [meta] object that | ||
contains non-standard meta-information. | ||
A "resource identifier object" **MAY** also include a `meta` member, whose value |
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.
There is no actual change here, just reflowing. This change can/should hence be reverted AFAICT.
_format/1.1/index.md
Outdated
@@ -510,7 +538,7 @@ Each member of a links object is a "link". A link **MUST** be represented as | |||
either: | |||
|
|||
* a string containing the link's URL. | |||
* <a id="document-links-link-object"></a>an object ("link object") which can | |||
* <a id="document-links-link-object"></a>an object ("link object") which can |
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.
There is no actual change here, just reflowing. This change can/should hence be reverted AFAICT.
After reading @EndangeredMassa's review at #1254 (review), I started wondering why we even need I'm assuming it's for two reasons:
|
@wimleers Thanks for reviewing! And yes, you are correct that the entire need for |
@dgeb @wimleers Hang on - this statement is not true:
The broader domain problem being solved by
By far the most common driver of this is that the server does not allow for client ID generation, not
You might argue the validity of any of those points from a design perspective, but for now I just |
@DLiblik I wholeheartedly agree that the spec should continue to be compatible with systems which do not support client-generated UUIDs. I believe the conditional |
10-4 @dgeb - and thanks for putting this all together. Btw, we've being doing this "our own way" since 2015 (we tucked a |
Bit late in, but as I pointed out in another PR this is the first specification name that is truncated. Any reason we shouldn't just call this a |
@sandstrom there are a few minor details to iron out in #1268. I hope we can land that very soon, and then follow up with local IDs and operations. |
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.
Looks good to me!
I'm concerned about the parameter name 'lid'. It doesn't convey any information about what the param is actually used for. Could we consider changing it to something more meaningful? I don't see any params that are two combined words in the spec anywhere so not sure if that is allowed or handled with hyphens or underscores but what about 'linkage_id', 'local_id', or something else that ties in with the description of what the purpose of the parameter is? |
@jesseclark I understand your point, but I actually prefer a succinct name in this case. It's clearly explained in the spec, so it's easy to look up ( Somewhat similarly, the spec also contain |
@sandstrom Also, |
lid is going to be impossible to Google.
…On Fri, Sep 21, 2018, 9:36 AM Jesse Clark ***@***.***> wrote:
@sandstrom <https://github.com/sandstrom> id has been so long in usage in
computing that it is fairly universally understood at this point, 'lid' not
so much. If someone is looking at a payload that contains 'lid' they almost
have to go look it up, while a name like 'linkageid' would at least give
some clue to the purpose and seeing that param with the same values in
different sections of a json document one could infer that the field linked
those sections together. 'lid' increases cognitive dissonance.
Also, 'meta' and 'prev' are also in somewhat common usage in programming.
Both are also abbreviations for one word while 'lid' crushes together two
abbreviations. Would 'mda' or 'pre' have been acceptable alternatives to
'meta' or 'prev'?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1244 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAUb2VvojqRRUW0IDmIE3lS2MM3s1SVfks5udRVxgaJpZM4Q74_a>
.
|
@jesseclark @krainboltgreene Stop bike shedding please. We really need local ids land to JSONAPI. IMHO It has been enough time discussing that. lt is just fine regarding the constraints the jsonapi team has set. So, let it go and keep going forward. |
This is a specification. Understandability and searchability are not
"bikeshed" topics. They're paramount.
On Sep 21, 2018 4:26 PM, "Jean-Baptiste Escoyez" <notifications@github.com> wrote:
@jesseclark <https://github.com/jesseclark> @kraigboltgreene Stop bike
shedding please. We really need local ids land to JSONAPI. IMHO It has been
enough time discussing that. lid is just fine regarding the constraints the
jsonapi team has set. So, let’s does it go and keep forward.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1244 (comment)>,
or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAUb2Rx4ggxAfdLRnKGlAUdEUBt4KDlOks5udXW-gaJpZM4Q74_a>
.
|
@krainboltgreene You are right. Sorry for the negative comment above. It does not bring anything good to the conversation. I'm just a bit concerned to re-discuss something that has been already discussed a lot in #1197 and in this PR. |
The discussion seems to have stalled to the naming of the local id attribute the last time. Multiple commentators are saying that |
Are there any news on this? Local IDs will help solve some concrete problems around duplicate data with nested records. For example, this is solved in Ember Data under JSON API ([see thread[(https://github.com/emberjs/data/pull/4441#issuecomment-524501809)) using a If the spec could introduce this much-discussed feature I think many would find it useable. |
@sandstrom please see #1435 for a discussion of extensions as well as the newly proposed local identities extension in #1436. I'm going to keep this PR open until we make a final decision about including local identities as an extension vs. directly in the base spec. Either way, we know they're an important addition. |
A client may include a "local ID" as a `lid` member in a resource object to uniquely identify the resource within the request document. Every representation of that resource, whether as a resource object or resource identifier object, must then include the matching `lid` member and value. This addition paves the way for requests of all kinds that may need to establish linkage between resources that have not yet been assigned a server-generated ID.
After exploring the concept of a local identities extension (#1436), @gabesullice and I have come back around to the original approach taken in this PR. We are planning to introduce a This fills a gap in the base spec in which resources can not reference themselves in relationships when ids are server-generated. For example: POST /people HTTP/1.1
Content-Type: application/vnd.api+json"
Accept: application/vnd.api+json
{
"data": {
"lid": "a",
"type": "person",
"attributes": {
"firstName": "John",
"lastName": "Doe"
},
"relationships": {
"bestFriend": {
"data": {
"lid": "a",
"type": "person"
}
}
}
}
} This will also prove useful for extensions, such as atomic operations (#1437), and undoubtedly many others. I should also note that, although the updated spec makes no allowance for Please review the updated language in this PR. We'd like to merge this into the 1.1 spec and get a new draft out soon. |
@dgeb I think this is a great outcome. We've been using this approach in practice for several years now in a proprietary MERGE extension we put in place in a number of systems now to handle batch (atomic) operations (slightly different syntax, but same idea: a local ID where a global one is not available). It's worked very well and we have yet to hit a transactional situation that broke down (to be clear, I'm not suggesting any changes to use MERGE as the verb - just indicating our particular flavor of rubber bands and popsicle sticks). For any transactions that assign a global ID to the object during the server transaction we also return both local and (assigned) global ID fields so that the client can tell which witch is which when reintegrating into client-side state. |
@dgeb Looks great! 💯 |
We are also using this approach for atomic operations. We even have extracted it so that it is reusable throughout our Ember app. I'm excited to see this landing in the 1.1 specs so that it can be implemented in "official" JSONAPI libs. 🎉 |
I think we've settled on a limited scope, well defined solution. This fills a gap in the base spec, while providing the necessary foundation for new extensions (like atomic operations in #1437). Thanks for the help, everyone! |
A client may include a "local ID" as a
lid
member in a resource object touniquely identify the resource within the request document. Every representation
of that resource, whether as a resource object or resource identifier object,
must then include the matching
lid
member and value.When a server receives a request document that contains resources with local
IDs, the server must include the matching
lid
member and value in everyrepresentation of that resource or resource identifier in the response document.
This addition paves the way for requests of all kinds that may need to establish
linkage between resources that have not yet been assigned a server-generated ID.
Note: This proposal obviously overlaps with a portion of #1197, but I'd like to more fully integrate the concept of local ids in the spec without restricting them to a section on "side posting".
Note: I'm proposing the name
lid
after considering negative feedback to mykey
proposal in #1197. I favor a name that references either "local" or "document" instead of "temporary", because this field does not really seem to have a temporal component to it. I also don't like squishing multiple words into one (liketempid
) but an abbreviation likelid
is different and consistent withid
.