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

replacing usage of update etag with AIP-154 ifMatch? #241

Open
toumorokoshi opened this issue Nov 6, 2024 · 4 comments
Open

replacing usage of update etag with AIP-154 ifMatch? #241

toumorokoshi opened this issue Nov 6, 2024 · 4 comments

Comments

@toumorokoshi
Copy link
Member

Today, there are two sightly different mechanics for validation of resource freshness. Those are:

in https://aep.dev/update, using an etag field in the resource body (proto), and rejecting the request if that does match.

In https://aep.dev/154/#guidance, using an etag header in the response, and an optional if-match header in the request for resource validation.

These are fundamentally incompatible methods, and we should provide a single way to do so. As an initial proposal, I propose aligning with AIP-154 and:

  • making etag a field that is returned back via grpc metadata, allowing it's usage in the HTTP header.
  • adding guidance in AIP-154 to use grpc metadata to include the if-match header.
@toumorokoshi
Copy link
Member Author

one disadvantage of putting etags in the header is you won't be able to get it from the resource body (e.g. when you curl and write the output to disk for stage purpose). So it might make sense to still have an etag field in the body as place where the value is duplicated.

@rofrankel
Copy link
Collaborator

From https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/If-Match:

The comparison with the stored ETag uses the strong comparison algorithm, meaning two files are considered identical byte by byte only. If a listed ETag has the W/ prefix indicating a weak entity tag, this comparison algorithm will never match it.

It sounds like this means that weak etags would no longer be supported. That seems like an unfortunate limitation.

@dv-stewarts
Copy link
Contributor

Not as important as the actual decision, but I think there some contradiction in the text of https://aep.dev/154/ which should also be resolved:

https://aep.dev/154/#guidance

Resources must support the If-Match header (and may support the If-None-Match header) if and only if resources provide the ETag.

https://aep.dev/154/#condition-headers

Services that provide ETags should support the If-Match and If-None-Match headers on incoming requests:

One is must/may for If-Match/If-None-Match and the other is should for both. Or am I reading it wrong?

@toumorokoshi
Copy link
Member Author

Not as important as the actual decision, but I think there some contradiction in the text of https://aep.dev/154/ which should also be resolved:

https://aep.dev/154/#guidance

Resources must support the If-Match header (and may support the If-None-Match header) if and only if resources provide the ETag.

https://aep.dev/154/#condition-headers

Services that provide ETags should support the If-Match and If-None-Match headers on incoming requests:

One is must/may for If-Match/If-None-Match and the other is should for both. Or am I reading it wrong?

Good callout! In the future I'd suggest filing a separate issue so we can keep the conversation focused.

To address your point though: Yes I think those statements are not consistent, and should be. I'd probably say the latter statement is what I'd lean toward, since etags are useful in their own right for identifying whether a resource changed, even if it's not used as a check-and-set.

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

No branches or pull requests

3 participants