Async interceptor payload + S3 Checksums #823

spfink · 2018-11-14T18:52:43Z

Exposes the ability to modify the AsyncRequestBody and AsyncResponseTransformer.

Currently this is done by adding two new interceptors.

For AsyncRequestBody:

default AsyncRequestBody modifyAsyncRequestBody(Context.ModifyRequest context,
                                                ExecutionAttributes executionAttributes) {
    return context.requestBody();
}

For AsyncResponseTransformer:

default AsyncResponseTransformer modifyAsyncResponseTransformer(Context.ModifyRequest context,
                                                                ExecutionAttributes executionAttributes) {
    return context.responseTransformer();
}

Possible other options include making modifyRequest return back a container object instead of an SdkRequest:

default SdkRequestContext modifyRequest(Context.ModifyRequest context, ExecutionAttributes executionAttributes) {
    return context.request();
}

Where SdkRequestContext contains SdkRequest, AsyncRequestBody and AsyncResponseTransformer.

Or we could introduce an AsyncExecutionInterceptor interface that again either has two methods as in the PR or one method with a container object for async.

In general, trying to avoid potential confusion of when these interceptors are valid and when the values will be populated. If we use the approach in the PR, users of the Sync client should not use the new interceptors. If we go with the container approach for modifyRequest, then the asyncRequestBody and asyncResponseTransformer will be null.

millems · 2018-11-14T19:00:03Z

The sync payload is currently on the HTTP request, and interceptors modify the HTTP request payload by wrapping the contentStreamProvider in modifyHttpRequest.

Why can't we do the same for async?

shorea · 2018-11-14T19:26:05Z

I like the idea of having separate callbacks. I would change modifyHttpRequest to just return the headers and modify the content in a separate callback.

SdkHttpRequest modifyHttpRequest(Context.ModifyHttpRequest context, ExecutionAttributes executionAttributes);

InputStream modifyHttpContent(Context.ModifyHttpContent context, ExecutionAttributes executionAttributes);

AsyncRequestBody modifyAsyncHttpContent(Context.ModifyHttpContent context, ExecutionAttributes executionAttributes);

It would be better if we didn't add the thing being modified into the context until after the modify method so you would have something like this. (this would also allow composite interceptors since the thing you want to modify isn't in an immutable object).

SdkHttpRequest modifyHttpRequest(SdkHttpRequest toModify, Context.ModifyHttpRequest context, ExecutionAttributes executionAttributes);

InputStream modifyHttpContent(InputStream toModify, Context.ModifyHttpContent context, ExecutionAttributes executionAttributes);

AsyncRequestBody modifyAsyncHttpContent(AsyncRequestBody toModify, Context.ModifyHttpContent context, ExecutionAttributes executionAttributes);

and then whatever stage comes after modifying http content could have an Either<InputStream, AsyncRequestBody> for further access to the content.

spfink · 2018-11-14T19:28:19Z

@millems, we certainly could wrap the SdkHttpContentPublisher and the TransformingAsyncResponseHandler. We would still need to either use a container object on modifyHttpRequest or expose separate methods in the interceptors.

@shorea, I do like the idea of being consistent and splitting out the sync content into a separate method as well.

zoewangg · 2018-11-14T20:05:00Z

@shorea +1 on your first option. Like the idea of splitting sync and async modifyHttpContents methods.

Would it make sense to further split the Context?
Context.ModifyHttpContent and Context.ModifyAsyncHttpContent

For the second option, I feel it could be a bit confusing because the it can get the inputStream from both toModify and context.httpContent(), even though context.httpContent() would be null at that time, but the person who implements this method might not know.

InputStream modifyHttpContent(InputStream toModify, Context.ModifyHttpContent context, ExecutionAttributes executionAttributes);

shorea · 2018-11-14T20:18:19Z

So I was proposing not to have the content in the context at all until after that modifyHttpContent stage, so it's only in one location (the parameter in modifyHttpContent and the next context object in future callbacks).

millems · 2018-11-14T20:52:21Z

As long as sync and async are consistent, I don't mind too much.

If we're going to have sync payloads on the HTTP request, we should have async payloads there as well.
If we use AsyncRequestBody in the interceptor for async, we should use RequestBody for async.

spfink · 2018-11-15T03:54:56Z

Updated PR to include separate content interceptors.

@millems, not sure I follow your latest comment. Are you saying we should put the response transformer and request body on the SdkHttpFullRequest/Response objects?

millems · 2018-11-16T01:01:22Z

...k-core/src/main/java/software/amazon/awssdk/core/client/builder/SdkDefaultClientBuilder.java

