-
Notifications
You must be signed in to change notification settings - Fork 114
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
Prefer: return=minimal
can hide resource item on server modification
#256
Comments
Prefer: return=minimal
can hide resource item modification via OnUpdate hookPrefer: return=minimal
can hide resource item on server modification
I have actually gotten to think about this a bit more. Quote from RFC7240:
I suppose this means we can do what ever we want. I guess the real question is, which use-cases are we considering for Here are the two use-cases I can think of:
Use-case 2 isn't limited to PATCH, but is equally valid for POST and PUT. For POST and PUT, one could assume that calculating the model changes involved assuming no changes to what ever was posted. If hooks are run, this doesn't hold, and the item must be updated from the server. Indeed, weather or not the client model should be updated could also theoretically depend on field-selection. My biggest issue with being smart for case 2, is that then the client would need to handle both 204 No Content (use calculated changes) and 201/200 (item changed by hooks). If the client is not likely to include such code, we probably don't want the added complexity server side to support it. If we want to consider field-selection (or embedding) as well, we will have an even harder time finding out if we should retur 200/201 or 204. Use-case 1 is still valid, and less complex to support. |
Thanks for the deep analysis. I don't think
In case of POST, when new item is sent, omit step 1, and instead of 200 server returns 201. |
I get the use-case, but I am not sure we should prioritize it, and if we do, not considering field-selection doesn't really give a accurate solution.
Field selection is valid to supply not only for GET requests, but also for PUT, PATCH and POST requests. If the client has a given model for a resource, where certain fields are selected, or perhaps even aliased, it makes sense to always ask to get the same fields returned, also on update operations. So for an extended version of your use-case, if the client does not use or ask for a field X (e.g. a read-only To support both this use-case and the other use-case I mentioned, it's far easier to just return 204 No Content always (as we do today), and if the client believes that it can correctly calculate the new model accurate enough for the client needs, that's fine; we don't need to push an updated model on the client in the case where we belive that the client might be wrong. Strictly speaking, we can never know server side which method the user will use to calculate a model update, nor if it is 100% compatible with the implementation server side. Even json-patch can behave differently with different implementations. We also don't know server side if the client asked for a minmal response because it want to calculate the change itself, or if did so for some other reason. |
I see now, didn't thought of passing projection to PUT/POST/PATCH. In any case server should return what it has. Whether or not, or how client consumes this data is not server's concern. I believe we can solve this, the way we are solving the issue for the whole document, i.e. calculate checksum for the projected fields before and after the hooks, then compare. Let me put this into code, to be more clear. |
One issue I see, is that PUT, can return 200 or 201 depending on whether item exists or not. However returning just 204 from here, will obscure that knowledge. Don't know how to proceed, maybe always return full body on 201, and ignore "Prefer" header. This creates inconsistency though. |
Good point. The RFC states the following:
I suppose we could return a 201 with an empty body as well. |
When `Prefer: return=minimal` HTTP request header is set, server will try to return `204 No Content` response code. However since rest-layer's hooks can modify the resource item server-side, rest-layer will return full body if that happens, even if `return=minimal` requested. If field projection is requested, only fields within projection are considered for the logic above. Resolves rs#256
Thanks @smyrman . Please find above a preview of one possible solution. |
@Dragomir-Ivanov, have you heard about anyone using The solution code looks interesting, but I am still not convinced about the use-case. I still think the use-case 1 that I mention is equally valid as use-case 2, and when unsure which use-case is the most important, choosing the option that leads to less complexity wins in my book. We can alter the current solution to return a more accurate header, e.g. On your use-case in particular, is there a specific reason that you want/need to use the |
Actually, I don't have any special sympathy to 204. I will be equally happy with 200/201 with empty body. My models are heavily updated and I use |
Maybe we should just try your solution, and get a feel for how it works in practice. Would you be willing to try it out before we do a PR, and get some feel for how the handling of it works client-side? If you can include some stats/estimations/examples on reduced traffic, that would be awesome, but of course completely optional. It it works out well for you, we can do the PR, if it doesn't add much value (over e.g. always returning no content / an empty body), then we stick with something closer to what we got. Another great advice for reduced traffic in general is to add a gzip middleware for all requests (in and out). We are using https://github.com/nytimes/gziphandler. |
Thanks @smyrman . Yes, |
Cool. |
Supplying
Prefer: return=minimal
HTTP header will prevent receiving resource item body in the response. This is useful to minimize traffic, when we know that server will not modify the resource item further. However this is not guaranteed, becauserest-layer
has hooks, that can modify the resource.My suggestion is, when such modification is detected, instead of returning 204, we return 200 with the full resource item body.
We can have such detection mechanism via check-summing the payload, before and after the hooks execution as @smyrman suggested.
The text was updated successfully, but these errors were encountered: