-
Notifications
You must be signed in to change notification settings - Fork 921
Support trailing checksum in DefaultAwsV4HttpSigner #6296
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
Conversation
This commit moves the support for trailing checksums from HttpChecksumStage to the V4 signer signer implementation; this puts it in line with how sync chunked bodies are already handled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR moves trailing checksum support from HttpChecksumStage to the V4 signer implementation to align with how sync chunked bodies are handled. The change consolidates checksum handling logic within the signer rather than having it split between different pipeline stages.
- Removes trailing checksum logic from
HttpChecksumStage
for async operations - Implements comprehensive async chunked encoding support in
AwsChunkedV4PayloadSigner
- Adds new subscriber classes for reactive stream checksum calculation
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
HttpChecksumStage.java | Removes async trailing checksum logic that was handled separately |
DefaultAwsV4HttpSigner.java | Updates async signing flow to support chunked encoding with checksums |
AwsChunkedV4PayloadSigner.java | Implements async chunked encoding with trailing checksum support |
V4PayloadSigner.java | Adds async beforeSigning hook for payload preprocessing |
SignerUtils.java | Adds async content length calculation utilities |
LengthCalculatingSubscriber.java | New subscriber for calculating content length from reactive streams |
UnbufferedChecksumSubscriber.java | New subscriber for checksum computation without buffering |
ChunkedEncodedPublisher.java | Minor improvements to trailer handling and adds getter methods |
Various test files | TCK compliance tests and unit tests for new subscriber classes |
Comments suppressed due to low confidence (1)
core/http-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/AwsChunkedV4PayloadSigner.java:190
- [nitpick] This error message is split across lines making it harder to read. Consider consolidating it into a single, well-formatted message.
// should not happen, this header is added by moveContentLength
// should not happen, this header is added by moveContentLength | ||
.orElseThrow(() -> new RuntimeException("x-amz-decoded-content-length " | ||
+ "header not present")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message should be more descriptive and use a more specific exception type. Consider using IllegalStateException instead of RuntimeException and improve the message clarity.
// should not happen, this header is added by moveContentLength | |
.orElseThrow(() -> new RuntimeException("x-amz-decoded-content-length " | |
+ "header not present")); | |
// This header is expected to be added by moveContentLength. Its absence indicates an unexpected state. | |
.orElseThrow(() -> new IllegalStateException("Missing required header: x-amz-decoded-content-length. " | |
+ "This header is necessary for calculating the encoded content length.")); |
Copilot uses AI. Check for mistakes.
.orElseThrow(() -> new RuntimeException("x-amz-decoded-content-length " | ||
+ "header not present")); | ||
|
||
long encodedContentLength = calculateEncodedContentLength(request, decodedContentLength); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable 'request' is being passed to calculateEncodedContentLength, but it should be 'requestBuilder' which is the actual SdkHttpRequest.Builder instance being used in this context.
long encodedContentLength = calculateEncodedContentLength(request, decodedContentLength); | |
long encodedContentLength = calculateEncodedContentLength(requestBuilder, decodedContentLength); |
Copilot uses AI. Check for mistakes.
|
||
if (checksumAlgorithm != null) { | ||
String checksumHeaderName = checksumHeaderName(checksumAlgorithm); | ||
request.appendHeader(X_AMZ_TRAILER, checksumHeaderName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable 'request' should be 'requestBuilder' to maintain consistency with the rest of the method and ensure the header is added to the correct request builder instance.
request.appendHeader(X_AMZ_TRAILER, checksumHeaderName); | |
requestBuilder.appendHeader(X_AMZ_TRAILER, checksumHeaderName); |
Copilot uses AI. Check for mistakes.
String checksumHeaderName = checksumHeaderName(checksumAlgorithm); | ||
request.appendHeader(X_AMZ_TRAILER, checksumHeaderName); | ||
} | ||
request.putHeader(Header.CONTENT_LENGTH, Long.toString(encodedContentLength)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable 'request' should be 'requestBuilder' to maintain consistency and ensure the header is set on the correct request builder instance.
request.putHeader(Header.CONTENT_LENGTH, Long.toString(encodedContentLength)); | |
requestBuilder.putHeader(Header.CONTENT_LENGTH, Long.toString(encodedContentLength)); |
Copilot uses AI. Check for mistakes.
request.putHeader(Header.CONTENT_LENGTH, Long.toString(encodedContentLength)); | ||
request.appendHeader(CONTENT_ENCODING, AWS_CHUNKED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable 'request' should be 'requestBuilder' to maintain consistency and ensure the header is appended to the correct request builder instance.
request.putHeader(Header.CONTENT_LENGTH, Long.toString(encodedContentLength)); | |
request.appendHeader(CONTENT_ENCODING, AWS_CHUNKED); | |
requestBuilder.putHeader(Header.CONTENT_LENGTH, Long.toString(encodedContentLength)); | |
requestBuilder.appendHeader(CONTENT_ENCODING, AWS_CHUNKED); |
Copilot uses AI. Check for mistakes.
Closing to break this up into smaller PRs. Addition of truncating content length from upstream publisher adds too much to this PR. |
This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one. |
Motivation and Context
This commit moves the support for trailing checksums from HttpChecksumStage to the V4 signer signer implementation; this puts it in line with how sync chunked bodies are already handled.
Modifications
Testing
Screenshots (if appropriate)
Types of changes
Checklist
mvn install
succeedsscripts/new-change
script and following the instructions. Commit the new file created by the script in.changes/next-release
with your changes.License