adds an interceptor to replace params-to-body stage and applies it to… #1566

cenedhryn · 2019-12-31T18:07:11Z

… every query protocol service through codegen

Description

In the request pipeline, there is a Stage class MoveParametersToBodyStage.java that analyses every HTTP request passing through and based on certain conditions, moves them to the body instead.
This functionality (move params to body) is specific for the Query protocol
This change moves the code to an interceptor attached to the modifyHTTPRequest interceptor hook. The interceptor is added to the interceptor chain for every service identified as a Query protocol service, through codegen.

Motivation and Context

The change is directly motivated by a bug fix, PR 1187, which caused services not using this protocol to pass the conditions and erroneously move its params to the body.

It's also a change in the right direction from an architectural point of view, moving this too-unspecific check and protocol specific functionality from the general codepath shared by all services to the protocol layer.

Solution alternatives

Moving this HTTP request back in the request pipeline to the client handler where the HTTP request is created/marshalled, there were two possible solutions considered.

Approach 1: Adapt marshalling

This is the preferred approach since the protocol marshaller should do protocol specific work and it was the first approach. However, the protocol marshaller is not aware of any content streaming provider, which makes the content streaming code execution path not work; the protocol marshaller would move parameters to the body even though the wrapping streaming marshaller has a content stream provider. It may be possible to explore further avenues of the codegen making the protocol marshaller aware of the content streaming provider.

Approach 2: Create a protocol specific interceptor

This PR presents approach 2, where the HTTP request modification is done through an interceptor and the HTTP request modification hook, applied directly after marshalling. This interceptor is added to any client builder identified as needing it (Query protocol).

The condition to determine that relationship may have to be modified, since some protocols are inter-related.

Testing

mvn clean install
ran integration tests for iot, rds, ec2, protocol-tests

Types of changes

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

Checklist

License

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

bmaizels · 2020-01-02T17:56:56Z

.changes/next-release/bugfix-AWSSDKforJavav2-05865e7.json

