Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request level plugin execution #4518

Conversation

sugmanue
Copy link
Contributor

@sugmanue sugmanue commented Sep 30, 2023

Motivation and Context

Request level plugin execution take 2

This change implements the request level plugin execution using the following strategy:

  1. The request execution pipeline will only need to use a request scoped SdkClientConfiguration.
  2. The logic to invoke the plugins will be baked into the codegen client and the resulting request scoped SdkClientConfiguration will be pass down the pipeline.

Request Scoped SdkClientConfiguration

Two uses of the client configuration will be updated to read and use the request scoped client configuration:

  1. To create the HttpClientDependencies that encapsulates an instance of the configuration and is used to execute the request (currently done here for the async case)
  2. Prepare the execution attributes that are used by request interceptors (currently done here)

Details

ClientExecutionParams will get a new property with type SdkClientConfiguration that will be set in the generated clients for each request. For instance,

    @Override
    public AbortMultipartUploadResponse abortMultipartUpload(AbortMultipartUploadRequest abortMultipartUploadRequest)
            throws NoSuchUploadException, AwsServiceException, SdkClientException, S3Exception {

        HttpResponseHandler<Response<AbortMultipartUploadResponse>> responseHandler = protocolFactory
                .createCombinedResponseHandler(AbortMultipartUploadResponse::builder,
                        new XmlOperationMetadata().withHasStreamingSuccessResponse(false));
        // The code below is newly codegen, this will use another codegen field that 
        // is a function that "knows" how to crate a request scoped configuration
        // out of the request and the client scoped one.
        SdkClientConfiguration clientConfiguration = this.clientConfigurationForRequest.apply(abortMultipartUploadRequest,
                this.clientConfiguration);
        // End of new code

        List<MetricPublisher> metricPublishers = resolveMetricPublishers(clientConfiguration, abortMultipartUploadRequest
                .overrideConfiguration().orElse(null));
        MetricCollector apiCallMetricCollector = metricPublishers.isEmpty() ? NoOpMetricCollector.create() : MetricCollector
                .create("ApiCall");
        try {
            apiCallMetricCollector.reportMetric(CoreMetric.SERVICE_ID, "S3");
            apiCallMetricCollector.reportMetric(CoreMetric.OPERATION_NAME, "AbortMultipartUpload");

            return clientHandler.execute(new ClientExecutionParams<AbortMultipartUploadRequest, AbortMultipartUploadResponse>()
                    .withOperationName("AbortMultipartUpload").withCombinedResponseHandler(responseHandler)
                    .withMetricCollector(apiCallMetricCollector)
                    // The code below is newly codegen, the request scoped config is
                    // passed down for the request execution pipeline to use.
                    .withRequestConfiguration(clientConfiguration)
                    // End of new code

                    .withInput(abortMultipartUploadRequest)
                    .withMarshaller(new AbortMultipartUploadRequestMarshaller(protocolFactory)));
        } finally {
            metricPublishers.forEach(p -> p.publish(apiCallMetricCollector.collect()));
        }
    }

