rs / rest-layer

REST Layer, Go (golang) REST API framework
http://rest-layer.io
MIT License
1.26k stars 114 forks source link

`Prefer: return=minimal` can hide resource item on server modification #256

Open Dragomir-Ivanov opened 5 years ago

Dragomir-Ivanov commented 5 years ago

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.

smyrman commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

smyrman commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Cool.