Problem/Motivation
We want to support partial updates of resources in REST module. Example: a client should be able to update the title of a node without having the full representation of that node. The client should be able to send only the title property in a JSON-LD data structure. The client should also be able to remove properties/fields. Example: a client wants to remove all term references from the field_tags property of a node, so it sends an empty flag for field_tags in JSON-LD.
HTTP PUT is not a good fit for partial updates since it has the semantics of completely replacing a whole resource with the transmitted data. Therefore we should use HTTP PATCH which reflects our needs more closely.
Proposed resolution
Implement a PATCH operation for rest.module.
Remaining tasks
- HTTP PATCH is not part of the official HTTP standard yet, so we have to research if it is widely supported by most clients (Javascript, cURL, Guzzle, ...)
- A patch has to be developed and reviewed
User interface changes
none.
API changes
A patch operation is added to entity resource plugins.
Original report by [mitchell]
A PATCH request to "/node/{node}/revision/{vid}" should contain, in the body information, different field values than a source revision or an equivalent patch file.
RFC: 5789 - PATCH Method for HTTP
Several applications extending the Hypertext Transfer Protocol (HTTP) require a feature to do partial resource modification. The existing HTTP PUT method only allows a complete replacement of a document. This proposal adds a new HTTP method, PATCH, to modify an existing HTTP resource.
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#16 | rest-patch-1826688-16.patch | 13.92 KB | klausi |
#16 | rest-patch-1826688-16-interdiff.txt | 5.47 KB | klausi |
#14 | rest-patch-1826688-14.patch | 13.13 KB | klausi |
#14 | rest-patch-1826688-14-interdiff.txt | 1.03 KB | klausi |
#12 | rest-patch-1826688-12.patch | 13.24 KB | klausi |
Comments
Comment #1
mitchell CreditAttribution: mitchell commentedJS clients can use data from #1811510: Enable JSON-LD entity serialization to generate diff payloads instead of sending back the entire entity. See #120955-73: Integrate Diff into Core and this article, Why PATCH Is Good for Your HTTP API, which describes PATCH with JSON and PATCH with XML.
Comment #2
mitchell CreditAttribution: mitchell commentedComment #2.0
mitchell CreditAttribution: mitchell commentedx
Comment #3
Grayside CreditAttribution: Grayside commentedThe explicit patch payload formats as discussed in the "Why PATCH..." article have not reached a standard practice AFAICT, so I assume you mean to allow partial-entity-updates, where the absence of a payload element means to leave it untouched.
Here is a use case. Configuration.
Comment #3.0
Grayside CreditAttribution: Grayside commentedx
Comment #4
klausiI updated the issue summary.
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedIf we want to go that route, we really want to implement full support for
application/json-patch
. There are a couple of implementations floating around, for example [1] and [2]. There are both slightly out of date now.[1] https://github.com/stefankoegl/python-json-patch
[2] https://github.com/mikemccabe/json-patch-php
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedThe issue summary suggests the patch payload to be in JSON-LD and #5 suggests to do it in straight JSON. I don't know if the two are actually compatible: at a minimum, I would expect JSON-LD to require a different mime type than
application/json-patch
. Which begs the question: while JSON-LD makes a lot of sense for receiving (GET), what's its value in sending (PATCH)? And is it even possible to GET in JSON-LD and send in JSON? For example, if we get entity references as URIs in JSON-LD, but need to send them as IDs in JSON, that could be problematic, right?Also, #5 doesn't link to the application/json-patch spec itself, but following the other links, I think the current one is http://tools.ietf.org/html/draft-ietf-appsawg-json-patch-08. Please correct if not.
Comment #7
Grayside CreditAttribution: Grayside commentedIn an ideal world, all output formats are viable input formats, and there is no "type accent" tainting the data in between operations.
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedNo, the two would not be compatible.
I think we want to ask the question the other way around. What is the benefit of introducing application/json-patch in the mix? Is it widely used? Are there implementations we want to integrate with that use it? What does it give us?
I think that it makes sense for the use case that they outline in the spec—working with simple JSON and where the document is extremely flexible, without a fixed set of property names which can be used. However, I don't think that it offers us anything, and I think the DX it offers is worse than PATCHing with the output format (JSON-LD for example) since you actually have to specify both operation AND data, rather than just data.
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedAlso, just saw this in a discussion of json-patch, which sums up how I see it:
http://news.ycombinator.com/item?id=4479442
Comment #10
klausiI agree that the json-patch format is just too verbose and requires a lot of effort on the client side to build the structure correctly.
I'm thinking of our usual JSON-LD format where we require @type and @id and all other properties are optional. Just include those properties in the payload that you want to overwrite.
Here is a fist patch that does not work, largely inspired by #1839366: REST module: PUT/update. I'm having trouble to detect empty array values for properties that should be deleted. The entity API fills up all known properties with empty arrays, so I don't know how to copy empty values from the deserialized entity to the stored entity.
Comment #12
klausiSo we need the entity API to distinguish between empty() and isset() properties in the magic methods. See #1868274: Improved EntityNG isset() test coverage for that, which is included in this patch.
Now PATCHing actually works as expected, "[]" for a field in JSON-LD triggers its deletion. Happy klausi is happy! :-)
Comment #13
moshe weitzman CreditAttribution: moshe weitzman commentedNice work.
Does entity->id() really return a UUID? Is that only the behavior when an entity has not yet been saved? Also, I think it would be clearer if we replaced
$patch_entity->{$entity_info['entity_keys']['id']}
with uuidTo be extra clear, I would say 'over the REST API'. Happens a few times in this file, and maybe others.
Comment #14
klausiNo, $entity->id() does not return a UUID, it returns the usual numeric integer. I just realized that it is not even necessary to copy over the ID property, so I removed that line and another one.
I'm using the term "web API" consistently throughout REST module, so I would leave it at that for now. If we want to change that we should do it an a separate issue.
Comment #15
moshe weitzman CreditAttribution: moshe weitzman commentedCould we change
'UUID was successfully updated.'
to some other field. Its a pretty unrealistic use case to change an entity's uuid.I opened #1869424: Use the terms 'form api' and 'rest api' instead of 'web api' as a followup.
Comment #16
klausiI wrote those tests with different entity types in mind and thought it might be easier to use a property that all of them share (the UUID), once we expand the test to other entity types. But yes, using a field for testing is more realistic.
* Fixed test case to overwrite a field instead of UUID.
* Switched to 204 success status code.
* Made patching not existing entities more robust and added test case.
Comment #17
moshe weitzman CreditAttribution: moshe weitzman commentedThats better - thanks.
Comment #18
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #19
fagoLooks like this commit contained #1868274: Improved EntityNG isset() test coverage which hasn't received any reviews yet :(
Comment #20
klausiYep, this has gone a bit too fast from RTBC to fixed. I should have mentioned it in the summary that this issue depends on another one, sorry for that.
I'm going to set the other issue to "active", so we can develop a followup there.
Comment #21.0
(not verified) CreditAttribution: commentedissue summary template.