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;
@@ -78,6 +79,7 @@ public void buildResponse() {
     response = 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

@@ -13,6 +13,16 @@
  */
 package feign.benchmark;
 
+import feign.Logger;
+import feign.Logger.Level;
+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 io.reactivex.netty.server.ErrorHandler;
 import okhttp3.OkHttpClient;
 import okhttp3.Request;
 import org.openjdk.jmh.annotations.Benchmark;
@@ -30,12 +40,7 @@
 import java.util.concurrent.TimeUnit;
 import feign.Feign;
 import feign.Response;
-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 rx.Observable;
 
 @Measurement(iterations = 5, time = 1)
 @Warmup(iterations = 10, time = 1)
@@ -53,17 +58,15 @@ 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()
         .url("http://localhost:" + SERVER_PORT + "/?Action=GetUser&Version=2010-05-08&limit=1")
@@ -89,7 +92,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

@@ -65,7 +65,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 {
@@ -84,7 +84,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 =
@@ -139,7 +139,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();
 
@@ -170,6 +170,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

@@ -13,8 +13,8 @@
  */
 package feign;
 
-import java.io.IOException;
 import static java.lang.String.format;
+import java.io.IOException;
 
 /**
  * Origin exception type for all Http Apis.
@@ -43,7 +43,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()),
+        format("%s reading %s %s", cause.getMessage(), request.httpMethod(), request.url()),
         cause);
   }
 
@@ -61,7 +61,9 @@ 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()), cause,
+        format("%s executing %s %s", cause.getMessage(), request.httpMethod(), request.url()),
+        request.httpMethod(),
+        cause,
         null);
   }
 }

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

@@ -44,7 +44,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

@@ -13,48 +13,89 @@
  */
 package feign;
 
+import static feign.Util.checkNotNull;
+import static feign.Util.valuesOrEmpty;
 import java.net.HttpURLConnection;
 import java.nio.charset.Charset;
 import java.util.Collection;
 import java.util.Map;
-import static feign.Util.checkNotNull;
-import static feign.Util.valuesOrEmpty;
 
 /**
  * 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);
   }
 
-  private final String method;
+  /**
+   * 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 HttpMethod httpMethod;
   private final String url;
   private final Map<String, Collection<String>> headers;
   private final byte[] body;
   private final Charset charset;
 
-  Request(String method, String url, Map<String, Collection<String>> headers, byte[] body,
+  Request(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. */
@@ -89,7 +130,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

@@ -166,7 +166,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

@@ -45,11 +45,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() {
@@ -121,11 +122,9 @@ public Builder body(String text, Charset charset) {
 
     /**
      * @see Response#request
-     *
-     *      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;
     }
@@ -168,7 +167,7 @@ public Body body() {
   }
 
   /**
-   * if present, the request that generated this response
+   * the request that generated this response
    */
   public Request request() {
     return request;