HttpPipelineBuilder proposal (Azure#43504)

HttpPipelineBuilder proposal

Author: alzimmermsft

sdk/clientcore/core/spotbugs-exclude.xml

@@ -15,6 +15,7 @@
     <Or>
       <Class name="io.clientcore.core.http.models.HttpRequest" />
       <Class name="io.clientcore.core.http.models.RequestOptions" />
+      <Class name="io.clientcore.core.http.pipeline.HttpPipelineBuilder" />
       <Class name="io.clientcore.core.http.pipeline.HttpPipelineNextPolicy" />
       <Class name="io.clientcore.core.http.pipeline.HttpRetryPolicy" />
       <Class name="io.clientcore.core.implementation.ReflectionUtilsMethodHandle" />
@@ -91,13 +92,17 @@
   </Match>
   <Match>
     <Bug pattern="DMI_RANDOM_USED_ONLY_ONCE" />
-    <Class name="io.clientcore.core.util.TestUtils" />
+    <Or>
+      <Class name="io.clientcore.core.http.pipeline.HttpPipelinePolicyTests$SyncPolicy" />
+      <Class name="io.clientcore.core.util.TestUtils" />
+    </Or>
   </Match>
   <Match>
     <Bug pattern="DM_CONVERT_CASE" />
     <Or>
       <Class name="io.clientcore.core.http.exception.HttpExceptionType" />
       <Class name="io.clientcore.core.http.models.HttpHeaderName" />
+      <Class name="io.clientcore.core.http.pipeline.HttpPipelineBuilder" />
       <Class name="io.clientcore.core.util.ServerSentEventUtils" />
       <Class name="io.clientcore.core.util.auth.AuthUtils" />
     </Or>
@@ -108,6 +113,7 @@
       <Class name="io.clientcore.core.http.RestProxyTests" />
       <Class name="io.clientcore.core.http.client.DefaultHttpClientIT" />
       <Class name="io.clientcore.core.http.client.SimpleBasicAuthHttpProxyServer" />
+      <Class name="io.clientcore.core.http.pipeline.HttpPipelinePolicyTests$SyncPolicy" />
       <Class name="io.clientcore.core.implementation.http.rest.RestProxyImplTests" />
       <Class name="io.clientcore.core.implementation.serializer.AdditionalPropertiesSerializerTests" />
       <Class name="io.clientcore.core.implementation.serializer.BinaryDataSerializationTests" />
@@ -340,6 +346,7 @@
   <Match>
     <Bug pattern="SIC_INNER_SHOULD_BE_STATIC_ANON" />
     <Or>
+      <Class name="io.clientcore.core.http.pipeline.HttpPipelinePolicyTests" />
       <Class name="io.clientcore.core.http.pipeline.HttpPipelineTests" />
       <Class name="io.clientcore.core.http.pipeline.RedirectPolicyTest" />
       <Class name="io.clientcore.core.http.pipeline.RetryPolicyTests" />

sdk/clientcore/core/src/main/java/io/clientcore/core/http/pipeline/HttpCredentialPolicy.java

@@ -0,0 +1,20 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+package io.clientcore.core.http.pipeline;
+
+/**
+ * This class is exactly the same as {@link HttpPipelinePolicy} but exists to provide a standard parent class for all
+ * credential policies and to differentiate them from other forms of {@link HttpPipelinePolicy}s.
+ */
+public abstract class HttpCredentialPolicy implements HttpPipelinePolicy {
+    /**
+     * Creates an instance of {@link HttpCredentialPolicy}.
+     */
+    public HttpCredentialPolicy() {
+    }
+
+    @Override
+    public final HttpPipelineOrder getOrder() {
+        return HttpPipelineOrder.AUTHENTICATION;
+    }
+}

sdk/clientcore/core/src/main/java/io/clientcore/core/http/pipeline/HttpInstrumentationPolicy.java

@@ -68,9 +68,9 @@
  * <p>
  * It propagates context to the downstream service following <a href="https://www.w3.org/TR/trace-context-1/">W3C Trace Context</a> specification.
  * <p>
- * The {@link HttpInstrumentationPolicy} should be added to the HTTP pipeline by client libraries. It should be added after
- * {@link HttpRetryPolicy} and {@link HttpRedirectPolicy} so that it's executed on each try or redirect and logging happens
- * in the scope of the span.
+ * The {@link HttpInstrumentationPolicy} should be added to the HTTP pipeline by client libraries. It should be added
+ * after {@link HttpRetryPolicy} and {@link HttpRedirectPolicy} so that it's executed on each try or redirect and
+ * logging happens in the scope of the span.
  * <p>
  * The policy supports basic customizations using {@link HttpInstrumentationOptions}.
  * <p>
@@ -644,4 +644,9 @@ public void close() throws IOException {
             originalBody.close();
         }
     }