@@ -204,6 +205,7 @@ protected SdkClientConfiguration finalizeChildConfiguration(SdkClientConfigurati
    private SdkClientConfiguration finalizeSyncConfiguration(SdkClientConfiguration config) {
        return config.toBuilder()
                     .option(SdkClientOption.SYNC_HTTP_CLIENT, resolveSyncHttpClient(config))
+                     .option(SdkClientOption.CLIENT_TYPE, "Sync")


core/sdk-core/src/main/java/software/amazon/awssdk/core/interceptor/Context.java

millems · 2018-11-16T01:04:39Z

core/sdk-core/src/main/java/software/amazon/awssdk/core/interceptor/Context.java

@@ -86,6 +106,9 @@ private Context() {}
    @ThreadSafe
    @SdkPublicApi
    public interface BeforeTransmission extends ModifyHttpRequest {
+        RequestBody requestBody();
+
+        AsyncRequestBody asyncRequestBody();


Shouldn't this be in AfterMarshalling?

Not if we have them as separate arguments in interceptors.

millems · 2018-11-16T01:05:32Z

core/sdk-core/src/main/java/software/amazon/awssdk/core/interceptor/Context.java

@@ -105,15 +144,16 @@ private Context() {}
     */
    @ThreadSafe
    @SdkPublicApi
-    public interface ModifyHttpResponse extends AfterTransmission {
+    public interface ModifyHttpResponseContent extends ModifyAsyncHttpResponseContent {
+        Publisher responsePublisher();
    }


No raw generics, please.
Shouldn't this be in AfterTransmision?
Where is the sync payload?

millems · 2018-11-16T01:06:32Z

core/sdk-core/src/main/java/software/amazon/awssdk/core/interceptor/ExecutionInterceptor.java


+// Disable CS to avoid "Unused Import" error. If we use the FQCN in the Javadoc, we'll run into line length issues instead.
+// CHECKSTYLE:OFF
+// CHECKSTYLE:ON


Remove comments?

IntelliJ moved these from where they should be around an import.

millems · 2018-11-16T01:08:01Z

core/sdk-core/src/main/java/software/amazon/awssdk/core/interceptor/ExecutionInterceptor.java

+     */
+    default Publisher<ByteBuffer> modifyAsyncHttpResponseContent(Publisher<ByteBuffer> toModify,
+                                                                 Context.ModifyAsyncHttpResponseContent context,
+                                                                 ExecutionAttributes executionAttributes) {


The values to be modified aren't usually passed separately. Any reason we're changing that just for these?

millems · 2018-11-16T01:11:01Z

...in/java/software/amazon/awssdk/services/s3/internal/handlers/GetBucketPolicyInterceptor.java

-                                   .map(c -> invokeSafely(() -> IoUtils.toUtf8String(c)))
-                                   .orElse(null);
+
+            String policy = requestBody.contentStreamProvider() == null ? null : invokeSafely(() ->IoUtils.toUtf8String(requestBody.contentStreamProvider().newStream()));


run checkstyle

millems · 2018-11-16T01:11:41Z

http-client-spi/src/main/java/software/amazon/awssdk/http/ExecuteRequest.java

-        Builder request(SdkHttpFullRequest request);
+        Builder request(SdkHttpRequest request);
+
+        Builder contentStreamProvider(Optional<ContentStreamProvider> contentStreamProvider);


Don't accept an optional parameter.

millems · 2018-11-16T01:12:11Z

http-client-spi/src/main/java/software/amazon/awssdk/http/InvokeableHttpRequest.java

@@ -26,5 +26,5 @@
 @SdkPublicApi
 public interface InvokeableHttpRequest extends Callable<SdkHttpFullResponse>, Abortable {
    @Override
-    SdkHttpFullResponse call() throws IOException;
+    SdkHttpFullResponse call() throws IOException; // ExecuteResponse - sdkResponse + inputStream


millems · 2018-11-16T02:17:41Z

@zoewangg @shorea The problem with removing http request from the AfterMarshalling context and http response from the AfterTransmission context, is that it makes afterMarshalling and afterTransmission much less useful. Those methods exist to see those values, so removing them means there's not really any reason to use them (or any way to see the values before they're modified by other interceptors).

If we moved them to the first parameter of afterMarshalling (eg.) it would fix the problem, but three parameters for every method sounds like a lot...

millems

Can you also update the execution interceptor protocol test to include the new methods?

core/sdk-core/src/main/java/software/amazon/awssdk/core/ClientType.java

millems · 2018-11-16T04:04:19Z

core/sdk-core/src/main/java/software/amazon/awssdk/core/checksums/MD5Checksum.java

+ * Implementation of {@link SdkChecksum} to calculate an MD5 checksum.
+ */
+@SdkInternalApi
+public class MD5Checksum implements SdkChecksum {


Md5Checksum*

millems · 2018-11-16T04:08:24Z

...dk-core/src/main/java/software/amazon/awssdk/core/client/handler/BaseAsyncClientHandler.java

@@ -193,7 +198,18 @@ public void onError(Throwable error) {

        @Override
        public void onStream(Publisher<ByteBuffer> publisher) {
-            delegate.onStream(publisher);
+            Publisher newPublisher = context.interceptorChain().modifyAsyncHttpResponse(context.interceptorContext()


Parameterize. Can you turn on intellij warnings for this?

...sdk-core/src/main/java/software/amazon/awssdk/core/client/handler/BaseSyncClientHandler.java

millems · 2018-11-16T04:12:59Z

core/sdk-core/src/main/java/software/amazon/awssdk/core/interceptor/Context.java

+
+        RequestBody requestBody();
+
+        AsyncRequestBody asyncRequestBody();


Javadoc. Should these be marked optional?

millems · 2018-11-16T04:21:38Z

...src/main/java/software/amazon/awssdk/http/apache/internal/impl/ApacheHttpRequestFactory.java

@@ -122,9 +122,9 @@ private HttpRequestBase wrapEntity(SdkHttpFullRequest request,
         * preparation for the retry. Eventually, these wrappers would
         * return incorrect validation result.
         */
-        if (request.contentStreamProvider().isPresent()) {
+        if (request.contentStreamProvider() != null && request.contentStreamProvider().isPresent()) {


Any method that returns optional should never return null. Sounds like there's a bug somewhere upstream.

Ping - why is contentStreamProvider null?

...src/main/java/software/amazon/awssdk/services/apigateway/internal/AcceptJsonInterceptor.java

millems · 2018-11-16T04:22:44Z

.../src/main/java/software/amazon/awssdk/services/s3/checksums/ChecksumValidatingPublisher.java

+    private final SdkChecksum sdkChecksum;
+    private final int contentLength;
+
+    public ChecksumValidatingPublisher(Publisher publisher,


Parameterize.

millems · 2018-11-16T04:22:53Z

.../src/main/java/software/amazon/awssdk/services/s3/checksums/ChecksumValidatingPublisher.java

+@SdkInternalApi
+public class ChecksumValidatingPublisher implements SdkPublisher<ByteBuffer> {
+
+    private final Publisher publisher;


Parameterize.

millems · 2018-11-16T04:23:11Z

core/sdk-core/src/main/java/software/amazon/awssdk/core/interceptor/InterceptorContext.java

+            return this;
+        }
+
+        public Builder responsePublisher(Publisher responsePublisher) {


Parameterize.

core/sdk-core/src/main/java/software/amazon/awssdk/core/ClientType.java

...sdk-core/src/main/java/software/amazon/awssdk/core/client/handler/ClientExecutionParams.java

millems · 2018-11-16T19:28:59Z

http-client-spi/src/main/java/software/amazon/awssdk/http/InvokeableHttpRequest.java

@@ -26,5 +26,5 @@
 @SdkPublicApi
 public interface InvokeableHttpRequest extends Callable<SdkHttpFullResponse>, Abortable {
    @Override
-    SdkHttpFullResponse call() throws IOException;
+    SdkHttpFullResponse call() throws IOException; // ExecuteResponse - sdkResponse + inputStream


http-client-spi/src/main/java/software/amazon/awssdk/http/SdkHttpFullResponse.java

millems · 2018-11-16T19:36:19Z

...src/main/java/software/amazon/awssdk/http/apache/internal/impl/ApacheHttpRequestFactory.java

@@ -122,9 +122,9 @@ private HttpRequestBase wrapEntity(SdkHttpFullRequest request,
         * preparation for the retry. Eventually, these wrappers would
         * return incorrect validation result.
         */
-        if (request.contentStreamProvider().isPresent()) {
+        if (request.contentStreamProvider() != null && request.contentStreamProvider().isPresent()) {


Ping - why is contentStreamProvider null?

millems · 2018-11-16T19:36:40Z

...t/java/software/amazon/awssdk/http/nio/netty/NettyNioAsyncHttpClientSpiVerificationTest.java

-                                         b.putHeader("Content-Length", contentLength);
-                                     }
-                                 }).build();
+        return (SdkHttpFullRequest) SdkHttpFullRequest.builder()


Why is this (and other similar casts) needed?

millems · 2018-11-16T19:37:42Z

services/s3/src/it/java/software/amazon/awssdk/services/s3/GetObjectIntegrationTest.java

@@ -25,6 +25,7 @@
 import java.io.FileInputStream;
 import java.io.IOException;
 import java.nio.file.Path;
+import org.apache.log4j.BasicConfigurator;


Unused import

millems · 2018-11-16T19:55:45Z

...ain/java/software/amazon/awssdk/protocols/json/internal/marshall/JsonProtocolMarshaller.java

@@ -136,7 +136,7 @@ private static JsonMarshallerRegistry createMarshallerRegistry() {
    }

    private SdkHttpFullRequest.Builder fillBasicRequestParams(OperationInfo operationInfo) {
-        return ProtocolUtils.createSdkHttpRequest(operationInfo, endpoint)
+        return (SdkHttpFullRequest.Builder) ProtocolUtils.createSdkHttpRequest(operationInfo, endpoint)


Needed cast?

millems · 2018-11-16T19:57:52Z

http-client-spi/src/main/java/software/amazon/awssdk/http/InvokeableHttpRequest.java

@@ -26,5 +26,5 @@
 @SdkPublicApi
 public interface InvokeableHttpRequest extends Callable<SdkHttpFullResponse>, Abortable {
    @Override
-    SdkHttpFullResponse call() throws IOException;
+    SdkHttpFullResponse call() throws IOException; // TODO: Change to container instad of SdkHttpFullResponse


I meant, can we actually do this?

millems · 2018-11-16T19:58:26Z

http-client-spi/src/test/java/software/amazon/awssdk/http/SdkHttpRequestResponseTest.java

@@ -416,7 +416,7 @@ private SdkHttpFullRequest validRequest() {
    }

    private SdkHttpFullRequest.Builder validRequestBuilder() {
-        return SdkHttpFullRequest.builder().protocol("http").host("localhost").method(SdkHttpMethod.GET);
+        return (SdkHttpFullRequest.Builder) SdkHttpFullRequest.builder().protocol("http").host("localhost").method(SdkHttpMethod.GET);


Cast still needed?

millems · 2018-11-16T19:58:47Z

...src/test/java/software/amazon/awssdk/http/nio/netty/NettyNioAsyncHttpClientWireMockTest.java

-                                         b.putHeader("Content-Length", contentLength);
-                                     }
-                                 }).build();
+        return (SdkHttpFullRequest) SdkHttpFullRequest.builder()


Cast still needed?

millems · 2018-11-16T21:17:43Z

...dk-core/src/main/java/software/amazon/awssdk/core/interceptor/ExecutionInterceptorChain.java

+            result = result.copy(b -> b.httpRequest(interceptorResult)
+                                       .asyncRequestBody(asyncRequestBody)
+                                       .requestBody(requestBody)
+                                       .build());


build isn't needed

millems · 2018-11-16T21:19:29Z

http-client-spi/src/main/java/software/amazon/awssdk/http/ExecuteResponse.java

+import software.amazon.awssdk.annotations.SdkPublicApi;
+
+@SdkPublicApi
+public class ExecuteResponse {


We should probably follow up with Andrew and see if we want to have a different name than ExecuteRequest and ExecuteResponse later.

…lidation

…04e9d0eb Pull request: release <- staging/142eddbf-4255-4523-bc71-564f04e9d0eb

millems reviewed Nov 16, 2018

View reviewed changes

millems approved these changes Nov 16, 2018

View reviewed changes

spfink force-pushed the finks/async-interceptor-payload branch from e2949ea to ad39430 Compare November 16, 2018 22:19

Refactoring interceptors for async payloads and adding S3 checksum va…

774b98b

…lidation

spfink force-pushed the finks/async-interceptor-payload branch from ad39430 to 774b98b Compare November 17, 2018 01:11

Merge branch 'master' into finks/async-interceptor-payload

9f8ad93

spfink merged commit 236147d into master Nov 17, 2018

shorea deleted the finks/async-interceptor-payload branch November 23, 2018 03:54

zoewangg mentioned this pull request Nov 27, 2018

S3: Payload Checksum Validation #18

Closed

aws-sdk-java-automation added a commit that referenced this pull request Apr 30, 2020

Merge pull request #823 from aws/staging/142eddbf-4255-4523-bc71-564f…

c7fe043

…04e9d0eb Pull request: release <- staging/142eddbf-4255-4523-bc71-564f04e9d0eb


		RequestBody requestBody();

		AsyncRequestBody asyncRequestBody();

Async interceptor payload + S3 Checksums #823

Async interceptor payload + S3 Checksums #823

Uh oh!

Conversation

spfink commented Nov 14, 2018

Uh oh!

millems commented Nov 14, 2018

Uh oh!

shorea commented Nov 14, 2018

Uh oh!

spfink commented Nov 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zoewangg commented Nov 14, 2018

Uh oh!

shorea commented Nov 14, 2018

Uh oh!

millems commented Nov 14, 2018

Uh oh!

spfink commented Nov 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

millems commented Nov 16, 2018

Uh oh!

millems left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

spfink commented Nov 14, 2018 •

edited

Loading