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 · 206193d · 206193d
1 parent 851749e
commit 206193d
Show file tree

Hide file tree

Showing 12 changed files with 302 additions and 10 deletions.
diff --git a/core/src/main/java/feign/Logger.java b/core/src/main/java/feign/Logger.java
@@ -14,6 +14,7 @@
 package feign;
 
 import static feign.Util.*;
+import static java.util.Objects.nonNull;
 
 import java.io.IOException;
 import java.io.PrintWriter;
@@ -60,7 +61,8 @@ 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()) {
@@ -91,12 +93,13 @@ protected void logRetry(String configKey, Level logLevel) {
 
   protected Response logAndRebufferResponse(
       String configKey, Level logLevel, 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()) {
@@ -145,6 +148,13 @@ protected IOException logIOException(
     return ioe;
   }
 
+  protected static String resolveProtocolVersion(Request.ProtocolVersion protocolVersion) {
+    if (nonNull(protocolVersion)) {
+      return protocolVersion.toString();
+    }
+    return "UNKNOWN";
+  }
+
   /** Controls the level of logging. */
   public enum Level {
     /** No 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
  *
  * <p>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
@@ -40,6 +40,28 @@ public enum HttpMethod {
     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.
@@ -121,6 +143,7 @@ public static Request create(
   private final Map<String, Collection<String>> headers;
   private final Body body;
   private final RequestTemplate requestTemplate;
+  private final ProtocolVersion protocolVersion;
 
   /**
    * Creates a new Request.
@@ -142,6 +165,7 @@ public static Request create(
     this.headers = checkNotNull(headers, "headers of %s %s", method, url);
     this.body = body;
     this.requestTemplate = requestTemplate;
+    protocolVersion = ProtocolVersion.HTTP_1_1;
   }
 
   /**
@@ -214,6 +238,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
@@ -15,6 +15,7 @@
 
 import static feign.Util.*;
 
+import feign.Request.ProtocolVersion;
 import java.io.*;
 import java.nio.charset.Charset;
 import java.nio.charset.StandardCharsets;
@@ -28,6 +29,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");
@@ -36,6 +38,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() {
@@ -53,6 +56,7 @@ public static final class Builder {
     Body body;
     Request request;
     private RequestTemplate requestTemplate;
+    private ProtocolVersion protocolVersion = ProtocolVersion.HTTP_1_1;
 
     Builder() {}
 
@@ -62,6 +66,7 @@ public static final class Builder {
       this.headers = source.headers;
       this.body = source.body;
       this.request = source.request;
+      this.protocolVersion = source.protocolVersion;
     }
 
     /**
@@ -129,6 +134,12 @@ 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.
      *
@@ -179,6 +190,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
@@ -14,6 +14,7 @@
 package feign;
 
 import static java.lang.String.format;
+import static java.util.Objects.nonNull;
 
 import java.io.ByteArrayOutputStream;
 import java.io.Closeable;
@@ -346,4 +347,14 @@ public static Map<String, Collection<String>> caseInsensitiveCopyOf(
 
     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,75 @@
+/**
+ * Copyright 2012-2021 The Feign Authors
+ *
+ * <p>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
+ *
+ * <p>http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * <p>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 static feign.Util.enumForName;
+import static org.junit.Assert.*;
+
+import feign.Request.ProtocolVersion;
+import java.util.Arrays;
+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;
+
+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
@@ -13,11 +13,14 @@
  */
 package feign;
 
+import static feign.Util.enumForName;
+import static java.util.Objects.nonNull;
+
 import feign.Logger.Level;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collections;
-import java.util.List;
+import feign.Request.ProtocolVersion;
+import java.io.IOException;
+import java.net.HttpURLConnection;
+import java.util.*;
 import java.util.concurrent.TimeUnit;
 import okhttp3.mockwebserver.MockResponse;
 import okhttp3.mockwebserver.MockWebServer;
@@ -159,6 +162,69 @@ 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 {
 
@@ -495,4 +561,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;
+    }
+  }
 }