ietf-wg-httpapi / idempotency

Repository for "The Idempotency-Key HTTP Header Field"
Other
17 stars 10 forks source link

Clarification of resource requirements #37

Open darrelmiller opened 7 months ago

darrelmiller commented 7 months ago

Resources MUST publish a idempotency related specification. This specification MUST include expiration related policy if applicable. A resource is responsible for managing the lifecycle of the idempotency key.

It does not seem appropriate to have a MUST requirement on out of band documentation.

This section should focus on resource behavior observable by the client. Discussion of fingerprinting seems unnecessary unless it affects behaviour visible to the client.

If the concurrent request error should be a 409 then the status code should be stated explicitly. Why is it a SHOULD and not a MUST?

Requiring a retry request to returning the same response does not seem appropriate. A first PATCH that ends up returning a 201 because the resource previously did not exist, should probably return a 200 on the second attempt.

Acconut commented 7 months ago

Requiring a retry request to returning the same response does not seem appropriate. A first PATCH that ends up returning a 201 because the resource previously did not exist, should probably return a 200 on the second attempt.

The benefit of reusing the response from the first request would be that it could be implement on the resource's side by caching the response. If a request arrives with an idempotency key and fingerprint which are similar to an entry in the cache, the cached response can be sent to the client without invoking any endpoint-specific logic. I am not sure if that justifies this requirement, but it would make implementations easy.

musiKk commented 7 months ago

I recently worked on a project where this decision had to be made. Should idempotent responses serve the old body or the current representation? The decision has been made to go with the latter. The decision was based on the principle that an idempotent retry avoids side effects but otherwise has the same semantics as the original request; hence it would respond with the current representation instead of an outdated one. Whether this decision was the right one or will cause problems, remains to be seen. The trade-off regarding a potential caching solution was accepted. Also the implementation is arguably easier and requires less infrastructure.

What makes the most sense probably depends on the specific use case. It would make sense to me that the spec either leaves this open (but addresses it so readers can make an informed decision) or recommends or prescribes one of the options based on observations from the real world (however this should be accomplished).

awwright commented 5 months ago

I agree, it defeats the point for a standard to say almost the entire behavior is implementation-defined. Otherwise the header is merely a reserved name. I'm concerned this header doesn't seem to have any requirements at all—there are no MUST requirements on the server that are observable from the client, or vice-versa. Most of the BCP14 language seems to be descriptive or specifying the scope of the specification, and not providing behavior guarantees.

asbjornu commented 2 months ago

Add me to the list of people who agree. I've also implemented idempotency guarantee systems that had to make the same decision @musiKk describes and we came to the same conclusion.