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

Clarification of resource requirements #37

Open
darrelmiller opened this issue Feb 19, 2024 · 4 comments
Open

Clarification of resource requirements #37

darrelmiller opened this issue Feb 19, 2024 · 4 comments

Comments

@darrelmiller
Copy link
Contributor

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
Copy link

Acconut commented Feb 20, 2024

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
Copy link

musiKk commented Feb 21, 2024

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
Copy link

awwright commented Apr 13, 2024

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
Copy link

asbjornu commented Aug 3, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Discussion
Development

No branches or pull requests

5 participants