-
Notifications
You must be signed in to change notification settings - Fork 955
Update codegen to automatically add chunked encoding signer property … #6551
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
base: master
Are you sure you want to change the base?
Conversation
| "requestValidationModeMember": "ChecksumMode", | ||
| "responseAlgorithms": ["CRC32C", "CRC32", "SHA1", "SHA256"] | ||
| "responseAlgorithms": ["CRC32C", "CRC32", "SHA1", "SHA256"], | ||
| "requestAlgorithmMember": "ChecksumAlgorithm" |
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.
Added requestAlgorithmMember because per smithy doc https://smithy.io/2.0/aws/aws-core.html#aws-protocols-httpchecksum-trait, either requestAlgorithmMember or requestChecksumRequired is required
The httpChecksum trait MUST define at least one of the request checksumming behavior, by setting the requestAlgorithmMember or requestChecksumRequired property,
| .modelProvider(ClientTestModels::serviceS3) | ||
| .classSpecProvider(ModelBasedAuthSchemeProviderSpec::new) | ||
| .caseName("mini-s3") | ||
| .caseName("s3-test") |
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.
I changed it to use real s3 model because why not 🤷🏼♀️ and it may help us catch S3 issues
| .caseName("s3-test") | ||
| .outputFileSuffix("fallback-provider") | ||
| .build(), | ||
| // S3 control |
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.
Added s3 control because it uses legacy signature version
| } | ||
|
|
||
| /** | ||
| * Returns a map from a list of operations to the list of auth-types modeled for those operations. The {@link AuthTrait} |
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.
I tried my best to explain what this method does here because it's quite hard to understand, lmk if it's still not clear.
…for streaming operations with http checksum trait
baff246 to
839f640
Compare
|



Motivation and Context
When streaming operations have HTTP checksum traits, they need chunked encoding enabled in the auth signer. Previously, this had to be manually configured. This PR automates the process by detecting these operations during code generation and automatically adding the
CHUNK_ENCODING_ENABLEDproperty to their auth scheme metadata.Additionally, the
ModelAuthSchemeKnowledgeIndexclass was refactored to improve maintainability and clarity.Modifications
Automatic Chunked Encoding Detection:
• Added shouldUseChunkedEncoding() to detect streaming operations with HTTP checksum traits
• Automatically marks these operations with CHUNK_ENCODING_ENABLED property in auth scheme metadata
Refactored ModelAuthSchemeKnowledgeIndex:
• Extracted helper methods for better separation of concerns:
• operationsShouldUseChunkedEncoding() - Identifies operations needing chunked encoding
• processOperationsWithAuthTraits() - Handles main processing loop
• determineAuthScheme() - Decides between custom auth or service defaults
• processRemainingChunkedEncodingOperations() - Handles chunked ops with service defaults
• hasAuthTrait() - Filters operations and validates auth traits
• Generalized validation logic with hasCustomServiceAuthSchemeOverride() instead of hardcoded service checks
• Added javadocs explaining flow and special cases
Test Updates:
• Added test models for S3 and S3Control with chunked encoding scenarios
• Updated existing tests to verify chunked encoding property is correctly added
Testing
• Added new test cases for streaming operations with HTTP checksums
• Verified chunked encoding property is correctly added to auth metadata
• All existing unit tests pass
• Local mvn install succeeds
Types of changes
• [x] New feature (non-breaking change which adds functionality)
Checklist
• [x] I have read the CONTRIBUTING document
• [x] Local run of mvn install succeeds
• [x] My code follows the code style of this project
• [x] My change requires a change to the Javadoc documentation
• [x] I have updated the Javadoc documentation accordingly
• [x] I have added tests to cover my changes
• [x] All new and existing tests passed
• [ ] I have added a changelog entry
• [ ] My change is to implement 1.11 parity feature - N/A
License
• [x] I confirm that this pull request can be released under the Apache 2 license