Request compression interceptor and non-streaming compression #4124

davidh44 · 2023-06-21T16:16:32Z

Motivation and Context

Adding request compression interceptor to handle compression, and implementing non-streaming compression.

Modifications

Added CompressionType enum for supported encodings, Compressor interface and GzipCompressor implementation.

Testing

To add tests once streaming compression is implemented.

Was able to send compressed PutMetricData request to CloudWatch successfully

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist

I have read the CONTRIBUTING document
Local run of mvn install succeeds
My code follows the code style of this project
My change requires a change to the Javadoc documentation
I have updated the Javadoc documentation accordingly
I have added tests to cover my changes
All new and existing tests passed
I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

I confirm that this pull request can be released under the Apache 2 license

cenedhryn · 2023-06-21T18:15:29Z

...ain/java/software/amazon/awssdk/core/internal/interceptor/RequestCompressionInterceptor.java

+
+    private static SdkHttpRequest updateContentEncodingHeader(SdkHttpRequest sdkHttpRequest,
+                                                             ExecutionAttributes executionAttributes) {
+        Compressor compressor = resolveCompressionType(executionAttributes);


Can this method take a Compressor as a parameter instead of resolving it? It simplifies the logic and is slightly more efficient.

good catch, updating

dagnir · 2023-06-21T22:15:56Z

core/sdk-core/src/main/java/software/amazon/awssdk/core/compression/Compressor.java

+@SdkPublicApi
+public interface Compressor {
+
+    /*


These should be javadoc strings

oops, adding second *

dagnir · 2023-06-21T22:16:42Z

core/sdk-core/src/main/java/software/amazon/awssdk/core/compression/Compressor.java

+    /*
+     * The compression algorithm type.
+     */
+    String contentType();


Is this supposed to be compressorType, per the javadoc?

right, updating

dagnir · 2023-06-21T22:17:23Z

core/sdk-core/src/main/java/software/amazon/awssdk/core/compression/Compressor.java

+    /*
+     * Compress an async stream.
+     */
+    Publisher<ByteBuffer> compressAsyncStream(InputStream inputStream);


Shouldn't this take a Publisher<ByteBuffer> as input?

yep, updating

dagnir · 2023-06-21T22:18:14Z

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/compression/GzipCompressor.java

+import software.amazon.awssdk.core.exception.SdkClientException;
+
+@SdkInternalApi
+public class GzipCompressor implements Compressor {


can probably be final

adding modifier

dagnir · 2023-06-21T22:28:51Z

core/sdk-core/src/main/java/software/amazon/awssdk/core/compression/CompressionType.java

+    public Compressor compressor() {
+        if (value == null) {
+            return null;
+        }
+        if (value.equals("gzip")) {
+            return new GzipCompressor();
+        }
+        return null;
+    }


Can we move this to the Compressor interface instead, so it matches what we do in SdkChecksum

good idea, migrating

dagnir · 2023-06-22T16:27:28Z

core/sdk-core/src/main/java/software/amazon/awssdk/core/compression/Compressor.java

+            case GZIP:
+                return new GzipCompressor();
+            default:
+                return null;


We should throw for unrecognized types

updated to throw IllegalArgumentException

dagnir · 2023-06-23T21:28:34Z

...ain/java/software/amazon/awssdk/core/internal/interceptor/RequestCompressionInterceptor.java

+        InputStream inputStream = sdkHttpFullRequest.contentStreamProvider().get().newStream();
+        InputStream compressedStream = compressor.compress(inputStream);
+        SdkHttpRequest sdkHttpRequest =
+            sdkHttpFullRequest.toBuilder()
+                              .contentStreamProvider(() -> compressedStream)
+                              .build();
+        sdkHttpRequest = updateContentEncodingHeader(sdkHttpRequest, compressor);


This is is only doing a one-time compression of the wrapped stream. The new content stream provider should be wrapping the existing provider, something like this:

ContentStreamProvider compressedStreamProvider = () -> compressor.compress(wrappedProvider.newStream());

gotchu, updated

dagnir · 2023-06-23T21:35:50Z

...ain/java/software/amazon/awssdk/core/internal/interceptor/RequestCompressionInterceptor.java

+        if (isTransferEncodingChunked(context)) {
+            InputStream compressedStream = compressor.compress(requestBody.contentStreamProvider().newStream());
+            try {
+                byte[] compressedBytes = IoUtils.toByteArray(compressedStream);
+                return Optional.of(RequestBody.fromBytes(compressedBytes));
+            } catch(IOException e){
+                throw SdkClientException.create(e.getMessage(), e);
+            }
+        }
+
+        CompressionContentStreamProvider streamProvider =
+            new CompressionContentStreamProvider(requestBody.contentStreamProvider(), compressor);
+        return Optional.of(RequestBody.fromContentProvider(streamProvider, requestBody.contentType()));


Why is there separate logic for chunked encoding? Does this interceptor need to care about chunked encoding at all?

The SEP states:
If isStreaming results in Transfer-Encoding: chunked, then the stream must be compressed before it is chunked

So if its chunked encoding, we'll compress the entire body. Otherwise, we'll compress in chunks

dagnir · 2023-06-23T21:36:52Z

...ain/java/software/amazon/awssdk/core/internal/interceptor/RequestCompressionInterceptor.java

+    }
+
+    @Override
+    public Optional<RequestBody> modifyHttpContent(Context.ModifyHttpRequest context, ExecutionAttributes executionAttributes) {


Why are we setting the content stream provider both here and modifyHttpRequest?

If its a non-streaming request, it won't be set in modifyHttpContent (will just return the empty Optional<RequestBody)

If its a streaming request, it won't be set in modifyHttpRequest (will just update the content encoding header and return the SdkHttpRequest)

Removing updates and moving the streaming compression to separate PR as discussed

sonarqubecloud · 2023-06-26T20:45:27Z

SonarCloud Quality Gate failed.

12 Bugs
0 Vulnerabilities
0 Security Hotspots
29 Code Smells

49.0% Coverage
0.7% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

…cce3d0c20 Pull request: release <- staging/13c825e9-633d-4d14-b4f3-339cce3d0c20

Request compression interceptor and non-streaming compression

f47eed1

davidh44 requested a review from a team as a code owner June 21, 2023 16:16

cenedhryn reviewed Jun 21, 2023

View reviewed changes

davidh44 added 2 commits June 21, 2023 11:31

Refactoring

d2a4b30

Refactoring

3c44e08

dagnir reviewed Jun 21, 2023

View reviewed changes

Refactoring and implement sync streaming compression

3629da4

davidh44 changed the title ~~Request compression interceptor and non-streaming compression~~ Request compression interceptor and non-streaming + sync streaming compression Jun 21, 2023

Refactoring

cf76e4d

dagnir reviewed Jun 22, 2023

View reviewed changes

Refactoring

dca42a8

dagnir reviewed Jun 23, 2023

View reviewed changes

Refactoring and remove sync compression updates

a12a6ef

davidh44 changed the title ~~Request compression interceptor and non-streaming + sync streaming compression~~ Request compression interceptor and non-streaming compression Jun 26, 2023

dagnir approved these changes Jun 26, 2023

View reviewed changes

davidh44 merged commit 6005427 into feature/master/request-compression Jun 26, 2023

davidh44 deleted the hdavidh/request-compression-interceptor branch June 26, 2023 19:09

aws-sdk-java-automation added a commit that referenced this pull request Jul 16, 2025

Merge pull request #4124 from aws/staging/13c825e9-633d-4d14-b4f3-339…

2ed214e

…cce3d0c20 Pull request: release <- staging/13c825e9-633d-4d14-b4f3-339cce3d0c20

Request compression interceptor and non-streaming compression #4124

Request compression interceptor and non-streaming compression #4124

Uh oh!

Conversation

davidh44 commented Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

Modifications

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidh44 Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jun 26, 2023

Uh oh!

Uh oh!

davidh44 commented Jun 21, 2023 •

edited

Loading

davidh44 Jun 26, 2023 •

edited

Loading