fix: return ETag header on GET for config resource endpoints#1603
Open
KirylKurnosenka wants to merge 8 commits into
Open
fix: return ETag header on GET for config resource endpoints#1603KirylKurnosenka wants to merge 8 commits into
KirylKurnosenka wants to merge 8 commits into
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
KirylKurnosenka
requested review from
Oleksii-Klimov and
astsiapanay
as code owners
June 9, 2026 19:02
This comment has been minimized.
This comment has been minimized.
…ests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
astsiapanay
previously approved these changes
Jun 11, 2026
Removed the try-catch in evaluateConditionalGet() that was silently swallowing If-Match failures (412) on GET requests. RFC 7232 does not exempt GET from If-Match evaluation; only If-None-Match has special GET/HEAD semantics (304). Both 304 and 412 now propagate as HttpException through the Future failure path to handleWriteError, consistent with how ResourceController handles conditional headers. Also simplified GetResult away — evaluateConditionalGet now returns Future<String> (the ETag or null). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
Author
|
/deploy-review
|
Contributor
Author
|
/deploy-review
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Applicable issues
Description of changes
GET v1/applications/public/{name}(handled byResourceController) correctly returns anETagresponse header and enforces RFC 7232 conditional headers. However,GET v1/roles/platform/{name},GET v1/models/public/{name}, and other config resource endpoints handled byConfigResourceControllerhad two issues:Missing ETag on 200 responses —
ConfigResourceControlleralready fetched blob metadata (including the ETag) to supportIf-None-Match/ 304 conditional GET, but the fetched ETag was discarded and never added to the response header on the normal 200 path.If-Match silently ignored on GET — the
evaluateConditionalGetmethod caught allHttpExceptions fromEtagHeader.validate()and only surfaced the 304 case, silently swallowingIf-Matchfailures. RFC 7232 §3.1 does not exempt GET fromIf-Matchevaluation, so a mismatchedIf-Matchheader should return 412, consistent withResourceController.Fix:
evaluateConditionalGetnow callsetag.validate()without a try-catch, lettingHttpException(304 or 412) propagate through theFuturefailure chain tohandleWriteError, exactly asResourceControllerdoes.evaluateConditionalGetfrom aGetResultrecord toFuture<String>(the ETag ornull), removing the now-unnecessary wrapper.handleSingleGetandhandleSchemaGetset theETagresponse header on 200 OK when the entity has a blob-backed ETag.ETagheader.Tests (
ConfigResourceConditionalHeaderTest):testGet200IncludesEtagHeader— plain GET on a blob-backed model assertsETagheader is present on the 200 response.testGet200OnStaleIfNoneMatch— assertsETagis returned on 200 when the client sends a staleIf-None-Match.testGet200OnMatchingIfMatch— GET with a matchingIf-Matchreturns 200 with anETagheader.testGet412OnStaleIfMatch— GET with a mismatchedIf-Matchreturns 412.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.