aws / aws-sdk-java-v2

The official AWS SDK for Java - Version 2
Apache License 2.0
2.21k stars 853 forks source link

SdkPresigner.Builder has a set of methods common with AwsClientBuilder but no common hierarchy #2448

Open L00kian opened 3 years ago

L00kian commented 3 years ago

Describe the Feature

At the moment SdkPresigner.Builder has 3 builder methods which are

Builder region(Region region);
Builder credentialsProvider(AwsCredentialsProvider credentialsProvider);
Builder endpointOverride(URI endpointOverride);

The same 3 methods exist within SdkClientBuilder (endpointOverride is inherited from SdkClientBuilder), however these methods belong to different hierarchies.

Is your Feature Request related to a problem?

For me as a developer there are different kinds of deployments

  1. Start application locally pointing to localstack (static region, anonymous credentials, endpoint overridden to localhost:port
  2. Start application locally pointing to stack in AWS cloud (static region, static credentials, no endpoint override)
  3. Start application in AWS (automatic region, automatic credentials from default chain, no endpoint override)

So for having a Presigner and a Client I'd have to have twice as much code doing just the same (providing configuration to AWS SDK constructs)

Proposed Solution

introduce 3 interfaces

public interface EndpointOverrideAware<B extends EndpointOverrideAware<T>, T> {
  B endpointOverride(URI endpointOverride);
}

public interface CredentialsProviderAware<B extends CredentialsProviderAware <T>, T> {
  B credentialsProvider(AwsCredentialsProvider credentialsProvider);
}

public interface RegionAware<B extends RegionAware <T>, T> {
  B region(Region region);
}

and inherit both SdkPresigner.Builder and SdkClientBuilder from these interfaces.

debora-ito commented 3 years ago

@L00kian I'm not fully clear on the advantage of the proposed solution versus the configuration format the SDK provides. Can you illustrate, maybe with some code examples?

L00kian commented 3 years ago

Hi @debora-ito, sure. At the moment to support the scenarios listed above in spring-boot applications I use a customiser created in different profiles looking like that.

    @Builder
    public static class ClientBuilderCustomizer {

        private final AwsCredentialsProvider credentialsProvider;
        private final Region region;
        private final URI endpointOverride;

        public <B extends AwsClientBuilder<B, C>, C> void customize(AwsClientBuilder<B, C> builder) {
            if (credentialsProvider != null) {
                builder.credentialsProvider(credentialsProvider);
            }
            if (region != null) {
                builder.region(region);
            }
            if (endpointOverride != null) {
                builder.endpointOverride(endpointOverride);
            }
        }

        public void customize(SdkPresigner.Builder builder) {
            if (credentialsProvider != null) {
                builder.credentialsProvider(credentialsProvider);
            }
            if (region != null) {
                builder.region(region);
            }
            if (endpointOverride != null) {
                builder.endpointOverride(endpointOverride);
            }
        }
    }

And effectively it does the same thing implement the same code twice just due to interface un-interoperability.

If the change would have been done as proposed the code will be as follows:

    @Builder
    public static class ClientBuilderCustomizer {

        private final AwsCredentialsProvider credentialsProvider;
        private final Region region;
        private final URI endpointOverride;

        public <B extends EndpointOverrideAware<B, C>, C> void customize(EndpointOverrideAware<B, C> builder) {
            if (endpointOverride != null) {
                builder.endpointOverride(endpointOverride);
            }
        }

        public <B extends CredentialsProviderAware<B, C>, C> void customize(CredentialsProviderAware<B, C> builder) {
            if (credentialsProvider != null) {
                builder.credentialsProvider(credentialsProvider);
            }
        }

        public <B extends RegionAware<B, C>, C> void customize(RegionAware<B, C> builder) {
            if (region != null) {
                builder.region(region);
            }
        }
    }

Or would allow even more granular control over configuration by having individual customisers per interface.

This change is non-breaking and I don't insist on any particular naming or hierarchy, mainly I was just wondering why it was implemented the way it is at the moment