CommunitySolidServer icon indicating copy to clipboard operation
CommunitySolidServer copied to clipboard

Return ETag headers on PUT requests

Open joachimvh opened this issue 4 years ago • 11 comments
trafficstars

As discussed in #628.

Currently the PUT/PATCH/DELETE functions return an array of ResourceIdentifiers indicating what was changed. It would be nice if instead a metadata object was returned. We can just use RepresentationMetadata for this. This could then also be returned for addResource, where the identifier of metadata would indicate what the new identifier is.

joachimvh avatar Feb 24 '21 11:02 joachimvh

#981 implements correct status codes by first checking existence of resources. This check would not be needed if metadata was returned that indicated this knowledge.

joachimvh avatar Oct 12 '21 13:10 joachimvh

At the moment, an updated etag is not provided on PUT either (and it seems other editing operations)

A minimal implementation would be to do a getRepresentation straight after setRepresentation in http/ldp/PutOperationHandler.js The resulting metadata could then be passed to the ResponseDescription, as is done in http/ldp/GetOperationHandler.js Obviously this is a case where setRepresentation returning a metadata object would make more sense.

At the moment not providing an updated etag means that any client gets a 412 Precondition failed if it doesn't do a GET before the next PUT (e.g. when doing multiple edits in the sourcePane)

Edit: in case it helps anyone, my workaround using a custom PutOperationHandler component is available here: https://github.com/josephguillaume/my-custom-solid-server/blob/86f2e99edb4312734bacdd6d6f1a6e618e863ccc/src/PutOperationHandler2.ts#L56

josephguillaume avatar Dec 11 '21 06:12 josephguillaume

Changed the title as we do return metadata objects now. To have ETag headers we should also return the last modified time in there. We should only do this if the DataAccessor does not modify the data (which is the case for the SparqlDataAccessor):

An origin server MUST NOT send a validator header field (Section 7.2), such as an ETag or Last-Modified field, in a successful response to PUT unless the request's representation data was saved without any transformation applied to the body

We already set the modified time when saving a resource so we can return that. But the FileDataAccessor will also have to be changed to make sure it actually uses that time for the stored file because otherwise there could be a discrepancy.

joachimvh avatar Oct 05 '22 15:10 joachimvh

I've had a look at this while fixing some other ETag issues in https://github.com/CommunitySolidServer/CommunitySolidServer/pull/1686. While it would not be wrong to add an ETag in some cases in the PUT response, it would not be a full solution for the problem presented here. First of all, as mentioned in the previous comment, it can only be returned if the server guarantees that the input representation will be saved as-is. This is the case when using a file backend, but not when using a SPARQL backend for example. Secondly, this only works for PUT. In case of a PATCH you can't return an ETag for example as the input will never match the actual representation.

All of this to say that this could be added as a minor optimization for clients, but clients should not depend on it as there are valid cases where it can't be returned.

joachimvh avatar Jul 27 '23 13:07 joachimvh

A response to a HTTP PATCH with 200/201 status code and payload (including a representation equivalent to subsequent GET on the same target resource), along with Content-Type (essentially Turtle or JSON-LD) and ETag may be sufficient when using SPARQL backends.

csarven avatar Jul 27 '23 13:07 csarven

Yes it would be an optimization for clients that take this into account, and similarly for PUT in such situations, but clients shouldn't depend on this happening as other Solid servers might not do this.

joachimvh avatar Jul 27 '23 14:07 joachimvh

Any particular reason why servers might respond 200/201 with ETag for PUT but might not for PATCH?

I suspect that since RFC 7232 is a MUST in Solid Protocol, clients can expect ETag. Granted that there is some variability in the type of validator that is used by different servers. That aside, there is a suggestion (MAY) to use a strong validator for RDF bearing representations.

csarven avatar Jul 28 '23 15:07 csarven

Any particular reason why servers might respond 200/201 with ETag for PUT but might not for PATCH?

It's based on my interpretation of this section of RFC 9110:

An origin server MUST NOT send a validator field (Section 8.8), such as an ETag or Last-Modified field, in a successful response to PUT unless the request's representation data was saved without any transformation applied to the content (i.e., the resource's new representation data is identical to the content received in the PUT request) and the validator field value reflects the new representation.

Which I read as saying that if you return an ETag for a request that modifies a resource, the ETag should correspond to the input representation that was received in the request. So if that representation does not match the one that is stored, which is the case for PATCH since there you don't send a full representation, you should not return an ETag.

joachimvh avatar Jul 31 '23 06:07 joachimvh

I ran into that as well. My take on it was that including ETag in the response of a PUT ensures that the ETag value corresponds to the representation data in the request, while all things equal. Had the PUT response came back with a different representation data (with the exception of 201 and no data or content-location), ETag would not have been appropriate because of request semantics of PUT - while PUT is idempotent, I'm not entirely sure if sending the exact same request may yield a different ETag value in the response.

Part of my understanding came from 200 responses including a message body including target resource's representation (and optionally with Content-Location.)

FWIW, https://datatracker.ietf.org/doc/html/rfc5789#section-2.1 includes an example of PATCH with ETag, and 200 would've been fine, so it says.

We/I can dig deeper if you like to make sure of the specific scenarios the above may be acceptable.

Is this more of a spec-level concern for you or more of a CSS architecture/development challenge?

csarven avatar Jul 31 '23 20:07 csarven

FWIW, https://datatracker.ietf.org/doc/html/rfc5789#section-2.1 includes an example of PATCH with ETag, and 200 would've been fine, so it says.

That does indeed seem to indicate that just returning an ETag without body is fine, which would be an easier solution.

Is this more of a spec-level concern for you or more of a CSS architecture/development challenge?

If this would only be CSS specific it is not that urgent for me, I just had a look now as I was fixing other ETag issues. Since in that case a client could still not rely on that behaviour as other Solid servers might not do this. If it is expected of a Solid server to return an ETag in those cases then this would need to be fixed in CSS.

joachimvh avatar Aug 01 '23 07:08 joachimvh

@csarven, during today's meeting you claimed that Solid Resource Servers are already required to include ETags on GET requests, but I cannot seem to find where that is specified. The only mention of them in the Protocol is in this sentence: "Servers MAY use the HTTP ETag header with a strong validator for RDF bearing representations in order to encourage clients to opt-in to using the If-Match header in their requests." The (obsolete) HTTP Conditional Requests spec (to which the protocol adheres) only contains a lot of conditional requirements, and the following recommendation: "An origin server SHOULD send an ETag for any selected representation for which detection of changes can be reasonably and consistently determined." Is this what you were refering to? Because that's part of RFC9110 anyway, which is why I asked what Solid needs to specify more ...

woutermont avatar Aug 16 '23 20:08 woutermont