Adding Method to Retryable Exception for evaluation (#744)

Closes #719

This change adds the original Request Method to `RetryableException`,
allowing implementers to determine if a retry should occur based on
method and exception type.

To support this, `Response` objects now require that the original
`Request` be present.  Test Cases, benchmarks, and documentation have
been added.

* Refactored Request Method Attribute on Requests
* Added `HttpMethod` enum that represents the supported HTTP methods
replacing String handling.
* Deprecated `Request#method()` in favor of `Request#httpMethod()`

CHANGELOG.md

@@ -1,6 +1,8 @@
 ### Version 10.0
 * Feign baseline is now JDK 8
 * Removed @Deprecated methods marked for removal on feign 10
+* `RetryException` includes the `Method` used for the offending `Request`
+* `Response` objects now contain the `Request` used.
 
 ### Version 9.6
 * Feign builder now supports flag `doNotCloseAfterDecode` to support lazy iteration of responses.

README.md

@@ -470,6 +470,37 @@ MyApi myApi = Feign.builder()
                  .target(MyApi.class, "https://api.hostname.com");
 ```
 
+### Error Handling
+If you need more control over handling unexpected responses, Feign instances can
+register a custom `ErrorDecoder` via the builder.
+
+```java
+MyApi myApi = Feign.builder()
+                 .errorDecoder(new MyErrorDecoder())
+                 .target(MyApi.class, "https://api.hostname.com");
+```
+
+All responses that result in an HTTP status not in the 2xx range will trigger the `ErrorDecoder`'s `decode` method, allowing
+you to handle the response, wrap the failure into a custom exception or perform any additional processing.
+If you want to retry the request again, throw a `RetryableException`.  This will invoke the registered
+`Retyer`.
+
+### Retry
+Feign, by default, will automatically retry `IOException`s, regardless of HTTP method, treating them as transient network
+related exceptions, and any `RetryableException` thrown from an `ErrorDecoder`.  To customize this
+behavior, register a custom `Retryer` instance via the builder.
+
+```java
+MyApi myApi = Feign.builder()
+                 .retryer(new MyRetryer())
+                 .target(MyApi.class, "https://api.hostname.com");
+```
+
+`Retryer`s are responsible for determining if a retry should occur by returning either a `true` or
+`false` from the method `continueOrPropagate(RetryableException e);`  A `Retryer` instance will be 
+created for each `Client` execution, allowing you to maintain state bewteen each request if desired.
+If the retry is determined to be unsucessful, the last `RetryException` will be thrown.
+
 #### Static and Default Methods
 Interfaces targeted by Feign may have static or default methods (if using Java 8+).
 These allows Feign clients to contain logic that is not expressly defined by the underlying API.

benchmark/pom.xml

@@ -66,17 +66,17 @@
     <dependency>
       <groupId>io.reactivex</groupId>
       <artifactId>rxnetty</artifactId>
-      <version>0.4.14</version>
+      <version>0.5.1</version>
     </dependency>
     <dependency>
-      <groupId>io.reactivex</groupId>
-      <artifactId>rxjava</artifactId>
-      <version>1.0.17</version>
+      <groupId>io.netty</groupId>
+      <artifactId>netty-buffer</artifactId>
+      <version>4.1.0.Beta7</version>
     </dependency>
     <dependency>
-      <groupId>io.netty</groupId>
-      <artifactId>netty-codec-http</artifactId>
-      <version>4.1.0.Beta8</version>
+      <groupId>io.reactivex</groupId>
+      <artifactId>rxjava</artifactId>
+      <version>1.0.14</version>
     </dependency>
     <dependency>
       <groupId>org.openjdk.jmh</groupId>

benchmark/src/main/java/feign/benchmark/DecoderIteratorsBenchmark.java

@@ -14,6 +14,7 @@
 package feign.benchmark;
 
 import com.fasterxml.jackson.core.type.TypeReference;
+import feign.Request;
 import feign.Response;
 import feign.Util;
 import feign.codec.Decoder;
@@ -77,6 +78,7 @@ public void buildResponse() {
         Response.builder()
             .status(200)
             .reason("OK")
+            .request(Request.create("GET", "/", Collections.emptyMap(), null, Util.UTF_8))
             .headers(Collections.emptyMap())
             .body(carsJson(Integer.valueOf(size)), Util.UTF_8)
             .build();

benchmark/src/main/java/feign/benchmark/RealRequestBenchmarks.java

@@ -14,13 +14,13 @@
 package feign.benchmark;
 
 import feign.Feign;
+import feign.Logger;
+import feign.Logger.Level;
 import feign.Response;
+import feign.Retryer;
 import io.netty.buffer.ByteBuf;
 import io.reactivex.netty.RxNetty;
 import io.reactivex.netty.protocol.http.server.HttpServer;
-import io.reactivex.netty.protocol.http.server.HttpServerRequest;
-import io.reactivex.netty.protocol.http.server.HttpServerResponse;
-import io.reactivex.netty.protocol.http.server.RequestHandler;
 import java.io.IOException;
 import java.util.concurrent.TimeUnit;
 import okhttp3.OkHttpClient;
@@ -53,21 +53,16 @@ public class RealRequestBenchmarks {
 
   @Setup
   public void setup() {
-    server =
-        RxNetty.createHttpServer(
-            SERVER_PORT,
-            new RequestHandler<ByteBuf, ByteBuf>() {
-              public rx.Observable handle(
-                  HttpServerRequest<ByteBuf> request, HttpServerResponse<ByteBuf> response) {
-                return response.flush();
-              }
-            });
+    server = RxNetty.createHttpServer(SERVER_PORT, (request, response) -> response.flush());
     server.start();
     client = new OkHttpClient();
     client.retryOnConnectionFailure();
     okFeign =
         Feign.builder()
             .client(new feign.okhttp.OkHttpClient(client))
+            .logLevel(Level.NONE)
+            .logger(new Logger.ErrorLogger())
+            .retryer(new Retryer.Default())
             .target(FeignTestInterface.class, "http://localhost:" + SERVER_PORT);
     queryRequest =
         new Request.Builder()
@@ -90,7 +85,10 @@ public okhttp3.Response query_baseCaseUsingOkHttp() throws IOException {
 
   /** How fast can we execute get commands synchronously using Feign? */
   @Benchmark
-  public Response query_feignUsingOkHttp() {
-    return okFeign.query();
+  public boolean query_feignUsingOkHttp() {
+    /* auto close the response */
+    try (Response ignored = okFeign.query()) {
+      return true;
+    }
   }
 }

core/src/main/java/feign/Client.java

@@ -62,7 +62,7 @@ public Default(SSLSocketFactory sslContextFactory, HostnameVerifier hostnameVeri
     @Override
     public Response execute(Request request, Options options) throws IOException {
       HttpURLConnection connection = convertAndSend(request, options);
-      return convertResponse(connection).toBuilder().request(request).build();
+      return convertResponse(connection, request);
     }
 
     HttpURLConnection convertAndSend(Request request, Options options) throws IOException {
@@ -81,7 +81,7 @@ HttpURLConnection convertAndSend(Request request, Options options) throws IOExce
       connection.setReadTimeout(options.readTimeoutMillis());
       connection.setAllowUserInteraction(false);
       connection.setInstanceFollowRedirects(options.isFollowRedirects());
-      connection.setRequestMethod(request.method());
+      connection.setRequestMethod(request.httpMethod().name());
 
       Collection<String> contentEncodingValues = request.headers().get(CONTENT_ENCODING);
       boolean gzipEncodedRequest =
@@ -136,7 +136,7 @@ HttpURLConnection convertAndSend(Request request, Options options) throws IOExce
       return connection;
     }
 
-    Response convertResponse(HttpURLConnection connection) throws IOException {
+    Response convertResponse(HttpURLConnection connection, Request request) throws IOException {
       int status = connection.getResponseCode();
       String reason = connection.getResponseMessage();
 
@@ -169,6 +169,7 @@ Response convertResponse(HttpURLConnection connection) throws IOException {
           .status(status)
           .reason(reason)
           .headers(headers)
+          .request(request)
           .body(stream, length)
           .build();
     }

core/src/main/java/feign/FeignException.java

@@ -42,7 +42,7 @@ public int status() {
 
   static FeignException errorReading(Request request, Response ignored, IOException cause) {
     return new FeignException(
-        format("%s reading %s %s", cause.getMessage(), request.method(), request.url()), cause);
+        format("%s reading %s %s", cause.getMessage(), request.httpMethod(), request.url()), cause);
   }
 
   public static FeignException errorStatus(String methodKey, Response response) {
@@ -59,7 +59,8 @@ public static FeignException errorStatus(String methodKey, Response response) {
 
   static FeignException errorExecuting(Request request, IOException cause) {
     return new RetryableException(
-        format("%s executing %s %s", cause.getMessage(), request.method(), request.url()),
+        format("%s executing %s %s", cause.getMessage(), request.httpMethod(), request.url()),
+        request.httpMethod(),
         cause,
         null);
   }

core/src/main/java/feign/Logger.java

@@ -46,7 +46,7 @@ protected static String methodTag(String configKey) {
   protected abstract void log(String configKey, String format, Object... args);
 
   protected void logRequest(String configKey, Level logLevel, Request request) {
-    log(configKey, "---> %s %s HTTP/1.1", request.method(), request.url());
+    log(configKey, "---> %s %s HTTP/1.1", request.httpMethod().name(), request.url());
     if (logLevel.ordinal() >= Level.HEADERS.ordinal()) {
 
       for (String field : request.headers().keySet()) {

core/src/main/java/feign/Request.java

@@ -24,41 +24,90 @@
 /** An immutable request to an http server. */
 public final class Request {
 
+  public enum HttpMethod {
+    GET,
+    HEAD,
+    POST,
+    PUT,
+    DELETE,
+    CONNECT,
+    OPTIONS,
+    TRACE,
+    PATCH
+  }
+
   /**
    * No parameters can be null except {@code body} and {@code charset}. All parameters must be
    * effectively immutable, via safe copies, not mutating or otherwise.
+   *
+   * @deprecated {@link #create(HttpMethod, String, Map, byte[], Charset)}
    */
   public static Request create(
       String method,
       String url,
       Map<String, Collection<String>> headers,
       byte[] body,
       Charset charset) {
-    return new Request(method, url, headers, body, charset);
+    checkNotNull(method, "httpMethod of %s", method);
+    HttpMethod httpMethod = HttpMethod.valueOf(method.toUpperCase());
+    return create(httpMethod, url, headers, body, charset);
+  }
+
+  /**
+   * Builds a Request. All parameters must be effectively immutable, via safe copies.
+   *
+   * @param httpMethod for the request.
+   * @param url for the request.
+   * @param headers to include.
+   * @param body of the request, can be {@literal null}
+   * @param charset of the request, can be {@literal null}
+   * @return a Request
+   */
+  public static Request create(
+      HttpMethod httpMethod,
+      String url,
+      Map<String, Collection<String>> headers,
+      byte[] body,
+      Charset charset) {
+    return new Request(httpMethod, url, headers, body, charset);
   }
 
-  private final String method;
+  private final HttpMethod httpMethod;
   private final String url;
   private final Map<String, Collection<String>> headers;
   private final byte[] body;
   private final Charset charset;
 
   Request(
-      String method,
+      HttpMethod method,
       String url,
       Map<String, Collection<String>> headers,
       byte[] body,
       Charset charset) {
-    this.method = checkNotNull(method, "method of %s", url);
+    this.httpMethod = checkNotNull(method, "httpMethod of %s", method.name());
     this.url = checkNotNull(url, "url");
     this.headers = checkNotNull(headers, "headers of %s %s", method, url);
     this.body = body; // nullable
     this.charset = charset; // nullable
   }
 
-  /* Method to invoke on the server. */
+  /**
+   * Http Method for this request.
+   *
+   * @return the HttpMethod string
+   * @deprecated @see {@link #httpMethod()}
+   */
   public String method() {
-    return method;
+    return httpMethod.name();
+  }
+
+  /**
+   * Http Method for the request.
+   *
+   * @return the HttpMethod.
+   */
+  public HttpMethod httpMethod() {
+    return this.httpMethod;
   }
 
   /* Fully resolved URL including query. */
@@ -93,7 +142,7 @@ public byte[] body() {
   @Override
   public String toString() {
     StringBuilder builder = new StringBuilder();
-    builder.append(method).append(' ').append(url).append(" HTTP/1.1\n");
+    builder.append(httpMethod).append(' ').append(url).append(" HTTP/1.1\n");
     for (String field : headers.keySet()) {
       for (String value : valuesOrEmpty(headers, field)) {
         builder.append(field).append(": ").append(value).append('\n');

core/src/main/java/feign/RequestTemplate.java

@@ -168,7 +168,7 @@ public static String expand(String template, Map<String, ?> variables) {
   }
 
   private static Map<String, Collection<String>> parseAndDecodeQueries(String queryLine) {
-    Map<String, Collection<String>> map = new LinkedHashMap<String, Collection<String>>();
+    Map<String, Collection<String>> map = new LinkedHashMap<>();
     if (emptyToNull(queryLine) == null) {
       return map;
     }

core/src/main/java/feign/Response.java

@@ -44,11 +44,12 @@ public final class Response implements Closeable {
 
   private Response(Builder builder) {
     checkState(builder.status >= 200, "Invalid status code: %s", builder.status);
+    checkState(builder.request != null, "original request is required");
     this.status = builder.status;
+    this.request = builder.request;
     this.reason = builder.reason; // nullable
     this.headers = Collections.unmodifiableMap(caseInsensitiveCopyOf(builder.headers));
     this.body = builder.body; // nullable
-    this.request = builder.request; // nullable
   }
 
   public Builder toBuilder() {
@@ -134,10 +135,9 @@ public Builder body(String text, Charset charset) {
 
     /**
      * @see Response#request
-     *     <p>NOTE: will add null check in version 10 which may require changes to custom
-     *     feign.Client or loggers
      */
     public Builder request(Request request) {
+      checkNotNull(request, "request is required");
       this.request = request;
       return this;
     }
@@ -175,7 +175,7 @@ public Body body() {
     return body;
   }
 
-  /** if present, the request that generated this response */
+  /** the request that generated this response */
   public Request request() {
     return request;
   }