+
+    @Override
+    public HttpPipelineOrder getOrder() {
+        return HttpPipelineOrder.INSTRUMENTATION;
+    }
 }

sdk/clientcore/core/src/main/java/io/clientcore/core/http/pipeline/HttpPipelineBuilder.java

@@ -4,11 +4,13 @@
 package io.clientcore.core.http.pipeline;
 
 import io.clientcore.core.http.client.HttpClient;
+import io.clientcore.core.instrumentation.logging.ClientLogger;
 import io.clientcore.core.util.configuration.Configuration;
 
 import java.util.ArrayList;
-import java.util.Arrays;
+import java.util.LinkedList;
 import java.util.List;
+import java.util.Objects;
 
 /**
  * This class provides a fluent builder API to help aid the configuration and instantiation of the {@link HttpPipeline},
@@ -28,7 +30,7 @@
  * <!-- end io.clientcore.core.http.HttpPipelineBuilder.noConfiguration -->
  *
  * <p>Create a pipeline using the default HTTP client and a retry policy</p>
- *
+ * 
  * <!-- src_embed io.clientcore.core.http.HttpPipelineBuilder.defaultHttpClientWithRetryPolicy -->
  * <pre>
  * HttpPipeline pipeline = new HttpPipelineBuilder&#40;&#41;
@@ -41,8 +43,19 @@
  * @see HttpPipeline
  */
 public class HttpPipelineBuilder {
+    private static final ClientLogger LOGGER = new ClientLogger(HttpPipelineBuilder.class);
+
     private HttpClient httpClient;
-    private List<HttpPipelinePolicy> pipelinePolicies;
+
+    private final LinkedList<HttpPipelinePolicy> beforeRedirect = new LinkedList<>();
+    private HttpRedirectPolicy redirectPolicy;
+    private final LinkedList<HttpPipelinePolicy> betweenRedirectAndRetry = new LinkedList<>();
+    private HttpRetryPolicy retryPolicy;
+    private final LinkedList<HttpPipelinePolicy> betweenRetryAndAuthentication = new LinkedList<>();
+    private HttpCredentialPolicy credentialPolicy;
+    private final LinkedList<HttpPipelinePolicy> betweenAuthenticationAndInstrumentation = new LinkedList<>();
+    private HttpInstrumentationPolicy instrumentationPolicy;
+    private final LinkedList<HttpPipelinePolicy> afterInstrumentation = new LinkedList<>();
 
     /**
      * Creates a new instance of HttpPipelineBuilder that can configure options for the {@link HttpPipeline} before
@@ -60,7 +73,32 @@ public HttpPipelineBuilder() {
      * @return A HttpPipeline with the options set from the builder.
      */
     public HttpPipeline build() {
-        List<HttpPipelinePolicy> policies = (pipelinePolicies == null) ? new ArrayList<>() : pipelinePolicies;
+        List<HttpPipelinePolicy> policies = new ArrayList<>(beforeRedirect);
+
+        if (redirectPolicy != null) {
+            policies.add(redirectPolicy);
+        }
+
+        policies.addAll(betweenRedirectAndRetry);
+
+        if (retryPolicy != null) {
+            policies.add(retryPolicy);
+        }
+
+        policies.addAll(betweenRetryAndAuthentication);
+
+        if (credentialPolicy != null) {
+            policies.add(credentialPolicy);
+        }
+
+        policies.addAll(betweenAuthenticationAndInstrumentation);
+
+        if (instrumentationPolicy != null) {
+            policies.add(instrumentationPolicy);
+        }
+
+        policies.addAll(afterInstrumentation);
+
         HttpClient client;
 
         if (httpClient != null) {
@@ -80,7 +118,6 @@ public HttpPipeline build() {
      * Sets the HttpClient that the pipeline will use to send requests.
      *
      * @param httpClient The HttpClient the pipeline will use when sending requests.
-     *
      * @return The updated HttpPipelineBuilder object.
      */
     public HttpPipelineBuilder httpClient(HttpClient httpClient) {
@@ -90,20 +127,82 @@ public HttpPipelineBuilder httpClient(HttpClient httpClient) {
     }
 
     /**
-     * Adds {@link HttpPipelinePolicy policies} to the set of policies that the pipeline will use when sending
-     * requests.
-     *
-     * @param policies Policies to add to the policy set.
+     * Adds an {@link HttpPipelinePolicy} to the builder.
+     * <p>
+     * The {@code policy} passed will be positioned based on {@link HttpPipelinePolicy#getOrder()}. If the
+     * {@link HttpPipelineOrder} is null an {@link IllegalArgumentException} will be thrown.
+     * <p>
+     * If the {@code policy} is one of the pillar policies ({@link HttpRedirectPolicy}, {@link HttpRetryPolicy},
+     * {@link HttpCredentialPolicy}, or {@link HttpInstrumentationPolicy}) the {@link HttpPipelineOrder} will be ignored
+     * as those policies are positioned in a specific location within the pipeline. If a duplicate pillar policy is
+     * added (for example two {@link HttpRetryPolicy}) the last one added will be used and a message will be logged.
      *
+     * @param policy The policy to add to the pipeline.
      * @return The updated HttpPipelineBuilder object.
      */
-    public HttpPipelineBuilder policies(HttpPipelinePolicy... policies) {
-        if (pipelinePolicies == null) {
-            pipelinePolicies = new ArrayList<>();
+    public HttpPipelineBuilder addPolicy(HttpPipelinePolicy policy) {
+        Objects.requireNonNull(policy, "'policy' cannot be null.");
+
+        if (tryAddPillar(policy)) {
+            return this;
         }
 
-        this.pipelinePolicies.addAll(Arrays.asList(policies));
+        HttpPipelineOrder order = policy.getOrder();
+        if (order == null) {
+            throw LOGGER.atError()
+                .addKeyValue("policyType", policy.getClass())
+                .log("Policy order cannot be null.", new IllegalArgumentException("Policy order cannot be null."));
+        }
+
+        if (order == HttpPipelineOrder.BEFORE_REDIRECT) {
+            beforeRedirect.add(policy);
+        } else if (order == HttpPipelineOrder.BETWEEN_REDIRECT_AND_RETRY) {
+            betweenRedirectAndRetry.add(policy);
+        } else if (order == HttpPipelineOrder.BETWEEN_RETRY_AND_AUTHENTICATION) {
+            betweenRetryAndAuthentication.add(policy);
+        } else if (order == HttpPipelineOrder.BETWEEN_AUTHENTICATION_AND_INSTRUMENTATION) {
+            betweenAuthenticationAndInstrumentation.add(policy);
+        } else if (order == HttpPipelineOrder.AFTER_INSTRUMENTATION) {
+            afterInstrumentation.add(policy);
+        } else {
+            throw LOGGER.atError()
+                .addKeyValue("policyType", policy.getClass())
+                .addKeyValue("order", order)
+                .log("Unknown policy order.", new IllegalArgumentException("Unknown policy order."));
+        }
 
         return this;
     }
+
+    private boolean tryAddPillar(HttpPipelinePolicy policy) {
+        HttpPipelinePolicy previous = null;
+        boolean added = false;
+
+        HttpPipelineOrder order = policy.getOrder();
+        if (order == HttpPipelineOrder.REDIRECT) {
+            previous = redirectPolicy;
+            redirectPolicy = (HttpRedirectPolicy) policy;
+            added = true;
+        } else if (order == HttpPipelineOrder.RETRY) {
+            previous = retryPolicy;
+            retryPolicy = (HttpRetryPolicy) policy;
+            added = true;
+        } else if (order == HttpPipelineOrder.AUTHENTICATION) {
+            previous = credentialPolicy;
+            credentialPolicy = (HttpCredentialPolicy) policy;
+            added = true;
+        } else if (order == HttpPipelineOrder.INSTRUMENTATION) {
+            previous = instrumentationPolicy;
+            instrumentationPolicy = (HttpInstrumentationPolicy) policy;
+            added = true;
+        }
+
+        if (previous != null) {
+            LOGGER.atWarning()
+                .addKeyValue("policyType", previous.getClass().getSimpleName())
+                .log("A pillar policy was replaced in the pipeline.");
+        }
+
+        return added;
+    }
 }