-
Notifications
You must be signed in to change notification settings - Fork 878
Revision Added as a Top-Level Member of Resource #824
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
base: gh-pages
Are you sure you want to change the base?
Conversation
I'd say that the client MUST send the revision for updating a resource if sent with the GET. The server uses the revision to ensure that updates are send for the right resource revision, so IMHO it's required in all following updates - otherwise it doesn't make sense at all to use revisions. Anither point is what's the content of the 409 response - the current or a merged version of the resource? |
@ziege I agree with you in that originally I wanted to say the something like: If a
I agree if the next version is 1.1 it should be backwards compatible with 1.0 so I changed MUST to either MAY or SHOULD. If and when this standard goes to 2.0 then we should change the above to MUST. To what the content of the response should be for a
I read that as the specification is agnostic on what to return as a response.
Another sidebar: Can someone explain the following line in the specification in the Responses section for updating resources:
I don't understand that statement. The |
By my reading, a MUST here actually would be backwards compatible, because it's a MUST that's conditional on getting a I have a different concern with the current wording of the PR, though, which is that I don't think it's accurate to say that Consider a request to a relationship endpoint. If
Likewise, I'd imagine that a So, really, the identifier is still I also still wonder if there's a way to make Etag and |
I agree with @ethanresnick here and share the concerns he's raised. If we allow There is a lot of subtlety and potential for confusion here. |
So rethinking having
Are they the same resource? Yes and No. They are the same resource but at different versions. For identity purposes though it is just The final affirmation for me is for resource linkage purposes it doesn't make sense to have Therefore I have come to the same conclusion as you guys and to quote @dgeb:
Do not want that now do we. Therefore @dgeb What was your take on the verbiage of if the optional |
After further reflection, I'm cautiously supportive of the MUST requirement for including |
I actually favor a MAY for including revision with PATCH. First of all, I don't think a GET should be a pre-requisite for PATCH. Also, some clients will want to force a change (say an update to an attribute) regardless of the resource's revision. By forcing the inclusion of On that same note, the one MUST I would say should be in place is that the server MUST reject a PATCH request that includes |
Isn't there always a GET before you can PATCH anything?
I'd never allow a client to bypass the revision check. Do you have an example when this could be a requirement?
Instead of a MUST I'd allow the server to decide if the request is acceptable or not although the revision doesn't match - this could be useful if you send changes that do not conflict with changes made in the meantime. A conflict doens't necessarily mean an update of the same property, the server can also define other conditions (e.g. properties that depend on each other and therefore cannot be merged). |
I think our differences come down to the question of what is expected of a server when passed a revision that isn't up to date. My assumption has been that the server should always outright reject such a request because the very point of passing in the revision is to ensure that the client and server are working with the same version of the resource. If that's not true, then it implies a sophisticated server that maintains multiple versions of each resource and is able to discern whether there's a conflict based upon a review of the history of changes between versions. |
I want to cautiously disagree with this statement. If a server is returning I currently have text basically saying the following: If a server supplies a
Excellent point and should be added to the specification. @tkellen To your point about etag and revision. I purposely left this out because they are not the same thing: etag is a version of the document as a whole, revision is a version of a specific, individual resource. To @dgeb point in a previous email thread, even if the document is just a single resource, with |
Added a 2nd draft, to summarize:
|
@@ -1453,6 +1470,9 @@ constraints (such as a uniqueness constraint on a property other than `id`). | |||
A server **MUST** return `409 Conflict` when processing a `PATCH` request in | |||
which the resource object's `type` and `id` do not match the server's endpoint. | |||
|
|||
A server **MUST** return `409 Conflict` when processing a `PATCH` request in |
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 think this is too prescriptive. A server may be able to resolve a conflict and accept it.
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.
Also, this normative text is redundant with the text on line 221.
I agree that most servers will likely outright reject mis-matching revisions, but I don't think we should normatively close the door on the possibility of a sophisticated server using JSON-API. For example, databases like datomic do have a full revision history of every change. |
@tkellen Thanks and excellent points, will add to a 3rd draft early next week based on other comments. |
Sure thing @scott-mcdonald! I'm super excited to see this land and to implement it for endpoints. |
One other thing to note, @bobholt assembled a json-api representation of all the normative statements in the spec (to be used for generating test suites). I'm working on making it possible to automatically generate that file, but it is currently still a manual process. If you could include any new normative statements in this file for the next revision that would be awesome: |
Added a 3rd draft based on feedback from @tkellen in this thread. |
@@ -1287,6 +1309,8 @@ primary data, the same request URL can be used for updates. | |||
|
|||
The `PATCH` request **MUST** include a single resource object as primary data. | |||
The resource object **MUST** contain `type` and `id` members. | |||
The resource object **MUST** contain `revision` member if the server supplied | |||
the `revision` member in a `GET` request. |
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.
We can't make a MUST level requirement of clients based on the server implementation. If a 1.1 server includes revision
, then a 1.0 client should still work without including revision
.
Added a 4th draft based on feedback from @dgeb. Basically changed MUST to SHOULD so the specification is backwards compatible when a 1.0 client calls a 1.1 or greater server. Used SHOULD instead of MAY to stress the importance of properly handling updates when |
This seems great @scott-mcdonald! Unfortunately, we can't land this until we make some infrastructural changes to the site to support multiple versions. This would be a part of 1.1-alpha. Also, can you please add your new normative statements to this file? Thanks! |
How would people feel about waiting until we have extensions figured out before landing this? I'm thinking that, if we make this an extension, we might be able to turn some of those SHOULDs into MUSTs or otherwise make stronger guarantees in way that's backwards compatible than we can if we try to add this to the base spec. |
As long as you lead the charge to getting extensions figured out I am game
|
@tkellen Works for me! |
Gentlemen, I like @ethanresnick train of thought with respect to the SHOULD really being MUST for strong guarantees in terms of optimistic concurrency control at the resource level. This is what most developers would think is the "normative" use case, IMHO. Again IMHO, I feel it is a stretch to think it should be an extension though. To me an extension is something a client would negotiate FOR because it is looking for an additional feature but why would a client negotiate for revision at the resource level; this is something a service would want to impose without negotiation. The Bulk extension makes perfect sense as an extension but resource version does not feel right. I guess I would have to see how this works before making a final judgment. From my perspective resource version is so fundamental it should be part of the base specification. The issue is with the backwards compatibility with 1.0 forever edict. By the way does that mean if the specification was at 4.2 it would always have to be backwards compatible to 1.0? Again IMHO, if you increment the major version number it is no longer backwards to the previous version, this is what most developers would think is the "normative" use case I believe. Under this pretext I would do the following if I was an owner: Keep the specification as it is currently written for 1.1 so it is backwards compatible to 1.0 with the SHOULD instead of the MUST. But if the specification ever goes to 2.0 change the SHOULD to MUST, etc. The 1.0 specification has the "jsonapi" object that has the version in question (1.0, 1.1, 2.0) so I don't see this as a big deal. But I am sure you gentlemen have already discussed this forever backwards compatibility to 1.0 "ad nauseam" so the previous statements are probably not going to fly... @tkellen as far as if this being a part of 1.1 alpha and being delayed I am in no big hurry so take your time. I currently have "version" in the attributes of a resource where needed for optimistic concurrency control, etc. When this did "land" was going to refactor to "revision" at the top level of resources, etc. BTW, I do see "caching" at the resource level leveraging |
A quick clarifying response from a skim of your note @scott-mcdonald. JSON-API will never bump major. If it does, it becomes a new project with a new media type. Backwards compatibility is a contract we refuse to break. |
Upon further consideration, I do agree with @scott-mcdonald that this should be a part of the base specification. |
@scott-mcdonald What you said makes a lot of sense, so I'll revise my position too: I'm not necessarily against specifying this feature (or most of it) in the base spec. What was really motivating my earlier comment was a feeling that I just want to make sure we don't rush this, which I was worried might happen if we try to work on it concurrently with our focus on extensions and #795, which are the two issues marked for 1.1. As @dgeb and @tkellen have also said: there's a fair amount of subtlety here. For example, here are some outstanding things I'm wondering about:
If we can work all these details out easily, then awesome; I am excited to see this land! But I do want to make sure we're able to work them out first. |
Hi @ethanresnick, Understood about not wanting to rush. IMHO, I feel this topic has been highly vetted to this point reflecting in high quality of what has been produced to date (4 drafts for example) which is a good thing. Before answering your specific points let me provide an Abstract from my point of view as a foundation: Abstract The primary use case for a Your Questions
The What I like about this approach is in the base specification revision is defined as an opaque identifier. This opens up further refinement in all kinds of extensions that could go further on the actual semantics of the revision value, i.e. resource level caching.
This is up for debate. I just felt if a service is supplying
Agreed and this is why I used the following verbiage in the Update section (please note the unable to resolve text): A server SHOULD return
I have not given this much thought and quite frankly should be it's own ISSUE and deployable item. I do know that Summary IMHO, I believe how revision is currently written primarily for optimistic concurrency control is at the right level for the base specification. I believe this maximizes flexibility for later extensions as in the base specification it is defined as an opaque identifier and we are not being too restrictive. I would release it as-is with it currently being on the more minimal side; let the community digest it and see what feedback, issues come up and then refactor as needed. |
Hi @scott-mcdonald, I've been thinking more about this recently and would like to get it merged as soon as possible. I agree with your answers to my second and third questions, and I think there are only three things left to settle:
|
Also, @scott-mcdonald, I apologize if it feels like I'm nitpicking; I don't mean to, and I know you've been trying to get this merged for a long time. Keep in mind, though, that once we add something to the base spec, we can never take it back. That's why I'm harping so much on making sure we have the meaning specified precisely; and why I'm trying to make sure that, as much as possible, we're complementing rather than duplicating Etags; and that we're designing the feature to give us a bit of extensibility for future use cases; etc. |
Fully support this! An example: a client is PATCHing a user record by updating its email. If the client knows for sure that the new email is correct (e.g., because it is this client who is actually changing it), then it may not matter what the current value of email is. IMHO, it should be up to the API designer whether to allow clients to update attributes this way. The spec should not mandate pointless back-and-forth (GET before PATCH, possibly multiple times) unless it makes sense for the particular API.
Accordingly, I do not support this proposal.
Fully support that too! IMHO, If we are talking about an abstract sophisticated server, simple string
I think MUSTs are out of question here because they assume that the server keeps track of past revisions and is able to identify the past revision with the same value of the attributes, so the only option is MAY. Hence the question: why is it even necessary for the spec to be concerned with it? Perhaps I am missing something, but what would be broken if it is left undefined and even implemented inconsistently from one endpoint to another endpoint of a given API? The only consequence I see is possible cache miss in the situation where it might have been hit.
Cannot it just say "If the resource is updated, value of its
By defining a dedicated header or by allowing to supply multiple revisions in
Why? IMHO, one use case for |
@@ -1453,6 +1481,14 @@ constraints (such as a uniqueness constraint on a property other than `id`). | |||
A server **MUST** return `409 Conflict` when processing a `PATCH` request in | |||
which the resource object's `type` and `id` do not match the server's endpoint. | |||
|
|||
A server **SHOULD** return `409 Conflict` when processing a `PATCH` request in | |||
which the resource object's `revision` does not match the server's endpoint | |||
and the server was *unable to resolve* the `revision` mismatch for updating |
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 would remove this and the following lines, see #824 (comment)
…for versioning of resources. Signed-off-by: Scott McDonald <[email protected]>
…ndex.md file. Signed-off-by: Scott McDonald <[email protected]>
@ethanresnick Hi Ethan, I have done another revision for this story based on our exchange; when you get a chance take a look and see what you think. I noticed the introduction of "1.1" so I moved my revision member changes to the 1.1 _format/index.md file from the 1.0 version which I think is correct. |
@scott-mcdonald Thanks for reworking this, Scott! On a quick glance, the current version looks much better! I'll do a careful review this weekend. |
@ethanresnick Did you ever have a chance to review this? |
@tkellen No, I didn't, and I want to apologize to @scott-mcdonald for that, since this is a very valuable contribution that's been sitting for far too long. I got about 80% through a review at one point, and found a couple problems, but then got focused on extensions. I'll try to go through this thread later this week and finish a review, so that we can get at least a beta version of this out ASAP. |
@@ -159,6 +159,7 @@ the client and represents a new resource to be created on the server. | |||
|
|||
In addition, a resource object **MAY** contain any of these top-level members: | |||
|
|||
* `revision`: a [revision string][revision] representing a specific version of an individual resource uniquely identified by `type` and `id`. |
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 could also be a recommendation as a resource meta
Hi |
Because we fetch the row before update and apply changes on that, a concurrency violation is only reported when two concurrent requests update the same row in parallel. Instead, we want to produce an error if the token sent by the user does not match the stored token. To do that, we need to convince EF Core to use that as original version. That's not too hard. Now the problem is that there is no way to send the token for relationships or deleting a resource. Skipped tests have been added to demonstrate this. We could fetch such related rows upfront to work around that, but that kinda defeats the purpose of using concurrency tokens in the first place. It may be more correct to fail when a user is trying to add a related resource that has changed since it was fetched. This reasoning may be a bit too puristic and impractical, but at least that's how EF Core seems to handle it. Solutions considerations: - Add 'version' to resource identifier object, so the client can send it. The spec does not explicitly forbid adding custom fields, however 'meta' would probably be the recommended approach. Instead of extending the definition, we could encode it in the StringId. - Once we have access to that token value, we need to somehow map that to 'the' resource property. What if there are multiple concurrency token properties on a resource? And depending on the database used, this could be typed as numeric, guid, timestamp, binary or something else. - Given that PostgreSQL uses a number (uint xmin), should we obfuscate or even encrypt that? If the latter, we need to add an option for api developers to set the encryption key. See also: json-api/json-api#600 json-api/json-api#824
*Update (2021-11-08): Rebased on latest changes in master branch* Because we fetch the row before update and apply changes on that, a concurrency violation is only reported when two concurrent requests update the same row in parallel. Instead, we want to produce an error if the token sent by the user does not match the stored token. To do that, we need to convince EF Core to use that as original version. That's not too hard. Now the problem is that there is no way to send the token for relationships or deleting a resource. Skipped tests have been added to demonstrate this. We could fetch such related rows upfront to work around that, but that kinda defeats the purpose of using concurrency tokens in the first place. It may be more correct to fail when a user is trying to add a related resource that has changed since it was fetched. This reasoning may be a bit too puristic and impractical, but at least that's how EF Core seems to handle it. Solutions considerations: - Add 'version' to resource identifier object, so the client can send it. The spec does not explicitly forbid adding custom fields, however 'meta' would probably be the recommended approach. Instead of extending the definition, we could encode it in the StringId. - Once we have access to that token value, we need to somehow map that to 'the' resource property. What if there are multiple concurrency token properties on a resource? And depending on the database used, this could be typed as numeric, guid, timestamp, binary or something else. - Given that PostgreSQL uses a number (uint xmin), should we obfuscate or even encrypt that? If the latter, we need to add an option for api developers to set the encryption key. See also: json-api/json-api#600 json-api/json-api#824
*Update (2021-11-08): Rebased on latest changes in master branch* Because we fetch the row before update and apply changes on that, a concurrency violation is only reported when two concurrent requests update the same row in parallel. Instead, we want to produce an error if the token sent by the user does not match the stored token. To do that, we need to convince EF Core to use that as original version. That's not too hard. Now the problem is that there is no way to send the token for relationships or deleting a resource. Skipped tests have been added to demonstrate this. We could fetch such related rows upfront to work around that, but that kinda defeats the purpose of using concurrency tokens in the first place. It may be more correct to fail when a user is trying to add a related resource that has changed since it was fetched. This reasoning may be a bit too puristic and impractical, but at least that's how EF Core seems to handle it. Solutions considerations: - Add 'version' to resource identifier object, so the client can send it. The spec does not explicitly forbid adding custom fields, however 'meta' would probably be the recommended approach. Instead of extending the definition, we could encode it in the StringId. - Once we have access to that token value, we need to somehow map that to 'the' resource property. What if there are multiple concurrency token properties on a resource? And depending on the database used, this could be typed as numeric, guid, timestamp, binary or something else. - Given that PostgreSQL uses a number (uint xmin), should we obfuscate or even encrypt that? If the latter, we need to add an option for api developers to set the encryption key. See also: json-api/json-api#600 json-api/json-api#824
*Update (2021-11-08): Rebased on latest changes in master branch* Because we fetch the row before update and apply changes on that, a concurrency violation is only reported when two concurrent requests update the same row in parallel. Instead, we want to produce an error if the token sent by the user does not match the stored token. To do that, we need to convince EF Core to use that as original version. That's not too hard. Now the problem is that there is no way to send the token for relationships or deleting a resource. Skipped tests have been added to demonstrate this. We could fetch such related rows upfront to work around that, but that kinda defeats the purpose of using concurrency tokens in the first place. It may be more correct to fail when a user is trying to add a related resource that has changed since it was fetched. This reasoning may be a bit too puristic and impractical, but at least that's how EF Core seems to handle it. Solutions considerations: - Add 'version' to resource identifier object, so the client can send it. The spec does not explicitly forbid adding custom fields, however 'meta' would probably be the recommended approach. Instead of extending the definition, we could encode it in the StringId. - Once we have access to that token value, we need to somehow map that to 'the' resource property. What if there are multiple concurrency token properties on a resource? And depending on the database used, this could be typed as numeric, guid, timestamp, binary or something else. - Given that PostgreSQL uses a number (uint xmin), should we obfuscate or even encrypt that? If the latter, we need to add an option for api developers to set the encryption key. See also: json-api/json-api#600 json-api/json-api#824
*Update (2021-11-08): Rebased on latest changes in master branch* Because we fetch the row before update and apply changes on that, a concurrency violation is only reported when two concurrent requests update the same row in parallel. Instead, we want to produce an error if the token sent by the user does not match the stored token. To do that, we need to convince EF Core to use that as original version. That's not too hard. Now the problem is that there is no way to send the token for relationships or deleting a resource. Skipped tests have been added to demonstrate this. We could fetch such related rows upfront to work around that, but that kinda defeats the purpose of using concurrency tokens in the first place. It may be more correct to fail when a user is trying to add a related resource that has changed since it was fetched. This reasoning may be a bit too puristic and impractical, but at least that's how EF Core seems to handle it. Solutions considerations: - Add 'version' to resource identifier object, so the client can send it. The spec does not explicitly forbid adding custom fields, however 'meta' would probably be the recommended approach. Instead of extending the definition, we could encode it in the StringId. - Once we have access to that token value, we need to somehow map that to 'the' resource property. What if there are multiple concurrency token properties on a resource? And depending on the database used, this could be typed as numeric, guid, timestamp, binary or something else. - Given that PostgreSQL uses a number (uint xmin), should we obfuscate or even encrypt that? If the latter, we need to add an option for api developers to set the encryption key. See also: json-api/json-api#600 json-api/json-api#824
I think this could be very well addressed using an extension introduced by v1.1. This would also give room to experiment as such an extension does not require changes to the base spec. Overall it should speed up the process. We could later promote such an extension to an official extension, which should provide similar benefits as adding to the base spec. |
…0 for versioning of resources.
Signed-off-by: Scott McDonald [email protected]