Move QueryParametersToBodyInterceptor to front of interceptor chain #4109

davidh44 · 2023-06-16T03:29:33Z

Motivation and Context

Move QueryParametersToBodyInterceptor to the front of interceptor chain so that it comes before other interceptors, e.g., checksum, compression. Some query protocols have custom interceptors, e.g., presigning, that need to come before QueryParametersToBodyInterceptor, so moving custom interceptors to front of interceptor chain for query protocols as well.

Modifications

Testing

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

…- for query protocols

dagnir

Can you make sure to run integ tests as well?

dagnir · 2023-06-19T19:14:16Z

...t/src/test/java/software/amazon/awssdk/services/protocolquery/MoveQueryParamsToBodyTest.java

Why is this test removed?

Now that QueryParametersToBodyInterceptor is moved to the front, a custom interceptor like this test interceptor AdditionalQueryParamInterceptor wouldn't work, since interceptors set in the client override config are added to the end of the chain after the global interceptors

If the test is broken, we should rewrite/fix it; we should continue to have a test to ensure this behavior stays in the client.

Discussed offline, we should be okay with this change since technically, you’re allowed to mix and match; you can have some params in the body and some in the URL

dagnir · 2023-06-19T19:19:51Z

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

-        if (model.getMetadata().isQueryProtocol()) {
-            TypeName listType = ParameterizedTypeName.get(List.class, ExecutionInterceptor.class);
-            builder.addStatement("$T protocolInterceptors = $T.singletonList(new $T())",
-                                 listType,
-                                 Collections.class,
-                                 QueryParametersToBodyInterceptor.class);
-            builder.addStatement("interceptors = $T.mergeLists(interceptors, protocolInterceptors)",
-                                 CollectionUtils.class);
-        }


Not sure I understand the need for the other changes. Any reason we can't just change the ordering of this statement?

In this finalizeServiceConfiguration() the service-specific interceptors are added to the SdkClientConfiguration as SdkClientOption.EXECUTION_INTERCEPTORS which are then merged with the global interceptors in SdkDefaultClientBuilder. So changing the ordering here would only move the interceptor to the front of the service-specific interceptors; it would still come after the global interceptors.

Alternatively, we could create another SdkClientOption here and merge it in SdkDefaultClientBuilder, but to keep cross-module dependencies down Matt proposed we add it in the SyncClientBuilderClass / AsyncClientBuilderClass

davidh44 · 2023-06-19T19:44:54Z

Can you make sure to run integ tests as well?

Integ tests ran successfully

dagnir · 2023-06-19T21:54:54Z

some query protocols have custom interceptors, e.g., presigning, that need to come before QueryParametersToBodyInterceptor,

What services are currently doing this? Seems like that wouldn't work if the endpoint provider returns a different endpoint.

dagnir · 2023-06-19T21:49:42Z

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

+        b.addStatement("$T interceptors = clientConfiguration.option($T.EXECUTION_INTERCEPTORS)", listType, SdkClientOption.class)
+            .addStatement("interceptors.add(0, new $T())", QueryParametersToBodyInterceptor.class);
+
+        List<String> customInterceptors = model.getCustomizationConfig().getInterceptors();
+        Collections.reverse(customInterceptors);
+        customInterceptors.forEach(i -> b.addStatement("interceptors.add(0, new $T())", ClassName.bestGuess(i)));


This is pretty confusing logic. Can we simplify this to remove the reverse and add(0?

Simplified to use CollectionUtils.mergeLists() for clarity

davidh44 · 2023-06-19T22:24:47Z

some query protocols have custom interceptors, e.g., presigning, that need to come before QueryParametersToBodyInterceptor,

What services are currently doing this? Seems like that wouldn't work if the endpoint provider returns a different endpoint.

Checked the (17) query protocols, these services have custom interceptors

DocDb / Neptune / RDS: Presigning interceptors
IAM: GlobalServiceExecutionInterceptor (wrapper for HelpfulUnknownHostExceptionInterceptor, stage is modifyException)
SQS: MessageMD5ChecksumInterceptor (stage is afterExecution)

We should be okay since the presigning interceptors depend on a source region parameter provided by the user, so it can come before the endpoint interceptors. The PresignRequestWireMockTest covers the scenario where the endpoint is overriden. Integ tests have also ran successfully after this change

dagnir · 2023-06-20T19:24:17Z

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

+
+        b.addStatement("$T interceptors = clientConfiguration.option($T.EXECUTION_INTERCEPTORS)",
+                       listType, SdkClientOption.class)
+            .addStatement("$T queryParamsToBodyInterceptor = new $T<>($T.asList(new $T()))",


nit: Collections.singletonList

oops, right that was how it was originally in BaseClientBuilderClass

sonarqubecloud · 2023-06-20T21:31:13Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

96.4% Coverage
7.6% Duplication

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

* Fixed issue with leased connection leaks when threads executing HTTP … (#4066) * Fixed issue with leased connection leaks when threads executing HTTP connections with Apache HttpClient were interrupted while the connection was in progress. * Added logic in MakeHttpRequestStage to check and abort request if interrupted * Add test cases for UrlConnectionHttpClient * Moved the fix to AfterTransmissionExecutionInterceptorsStage to just close the stream instaed of aborting the reqyest in MakeHttpRequestStage * Removing test cases related to UrlConnectionHttp since adding depenency in protocol-test for urlConnectionClient cause failues since it uses default Client all the places * Updated after Zoe's comments * Now it's possible to configure NettyNioAsyncHttpClient for non blocking DNS (#3990) * Now it's possible to configure NettyNioAsyncHttpClient in order to use a non blocking DNS resolver. * Add package mapping for netty-resolver-dns. --------- Co-authored-by: Matthew Miller <[email protected]> * Amazon Connect Service Update: This release adds search APIs for Prompts, Quick Connects and Hours of Operations, which can be used to search for those resources within a Connect Instance. * AWS Certificate Manager Private Certificate Authority Update: Document-only update to refresh CLI documentation for AWS Private CA. No change to the service. * Release 2.20.83. Updated CHANGELOG.md, README.md and all pom.xml. * Add "unsafe" AsyncRequestBody constructors for byte[] and ByteBuffers (#3925) * Update to next snapshot version: 2.20.84-SNAPSHOT * Use WeakHashMap in IdleConenctionReaper (#4087) * Use WeakHashMap in IdleConenctionReaper to not prevent connection manager from getting GC'd * Checkstyle fix * Update S3IntegrationTestBase.java (#4079) * Amazon Rekognition Update: This release adds support for improved accuracy with user vector in Amazon Rekognition Face Search. Adds new APIs: AssociateFaces, CreateUser, DeleteUser, DisassociateFaces, ListUsers, SearchUsers, SearchUsersByImage. Also adds new face metadata that can be stored: user vector. * Amazon DynamoDB Update: Documentation updates for DynamoDB * Amazon FSx Update: Amazon FSx for NetApp ONTAP now supports joining a storage virtual machine (SVM) to Active Directory after the SVM has been created. * Amazon SageMaker Service Update: Sagemaker Neo now supports compilation for inferentia2 (ML_INF2) and Trainium1 (ML_TRN1) as available targets. With these devices, you can run your workloads at highest performance with lowest cost. inferentia2 (ML_INF2) is available in CMH and Trainium1 (ML_TRN1) is available in IAD currently * AWS Amplify UI Builder Update: AWS Amplify UIBuilder is launching Codegen UI, a new feature that enables you to generate your amplify uibuilder components and forms. * Amazon OpenSearch Service Update: This release adds support for SkipUnavailable connection property for cross cluster search * Amazon DynamoDB Streams Update: Documentation updates for DynamoDB Streams * Updated endpoints.json and partitions.json. * Release 2.20.84. Updated CHANGELOG.md, README.md and all pom.xml. * Update to next snapshot version: 2.20.85-SNAPSHOT * docs: add scrocquesel as a contributor for code (#4091) * docs: update README.md [skip ci] * docs: update .all-contributorsrc [skip ci] --------- Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com> Co-authored-by: Debora N. Ito <[email protected]> * AWS CloudTrail Update: This feature allows users to view dashboards for CloudTrail Lake event data stores. * AWS WAFV2 Update: You can now detect and block fraudulent account creation attempts with the new AWS WAF Fraud Control account creation fraud prevention (ACFP) managed rule group AWSManagedRulesACFPRuleSet. * AWS Well-Architected Tool Update: AWS Well-Architected now supports Profiles that help customers prioritize which questions to focus on first by providing a list of prioritized questions that are better aligned with their business goals and outcomes. * Amazon Lightsail Update: This release adds pagination for the Get Certificates API operation. * Amazon Verified Permissions Update: GA release of Amazon Verified Permissions. * EC2 Image Builder Update: Change the Image Builder ImagePipeline dateNextRun field to more accurately describe the data. * Amazon CodeGuru Security Update: Initial release of Amazon CodeGuru Security APIs * Amazon Simple Storage Service Update: Integrate double encryption feature to SDKs. * Elastic Disaster Recovery Service Update: Added APIs to support network replication and recovery using AWS Elastic Disaster Recovery. * AWS SimSpace Weaver Update: This release fixes using aws-us-gov ARNs in API calls and adds documentation for snapshot APIs. * AWS SecurityHub Update: Add support for Security Hub Automation Rules * Amazon Elastic Compute Cloud Update: This release introduces a new feature, EC2 Instance Connect Endpoint, that enables you to connect to a resource over TCP, without requiring the resource to have a public IPv4 address. * Updated endpoints.json and partitions.json. * Release 2.20.85. Updated CHANGELOG.md, README.md and all pom.xml. * Update to next snapshot version: 2.20.86-SNAPSHOT * Create secondary indices based on table bean annotations (#3923) (#4004) * Create secondary indices based on table bean annotations (#3923) * detect and group indices present in table schema into LSIs and GSIs * pass request with indices information appended further * Remove specifying provisioned throughput for GSIs (#3923) * If there's no information about the billing mode of the new table, then it'll be using the PAY_PER_REQUEST one. It means that all GSIs related to this table will be doing the same and there's no need to hard code any provisioned throughput like it was done * Allow passing empty indices list to CreateTableOperation (#3923) * CreateTableRequest cannot handle empty list of indices of any type. It throws exception when given such a list. At the same time, it nicely handles the cases when indices lists are null. Make sure then that when empty indices list is passed CreateTableOperation, then in the CreateTableRequest it's just reflected as null. --------- Co-authored-by: Adrian Chlebosz <[email protected]> Co-authored-by: Olivier L Applin <[email protected]> * Add EnhancedType parameters to static builder methods of StaticTableSchema and StaticImmitableTableSchema (#4077) * Amazon Elastic File System Update: Documentation updates for EFS. * Amazon GuardDuty Update: Updated descriptions for some APIs. * Amazon Location Service Update: Amazon Location Service adds categories to places, including filtering on those categories in searches. Also, you can now add metadata properties to your geofences. * AWS Audit Manager Update: This release introduces 2 Audit Manager features: CSV exports and new manual evidence options. You can now export your evidence finder results in CSV format. In addition, you can now add manual evidence to a control by entering free-form text or uploading a file from your browser. * Updated endpoints.json and partitions.json. * Release 2.20.86. Updated CHANGELOG.md, README.md and all pom.xml. * Update to next snapshot version: 2.20.87-SNAPSHOT * EnumAttributeConverter: enums can be identified by toString() or name(). toString() is the default for backward compatibility (#3971) Co-authored-by: Zoe Wang <[email protected]> * AWS Application Discovery Service Update: Add Amazon EC2 instance recommendations export * AWS Account Update: Improve pagination support for ListRegions * Amazon Simple Storage Service Update: This release adds SDK support for request-payer request header and request-charged response header in the "GetBucketAccelerateConfiguration", "ListMultipartUploads", "ListObjects", "ListObjectsV2" and "ListObjectVersions" S3 APIs. * Amazon Connect Service Update: Updates the *InstanceStorageConfig APIs to support a new ResourceType: SCREEN_RECORDINGS to enable screen recording and specify the storage configurations for publishing the recordings. Also updates DescribeInstance and ListInstances APIs to include InstanceAccessUrl attribute in the API response. * AWS Identity and Access Management Update: Documentation updates for AWS Identity and Access Management (IAM). * Release 2.20.87. Updated CHANGELOG.md, README.md and all pom.xml. * Update to next snapshot version: 2.20.88-SNAPSHOT * Fix the StackOverflowException in WaiterExecutor in case of large retries count. (#3956) * Move checksum calculation from afterMarshalling to modifyHttpRequest (#4108) * Update HttpChecksumRequiredInterceptor * Update HttpChecksumInHeaderInterceptor * Update tests and remove constant * Add back constant to resolve japicmp * Add back javadocs * docs: add dave-fn as a contributor for code (#4092) * docs: update README.md [skip ci] * docs: update .all-contributorsrc [skip ci] * Removing unnecessary vscode file --------- Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com> Co-authored-by: Debora N. Ito <[email protected]> * Amazon Route 53 Domains Update: Update MaxItems upper bound to 1000 for ListPricesRequest * Amazon EC2 Container Service Update: Documentation only update to address various tickets. * AWS CloudFormation Update: Specify desired CloudFormation behavior in the event of ChangeSet execution failure using the CreateChangeSet OnStackFailure parameter * AWS Price List Service Update: This release updates the PriceListArn regex pattern. * AWS Glue Update: This release adds support for creating cross region table/database resource links * Amazon Elastic Compute Cloud Update: API changes to AWS Verified Access to include data from trust providers in logs * Amazon SageMaker Service Update: Amazon Sagemaker Autopilot releases CreateAutoMLJobV2 and DescribeAutoMLJobV2 for Autopilot customers with ImageClassification, TextClassification and Tabular problem type config support. * Release 2.20.88. Updated CHANGELOG.md, README.md and all pom.xml. * Update to next snapshot version: 2.20.89-SNAPSHOT * AWS Lambda Update: This release adds RecursiveInvocationException to the Invoke API and InvokeWithResponseStream API. * AWS Config Update: Updated ResourceType enum with new resource types onboarded by AWS Config in May 2023. * Amazon Appflow Update: This release adds new API to reset connector metadata cache * Amazon Elastic Compute Cloud Update: Adds support for targeting Dedicated Host allocations by assetIds in AWS Outposts * Amazon Redshift Update: Added support for custom domain names for Redshift Provisioned clusters. This feature enables customers to create a custom domain name and use ACM to generate fully secure connections to it. * Updated endpoints.json and partitions.json. * Release 2.20.89. Updated CHANGELOG.md, README.md and all pom.xml. * Update to next snapshot version: 2.20.90-SNAPSHOT * Move QueryParametersToBodyInterceptor to front of interceptor chain (#4109) * Move QueryParametersToBodyInterceptor to front of interceptor chain * Move customization.config interceptors to front of interceptor chain - for query protocols * Refactoring * Add codegen tests * Refactoring * Refactoring --------- Co-authored-by: John Viegas <[email protected]> Co-authored-by: Martin <[email protected]> Co-authored-by: Matthew Miller <[email protected]> Co-authored-by: AWS <> Co-authored-by: aws-sdk-java-automation <[email protected]> Co-authored-by: Stephen Flavin <[email protected]> Co-authored-by: Zoe Wang <[email protected]> Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com> Co-authored-by: Debora N. Ito <[email protected]> Co-authored-by: Adrian Chlebosz <[email protected]> Co-authored-by: Adrian Chlebosz <[email protected]> Co-authored-by: Olivier L Applin <[email protected]> Co-authored-by: Benjamin Maizels <[email protected]> Co-authored-by: flitt <[email protected]>

… chain (#4109)" This reverts commit c7c731f.

…4109) * Move QueryParametersToBodyInterceptor to front of interceptor chain * Move customization.config interceptors to front of interceptor chain - for query protocols * Refactoring * Add codegen tests * Refactoring * Refactoring

… chain (#4109)" This reverts commit c7c731f.

Move QueryParametersToBodyInterceptor to front of interceptor chain

57d7f59

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

davidh44 added 2 commits June 16, 2023 16:56

Move customization.config interceptors to front of interceptor chain …

f31c32e

…- for query protocols

Refactoring

5e5a458

dagnir reviewed Jun 19, 2023

View reviewed changes

Add codegen tests

67f47d5

dagnir reviewed Jun 19, 2023

View reviewed changes

Refactoring

8f7f99d

davidh44 changed the base branch from master to feature/master/request-compression June 19, 2023 23:14

davidh44 changed the base branch from feature/master/request-compression to master June 19, 2023 23:15

dagnir reviewed Jun 20, 2023

View reviewed changes

Refactoring

a37f419

dagnir approved these changes Jun 21, 2023

View reviewed changes

davidh44 merged commit c7c731f into master Jun 21, 2023

davidh44 deleted the hdavidh/move-queryParametersToBodyInterceptors-to-front-of-interceptor-chain branch June 21, 2023 16:28

davidh44 added a commit that referenced this pull request Jun 27, 2023

Revert "Move QueryParametersToBodyInterceptor to front of interceptor…

07f95f3

… chain (#4109)" This reverts commit c7c731f.

davidh44 mentioned this pull request Jun 27, 2023

Revert "Move QueryParametersToBodyInterceptor to front of interceptor… #4138

Closed

12 tasks

davidh44 restored the hdavidh/move-queryParametersToBodyInterceptors-to-front-of-interceptor-chain branch June 27, 2023 17:24

aws-sdk-java-automation pushed a commit that referenced this pull request Jun 27, 2023

Revert "Move QueryParametersToBodyInterceptor to front of interceptor…

f60c196

… chain (#4109)" This reverts commit c7c731f.

davidh44 deleted the hdavidh/move-queryParametersToBodyInterceptors-to-front-of-interceptor-chain branch July 5, 2023 06:17

L-Applin pushed a commit that referenced this pull request Jul 24, 2023

Revert "Move QueryParametersToBodyInterceptor to front of interceptor…

5ba7741

… chain (#4109)" This reverts commit c7c731f.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move QueryParametersToBodyInterceptor to front of interceptor chain #4109

Move QueryParametersToBodyInterceptor to front of interceptor chain #4109

davidh44 commented Jun 16, 2023 •

edited

Loading

dagnir left a comment

dagnir Jun 19, 2023

davidh44 Jun 19, 2023

dagnir Jun 19, 2023

davidh44 Jun 20, 2023

dagnir Jun 19, 2023

davidh44 Jun 19, 2023

davidh44 commented Jun 19, 2023 •

edited

Loading

dagnir commented Jun 19, 2023

dagnir Jun 19, 2023

davidh44 Jun 19, 2023

davidh44 commented Jun 19, 2023 •

edited

Loading

dagnir Jun 20, 2023

davidh44 Jun 20, 2023

sonarqubecloud bot commented Jun 20, 2023

Move QueryParametersToBodyInterceptor to front of interceptor chain #4109

Move QueryParametersToBodyInterceptor to front of interceptor chain #4109

Conversation

davidh44 commented Jun 16, 2023 • edited Loading

Motivation and Context

Modifications

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

dagnir left a 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

davidh44 commented Jun 19, 2023 • edited Loading

dagnir commented Jun 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidh44 commented Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jun 20, 2023

davidh44 commented Jun 16, 2023 •

edited

Loading

davidh44 commented Jun 19, 2023 •

edited

Loading

davidh44 commented Jun 19, 2023 •

edited

Loading