The sync and async client handlers were also updated to handle the request scoped client configuration, (currently done here for the sync case). For this case the client configuration is not exposed directly but instead lives in the parent class inside another class, HttpClientDependencies. The RequestExecutionBuilder, sync and async classes were changed to accept an instance of HttpClientDependencies instead of using the parent class field value.

    public interface RequestExecutionBuilder {
        // Other methods omitted 

        // New code
        RequestExecutionBuilder httpClientDependencies(HttpClientDependencies httpClientDependencies);

        HttpClientDependencies httpClientDependencies();

        default RequestExecutionBuilder httpClientDependencies(Consumer<HttpClientDependencies.Builder> mutator) {
            HttpClientDependencies.Builder builder = httpClientDependencies().toBuilder();
            mutator.accept(builder);
            return httpClientDependencies(builder.build());
        }

and the invoke methods in the clients handlers will be modfied to take the SdkClientConfiguration to be used inside the HttpClientDependencies instance. E.g.,

    private <InputT extends SdkRequest, OutputT> CompletableFuture<OutputT> invoke(
        // New Code:
        SdkClientConfiguration clientConfiguration,
        // End of new code
        SdkHttpFullRequest request,
        AsyncRequestBody requestProvider,
        InputT originalRequest,
        ExecutionContext executionContext,
        TransformingAsyncResponseHandler<Response<OutputT>> responseHandler) {
        return client.requestExecutionBuilder()
                     .requestProvider(requestProvider)
                     .request(request)
                     .originalRequest(originalRequest)
                     .executionContext(executionContext)
                     // New code:
                     .httpClientDependencies(c -> c.clientConfiguration(clientConfiguration))
                     // End of new code
                     .execute(responseHandler);

Plugin Execution

The plugins execution happens inside a codegen method with the following signature:

    protected SdkClientConfiguration updateSdkClientConfiguration(SdkRequest request, SdkClientConfiguration clientConfiguration)

This method is responsible for converting a given request and an client configuration and return back a request scoped client configuration. Internally does so by:

  1. Creating an internal ConfigurationUpdater
  2. Grabbing the configured plugins from the overrideConfiguration
  3. Running the exiting method to run the plugin using those values.

For instance for S3:

    protected SdkClientConfiguration updateSdkClientConfiguration(SdkRequest request, SdkClientConfiguration clientConfiguration) {
        ConfigurationUpdater<SdkServiceClientConfiguration.Builder> configurationUpdater = (consumer, configBuilder) -> {
            S3ServiceClientConfigurationBuilder.BuilderInternal serviceConfigBuilder = S3ServiceClientConfigurationBuilder
                    .builder(configBuilder);
            consumer.accept(serviceConfigBuilder);
            return serviceConfigBuilder.buildSdkClientConfiguration();
        };
        List<SdkPlugin> plugins = request.overrideConfiguration().map(c -> c.plugins()).orElse(Collections.emptyList());
        return SdkClientConfigurationUtil.invokePlugins(clientConfiguration, plugins, configurationUpdater);
    }

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

@sugmanue sugmanue force-pushed the sugmanue/sra-plugins-add-plugins-execution-request branch from f4fb3e6 to 81d1082 Compare September 30, 2023 22:57
@sugmanue sugmanue force-pushed the sugmanue/sra-plugins-add-plugins-execution-request branch from 81d1082 to b4d643a Compare October 2, 2023 15:29
@sugmanue sugmanue force-pushed the sugmanue/sra-plugins-add-plugins-execution-request branch 2 times, most recently from a5d2fc9 to d2b7722 Compare October 3, 2023 17:09
@sugmanue sugmanue force-pushed the sugmanue/sra-plugins-add-plugins-execution-request branch from d2b7722 to bdf497b Compare October 3, 2023 17:11
@sugmanue sugmanue marked this pull request as ready for review October 3, 2023 17:35
@sugmanue sugmanue requested a review from a team as a code owner October 3, 2023 17:35
@@ -32,7 +32,8 @@
import software.amazon.awssdk.metrics.MetricPublisher;
import software.amazon.awssdk.profiles.ProfileFileSupplier;

@SdkInternalApi
// TODO(sra-plugins) Move this to codegen such that we can actually convert it to @SdkInternalApi
@SdkProtectedApi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an upcoming PR/tracked somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this one SMITHY-2106 to track it, but FWIW, I'm actively working on this change on top of this PR.

@sugmanue sugmanue force-pushed the sugmanue/sra-plugins-add-plugins-execution-request branch from 5ca9343 to e14811c Compare October 4, 2023 05:25
@sugmanue sugmanue force-pushed the sugmanue/sra-plugins-add-plugins-execution-request branch from e14811c to 1ee010c Compare October 4, 2023 05:27
@sonarcloud
Copy link

sonarcloud bot commented Oct 4, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 9 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 2 Security Hotspots
Code Smell A 482 Code Smells

85.5% 85.5% Coverage
4.2% 4.2% Duplication

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

@sugmanue sugmanue merged commit 9a0d9b8 into feature/master/sra-identity-auth Oct 4, 2023
15 of 16 checks passed
@sugmanue sugmanue deleted the sugmanue/sra-plugins-add-plugins-execution-request branch October 4, 2023 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants