Skip to content
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

Open
Dragomir-Ivanov opened this issue Aug 19, 2019 · 12 comments
Open
Labels
proposal A suggestion for change that has not been accepted

Comments

@Dragomir-Ivanov
Copy link
Contributor

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, because rest-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.

@Dragomir-Ivanov Dragomir-Ivanov changed the title Prefer: return=minimal can hide resource item modification via OnUpdate hook Prefer: return=minimal can hide resource item on server modification Aug 19, 2019
@smyrman
Copy link
Collaborator

smyrman commented Aug 20, 2019

I have actually gotten to think about this a bit more.

Quote from RFC7240:

The determination of what constitutes an appropriate minimal response is solely at the discretion of the server.

I suppose this means we can do what ever we want. I guess the real question is, which use-cases are we considering for return=minimal?

Here are the two use-cases I can think of:

  1. The client prefer to omit the response because it is not going to need the updated model. E.g. a create or update operation that will change the view once complete.
  2. The client prefer to omit the response because we want to calculate model changes client-side. E.g. a list-view edit solution.

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.

@Dragomir-Ivanov
Copy link
Contributor Author

Dragomir-Ivanov commented Aug 21, 2019

Thanks for the deep analysis. I don't think field-selection is even applicable, at least for the issue in my mind.
Here is my workflow in mind for PUT/PATCH.

  1. Client fetches a resource item.
  2. Client makes some field modifications on the item.
  3. Client sends the item to the server, holding item's current state, and says "Hey if you don't modify the item further, don't bother sending it back, I already know how it looks like".
  4. Server receives the item, runs some hooks and store it. If the server wishes to honor the Prefer HTTP header, it can check if after all hooks the item is the same. If the same it return 204, if not it returns 200.
  5. Client receives the response, and since he had requested Prefer he has to be prepared to handle both 200 and 204, because server may not honor the Prefer and return 200 always.

In case of POST, when new item is sent, omit step 1, and instead of 200 server returns 201.
Hope this gives some light.

@smyrman
Copy link
Collaborator

smyrman commented Aug 21, 2019

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.

I don't think field-selection is even applicable,

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 updatedAt field), then even if the server updates the given field via a hook, the most correct response to the client is still 204 No Content.

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.

@Dragomir-Ivanov
Copy link
Contributor Author

Dragomir-Ivanov commented Aug 21, 2019

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.

@Dragomir-Ivanov
Copy link
Contributor Author

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.

@smyrman
Copy link
Collaborator

smyrman commented Aug 22, 2019

Good point.

The RFC states the following:

Typically, such responses would utilize the 204
(No Content) status, but other codes MAY be used as appropriate, such
as a 200 (OK) status with a zero-length response entity.

I suppose we could return a 201 with an empty body as well.

Dragomir-Ivanov added a commit to Dragomir-Ivanov/rest-layer that referenced this issue Aug 22, 2019
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
@Dragomir-Ivanov
Copy link
Contributor Author

Thanks @smyrman . Please find above a preview of one possible solution.
No tests and README updates for now.

@smyrman
Copy link
Collaborator

smyrman commented Aug 23, 2019

@Dragomir-Ivanov, have you heard about anyone using Prefer return=minimal in the way you propose?

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. 201 Created with an empty body when a new resource is created over 204 No Content on update. This could be useful as messages/notifications in the view would be able to differentiate between the main outcomes of a request: "item created", " item updated" or "item update failed". But I am not sure we should do anything more than this on this point.

On your use-case in particular, is there a specific reason that you want/need to use the Prefer return=minimal header yourself? In which cases is it useful for the client to implement both 204 and 200/201 handling over just implementing the latter to update a client-side model?

@Dragomir-Ivanov
Copy link
Contributor Author

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 Prefer return=minimal to minimize traffic generated for my clients, because they use mobile quite heavily, as well conserve traffic for my servers ( paying less ).

@smyrman
Copy link
Collaborator

smyrman commented Aug 23, 2019

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.

@Dragomir-Ivanov
Copy link
Contributor Author

Thanks @smyrman . Yes, gzip is mandatory these days. I am doing it on the reverse-proxy server, though, along with HTTPS termination. Using Caddy server.

@smyrman
Copy link
Collaborator

smyrman commented Aug 23, 2019

Cool.

@smyrman smyrman added the proposal A suggestion for change that has not been accepted label Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal A suggestion for change that has not been accepted
Projects
None yet
Development

No branches or pull requests

2 participants