feat: add ETag support with conditional request handling#327
feat: add ETag support with conditional request handling#327alecthomas wants to merge 4 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a2f9b13bb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ea37c2822
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74883f9da7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if resp.StatusCode == http.StatusPreconditionFailed { | ||
| return nil, ErrPreconditionFailed |
There was a problem hiding this comment.
When a conditional HEAD fails, the APIV1 handler includes the current ETag on the 412 response, and Open() already returns those headers on the same status. Dropping resp.Header here means callers using Stat(ctx, key, WithIfMatch(staleETag)) cannot read the server's current ETag from the failed validation and must issue another request, despite the response carrying it.
Useful? React with 👍 / 👎.
Add content-hash (SHA-256) based ETags to all cache backends, with full If-Match and If-None-Match support threaded from the HTTP serving layer through to storage backends. ETag generation: - All backends (disk, memory, S3) compute SHA-256 content hashes during Create() via a shared HashingWriter, producing consistent ETags across tiers (same content = same ETag regardless of backend). - S3 backend does a server-side copy-to-self after upload to update stored metadata with the computed ETag. Cache interface changes: - Open() now accepts variadic Precondition options (WithIfMatch, WithIfNoneMatch) for atomic precondition checking, eliminating TOCTOU races between Stat() and Open() that would cause data corruption with future range request support. - Returns ErrNotModified or ErrPreconditionFailed when preconditions are not met, along with stored headers (including ETag). - CheckPreconditions() helper for use by Stat/PUT/DELETE paths. APIV1 HTTP handler: - GET/HEAD: If-None-Match → 304, If-Match → 412 - POST: If-Match/If-None-Match → 412, returns new ETag in response - DELETE: If-Match/If-None-Match → 412 - 304 responses include full stored headers per RFC 7232 §4.1 Client: - Precondition type with WithIfMatch/WithIfNoneMatch options - Open, Stat, Create, Delete all accept preconditions (variadic, backward compatible) - ErrNotModified and ErrPreconditionFailed sentinels - 304 and 412 responses return headers (including ETag) RFC compliance: - If-Match uses strong comparison (W/ prefix ETags don't match) - If-None-Match uses weak comparison per RFC 7232 §2.3.2 - Conditional request headers stripped from stored object metadata
Per RFC 7232, the '*' wildcard tests for resource existence, not for the presence of an ETag. An existing object without an ETag should still match '*' for both If-Match (§3.1) and If-None-Match (§3.2). This ensures backward compatibility with pre-ETag cache entries.
When Stat returns ErrNotExist in putObject, If-Match (including *) must fail (nothing to match), and If-None-Match (including *) must pass (nothing to conflict with). Previously, empty headers were passed to CheckPreconditions which couldn't distinguish non-existent from existing-without-ETag.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1c8b07fe2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if v := r.Header.Get("If-None-Match"); v != "" { | ||
| conds = append(conds, cache.WithIfNoneMatch(v)) | ||
| } | ||
| if v := r.Header.Get("If-Match"); v != "" { | ||
| conds = append(conds, cache.WithIfMatch(v)) |
There was a problem hiding this comment.
When a client or proxy sends the same list-valued precondition header more than once, Header.Get only evaluates the first field value and ignores the rest. For example, a GET with two If-None-Match fields where the current ETag is in the second field will stream a 200 response instead of returning 304; the same extraction pattern is used by HEAD/POST/DELETE. Combine r.Header.Values(...) before passing the value to the ETag parser.
Useful? React with 👍 / 👎.
The type now carries more than just preconditions (will include byte ranges for range request support). Rename to reflect the broader scope: - Preconditions -> OpenOptions - Precondition -> OpenOption - ResolvePreconditions -> ResolveOpenOptions - applyPreconditions -> applyOpenOptions CheckPreconditions and ErrPreconditionFailed retain their names since they specifically deal with precondition semantics.
1 participant
Add content-hash (SHA-256) based ETags to all cache backends, with full
If-Match and If-None-Match support threaded from the HTTP serving layer
through to storage backends.
ETag generation:
Create() via a shared HashingWriter, producing consistent ETags across
tiers (same content = same ETag regardless of backend).
stored metadata with the computed ETag.
Cache interface changes:
WithIfNoneMatch) for atomic precondition checking, eliminating
TOCTOU races between Stat() and Open() that would cause data
corruption with future range request support.
are not met, along with stored headers (including ETag).
APIV1 HTTP handler:
Client:
backward compatible)
RFC compliance: