HTTP protocol version on the logging (#1543)

* [HTTP version] Add HTTP version to Response, the default client; Update SLF4J unit-test * [HTTP version] Mock client * [HTTP version] Apache HTTP Client * [HTTP version] protocol -> protocolVersion; Replace protocol number with full name * [HTTP version] Code style, rollback to old one * [HTTP version] Google HTTP Client * [HTTP version] HTTP_PROTOCOL -> HTTP_PROTOCOL_VERSION * [HTTP version] HC5 * [HTTP version] Java11 Client * [HTTP version] OkHttpClient * [HTTP version] Code style, rollback to old one * [HTTP version] Make some required changes: restore log messages for back compatibility, replace string protocol version with enum, replace fragile conversion of alien enums by string case-insensitive comparision * [HTTP version] Code style, rollback to old one; Remove unused constants * [HTTP version] Update imports * [HTTP version] Test coverage * [HTTP version] Fix license issue * [HTTP version] Beatify and simplify the unit-test
OpenFeign · Nov 30, 2021 · 1cce855 · 1cce855
1 parent a686b2f
commit 1cce855
Show file tree

Hide file tree

Showing 12 changed files with 285 additions and 11 deletions.
diff --git a/core/src/main/java/feign/Logger.java b/core/src/main/java/feign/Logger.java
@@ -20,6 +20,7 @@
 import java.util.logging.LogRecord;
 import java.util.logging.SimpleFormatter;
 import static feign.Util.*;
+import static java.util.Objects.nonNull;
 
 /**
  * Simple logging abstraction for debug messages. Adapted from {@code retrofit.RestAdapter.Log}.
@@ -61,7 +62,9 @@ protected boolean shouldLogResponseHeader(String header) {
   }
 
   protected void logRequest(String configKey, Level logLevel, Request request) {
-    log(configKey, "---> %s %s HTTP/1.1", request.httpMethod().name(), request.url());
+    String protocolVersion = resolveProtocolVersion(request.protocolVersion());
+    log(configKey, "---> %s %s %s", request.httpMethod().name(), request.url(),
+        protocolVersion);
     if (logLevel.ordinal() >= Level.HEADERS.ordinal()) {
 
       for (String field : request.headers().keySet()) {
@@ -97,11 +100,12 @@ protected Response logAndRebufferResponse(String configKey,
                                             Response response,
                                             long elapsedTime)
       throws IOException {
+    String protocolVersion = resolveProtocolVersion(response.protocolVersion());
     String reason =
         response.reason() != null && logLevel.compareTo(Level.NONE) > 0 ? " " + response.reason()
             : "";
     int status = response.status();
-    log(configKey, "<--- HTTP/1.1 %s%s (%sms)", status, reason, elapsedTime);
+    log(configKey, "<--- %s %s%s (%sms)", protocolVersion, status, reason, elapsedTime);
     if (logLevel.ordinal() >= Level.HEADERS.ordinal()) {
 
       for (String field : response.headers().keySet()) {
@@ -148,6 +152,13 @@ protected IOException logIOException(String configKey,
     return ioe;
   }
 
+  protected static String resolveProtocolVersion(Request.ProtocolVersion protocolVersion) {
+    if (nonNull(protocolVersion)) {
+      return protocolVersion.toString();
+    }
+    return "UNKNOWN";
+  }
+
   /**
    * Controls the level of logging.
    */

diff --git a/core/src/main/java/feign/Request.java b/core/src/main/java/feign/Request.java
@@ -1,5 +1,5 @@
 /**
- * Copyright 2012-2020 The Feign Authors
+ * Copyright 2012-2021 The Feign Authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
  * in compliance with the License. You may obtain a copy of the License at
@@ -33,6 +33,25 @@ public enum HttpMethod {
     GET, HEAD, POST, PUT, DELETE, CONNECT, OPTIONS, TRACE, PATCH
   }
 
+  public enum ProtocolVersion {
+    HTTP_1_0("HTTP/1.0"), HTTP_1_1("HTTP/1.1"), HTTP_2("HTTP/2.0"), MOCK;
+
+    String protocolVersion;
+
+    ProtocolVersion() {
+      protocolVersion = name();
+    }
+
+    ProtocolVersion(String protocolVersion) {
+      this.protocolVersion = protocolVersion;
+    }
+
+    @Override
+    public String toString() {
+      return protocolVersion;
+    }
+  }
+
   /**
    * No parameters can be null except {@code body} and {@code charset}. All parameters must be
    * effectively immutable, via safe copies, not mutating or otherwise.
@@ -110,6 +129,7 @@ public static Request create(HttpMethod httpMethod,
   private final Map<String, Collection<String>> headers;
   private final Body body;
   private final RequestTemplate requestTemplate;
+  private final ProtocolVersion protocolVersion;
 
   /**
    * Creates a new Request.
@@ -130,6 +150,7 @@ public static Request create(HttpMethod httpMethod,
     this.headers = checkNotNull(headers, "headers of %s %s", method, url);
     this.body = body;
     this.requestTemplate = requestTemplate;
+    protocolVersion = ProtocolVersion.HTTP_1_1;
   }
 
   /**
@@ -203,6 +224,15 @@ public int length() {
     return this.body.length();
   }
 
+  /**
+   * Request HTTP protocol version
+   *
+   * @return HTTP protocol version
+   */
+  public ProtocolVersion protocolVersion() {
+    return protocolVersion;
+  }
+
   /**
    * Request as an HTTP/1.1 request.
    *

diff --git a/core/src/main/java/feign/Response.java b/core/src/main/java/feign/Response.java
@@ -17,6 +17,7 @@
 import java.nio.charset.Charset;
 import java.nio.charset.StandardCharsets;
 import java.util.*;
+import feign.Request.ProtocolVersion;
 import static feign.Util.*;
 
 /**
@@ -29,6 +30,7 @@ public final class Response implements Closeable {
   private final Map<String, Collection<String>> headers;
   private final Body body;
   private final Request request;
+  private final ProtocolVersion protocolVersion;
 
   private Response(Builder builder) {
     checkState(builder.request != null, "original request is required");
@@ -37,7 +39,7 @@ private Response(Builder builder) {
     this.reason = builder.reason; // nullable
     this.headers = caseInsensitiveCopyOf(builder.headers);
     this.body = builder.body; // nullable
-
+    this.protocolVersion = builder.protocolVersion;
   }
 
   public Builder toBuilder() {
@@ -55,6 +57,7 @@ public static final class Builder {
     Body body;
     Request request;
     private RequestTemplate requestTemplate;
+    private ProtocolVersion protocolVersion = ProtocolVersion.HTTP_1_1;
 
     Builder() {}
 
@@ -64,6 +67,7 @@ public static final class Builder {
       this.headers = source.headers;
       this.body = source.body;
       this.request = source.request;
+      this.protocolVersion = source.protocolVersion;
     }
 
     /** @see Response#status */
@@ -117,6 +121,14 @@ public Builder request(Request request) {
       return this;
     }
 
+    /**
+     * HTTP protocol version
+     */
+    public Builder protocolVersion(ProtocolVersion protocolVersion) {
+      this.protocolVersion = protocolVersion;
+      return this;
+    }
+
     /**
      * The Request Template used for the original request.
      *
@@ -173,6 +185,15 @@ public Request request() {
     return request;
   }
 
+  /**
+   * the HTTP protocol version
+   *
+   * @return HTTP protocol version or empty if a client does not provide it
+   */
+  public ProtocolVersion protocolVersion() {
+    return protocolVersion;
+  }
+
   public Charset charset() {
 
     Collection<String> contentTypeHeaders = headers().get("Content-Type");

diff --git a/core/src/main/java/feign/Util.java b/core/src/main/java/feign/Util.java
@@ -35,6 +35,7 @@
 import java.util.function.Supplier;
 import java.util.stream.Stream;
 import static java.lang.String.format;
+import static java.util.Objects.nonNull;
 
 /**
  * Utilities, typically copied in from guava, so as to avoid dependency conflicts.
@@ -381,4 +382,14 @@ public static Map<String, Collection<String>> caseInsensitiveCopyOf(Map<String,
     return Collections.unmodifiableMap(result);
   }
 
+  public static <T extends Enum<?>> T enumForName(Class<T> enumClass, Object object) {
+    String name = (nonNull(object)) ? object.toString() : null;
+    for (T enumItem : enumClass.getEnumConstants()) {
+      if (enumItem.name().equalsIgnoreCase(name) || enumItem.toString().equalsIgnoreCase(name)) {
+        return enumItem;
+      }
+    }
+    return null;
+  }
+
 }
diff --git a/core/src/test/java/feign/EnumForNameTest.java b/core/src/test/java/feign/EnumForNameTest.java
@@ -0,0 +1,79 @@
+/**
+ * Copyright 2012-2021 The Feign Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
+ * in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package feign;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
+import org.junit.runners.Parameterized.Parameters;
+import java.util.Arrays;
+import feign.Request.ProtocolVersion;
+import static feign.Util.enumForName;
+import static org.junit.Assert.*;
+
+public class EnumForNameTest {
+
+  @RunWith(Parameterized.class)
+  public static class KnownEnumValues {
+
+    @Parameter
+    public Object name;
+    @Parameter(1)
+    public ProtocolVersion expectedProtocolVersion;
+
+    @Parameters
+    public static Iterable<Object[]> data() {
+      return Arrays.asList(new Object[][] {
+          {ProtocolVersion.HTTP_1_0, ProtocolVersion.HTTP_1_0},
+          {"HTTP/1.0", ProtocolVersion.HTTP_1_0},
+          {ProtocolVersion.HTTP_1_1, ProtocolVersion.HTTP_1_1},
+          {"HTTP/1.1", ProtocolVersion.HTTP_1_1},
+          {ProtocolVersion.HTTP_2, ProtocolVersion.HTTP_2},
+          {"HTTP/2.0", ProtocolVersion.HTTP_2}
+      });
+    }
+
+    @Test
+    public void getKnownEnumValue() {
+      assertEquals("known enum value: " + name, expectedProtocolVersion,
+          enumForName(ProtocolVersion.class, name));
+    }
+
+  }
+
+  @RunWith(Parameterized.class)
+  public static class UnknownEnumValues {
+
+    @Parameter
+    public Object name;
+
+    @Parameters
+    public static Iterable<Object[]> data() {
+      return Arrays.asList(new Object[][] {
+          {Request.HttpMethod.GET},
+          {"SPDY/3"},
+          {null},
+          {"HTTP/2"}
+      });
+    }
+
+    @Test
+    public void getKnownEnumValue() {
+      assertNull("unknown enum value: " + name, enumForName(ProtocolVersion.class, name));
+    }
+
+  }
+
+}
diff --git a/core/src/test/java/feign/LoggerTest.java b/core/src/test/java/feign/LoggerTest.java
@@ -27,12 +27,14 @@
 import org.junit.runners.Parameterized;
 import org.junit.runners.Parameterized.Parameters;
 import org.junit.runners.model.Statement;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collections;
-import java.util.List;
+import java.io.IOException;
+import java.net.HttpURLConnection;
+import java.util.*;
 import java.util.concurrent.TimeUnit;
 import feign.Logger.Level;
+import feign.Request.ProtocolVersion;
+import static java.util.Objects.nonNull;
+import static feign.Util.enumForName;
 
 @RunWith(Enclosed.class)
 public class LoggerTest {
@@ -142,6 +144,51 @@ public void reasonPhraseOptional() {
     }
   }
 
+  @RunWith(Parameterized.class)
+  public static class HttpProtocolVersionTest extends LoggerTest {
+
+    private final Level logLevel;
+    private final String protocolVersionName;
+
+    public HttpProtocolVersionTest(Level logLevel, String protocolVersionName,
+        List<String> expectedMessages) {
+      this.logLevel = logLevel;
+      this.protocolVersionName = protocolVersionName;
+      logger.expectMessages(expectedMessages);
+    }
+
+    @Parameters
+    public static Iterable<Object[]> data() {
+      return Arrays.asList(new Object[][] {
+          {Level.BASIC, null, Arrays.asList(
+              "\\[SendsStuff#login\\] ---> POST http://localhost:[0-9]+/ HTTP/1.1",
+              "\\[SendsStuff#login\\] <--- HTTP/1.1 200 \\([0-9]+ms\\)")},
+          {Level.BASIC, "HTTP/1.1", Arrays.asList(
+              "\\[SendsStuff#login\\] ---> POST http://localhost:[0-9]+/ HTTP/1.1",
+              "\\[SendsStuff#login\\] <--- HTTP/1.1 200 \\([0-9]+ms\\)")},
+          {Level.BASIC, "HTTP/2.0", Arrays.asList(
+              "\\[SendsStuff#login\\] ---> POST http://localhost:[0-9]+/ HTTP/1.1",
+              "\\[SendsStuff#login\\] <--- HTTP/2.0 200 \\([0-9]+ms\\)")},
+          {Level.BASIC, "HTTP-XYZ", Arrays.asList(
+              "\\[SendsStuff#login\\] ---> POST http://localhost:[0-9]+/ HTTP/1.1",
+              "\\[SendsStuff#login\\] <--- UNKNOWN 200 \\([0-9]+ms\\)")}
+      });
+    }
+
+    @Test
+    public void testHttpProtocolVersion() {
+      server.enqueue(new MockResponse().setStatus("HTTP/1.1 " + 200));
+
+      SendsStuff api = Feign.builder()
+          .client(new TestProtocolVersionClient(protocolVersionName))
+          .logger(logger)
+          .logLevel(logLevel)
+          .target(SendsStuff.class, "http://localhost:" + server.getPort());
+
+      api.login("netflix", "denominator", "password");
+    }
+  }
+
   @RunWith(Parameterized.class)
   public static class ReadTimeoutEmitsTest extends LoggerTest {
 
@@ -424,4 +471,25 @@ public void evaluate() throws Throwable {
       };
     }
   }
+
+  private static final class TestProtocolVersionClient extends Client.Default {
+    private final String protocolVersionName;
+
+    public TestProtocolVersionClient(String protocolVersionName) {
+      super(null, null);
+      this.protocolVersionName = protocolVersionName;
+    }
+
+    @Override
+    Response convertResponse(HttpURLConnection connection, Request request)
+        throws IOException {
+      Response response = super.convertResponse(connection, request);
+      if (nonNull((protocolVersionName))) {
+        response = response.toBuilder()
+            .protocolVersion(enumForName(ProtocolVersion.class, protocolVersionName))
+            .build();
+      }
+      return response;
+    }
+  }
 }