[v3] Adding ServiceClient methods/ctors

[conniey]

@anuchandy @JonathanGiles Any suggestions would be helpful. :)

I've been trying to use Azure App Config with our new ServiceClient, and I've had to do a bit of weirdness because ServiceClient takes in a HttpPipeline for its constructor.

Problems that cropped up:

1. Customers calling AzConfigClient may to want to pass in their own HttpClient (ie. proxy)
2. Customers using AzConfigClient may want to add additional HttpPipelinePolicies in conjunction with our default ones. (ie. additional logging or headers)

As a result, I added in PipelineOptions.java.

Code: AzConfigClient.java

class AzConfigClient {
    public AzConfigClient(String connectionString, PipelineOptions pipelineOptions) {
        super(createPipeline(new ApplicationConfigCredentials(connectionString), pipelineOptions));

        this.service = RestProxy.create(ApplicationConfigService.class, this);
        // Oddness is here because I had to create the creds twice since the super class required the
        // HttpPipeline already.
        this.baseUri = new ApplicationConfigCredentials(connectionString).baseUri();
    }
}

class PipelineOptions {
    public HttpClient client() {}
    public Iterable<HttpPipelinePolicy> additionalPolicies() {}
}

Would it be easier to add additional constructors/methods to ServiceClient? My proposal would be:

public abstract class ServiceClient {
    protected ServiceClient() {
        this(HttpClient.createDefault());
    }

    /**
     * Creates a ServiceClient with the given HttpClient and additional policies.
     * @param client HttpClient to use for service calls
     * @param additionalPolicies Any additional policies that should be added. These are executed after the default policies.
     */
    protected ServiceClient(HttpClient client, HttpPipelinePolicy... additionalPolicies) { }

    /**
     * Gets the default pipeline policies for this service client to execute.
     *
     * Example:
     * Azure Application Configuration requires: Custom CredentialPolicy, RetryPolicy, UserAgentPolicy, RequestIdPolicy
     */
    protected abstract List<HttpPipelinePolicy> getPipelinePolicies();
}

[conniey]

I also noticed that RestProxy and AzureProxy can create defaultPipelines. Should I be moving the logic there somehow?

[Edit] RestProxy.createDefaultPipeline() methods AzureProxy.createDefaultPipeline() methods

[anuchandy]

@JonathanGiles @jianghaolu @conniey

The pattern for creating various clients is something always wanted to discuss between us. Question is, should we try to come up with a general pattern that our customers eventually get familiar with, so that the experience is consistent when working across various SDKs.

Let's use AzConfigClient as an example.

Pattern:1

Starting with a pattern our Java customers are already familiar with:

Creating client with default configuration:

String connectionString = "";
AzConfigClient client = AzConfigClient
        .authenticate(String connectionString);

1. authenticate is a static method in AzConfigClient.
2. AzConfigClient constructor is private.

This creates a AzConfigClient with default HttpClient, Policies and other settings.

Creating client with configuration:

String connectionString = "";
AzConfigClient client = AzConfigClient
        .configure()
        // Configure logging
        .loggingLevel(LoggingLevel.HEADER_AND_BODY)
        // Configure custom policies
        .policy(new RequestIdPolicy())
        .policy(new UserAgentPolicy())
        // Configure Http settings
        .proxy(new ProxyOptions(Type.Http, new InetAddress("proxyIp", port)))
        // Authenticate
        .authenticate(connectionString);

1. configure is a static method in AzConfigClient.
2. configure() returns an inner configurations class, exposing config methods and authenitcate() method that returns AzConfigClient.
3. configuration class exposes methods to enable standard config such as logging, add policies, enable various options on default HttpClient.

Creating client with configuration and HttpClient

HttpClient httpClient = new HttpClient.createDefault()
       .proxy(new ProxyOptions(Type.Http, new InetAddress("proxyIp", port)));

String connectionString = "";
AzConfigClient client = AzConfigClient
        .configure()
        // Configure logging
        .loggingLevel(LoggingLevel.HEADER_AND_BODY)
        // Configure custom policies
        .policy(new RequestIdPolicy())
        .policy(new UserAgentPolicy())
        // Set Http client
        .httpClient(httpClient)
        // Authenticate
        .authenticate(connectionString);

Pattern:2

This pattern is similar to our current pattern to configure HttpClient. I'm not sure this is really possible but we can still explore this.

[JonathanGiles]

I am generally a fan of pattern one. However, a few comments:

1. I wouldn't make the authenticate method the terminal method - I would probably have a build or similar.
2. Introducing a fluent builder pattern like this implies that the client should be immutable in all cases.

fwiw, this is equivalent to the AWS SDK approach too.

[anuchandy]

@JonathanGiles - thanks for the feedback. Just to make sure we all are in the same page, the flow will looks like below, is this something we should start with?

[conniey]

Thanks for making that image! Makes it easier to follow. I like this approach.

[conniey]

@anuchandy Is there a library that you use to create your builders and immutable classes like https://immutables.github.io/?

[JonathanGiles]

@conniey I suspect the right answer is that we will hand-build builder code, rather than use an external library.

@anuchandy I would consider a slightly different vocabulary: I would rename configure to builder. Also, I'm not sure if you planned any particular special-casing for the credentials method, but I would list it as another option in the 'main loop' that you specify. I don't think it should be considered as a terminal operation. In fact, I think the real discussion should be this: should 'required' arguments like the credentials need to be specified at the time the user calls builder() (e.g. builder(Credentials)), to ensure that an instance is not created that cannot function.

[anuchandy]

@conniey Our client (e.g. AzConfigClient) is already immutable once created, so that part is good. As Jonathan pointed we can hand write the builder flow based on the client properties.

@JonathanGiles Fine with either builder() or configure(). Agree, required client parameters should be in enforced then optionals can follow. Which one of the following we should go with? or any thing else.

Option 2 has the advantage of more readability when we've MANY required parameters, not the case with AzConfig though.

[JonathanGiles]

I've always used approach one, but I am happy to explore approach two as well. I don't want an explosion of classes, that's my main criteria.