Storage Content Validation - Audit DecodedResponse#49147
Open
gunjansingh-msft wants to merge 5 commits into
Open
Storage Content Validation - Audit DecodedResponse#49147gunjansingh-msft wants to merge 5 commits into
gunjansingh-msft wants to merge 5 commits into
Conversation
gunjansingh-msft
requested review from
a team,
alzimmermsft,
browndav-msft,
ibrandes,
kyleknap and
seanmcc-msft
as code owners
Strip overrides and tests that aren't required for transparent HttpResponse behavior, leaving exactly the compiler-required abstract methods plus one discretionary override that fixes a proven base-class bug. DecodedResponse.java: - Remove close() override. Current consumers fully drain the body Flux, so Reactor's onComplete signals release the underlying transport; the explicit close-forwarding was defensive against a try-with-resources / status-only pattern that no caller in the codebase actually uses. - Collapse no-arg getBodyAsString() to delegate to the charset overload, pinning UTF-8 in one place instead of duplicating the lambda. - Final override surface (8 methods): the 7 abstract methods on HttpResponse in azure-core 1.57.1 (compiler-required) plus getBodyAsBinaryData, which is concrete in the base but seeds BinaryData with the wire Content-Length header, making BinaryData.getLength() return the encoded size (frames + CRC trailers) instead of the decoded payload size. DecodedResponseTests.java: - Remove closeDelegatesToWrappedResponse and closeDoesNotSubscribeToDecodedBody (no override left to validate). - Drop the close-counting assertion from the writeBodyTo test and rename it to inheritedWriteBodyToWritesDecodedBytes; it now uses MockHttpResponse directly. - Strengthen getBodyAsBinaryDataReportsDecodedSizeNotContentLength: assert data.getLength() equals the decoded size (post-consumption). Without the override this returns 71 instead of 7, empirically reproduced. - Drop now-unused trackingResponse helper, AtomicInteger import. Tests: 10 of 10 passing. Each test maps 1:1 to one override or one inherited integration; no test is redundant.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refines the DecodedResponse HTTP response wrapper used by storage content-validation decoding, and adds unit tests to validate that the wrapper preserves response metadata while exposing the decoded body stream.
Changes:
- Refactors
DecodedResponseto delegate status code and headers to the original response instead of caching copies. - Pins
DecodedResponse#getBodyAsString()to UTF-8 by default. - Adds
DecodedResponseTestscovering decoded-body behaviors and inheritedHttpResponsehelpers (buffer,writeBodyTo,getBodyAsInputStream).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| sdk/storage/azure-storage-common/src/main/java/com/azure/storage/common/policy/DecodedResponse.java | Delegates status/headers to the underlying response and defaults getBodyAsString() to UTF-8. |
| sdk/storage/azure-storage-common/src/test/java/com/azure/storage/common/policy/DecodedResponseTests.java | Adds unit coverage for decoded-body accessors and inherited HttpResponse utilities. |
Comment on lines
69
to
72
| @Override | ||
| public Mono<String> getBodyAsString() { | ||
| return FluxUtil.collectBytesInByteBufferStream(decodedBody).map(String::new); | ||
| return getBodyAsString(StandardCharsets.UTF_8); | ||
| } |
Member
There was a problem hiding this comment.
we should preserve the content type chosen like the copilot comment suggests. lets change this to return getBodyAsByteArray().map(b -> CoreUtils.bomAwareToString(b, originalResponse.getHeaderValue(HttpHeaderName.CONTENT_TYPE)));
Comment on lines
+155
to
+159
| @Test | ||
| public void inheritedBufferReturnsResponseBackedByDecodedBody() { | ||
| byte[] decoded = bytes("buffered"); | ||
| DecodedResponse wrapper | ||
| = new DecodedResponse(mockResponse(200, new HttpHeaders(), bytes("encoded")), fluxOf(decoded)); |
Member
There was a problem hiding this comment.
we should take this suggestion.
ibrandes
changed the title
ibrandes
reviewed
May 11, 2026
Member
ibrandes
left a comment
ibrandes
left a comment
There was a problem hiding this comment.
looking good - we should address the 2 open copilot comments though
Comment on lines
69
to
72
| @Override | ||
| public Mono<String> getBodyAsString() { | ||
| return FluxUtil.collectBytesInByteBufferStream(decodedBody).map(String::new); | ||
| return getBodyAsString(StandardCharsets.UTF_8); | ||
| } |
Member
There was a problem hiding this comment.
we should preserve the content type chosen like the copilot comment suggests. lets change this to return getBodyAsByteArray().map(b -> CoreUtils.bomAwareToString(b, originalResponse.getHeaderValue(HttpHeaderName.CONTENT_TYPE)));
Comment on lines
+155
to
+159
| @Test | ||
| public void inheritedBufferReturnsResponseBackedByDecodedBody() { | ||
| byte[] decoded = bytes("buffered"); | ||
| DecodedResponse wrapper | ||
| = new DecodedResponse(mockResponse(200, new HttpHeaders(), bytes("encoded")), fluxOf(decoded)); |
Member
There was a problem hiding this comment.
we should take this suggestion.
DecodedResponse.java (comment r3220562536): - Restore HttpResponse#getBodyAsString() base contract by switching from unconditional UTF-8 to CoreUtils.bomAwareToString(bytes, contentType), matching BufferedHttpResponse's idiom. The previous UTF-8 pin silently dropped BOM detection and Content-Type charset honoring, which callers viewing this as a generic HttpResponse expect. DecodedResponseTests.java (comment r3220562553): - Add inheritedGetBodyAsBinaryDataReturnsDecodedBytes: exercises the inherited HttpResponse#getBodyAsBinaryData() and asserts the bytes match the decoded payload. Uses a divergent Content-Length header to make the wire vs decoded distinction explicit and guard against header-forwarding regressions. - Add getBodyAsStringHonorsCharsetFromContentTypeHeader: proves the bom-aware fix actually honors a charset declared in Content-Type (would fail if the previous UTF-8 pinning were still in place). - Add getBodyAsStringDetectsUtf8BomAndStripsIt: pins the BOM-detection arm of CoreUtils.bomAwareToString. - Update getBodyAsStringDefaultsToUtf8WhenNoCharsetSpecified docstring to describe the new bom-aware fallback semantics rather than the old unconditional UTF-8 behavior. Tests: 12 of 12 passing.
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.
From HttpResponse, the constructor only stores the HttpRequest; getRequest() is final and returns that. Everything else is either abstract (must implement) or has a default on the base class. So for a body-swapping wrapper, we must implement the seven abstract members.