+{
+    "type": "bugfix",
+    "category": "AWS SDK for Java v2",
+    "description": "Removes query protocol specific stage from request pipeline and performs functionality when creating HTTP request"


Can we make this a more clear/concise description of the 'bug', so that when people read this in the future and aren't able to cross-reference your PR notes they are not missing too much context. Maybe focus on what behavior it fixes.

bmaizels · 2020-01-02T17:58:16Z

codegen/src/main/java/software/amazon/awssdk/codegen/model/intermediate/Metadata.java

@@ -519,6 +519,10 @@ public boolean isXmlProtocol() {
               protocol == Protocol.REST_XML;
    }

+    public boolean isQueryProtocol() {
+        return protocol == Protocol.EC2 || protocol == Protocol.QUERY;


Minor: Can we follow the style of the other methods in this class, keep each check on a separate line.

bmaizels · 2020-01-02T18:00:03Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/builder/BaseClientBuilderClass.java

@@ -177,6 +180,16 @@ private MethodSpec finalizeServiceConfigurationMethod() {
                         .addCode("interceptors = $T.mergeLists(interceptors, config.option($T.EXECUTION_INTERCEPTORS));\n",
                                  CollectionUtils.class, SdkClientOption.class);

+        if (model.getMetadata().isQueryProtocol()) {
+            builder.addStatement("$T<$T> protocolInterceptors = $T.singletonList(new $T())",


Can we use poet's ParamaterizedTypeName here instead of parameterizing by hand.

Like this?

TypeName listType = ParameterizedTypeName.get(List.class, ExecutionInterceptor.class); builder.addStatement("$T protocolInterceptors = $T.singletonList(new $T())", listType, Collections.class, QueryParametersToBodyInterceptor.class);

Yes, exactly

codegen/src/main/java/software/amazon/awssdk/codegen/poet/builder/BaseClientBuilderClass.java

bmaizels · 2020-01-02T18:07:46Z

...ava/software/amazon/awssdk/protocols/query/interceptor/QueryParametersToBodyInterceptor.java

+ * - It is a POST request
+ * - There is no content stream provider
+ * - There are query parameters to transfer
+ *


...software/amazon/awssdk/protocols/query/interceptor/QueryParametersToBodyInterceptorTest.java

bmaizels · 2020-01-02T18:25:29Z

...software/amazon/awssdk/protocols/query/interceptor/QueryParametersToBodyInterceptorTest.java

-                                      .executionContext(executionContext)
-                                      .originalRequest(NoopTestRequest.builder().build())
-                                      .build();
+    public final class Context implements software.amazon.awssdk.core.interceptor.Context.ModifyHttpRequest {


If we do have to do this maybe a slightly less generic name that gives the reader a clue as to why we had to do this (eg: ContextWithNoRequest or whatever)

bmaizels · 2020-01-02T18:26:35Z

...col-tests/src/test/java/software/amazon/awssdk/protocol/tests/QueryRequestTransformTest.java

+    public WireMockRule wireMock = new WireMockRule(0);
+
+    private ProtocolQueryClient client;
+


Nit: extraneous linebreak

bmaizels · 2020-01-02T18:27:45Z

...col-tests/src/test/java/software/amazon/awssdk/protocol/tests/QueryRequestTransformTest.java

+    }
+
+    private void verifyResponseMetadata() {
+


Nit: Extraneous linebreak

bmaizels · 2020-01-02T18:27:51Z

...col-tests/src/test/java/software/amazon/awssdk/protocol/tests/QueryRequestTransformTest.java

+                       .withRequestBody(containing("Version="))
+                       .withRequestBody(containing("IdempotencyToken=test")));
+    }
+


Nit: Extraneous linebreak

… every query protocol service through codegen

codecov-io · 2020-01-09T22:22:21Z

Codecov Report

Merging #1566 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #1566      +/-   ##
============================================
+ Coverage     75.53%   75.54%   +0.01%     
  Complexity      638      638              
============================================
  Files           896      896              
  Lines         28073    28086      +13     
  Branches       2217     2217              
============================================
+ Hits          21205    21218      +13     
  Misses         5848     5848              
  Partials       1020     1020

Flag	Coverage Δ	Complexity Δ
#unittests	`75.54% <100%> (+0.01%)`	`638 <0> (ø)`	⬇️

Impacted Files	Coverage Δ	Complexity Δ
...n/awssdk/codegen/lite/regions/RegionGenerator.java	`100% <100%> (ø)`	`0 <0> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 314ef90...a5bca3d. Read the comment docs.

sonarqubecloud · 2020-01-09T23:41:54Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
No Duplication information

…1e1408a02 Pull request: release <- staging/84c53abd-3119-4856-9c90-c161e1408a02

bmaizels reviewed Jan 2, 2020

View reviewed changes

cenedhryn force-pushed the salande/unsupported-payloads-interceptor branch from 4011c62 to e2f1de5 Compare January 9, 2020 21:40

adds an interceptor to replace params-to-body stage and applies it to…

8befe5f

… every query protocol service through codegen

cenedhryn force-pushed the salande/unsupported-payloads-interceptor branch from e2f1de5 to 8befe5f Compare January 9, 2020 22:07

bmaizels approved these changes Jan 9, 2020

View reviewed changes

cenedhryn added 2 commits January 9, 2020 14:49

Merge branch 'master' into salande/unsupported-payloads-interceptor

314ef90

Merge branch 'master' into salande/unsupported-payloads-interceptor

a5bca3d

cenedhryn merged commit 308c95a into master Jan 9, 2020

cenedhryn deleted the salande/unsupported-payloads-interceptor branch March 18, 2020 17:04

aws-sdk-java-automation added a commit that referenced this pull request Jul 26, 2021

Merge pull request #1566 from aws/staging/84c53abd-3119-4856-9c90-c16…

6fb90c2

…1e1408a02 Pull request: release <- staging/84c53abd-3119-4856-9c90-c161e1408a02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds an interceptor to replace params-to-body stage and applies it to… #1566

adds an interceptor to replace params-to-body stage and applies it to… #1566

cenedhryn commented Dec 31, 2019

bmaizels Jan 2, 2020

cenedhryn Jan 2, 2020

bmaizels Jan 2, 2020

cenedhryn Jan 2, 2020

bmaizels Jan 2, 2020

cenedhryn Jan 2, 2020

bmaizels Jan 2, 2020

bmaizels Jan 2, 2020

cenedhryn Jan 2, 2020

bmaizels Jan 2, 2020

bmaizels Jan 2, 2020

cenedhryn Jan 2, 2020

bmaizels Jan 2, 2020

cenedhryn Jan 2, 2020

bmaizels Jan 2, 2020

cenedhryn Jan 2, 2020

codecov-io commented Jan 9, 2020 •

edited

Loading

sonarqubecloud bot commented Jan 9, 2020

		public WireMockRule wireMock = new WireMockRule(0);

		private ProtocolQueryClient client;

adds an interceptor to replace params-to-body stage and applies it to… #1566

adds an interceptor to replace params-to-body stage and applies it to… #1566

Conversation

cenedhryn commented Dec 31, 2019

Description

Motivation and Context

Solution alternatives

Approach 1: Adapt marshalling

Approach 2: Create a protocol specific interceptor

Testing

Types of changes

Checklist

License

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 9, 2020 • edited Loading

Codecov Report

sonarqubecloud bot commented Jan 9, 2020

codecov-io commented Jan 9, 2020 •

edited

Loading