From 806149c21d8fa0c2844a1939bcd7315d86d9d818 Mon Sep 17 00:00:00 2001 From: Kevin Davis Date: Wed, 18 Jul 2018 13:06:30 -0400 Subject: [PATCH 1/4] Adding Method to Retryable Exception for evaluation 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. --- CHANGELOG.md | 2 ++ README.md | 31 +++++++++++++++++ benchmark/pom.xml | 14 ++++---- .../benchmark/DecoderIteratorsBenchmark.java | 2 ++ .../benchmark/RealRequestBenchmarks.java | 34 +++++++++++-------- core/src/main/java/feign/Client.java | 5 +-- core/src/main/java/feign/FeignException.java | 6 ++-- core/src/main/java/feign/Request.java | 14 +++++--- core/src/main/java/feign/RequestTemplate.java | 2 +- core/src/main/java/feign/Response.java | 12 ++++--- .../main/java/feign/RetryableException.java | 11 ++++-- .../main/java/feign/codec/ErrorDecoder.java | 6 +++- core/src/test/java/feign/FeignTest.java | 6 ++-- core/src/test/java/feign/ResponseTest.java | 3 ++ .../java/feign/client/DefaultClientTest.java | 1 + .../java/feign/codec/DefaultDecoderTest.java | 18 ++++++---- .../feign/codec/DefaultErrorDecoderTest.java | 7 ++++ .../java/feign/stream/StreamDecoderTest.java | 3 ++ .../test/java/feign/gson/GsonCodecTest.java | 8 +++++ .../feign/httpclient/ApacheHttpClient.java | 5 +-- .../jackson/jaxb/JacksonJaxbCodecTest.java | 4 +++ .../java/feign/jackson/JacksonCodecTest.java | 11 ++++++ .../feign/jackson/JacksonIteratorTest.java | 5 +++ .../test/java/feign/jaxb/JAXBCodecTest.java | 5 +++ .../main/java/feign/okhttp/OkHttpClient.java | 14 ++++---- .../test/java/feign/sax/SAXDecoderTest.java | 5 +++ .../java/feign/slf4j/Slf4jLoggerTest.java | 2 ++ 27 files changed, 180 insertions(+), 56 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 220fa35c1..c26ac6596 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/README.md b/README.md index 6e6446dfe..5e6e5dbff 100644 --- a/README.md +++ b/README.md @@ -473,6 +473,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. diff --git a/benchmark/pom.xml b/benchmark/pom.xml index 4857b42e1..b77a3bf83 100644 --- a/benchmark/pom.xml +++ b/benchmark/pom.xml @@ -66,17 +66,17 @@ io.reactivex rxnetty - 0.4.14 + 0.5.1 - io.reactivex - rxjava - 1.0.17 + io.netty + netty-buffer + 4.1.0.Beta7 - io.netty - netty-codec-http - 4.1.0.Beta8 + io.reactivex + rxjava + 1.0.14 org.openjdk.jmh diff --git a/benchmark/src/main/java/feign/benchmark/DecoderIteratorsBenchmark.java b/benchmark/src/main/java/feign/benchmark/DecoderIteratorsBenchmark.java index 79ed9ad71..d8827eb5f 100644 --- a/benchmark/src/main/java/feign/benchmark/DecoderIteratorsBenchmark.java +++ b/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(); diff --git a/benchmark/src/main/java/feign/benchmark/RealRequestBenchmarks.java b/benchmark/src/main/java/feign/benchmark/RealRequestBenchmarks.java index 0fcbd7307..57d3e6c46 100644 --- a/benchmark/src/main/java/feign/benchmark/RealRequestBenchmarks.java +++ b/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() { - public rx.Observable handle(HttpServerRequest request, - HttpServerResponse 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; + } } } diff --git a/core/src/main/java/feign/Client.java b/core/src/main/java/feign/Client.java index 02fb8d599..6bb292891 100644 --- a/core/src/main/java/feign/Client.java +++ b/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).toBuilder().request(request).build(); } HttpURLConnection convertAndSend(Request request, Options options) throws IOException { @@ -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(); } diff --git a/core/src/main/java/feign/FeignException.java b/core/src/main/java/feign/FeignException.java index 59cf7c866..513b2f433 100644 --- a/core/src/main/java/feign/FeignException.java +++ b/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. @@ -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.method(), request.url()), + request.method(), + cause, null); } } diff --git a/core/src/main/java/feign/Request.java b/core/src/main/java/feign/Request.java index f3b2aa12c..294aa17dd 100644 --- a/core/src/main/java/feign/Request.java +++ b/core/src/main/java/feign/Request.java @@ -13,18 +13,22 @@ */ 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 Methods { + 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. @@ -37,7 +41,7 @@ public static Request create(String method, return new Request(method, url, headers, body, charset); } - private final String method; + private final Methods method; private final String url; private final Map> headers; private final byte[] body; @@ -45,7 +49,7 @@ public static Request create(String method, Request(String method, String url, Map> headers, byte[] body, Charset charset) { - this.method = checkNotNull(method, "method of %s", url); + this.method = Methods.valueOf(checkNotNull(method, "method of %s", url)); this.url = checkNotNull(url, "url"); this.headers = checkNotNull(headers, "headers of %s %s", method, url); this.body = body; // nullable @@ -54,7 +58,7 @@ public static Request create(String method, /* Method to invoke on the server. */ public String method() { - return method; + return method.name(); } /* Fully resolved URL including query. */ diff --git a/core/src/main/java/feign/RequestTemplate.java b/core/src/main/java/feign/RequestTemplate.java index f59294f93..a6611ea17 100644 --- a/core/src/main/java/feign/RequestTemplate.java +++ b/core/src/main/java/feign/RequestTemplate.java @@ -166,7 +166,7 @@ public static String expand(String template, Map variables) { } private static Map> parseAndDecodeQueries(String queryLine) { - Map> map = new LinkedHashMap>(); + Map> map = new LinkedHashMap<>(); if (emptyToNull(queryLine) == null) { return map; } diff --git a/core/src/main/java/feign/Response.java b/core/src/main/java/feign/Response.java index 661bb80cd..195156006 100644 --- a/core/src/main/java/feign/Response.java +++ b/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,12 +122,13 @@ 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) { - this.request = request; + checkNotNull(request, "the original request is required on all responses"); + + /* don't keep the body, we don't want to tie up memory on large requests */ + this.request = Request.create( + request.method(), request.url(), request.headers(), null, request.charset()); return this; } diff --git a/core/src/main/java/feign/RetryableException.java b/core/src/main/java/feign/RetryableException.java index 79b8eafd9..5e04087fe 100644 --- a/core/src/main/java/feign/RetryableException.java +++ b/core/src/main/java/feign/RetryableException.java @@ -24,20 +24,23 @@ public class RetryableException extends FeignException { private static final long serialVersionUID = 1L; private final Long retryAfter; + private final String method; /** * @param retryAfter usually corresponds to the {@link feign.Util#RETRY_AFTER} header. */ - public RetryableException(String message, Throwable cause, Date retryAfter) { + public RetryableException(String message, String method, Throwable cause, Date retryAfter) { super(message, cause); + this.method = method; this.retryAfter = retryAfter != null ? retryAfter.getTime() : null; } /** * @param retryAfter usually corresponds to the {@link feign.Util#RETRY_AFTER} header. */ - public RetryableException(String message, Date retryAfter) { + public RetryableException(String message, String method, Date retryAfter) { super(message); + this.method = method; this.retryAfter = retryAfter != null ? retryAfter.getTime() : null; } @@ -48,4 +51,8 @@ public RetryableException(String message, Date retryAfter) { public Date retryAfter() { return retryAfter != null ? new Date(retryAfter) : null; } + + public String method() { + return this.method; + } } diff --git a/core/src/main/java/feign/codec/ErrorDecoder.java b/core/src/main/java/feign/codec/ErrorDecoder.java index 6482203f8..841b5bc16 100644 --- a/core/src/main/java/feign/codec/ErrorDecoder.java +++ b/core/src/main/java/feign/codec/ErrorDecoder.java @@ -93,7 +93,11 @@ public Exception decode(String methodKey, Response response) { FeignException exception = errorStatus(methodKey, response); Date retryAfter = retryAfterDecoder.apply(firstOrNull(response.headers(), RETRY_AFTER)); if (retryAfter != null) { - return new RetryableException(exception.getMessage(), exception, retryAfter); + return new RetryableException( + exception.getMessage(), + response.request().method(), + exception, + retryAfter); } return exception; } diff --git a/core/src/test/java/feign/FeignTest.java b/core/src/test/java/feign/FeignTest.java index 0ddcf744a..25cbfe217 100644 --- a/core/src/test/java/feign/FeignTest.java +++ b/core/src/test/java/feign/FeignTest.java @@ -483,7 +483,7 @@ public void retryableExceptionInDecoder() throws Exception { public Object decode(Response response, Type type) throws IOException { String string = super.decode(response, type).toString(); if ("retry!".equals(string)) { - throw new RetryableException(string, null); + throw new RetryableException(string, "post", null); } return string; } @@ -524,7 +524,7 @@ public void ensureRetryerClonesItself() { .errorDecoder(new ErrorDecoder() { @Override public Exception decode(String methodKey, Response response) { - return new RetryableException("play it again sam!", null); + return new RetryableException("play it again sam!", "post", null); } }).target(TestInterface.class, "http://localhost:" + server.getPort()); @@ -541,6 +541,7 @@ public void whenReturnTypeIsResponseNoErrorHandling() { .status(302) .reason("Found") .headers(headers) + .request(Request.create("GET", "/", Collections.emptyMap(), null, Util.UTF_8)) .body(new byte[0]) .build(); @@ -740,6 +741,7 @@ private Response responseWithText(String text) { return Response.builder() .body(text, Util.UTF_8) .status(200) + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(new HashMap>()) .build(); } diff --git a/core/src/test/java/feign/ResponseTest.java b/core/src/test/java/feign/ResponseTest.java index ae5ee00e3..1d80c3f42 100644 --- a/core/src/test/java/feign/ResponseTest.java +++ b/core/src/test/java/feign/ResponseTest.java @@ -30,6 +30,7 @@ public void reasonPhraseIsOptional() { Response response = Response.builder() .status(200) .headers(Collections.>emptyMap()) + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .body(new byte[0]) .build(); @@ -45,6 +46,7 @@ public void canAccessHeadersCaseInsensitively() { Response response = Response.builder() .status(200) .headers(headersMap) + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .body(new byte[0]) .build(); assertThat(response.headers().get("content-type")).isEqualTo(valueList); @@ -60,6 +62,7 @@ public void headerValuesWithSameNameOnlyVaryingInCaseAreMerged() { Response response = Response.builder() .status(200) .headers(headersMap) + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .body(new byte[0]) .build(); diff --git a/core/src/test/java/feign/client/DefaultClientTest.java b/core/src/test/java/feign/client/DefaultClientTest.java index 3ebb9b3f6..c14675d60 100644 --- a/core/src/test/java/feign/client/DefaultClientTest.java +++ b/core/src/test/java/feign/client/DefaultClientTest.java @@ -13,6 +13,7 @@ */ package feign.client; +import feign.FeignException; import java.io.IOException; import java.net.ProtocolException; import javax.net.ssl.HostnameVerifier; diff --git a/core/src/test/java/feign/codec/DefaultDecoderTest.java b/core/src/test/java/feign/codec/DefaultDecoderTest.java index d646d53f2..99a827834 100644 --- a/core/src/test/java/feign/codec/DefaultDecoderTest.java +++ b/core/src/test/java/feign/codec/DefaultDecoderTest.java @@ -13,20 +13,22 @@ */ package feign.codec; -import org.junit.Rule; -import org.junit.Test; -import org.junit.rules.ExpectedException; -import org.w3c.dom.Document; +import static feign.Util.UTF_8; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; import java.io.ByteArrayInputStream; import java.io.InputStream; import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.Map; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; +import org.w3c.dom.Document; +import feign.Request; import feign.Response; -import static feign.Util.UTF_8; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNull; +import feign.Util; public class DefaultDecoderTest { @@ -73,6 +75,7 @@ private Response knownResponse() { .status(200) .reason("OK") .headers(headers) + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .body(inputStream, content.length()) .build(); } @@ -82,6 +85,7 @@ private Response nullBodyResponse() { .status(200) .reason("OK") .headers(Collections.>emptyMap()) + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .build(); } } diff --git a/core/src/test/java/feign/codec/DefaultErrorDecoderTest.java b/core/src/test/java/feign/codec/DefaultErrorDecoderTest.java index 417297b68..e2e3103c2 100644 --- a/core/src/test/java/feign/codec/DefaultErrorDecoderTest.java +++ b/core/src/test/java/feign/codec/DefaultErrorDecoderTest.java @@ -13,6 +13,9 @@ */ package feign.codec; +import feign.Request; +import feign.Util; +import java.util.Collections; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -43,6 +46,7 @@ public void throwsFeignException() throws Throwable { Response response = Response.builder() .status(500) .reason("Internal server error") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(headers) .build(); @@ -57,6 +61,7 @@ public void throwsFeignExceptionIncludingBody() throws Throwable { Response response = Response.builder() .status(500) .reason("Internal server error") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(headers) .body("hello world", UTF_8) .build(); @@ -69,6 +74,7 @@ public void testFeignExceptionIncludesStatus() throws Throwable { Response response = Response.builder() .status(400) .reason("Bad request") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(headers) .build(); @@ -87,6 +93,7 @@ public void retryAfterHeaderThrowsRetryableException() throws Throwable { Response response = Response.builder() .status(503) .reason("Service Unavailable") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(headers) .build(); diff --git a/core/src/test/java/feign/stream/StreamDecoderTest.java b/core/src/test/java/feign/stream/StreamDecoderTest.java index 909a5d2d1..3bf5fc725 100644 --- a/core/src/test/java/feign/stream/StreamDecoderTest.java +++ b/core/src/test/java/feign/stream/StreamDecoderTest.java @@ -16,8 +16,10 @@ import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.ObjectMapper; import feign.Feign; +import feign.Request; import feign.RequestLine; import feign.Response; +import feign.Util; import java.io.BufferedReader; import java.io.Closeable; import java.io.IOException; @@ -81,6 +83,7 @@ public void shouldCloseIteratorWhenStreamClosed() throws IOException { .status(200) .reason("OK") .headers(Collections.emptyMap()) + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .body("", UTF_8) .build(); diff --git a/gson/src/test/java/feign/gson/GsonCodecTest.java b/gson/src/test/java/feign/gson/GsonCodecTest.java index ddfc47038..4efe8d619 100644 --- a/gson/src/test/java/feign/gson/GsonCodecTest.java +++ b/gson/src/test/java/feign/gson/GsonCodecTest.java @@ -17,6 +17,8 @@ import com.google.gson.reflect.TypeToken; import com.google.gson.stream.JsonReader; import com.google.gson.stream.JsonWriter; +import feign.Request; +import feign.Util; import org.junit.Test; import java.io.IOException; import java.util.Arrays; @@ -57,6 +59,7 @@ public void decodesMapObjectNumericalValuesAsInteger() throws Exception { Response response = Response.builder() .status(200) .reason("OK") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(Collections.>emptyMap()) .body("{\"foo\": 1}", UTF_8) .build(); @@ -115,6 +118,7 @@ public void decodes() throws Exception { .status(200) .reason("OK") .headers(Collections.>emptyMap()) + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .body(zonesJson, UTF_8) .build(); assertEquals(zones, @@ -127,6 +131,7 @@ public void nullBodyDecodesToNull() throws Exception { .status(204) .reason("OK") .headers(Collections.>emptyMap()) + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .build(); assertNull(new GsonDecoder().decode(response, String.class)); } @@ -137,6 +142,7 @@ public void emptyBodyDecodesToNull() throws Exception { .status(204) .reason("OK") .headers(Collections.>emptyMap()) + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .body(new byte[0]) .build(); assertNull(new GsonDecoder().decode(response, String.class)); @@ -189,6 +195,7 @@ public void customDecoder() throws Exception { .status(200) .reason("OK") .headers(Collections.>emptyMap()) + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .body(zonesJson, UTF_8) .build(); assertEquals(zones, decoder.decode(response, new TypeToken>() {}.getType())); @@ -224,6 +231,7 @@ public void notFoundDecodesToEmpty() throws Exception { .status(404) .reason("NOT FOUND") .headers(Collections.>emptyMap()) + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .build(); assertThat((byte[]) new GsonDecoder().decode(response, byte[].class)).isEmpty(); } diff --git a/httpclient/src/main/java/feign/httpclient/ApacheHttpClient.java b/httpclient/src/main/java/feign/httpclient/ApacheHttpClient.java index 6766b0cc2..0a16ac619 100644 --- a/httpclient/src/main/java/feign/httpclient/ApacheHttpClient.java +++ b/httpclient/src/main/java/feign/httpclient/ApacheHttpClient.java @@ -82,7 +82,7 @@ public Response execute(Request request, Request.Options options) throws IOExcep throw new IOException("URL '" + request.url() + "' couldn't be parsed into a URI", e); } HttpResponse httpResponse = client.execute(httpUriRequest); - return toFeignResponse(httpResponse).toBuilder().request(request).build(); + return toFeignResponse(httpResponse, request); } HttpUriRequest toHttpUriRequest(Request request, Request.Options options) @@ -167,7 +167,7 @@ private ContentType getContentType(Request request) { return contentType; } - Response toFeignResponse(HttpResponse httpResponse) throws IOException { + Response toFeignResponse(HttpResponse httpResponse, Request request) throws IOException { StatusLine statusLine = httpResponse.getStatusLine(); int statusCode = statusLine.getStatusCode(); @@ -190,6 +190,7 @@ Response toFeignResponse(HttpResponse httpResponse) throws IOException { .status(statusCode) .reason(reason) .headers(headers) + .request(request) .body(toFeignBody(httpResponse)) .build(); } diff --git a/jackson-jaxb/src/test/java/feign/jackson/jaxb/JacksonJaxbCodecTest.java b/jackson-jaxb/src/test/java/feign/jackson/jaxb/JacksonJaxbCodecTest.java index ccd9533fd..96628b81e 100644 --- a/jackson-jaxb/src/test/java/feign/jackson/jaxb/JacksonJaxbCodecTest.java +++ b/jackson-jaxb/src/test/java/feign/jackson/jaxb/JacksonJaxbCodecTest.java @@ -13,6 +13,8 @@ */ package feign.jackson.jaxb; +import feign.Request; +import feign.Util; import org.junit.Test; import java.util.Collection; import java.util.Collections; @@ -42,6 +44,7 @@ public void decodeTest() throws Exception { Response response = Response.builder() .status(200) .reason("OK") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(Collections.>emptyMap()) .body("{\"value\":\"Test\"}", UTF_8) .build(); @@ -57,6 +60,7 @@ public void notFoundDecodesToEmpty() throws Exception { Response response = Response.builder() .status(404) .reason("NOT FOUND") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(Collections.>emptyMap()) .build(); assertThat((byte[]) new JacksonJaxbJsonDecoder().decode(response, byte[].class)).isEmpty(); diff --git a/jackson/src/test/java/feign/jackson/JacksonCodecTest.java b/jackson/src/test/java/feign/jackson/JacksonCodecTest.java index 3aca54cbe..12db55cc5 100644 --- a/jackson/src/test/java/feign/jackson/JacksonCodecTest.java +++ b/jackson/src/test/java/feign/jackson/JacksonCodecTest.java @@ -23,6 +23,8 @@ import com.fasterxml.jackson.databind.deser.std.StdDeserializer; import com.fasterxml.jackson.databind.module.SimpleModule; import com.fasterxml.jackson.databind.ser.std.StdSerializer; +import feign.Request; +import feign.Util; import org.junit.Test; import java.io.Closeable; import java.io.IOException; @@ -95,6 +97,7 @@ public void decodes() throws Exception { Response response = Response.builder() .status(200) .reason("OK") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(Collections.>emptyMap()) .body(zonesJson, UTF_8) .build(); @@ -107,6 +110,7 @@ public void nullBodyDecodesToNull() throws Exception { Response response = Response.builder() .status(204) .reason("OK") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(Collections.>emptyMap()) .build(); assertNull(new JacksonDecoder().decode(response, String.class)); @@ -117,6 +121,7 @@ public void emptyBodyDecodesToNull() throws Exception { Response response = Response.builder() .status(204) .reason("OK") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(Collections.>emptyMap()) .body(new byte[0]) .build(); @@ -136,6 +141,7 @@ public void customDecoder() throws Exception { Response response = Response.builder() .status(200) .reason("OK") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(Collections.>emptyMap()) .body(zonesJson, UTF_8) .build(); @@ -172,6 +178,7 @@ public void decodesIterator() throws Exception { Response response = Response.builder() .status(200) .reason("OK") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(Collections.>emptyMap()) .body(zonesJson, UTF_8) .build(); @@ -194,6 +201,7 @@ public void nullBodyDecodesToNullIterator() throws Exception { Response response = Response.builder() .status(204) .reason("OK") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(Collections.>emptyMap()) .build(); assertNull(JacksonIteratorDecoder.create().decode(response, Iterator.class)); @@ -204,6 +212,7 @@ public void emptyBodyDecodesToNullIterator() throws Exception { Response response = Response.builder() .status(204) .reason("OK") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(Collections.>emptyMap()) .body(new byte[0]) .build(); @@ -275,6 +284,7 @@ public void notFoundDecodesToEmpty() throws Exception { Response response = Response.builder() .status(404) .reason("NOT FOUND") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(Collections.>emptyMap()) .build(); assertThat((byte[]) new JacksonDecoder().decode(response, byte[].class)).isEmpty(); @@ -286,6 +296,7 @@ public void notFoundDecodesToEmptyIterator() throws Exception { Response response = Response.builder() .status(404) .reason("NOT FOUND") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(Collections.>emptyMap()) .build(); assertThat((byte[]) JacksonIteratorDecoder.create().decode(response, byte[].class)).isEmpty(); diff --git a/jackson/src/test/java/feign/jackson/JacksonIteratorTest.java b/jackson/src/test/java/feign/jackson/JacksonIteratorTest.java index 7ba81aa21..5fe83a635 100644 --- a/jackson/src/test/java/feign/jackson/JacksonIteratorTest.java +++ b/jackson/src/test/java/feign/jackson/JacksonIteratorTest.java @@ -14,7 +14,9 @@ package feign.jackson; import com.fasterxml.jackson.databind.ObjectMapper; +import feign.Request; import feign.Response; +import feign.Util; import feign.codec.DecodeException; import feign.jackson.JacksonIteratorDecoder.JacksonIterator; import org.junit.Rule; @@ -86,6 +88,7 @@ public void close() throws IOException { Response response = Response.builder() .status(200) .reason("OK") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(Collections.>emptyMap()) .body(inputStream, jsonBytes.length) .build(); @@ -109,6 +112,7 @@ public void close() throws IOException { Response response = Response.builder() .status(200) .reason("OK") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(Collections.>emptyMap()) .body(inputStream, jsonBytes.length) .build(); @@ -138,6 +142,7 @@ JacksonIterator iterator(Class type, String json) throws IOException { Response response = Response.builder() .status(200) .reason("OK") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(Collections.>emptyMap()) .body(json, UTF_8) .build(); diff --git a/jaxb/src/test/java/feign/jaxb/JAXBCodecTest.java b/jaxb/src/test/java/feign/jaxb/JAXBCodecTest.java index 915de3178..c784d70e6 100644 --- a/jaxb/src/test/java/feign/jaxb/JAXBCodecTest.java +++ b/jaxb/src/test/java/feign/jaxb/JAXBCodecTest.java @@ -13,6 +13,8 @@ */ package feign.jaxb; +import feign.Request; +import feign.Util; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -164,6 +166,7 @@ public void decodesXml() throws Exception { Response response = Response.builder() .status(200) .reason("OK") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(Collections.>emptyMap()) .body(mockXml, UTF_8) .build(); @@ -188,6 +191,7 @@ class ParameterizedHolder { Response response = Response.builder() .status(200) .reason("OK") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(Collections.>emptyMap()) .body("", UTF_8) .build(); @@ -201,6 +205,7 @@ public void notFoundDecodesToEmpty() throws Exception { Response response = Response.builder() .status(404) .reason("NOT FOUND") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(Collections.>emptyMap()) .build(); assertThat((byte[]) new JAXBDecoder(new JAXBContextFactory.Builder().build()) diff --git a/okhttp/src/main/java/feign/okhttp/OkHttpClient.java b/okhttp/src/main/java/feign/okhttp/OkHttpClient.java index 7eae32e2d..94064c709 100644 --- a/okhttp/src/main/java/feign/okhttp/OkHttpClient.java +++ b/okhttp/src/main/java/feign/okhttp/OkHttpClient.java @@ -90,12 +90,14 @@ static Request toOkHttpRequest(feign.Request input) { return requestBuilder.build(); } - private static feign.Response toFeignResponse(Response input) throws IOException { + private static feign.Response toFeignResponse(Response response, feign.Request request) + throws IOException { return feign.Response.builder() - .status(input.code()) - .reason(input.message()) - .headers(toMap(input.headers())) - .body(toBody(input.body())) + .status(response.code()) + .reason(response.message()) + .request(request) + .headers(toMap(response.headers())) + .body(toBody(response.body())) .build(); } @@ -159,6 +161,6 @@ public feign.Response execute(feign.Request input, feign.Request.Options options } Request request = toOkHttpRequest(input); Response response = requestScoped.newCall(request).execute(); - return toFeignResponse(response).toBuilder().request(input).build(); + return toFeignResponse(response, input).toBuilder().request(input).build(); } } diff --git a/sax/src/test/java/feign/sax/SAXDecoderTest.java b/sax/src/test/java/feign/sax/SAXDecoderTest.java index c57f88028..3755fec15 100644 --- a/sax/src/test/java/feign/sax/SAXDecoderTest.java +++ b/sax/src/test/java/feign/sax/SAXDecoderTest.java @@ -13,6 +13,8 @@ */ package feign.sax; +import feign.Request; +import feign.Util; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -72,6 +74,7 @@ private Response statusFailedResponse() { return Response.builder() .status(200) .reason("OK") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(Collections.>emptyMap()) .body(statusFailed, UTF_8) .build(); @@ -82,6 +85,7 @@ public void nullBodyDecodesToNull() throws Exception { Response response = Response.builder() .status(204) .reason("OK") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(Collections.>emptyMap()) .build(); assertNull(decoder.decode(response, String.class)); @@ -93,6 +97,7 @@ public void notFoundDecodesToEmpty() throws Exception { Response response = Response.builder() .status(404) .reason("NOT FOUND") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(Collections.>emptyMap()) .build(); assertThat((byte[]) decoder.decode(response, byte[].class)).isEmpty(); diff --git a/slf4j/src/test/java/feign/slf4j/Slf4jLoggerTest.java b/slf4j/src/test/java/feign/slf4j/Slf4jLoggerTest.java index e4ebb7bd2..b046e27bc 100644 --- a/slf4j/src/test/java/feign/slf4j/Slf4jLoggerTest.java +++ b/slf4j/src/test/java/feign/slf4j/Slf4jLoggerTest.java @@ -13,6 +13,7 @@ */ package feign.slf4j; +import feign.Util; import org.junit.Rule; import org.junit.Test; import org.slf4j.LoggerFactory; @@ -33,6 +34,7 @@ public class Slf4jLoggerTest { Response.builder() .status(200) .reason("OK") + .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8)) .headers(Collections.>emptyMap()) .body(new byte[0]) .build(); From 5993c5e75f44da6e039e630ee9db9b5e018c27f9 Mon Sep 17 00:00:00 2001 From: Kevin Davis Date: Fri, 20 Jul 2018 12:20:42 -0400 Subject: [PATCH 2/4] 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()` --- core/src/main/java/feign/Client.java | 4 +- core/src/main/java/feign/FeignException.java | 6 +-- core/src/main/java/feign/Logger.java | 2 +- core/src/main/java/feign/Request.java | 53 ++++++++++++++++--- core/src/main/java/feign/Response.java | 2 +- .../main/java/feign/RetryableException.java | 15 +++--- .../main/java/feign/codec/ErrorDecoder.java | 2 +- core/src/test/java/feign/FeignTest.java | 5 +- .../java/feign/client/DefaultClientTest.java | 9 ++-- 9 files changed, 70 insertions(+), 28 deletions(-) diff --git a/core/src/main/java/feign/Client.java b/core/src/main/java/feign/Client.java index 6bb292891..431711486 100644 --- a/core/src/main/java/feign/Client.java +++ b/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, request).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 contentEncodingValues = request.headers().get(CONTENT_ENCODING); boolean gzipEncodedRequest = diff --git a/core/src/main/java/feign/FeignException.java b/core/src/main/java/feign/FeignException.java index 513b2f433..b4b39e62a 100644 --- a/core/src/main/java/feign/FeignException.java +++ b/core/src/main/java/feign/FeignException.java @@ -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().name(), request.url()), cause); } @@ -61,8 +61,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()), - request.method(), + format("%s executing %s %s", cause.getMessage(), request.httpMethod(), request.url()), + request.httpMethod(), cause, null); } diff --git a/core/src/main/java/feign/Logger.java b/core/src/main/java/feign/Logger.java index e34dd551c..dd4f99e1d 100644 --- a/core/src/main/java/feign/Logger.java +++ b/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()) { diff --git a/core/src/main/java/feign/Request.java b/core/src/main/java/feign/Request.java index 294aa17dd..e3a37e919 100644 --- a/core/src/main/java/feign/Request.java +++ b/core/src/main/java/feign/Request.java @@ -25,40 +25,77 @@ */ public final class Request { - public enum Methods { + 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> 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 Methods 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> headers, + byte[] body, + Charset charset) { + return new Request(httpMethod, url, headers, body, charset); + + } + + private final HttpMethod httpMethod; private final String url; private final Map> headers; private final byte[] body; private final Charset charset; - Request(String method, String url, Map> headers, byte[] body, + Request(HttpMethod method, String url, Map> headers, byte[] body, Charset charset) { - this.method = Methods.valueOf(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.name(); + return httpMethod.name(); + } + + /** + * Http Method for the request. + * + * @return the HttpMethod. + */ + public HttpMethod httpMethod() { + return this.httpMethod; } /* Fully resolved URL including query. */ @@ -93,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'); diff --git a/core/src/main/java/feign/Response.java b/core/src/main/java/feign/Response.java index 195156006..4b0545aa0 100644 --- a/core/src/main/java/feign/Response.java +++ b/core/src/main/java/feign/Response.java @@ -128,7 +128,7 @@ public Builder request(Request request) { /* don't keep the body, we don't want to tie up memory on large requests */ this.request = Request.create( - request.method(), request.url(), request.headers(), null, request.charset()); + request.httpMethod(), request.url(), request.headers(), null, request.charset()); return this; } diff --git a/core/src/main/java/feign/RetryableException.java b/core/src/main/java/feign/RetryableException.java index 5e04087fe..cc2e1b702 100644 --- a/core/src/main/java/feign/RetryableException.java +++ b/core/src/main/java/feign/RetryableException.java @@ -13,6 +13,7 @@ */ package feign; +import feign.Request.HttpMethod; import java.util.Date; /** @@ -24,23 +25,23 @@ public class RetryableException extends FeignException { private static final long serialVersionUID = 1L; private final Long retryAfter; - private final String method; + private final HttpMethod httpMethod; /** * @param retryAfter usually corresponds to the {@link feign.Util#RETRY_AFTER} header. */ - public RetryableException(String message, String method, Throwable cause, Date retryAfter) { + public RetryableException(String message, HttpMethod httpMethod, Throwable cause, Date retryAfter) { super(message, cause); - this.method = method; + this.httpMethod = httpMethod; this.retryAfter = retryAfter != null ? retryAfter.getTime() : null; } /** * @param retryAfter usually corresponds to the {@link feign.Util#RETRY_AFTER} header. */ - public RetryableException(String message, String method, Date retryAfter) { + public RetryableException(String message, HttpMethod httpMethod, Date retryAfter) { super(message); - this.method = method; + this.httpMethod = httpMethod; this.retryAfter = retryAfter != null ? retryAfter.getTime() : null; } @@ -52,7 +53,7 @@ public Date retryAfter() { return retryAfter != null ? new Date(retryAfter) : null; } - public String method() { - return this.method; + public HttpMethod method() { + return this.httpMethod; } } diff --git a/core/src/main/java/feign/codec/ErrorDecoder.java b/core/src/main/java/feign/codec/ErrorDecoder.java index 841b5bc16..2da7aefba 100644 --- a/core/src/main/java/feign/codec/ErrorDecoder.java +++ b/core/src/main/java/feign/codec/ErrorDecoder.java @@ -95,7 +95,7 @@ public Exception decode(String methodKey, Response response) { if (retryAfter != null) { return new RetryableException( exception.getMessage(), - response.request().method(), + response.request().httpMethod(), exception, retryAfter); } diff --git a/core/src/test/java/feign/FeignTest.java b/core/src/test/java/feign/FeignTest.java index 25cbfe217..e3cc35b4b 100644 --- a/core/src/test/java/feign/FeignTest.java +++ b/core/src/test/java/feign/FeignTest.java @@ -15,6 +15,7 @@ import com.google.gson.Gson; import com.google.gson.reflect.TypeToken; +import feign.Request.HttpMethod; import okhttp3.mockwebserver.MockResponse; import okhttp3.mockwebserver.SocketPolicy; import okhttp3.mockwebserver.MockWebServer; @@ -483,7 +484,7 @@ public void retryableExceptionInDecoder() throws Exception { public Object decode(Response response, Type type) throws IOException { String string = super.decode(response, type).toString(); if ("retry!".equals(string)) { - throw new RetryableException(string, "post", null); + throw new RetryableException(string, HttpMethod.POST, null); } return string; } @@ -524,7 +525,7 @@ public void ensureRetryerClonesItself() { .errorDecoder(new ErrorDecoder() { @Override public Exception decode(String methodKey, Response response) { - return new RetryableException("play it again sam!", "post", null); + return new RetryableException("play it again sam!", HttpMethod.POST, null); } }).target(TestInterface.class, "http://localhost:" + server.getPort()); diff --git a/core/src/test/java/feign/client/DefaultClientTest.java b/core/src/test/java/feign/client/DefaultClientTest.java index c14675d60..4e4bda106 100644 --- a/core/src/test/java/feign/client/DefaultClientTest.java +++ b/core/src/test/java/feign/client/DefaultClientTest.java @@ -13,20 +13,23 @@ */ package feign.client; -import feign.FeignException; +import static org.hamcrest.core.Is.isA; +import static org.junit.Assert.assertEquals; + import java.io.IOException; import java.net.ProtocolException; + import javax.net.ssl.HostnameVerifier; import javax.net.ssl.SSLSession; + import org.junit.Test; + import feign.Client; import feign.Feign; import feign.Feign.Builder; import feign.RetryableException; import okhttp3.mockwebserver.MockResponse; import okhttp3.mockwebserver.SocketPolicy; -import static org.hamcrest.core.Is.isA; -import static org.junit.Assert.assertEquals; /** * Tests client-specific behavior, such as ensuring Content-Length is sent when specified. From e918aca5467378a4a2859bd97cf21016777c2dc1 Mon Sep 17 00:00:00 2001 From: "Davis, Kevin" Date: Sat, 21 Jul 2018 10:14:08 -0400 Subject: [PATCH 3/4] Adding original Reqeust body to the Response --- core/src/main/java/feign/Response.java | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/feign/Response.java b/core/src/main/java/feign/Response.java index 4b0545aa0..366f27518 100644 --- a/core/src/main/java/feign/Response.java +++ b/core/src/main/java/feign/Response.java @@ -124,11 +124,8 @@ public Builder body(String text, Charset charset) { * @see Response#request */ public Builder request(Request request) { - checkNotNull(request, "the original request is required on all responses"); - - /* don't keep the body, we don't want to tie up memory on large requests */ - this.request = Request.create( - request.httpMethod(), request.url(), request.headers(), null, request.charset()); + checkNotNull(request, "request is required"); + this.request = request; return this; } @@ -170,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; From bf3df0fe0a8b24a6c8fd82d51d49eabce288db32 Mon Sep 17 00:00:00 2001 From: "Davis, Kevin" Date: Mon, 23 Jul 2018 20:20:18 -0400 Subject: [PATCH 4/4] Removing unnecessary call to HttpMethods.name There is no need to have an explicit call to `Enum.name`. Removed. Style Compliance. --- core/src/main/java/feign/FeignException.java | 2 +- core/src/main/java/feign/Request.java | 2 +- core/src/main/java/feign/RetryableException.java | 3 ++- core/src/test/java/feign/client/DefaultClientTest.java | 4 ---- 4 files changed, 4 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/feign/FeignException.java b/core/src/main/java/feign/FeignException.java index b4b39e62a..95d55a9c7 100644 --- a/core/src/main/java/feign/FeignException.java +++ b/core/src/main/java/feign/FeignException.java @@ -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.httpMethod().name(), request.url()), + format("%s reading %s %s", cause.getMessage(), request.httpMethod(), request.url()), cause); } diff --git a/core/src/main/java/feign/Request.java b/core/src/main/java/feign/Request.java index e3a37e919..3bec9cf6d 100644 --- a/core/src/main/java/feign/Request.java +++ b/core/src/main/java/feign/Request.java @@ -46,7 +46,7 @@ public static Request create(String method, } /** - * Builds a Request. All parameters must be effectively immutable, via safe copies. + * Builds a Request. All parameters must be effectively immutable, via safe copies. * * @param httpMethod for the request. * @param url for the request. diff --git a/core/src/main/java/feign/RetryableException.java b/core/src/main/java/feign/RetryableException.java index cc2e1b702..8fd32c432 100644 --- a/core/src/main/java/feign/RetryableException.java +++ b/core/src/main/java/feign/RetryableException.java @@ -30,7 +30,8 @@ public class RetryableException extends FeignException { /** * @param retryAfter usually corresponds to the {@link feign.Util#RETRY_AFTER} header. */ - public RetryableException(String message, HttpMethod httpMethod, Throwable cause, Date retryAfter) { + public RetryableException(String message, HttpMethod httpMethod, Throwable cause, + Date retryAfter) { super(message, cause); this.httpMethod = httpMethod; this.retryAfter = retryAfter != null ? retryAfter.getTime() : null; diff --git a/core/src/test/java/feign/client/DefaultClientTest.java b/core/src/test/java/feign/client/DefaultClientTest.java index 4e4bda106..1eef8e6c8 100644 --- a/core/src/test/java/feign/client/DefaultClientTest.java +++ b/core/src/test/java/feign/client/DefaultClientTest.java @@ -15,15 +15,11 @@ import static org.hamcrest.core.Is.isA; import static org.junit.Assert.assertEquals; - import java.io.IOException; import java.net.ProtocolException; - import javax.net.ssl.HostnameVerifier; import javax.net.ssl.SSLSession; - import org.junit.Test; - import feign.Client; import feign.Feign; import feign.Feign.Builder;