Refactor RequestChecksumInterceptor into distinct interceptors
#4384
Conversation
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
landonxjames
left a comment
landonxjames
left a comment
There was a problem hiding this comment.
Looks good, a few questions, but no blockers
| // This decorator must decorate after any of the following: | ||
| // - HttpRequestChecksumDecorator | ||
| // - HttpRequestCompressionDecorator | ||
| override val order: Byte = (minOf(HttpRequestChecksumDecorator.ORDER, HttpRequestCompressionDecorator.ORDER) - 1).toByte() |
There was a problem hiding this comment.
If we want it to always run after wouldn't it be maxOf() +1? If I remember correctly higher orders run later
ex:
| match self { | ||
| Self::UnsizedRequestBody => write!( | ||
| f, | ||
| "Only request bodies with a known size can be chunk-encoded." |
There was a problem hiding this comment.
Maybe ... can be aws-chunked encoded to avoid confusion with transfer-encoding: chunked?
| _runtime_components: &RuntimeComponents, | ||
| cfg: &mut ConfigBag, | ||
| ) -> Result<(), BoxError> { | ||
| if must_not_use_chunked_encoding(ctx.request(), cfg) { |
There was a problem hiding this comment.
Since modify_before_signing will always run before this is there any point in checking must_not_use_chunked_encoding again? Is there a situation where the result there could change between signing and transmit?
| /// order to correctly calculate the total size of the body accurately. | ||
| trailer_lengths: Vec<u64>, | ||
| /// Whether the aws-chunked encoding is disabled. This could occur, for instance, | ||
| /// if a user specifies a custom checksum, rendering aws-chunked encoding unnecessary. |
There was a problem hiding this comment.
That might render it unnecessary, but I wonder if it would have performance implications? We should probably ask about when it should be disabled based on checksum behaviors.
2 participants
Motivation and Context
This PR breaks up
RequestChecksumInterceptorinto two interceptors:RequestChecksumInterceptorandAwsChunkedContentEncodingInterceptorwith additional code restructuring to support the upcoming aws-chunked content encoding enhancement.Description
RequestChecksumInterceptorpreviously handled two distinct responsibilities:This refactor extracts the second responsibility (aws-chunked logic) into
AwsChunkedContentEncodingInterceptor, aiming to enable it via the dedicated trait when available in Smithy.In addition, both interceptors now wrap bodies in
modify_before_transmitinstead ofmodify_before_signingper design requirements.Tips for merging to
feature/http-1.xThis refactor reluctantly adds
http_0xconstructs that will conflict withfeature/http-1.x. Update these files when merging:aws-inlineable/src/http_request_checksum.rs- Apply same 1.x updates as done in branch (note:test_checksum_body_is_retryablewas removed as it tested a utility only used in streaming cases against non-streaming bodies)aws-inlineable/src/aws_chunked.rs- Use 1.x for header names, values, and body trait methods in unit tests, as inhttp_request_checksum.rsaws/rust-runtime/aws-runtime/src/content_encoding.rs- Needs 1.x updatesto
because the
.encoded_length()method has been moved fromAwsChunkedBodytoAwsChunkedBodyOptions(due to thecontent-lengthheader needing encoded length during signing but body creation deferred tomodify_before_transmit)Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.