diff --git a/core/src/main/java/feign/AsyncFeign.java b/core/src/main/java/feign/AsyncFeign.java index 38830b423..cbf3cec1f 100644 --- a/core/src/main/java/feign/AsyncFeign.java +++ b/core/src/main/java/feign/AsyncFeign.java @@ -245,6 +245,7 @@ public AsyncFeign internalBuild() { dismiss404, closeAfterDecode, decodeVoid, + decodeErrorResponses, responseInterceptorChain()), AsyncResponseHandler.class, capabilities); diff --git a/core/src/main/java/feign/AsyncResponseHandler.java b/core/src/main/java/feign/AsyncResponseHandler.java index c6c4feb8f..51ba79381 100644 --- a/core/src/main/java/feign/AsyncResponseHandler.java +++ b/core/src/main/java/feign/AsyncResponseHandler.java @@ -38,6 +38,28 @@ class AsyncResponseHandler { boolean closeAfterDecode, boolean decodeVoid, ResponseInterceptor.Chain executionChain) { + this( + logLevel, + logger, + decoder, + errorDecoder, + dismiss404, + closeAfterDecode, + decodeVoid, + false, + executionChain); + } + + AsyncResponseHandler( + Level logLevel, + Logger logger, + Decoder decoder, + ErrorDecoder errorDecoder, + boolean dismiss404, + boolean closeAfterDecode, + boolean decodeVoid, + boolean decodeErrorResponses, + ResponseInterceptor.Chain executionChain) { this.responseHandler = new ResponseHandler( logLevel, @@ -47,6 +69,7 @@ class AsyncResponseHandler { dismiss404, closeAfterDecode, decodeVoid, + decodeErrorResponses, executionChain); } diff --git a/core/src/main/java/feign/BaseBuilder.java b/core/src/main/java/feign/BaseBuilder.java index 81374deb0..d7cb586f2 100644 --- a/core/src/main/java/feign/BaseBuilder.java +++ b/core/src/main/java/feign/BaseBuilder.java @@ -63,6 +63,7 @@ public abstract class BaseBuilder, T> implements Clo protected InvocationHandlerFactory invocationHandlerFactory = new DefaultInvocationHandlerFactory(); protected boolean dismiss404; + protected boolean decodeErrorResponses; protected ExceptionPropagationPolicy propagationPolicy = NONE; protected List capabilities = new ArrayList<>(); @@ -246,6 +247,64 @@ public B decode404() { return thisB(); } + /** + * Returns an error response as a value instead of throwing, when the upstream describes the + * failure in the response body rather than by status alone. + * + *

Some teams answer a failed call with a status in the 4xx/5xx range and a body + * describing what went wrong. By default Feign hands any non-2xx response to the {@link + * #errorDecoder(ErrorDecoder) error decoder} and throws, so that body is unreachable. With this + * flag set, the body is decoded into the method's declared return type and returned: + * + * 

+   * interface MyApi {
+   *   @RequestLine("POST /users")
+   *   BaseResponse createUser(NewUser user);
+   * }
+   *
+   * Feign.builder()
+   *      .decoder(new JacksonDecoder())
+   *      .decodeErrorResponses()
+   *      .target(MyApi.class, "https://api.example.com");
+   *
+ * + *

The flag engages only when every one of the following holds; in any other case the + * response is thrown exactly as it is today: + * + *

    + *
  • the response status is 400 or above — 3xx is left to {@link + * RedirectionInterceptor}, and is not a failure; + *
  • the {@link #decoder(Decoder) decoder} accepts the response, checked via {@link + * feign.codec.PredicatedDecoder#canDecode}, so a decoder that declares itself as JSON will + * not be handed an HTML error page from a proxy. A decoder that is not a {@code + * PredicatedDecoder} declares nothing, so this check cannot be made and is skipped; + *
  • the {@link #errorDecoder(ErrorDecoder) error decoder} did not classify the response as + * {@link RetryableException retryable}. Retryable failures are thrown as before, so {@link + * Retryer} keeps working; + *
  • the body actually decodes. If it does not, the error decoder's exception is thrown, with + * the decode failure attached as {@linkplain Throwable#addSuppressed suppressed}. + *
+ * + *

This flag applies to every method on the client. It does not check that the return + * type is one an error body makes sense as, because there is nothing reliable to check against: + * most decoders ignore unknown properties, so an error body will decode into an unrelated type + * and produce an object with every field null rather than failing. On a client with this flag + * set, a method whose return type is not an error-body shape will return such an object instead + * of throwing. Use a separate client for methods that should keep throwing. + * + *

Note for custom decoders: when this flag engages, the {@link Response} passed to the + * decoder has its status rewritten to {@code 200}. This is deliberate — several decoders, + * including {@code JacksonDecoder} and {@link feign.optionals.OptionalDecoder}, return an empty + * value for 404 and 204 without reading the body, which would discard the very body being asked + * for. A decoder that branches on {@link Response#status()} will therefore not see the real + * status; read it from the response passed to a {@link ResponseInterceptor}, or declare {@code + * TypedResponse}, whose {@link TypedResponse#status()} always reports the true status. + */ + public B decodeErrorResponses() { + this.decodeErrorResponses = true; + return thisB(); + } + public B errorDecoder(ErrorDecoder errorDecoder) { this.errorDecoder = errorDecoder; return thisB(); diff --git a/core/src/main/java/feign/Feign.java b/core/src/main/java/feign/Feign.java index 0f00726a5..3412d67c8 100644 --- a/core/src/main/java/feign/Feign.java +++ b/core/src/main/java/feign/Feign.java @@ -224,6 +224,7 @@ public Feign internalBuild() { dismiss404, closeAfterDecode, decodeVoid, + decodeErrorResponses, responseInterceptorChain()); MethodHandler.Factory methodHandlerFactory = new SynchronousMethodHandler.Factory( diff --git a/core/src/main/java/feign/InvocationContext.java b/core/src/main/java/feign/InvocationContext.java index 1f7151dbe..863da96b6 100755 --- a/core/src/main/java/feign/InvocationContext.java +++ b/core/src/main/java/feign/InvocationContext.java @@ -21,6 +21,7 @@ import feign.codec.DecodeException; import feign.codec.Decoder; import feign.codec.ErrorDecoder; +import feign.codec.PredicatedDecoder; import java.io.IOException; import java.lang.reflect.Type; @@ -32,6 +33,7 @@ public class InvocationContext { private final boolean dismiss404; private final boolean closeAfterDecode; private final boolean decodeVoid; + private final boolean decodeErrorResponses; private final Response response; private final Type returnType; @@ -44,12 +46,35 @@ public class InvocationContext { boolean decodeVoid, Response response, Type returnType) { + this( + configKey, + decoder, + errorDecoder, + dismiss404, + closeAfterDecode, + decodeVoid, + false, + response, + returnType); + } + + InvocationContext( + String configKey, + Decoder decoder, + ErrorDecoder errorDecoder, + boolean dismiss404, + boolean closeAfterDecode, + boolean decodeVoid, + boolean decodeErrorResponses, + Response response, + Type returnType) { this.configKey = configKey; this.decoder = decoder; this.errorDecoder = errorDecoder; this.dismiss404 = dismiss404; this.closeAfterDecode = closeAfterDecode; this.decodeVoid = decodeVoid; + this.decodeErrorResponses = decodeErrorResponses; this.response = response; this.returnType = returnType; } @@ -71,13 +96,26 @@ public Object proceed() throws Exception { return disconnectResponseBodyIfNeeded(response); } + Response response = this.response; try { final boolean shouldDecodeResponseBody = (response.status() >= 200 && response.status() < 300) || (response.status() == 404 && dismiss404 && !isVoidType(returnType)); if (!shouldDecodeResponseBody) { - throw decodeError(configKey, response); + if (!shouldDecodeErrorResponseBody(response)) { + throw decodeError(configKey, response); + } + // Buffer once: both the error decoder and the body decoder below need to read it, and a + // response body is not generally replayable. + response = bufferBody(response); + Exception error = errorDecoder.decode(configKey, response); + if (error instanceof RetryableException) { + // Retryable failures stay exceptions, so Retryer keeps behaving as it does today. + ensureClosed(response.body()); + throw error; + } + return decodeErrorResponseBody(response, error); } if (isVoidType(returnType) && !decodeVoid) { @@ -99,6 +137,60 @@ public Object proceed() throws Exception { } } + /** + * Whether the error body should be returned as a value rather than thrown, per {@link + * feign.BaseBuilder#decodeErrorResponses()}. The return type is deliberately not inspected: the + * method's signature is the declaration of what an error body should decode to. Retryability is + * not checked here either, since it requires reading the body, which the caller only does once it + * knows the flag is in play. + */ + private boolean shouldDecodeErrorResponseBody(Response response) { + if (!decodeErrorResponses || response.status() < 400) { + return false; + } + // A decoder that declares what it handles gets to refuse an HTML error page from a proxy. One + // that declares nothing cannot be asked, so the status range above is the only gate. + return !(decoder instanceof PredicatedDecoder) + || ((PredicatedDecoder) decoder).canDecode(response, returnType); + } + + /** + * Decodes the error body, falling back to throwing {@code error} if it does not decode. The + * decoder is handed a response whose status reads {@code 200}: decoders commonly short-circuit + * 404 and 204 to an empty value without reading the body, which would discard the envelope that + * was asked for. {@link TypedResponse} is still built from the real response, so its status stays + * truthful. + */ + private Object decodeErrorResponseBody(Response response, Exception error) throws Exception { + Response decodable = response.toBuilder().status(200).build(); + Class rawType = Types.getRawType(returnType); + try { + if (TypedResponse.class.isAssignableFrom(rawType)) { + Type bodyType = Types.resolveLastTypeParameter(returnType, TypedResponse.class); + return TypedResponse.builder(response).body(decode(decodable, bodyType)).build(); + } + return decode(decodable, returnType); + } catch (FeignException e) { + if (error == null) { + // A custom ErrorDecoder may return null; the decode failure is then the only diagnosis. + throw e; + } + error.addSuppressed(e); + throw error; + } + } + + private static Response bufferBody(Response response) throws IOException { + if (response.body() == null) { + return response; + } + try { + return response.toBuilder().body(Util.toByteArray(response.body().asInputStream())).build(); + } finally { + ensureClosed(response.body()); + } + } + private static Response disconnectResponseBodyIfNeeded(Response response) throws IOException { final boolean shouldDisconnectResponseBody = response.body() != null diff --git a/core/src/main/java/feign/ResponseHandler.java b/core/src/main/java/feign/ResponseHandler.java index e613280f0..eca6fdbac 100755 --- a/core/src/main/java/feign/ResponseHandler.java +++ b/core/src/main/java/feign/ResponseHandler.java @@ -40,6 +40,8 @@ public class ResponseHandler { private final boolean decodeVoid; + private final boolean decodeErrorResponses; + private final ResponseInterceptor.Chain executionChain; public ResponseHandler( @@ -51,6 +53,28 @@ public ResponseHandler( boolean closeAfterDecode, boolean decodeVoid, ResponseInterceptor.Chain executionChain) { + this( + logLevel, + logger, + decoder, + errorDecoder, + dismiss404, + closeAfterDecode, + decodeVoid, + false, + executionChain); + } + + public ResponseHandler( + Level logLevel, + Logger logger, + Decoder decoder, + ErrorDecoder errorDecoder, + boolean dismiss404, + boolean closeAfterDecode, + boolean decodeVoid, + boolean decodeErrorResponses, + ResponseInterceptor.Chain executionChain) { super(); this.logLevel = logLevel; this.logger = logger; @@ -59,6 +83,7 @@ public ResponseHandler( this.dismiss404 = dismiss404; this.closeAfterDecode = closeAfterDecode; this.decodeVoid = decodeVoid; + this.decodeErrorResponses = decodeErrorResponses; this.executionChain = executionChain; } @@ -74,6 +99,7 @@ public Object handleResponse( dismiss404, closeAfterDecode, decodeVoid, + decodeErrorResponses, response, returnType)); } catch (final IOException e) { diff --git a/core/src/test/java/feign/DecodeErrorResponsesTest.java b/core/src/test/java/feign/DecodeErrorResponsesTest.java new file mode 100644 index 000000000..85058d87f --- /dev/null +++ b/core/src/test/java/feign/DecodeErrorResponsesTest.java @@ -0,0 +1,208 @@ +/* + * Copyright © 2012 The Feign Authors (feign@commonhaus.dev) + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package feign; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; + +import com.google.gson.Gson; +import feign.codec.Decoder; +import feign.codec.PredicatedDecoder; +import java.io.IOException; +import java.lang.reflect.Type; +import java.util.Optional; +import okhttp3.mockwebserver.MockResponse; +import okhttp3.mockwebserver.MockWebServer; +import org.junit.jupiter.api.Test; + +/** Tests for {@link BaseBuilder#decodeErrorResponses()}. */ +class DecodeErrorResponsesTest { + + private static final String ERROR_BODY = + "{\"message\":\"nope\",\"httpStatusCode\":500,\"isFailed\":true,\"errorCode\":42}"; + + public final MockWebServer server = new MockWebServer(); + + interface TestInterface { + @RequestLine("GET /") + BaseResponse get(); + + @RequestLine("GET /") + TypedResponse getTyped(); + + @RequestLine("GET /") + Optional getOptional(); + + @RequestLine("GET /") + Unrelated getUnrelated(); + } + + public static class BaseResponse { + String message; + int httpStatusCode; + Boolean isFailed; + int errorCode; + } + + public static class Unrelated { + String name; + } + + private TestInterface api(Feign.Builder builder) { + return builder + .decoder(new JsonDecoder()) + .retryer(Retryer.NEVER_RETRY) + .target(TestInterface.class, "http://localhost:" + server.getPort()); + } + + private Feign.Builder builder() { + return Feign.builder().decodeErrorResponses(); + } + + private static MockResponse json(int code, String body) { + return new MockResponse() + .setResponseCode(code) + .addHeader("Content-Type", "application/json") + .setBody(body); + } + + @Test + void decodesErrorBodyInsteadOfThrowing() { + server.enqueue(json(500, ERROR_BODY)); + + BaseResponse response = api(builder()).get(); + + assertThat(response.message).isEqualTo("nope"); + assertThat(response.isFailed).isTrue(); + assertThat(response.errorCode).isEqualTo(42); + } + + @Test + void decodesErrorBodyOn404() { + // Regression guard: decoders commonly short-circuit 404 to an empty value without reading the + // body, which would discard the envelope. + server.enqueue(json(404, ERROR_BODY)); + + assertThat(api(builder()).get().message).isEqualTo("nope"); + } + + @Test + void throwsWhenFlagIsNotSet() { + server.enqueue(json(500, ERROR_BODY)); + + assertThatExceptionOfType(FeignException.class).isThrownBy(() -> api(Feign.builder()).get()); + } + + @Test + void appliesToEveryMethodOnTheClient() { + // Documented consequence of the flag being per-client: a method whose return type is not an + // error-body shape decodes the error body anyway, because decoders ignore unknown properties. + // Methods that should keep throwing belong on a separate client. + server.enqueue(json(500, ERROR_BODY)); + + Unrelated response = api(builder()).getUnrelated(); + + assertThat(response).isNotNull(); + assertThat(response.name).isNull(); + } + + @Test + void throwsWhenDecoderDoesNotAcceptTheResponse() { + server.enqueue( + new MockResponse() + .setResponseCode(502) + .addHeader("Content-Type", "text/html") + .setBody("Bad Gateway")); + + assertThatExceptionOfType(FeignException.class).isThrownBy(() -> api(builder()).get()); + } + + @Test + void throwsWhenBodyDoesNotDecode() { + server.enqueue(json(500, "not json at all")); + + assertThatExceptionOfType(FeignException.class) + .isThrownBy(() -> api(builder()).get()) + .satisfies(e -> assertThat(e.status()).isEqualTo(500)) + // the decode failure is kept, so the cause is still diagnosable + .satisfies(e -> assertThat(e.getSuppressed()).hasSize(1)); + } + + @Test + void retryableFailuresStillRetry() { + server.enqueue(json(503, ERROR_BODY).addHeader("Retry-After", "1")); + server.enqueue(json(200, "{\"message\":\"ok\"}")); + + BaseResponse response = + builder() + .decoder(new JsonDecoder()) + .retryer(new DefaultRetryer(1, 1, 2)) + .target(TestInterface.class, "http://localhost:" + server.getPort()) + .get(); + + assertThat(response.message).isEqualTo("ok"); + assertThat(server.getRequestCount()).isEqualTo(2); + } + + @Test + void typedResponseReportsTheRealStatus() { + server.enqueue(json(500, ERROR_BODY)); + + TypedResponse response = api(builder()).getTyped(); + + assertThat(response.status()).isEqualTo(500); + assertThat(response.body().message).isEqualTo("nope"); + } + + @Test + void optionalIsPresentForAnErrorBody() { + server.enqueue(json(500, ERROR_BODY)); + + TestInterface api = + builder() + .decoder(new feign.optionals.OptionalDecoder(new JsonDecoder())) + .retryer(Retryer.NEVER_RETRY) + .target(TestInterface.class, "http://localhost:" + server.getPort()); + + assertThat(api.getOptional()).isPresent().get().extracting("message").isEqualTo("nope"); + } + + @Test + void threeHundredsAreLeftAlone() { + // 304 rather than a redirect: the client follows a Location itself, before Feign sees it. + server.enqueue(json(304, ERROR_BODY)); + + assertThatExceptionOfType(FeignException.class).isThrownBy(() -> api(builder()).get()); + } + + /** A minimal JSON decoder that declares itself, so {@code canDecode} is exercised. */ + static class JsonDecoder implements Decoder, PredicatedDecoder { + private final Gson gson = new Gson(); + + @Override + public boolean canDecode(Response response, Type type) { + return Util.isJsonContentType(response); + } + + @Override + public Object decode(Response response, Type type) throws IOException { + if (response.body() == null) { + return null; + } + return gson.fromJson(response.body().asReader(Util.UTF_8), type); + } + } +}