Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

All decoders follow rule: if a status is 404 it returns empty or null value #1597

Merged
merged 2 commits into from
Mar 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions core/src/main/java/feign/AsyncFeign.java
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public static class AsyncBuilder<C> {

private Decoder decoder = new Decoder.Default();
private ErrorDecoder errorDecoder = new ErrorDecoder.Default();
private boolean decode404;
private boolean dismiss404;
private boolean closeAfterDecode = true;

public AsyncBuilder() {
Expand Down Expand Up @@ -104,9 +104,18 @@ public AsyncBuilder<C> decoder(Decoder decoder) {

/**
* @see Builder#decode404()
* @deprecated
*/
public AsyncBuilder<C> decode404() {
this.decode404 = true;
this.dismiss404 = true;
return this;
}

/**
* @see Builder#dismiss404()
*/
public AsyncBuilder<C> dismiss404() {
this.dismiss404 = true;
return this;
}

Expand Down Expand Up @@ -256,7 +265,7 @@ protected AsyncFeign(AsyncBuilder<C> asyncBuilder) {
asyncBuilder.logger,
asyncBuilder.decoder,
asyncBuilder.errorDecoder,
asyncBuilder.decode404,
asyncBuilder.dismiss404,
asyncBuilder.closeAfterDecode);

asyncBuilder.builder.client(this::stageExecution);
Expand Down
8 changes: 4 additions & 4 deletions core/src/main/java/feign/AsyncResponseHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -37,17 +37,17 @@ class AsyncResponseHandler {

private final Decoder decoder;
private final ErrorDecoder errorDecoder;
private final boolean decode404;
private final boolean dismiss404;
private final boolean closeAfterDecode;

AsyncResponseHandler(Level logLevel, Logger logger, Decoder decoder, ErrorDecoder errorDecoder,
boolean decode404, boolean closeAfterDecode) {
boolean dismiss404, boolean closeAfterDecode) {
super();
this.logLevel = logLevel;
this.logger = logger;
this.decoder = decoder;
this.errorDecoder = errorDecoder;
this.decode404 = decode404;
this.dismiss404 = dismiss404;
this.closeAfterDecode = closeAfterDecode;
}

Expand Down Expand Up @@ -88,7 +88,7 @@ void handleResponse(CompletableFuture<Object> resultFuture,
shouldClose = closeAfterDecode;
resultFuture.complete(result);
}
} else if (decode404 && response.status() == 404 && !isVoidType(returnType)) {
} else if (dismiss404 && response.status() == 404 && !isVoidType(returnType)) {
final Object result = decode(response, returnType);
shouldClose = closeAfterDecode;
resultFuture.complete(result);
Expand Down
29 changes: 26 additions & 3 deletions core/src/main/java/feign/Feign.java
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ public static class Builder {
private Options options = new Options();
private InvocationHandlerFactory invocationHandlerFactory =
new InvocationHandlerFactory.Default();
private boolean decode404;
private boolean dismiss404;
private boolean closeAfterDecode = true;
private ExceptionPropagationPolicy propagationPolicy = NONE;
private boolean forceDecoding = false;
Expand Down Expand Up @@ -180,9 +180,32 @@ public Builder mapAndDecode(ResponseMapper mapper, Decoder decoder) {
* custom {@link #client(Client) client}.
*
* @since 8.12
* @deprecated
*/
public Builder decode404() {
this.decode404 = true;
this.dismiss404 = true;
return this;
}

/**
* This flag indicates that the {@link #decoder(Decoder) decoder} should process responses with
* 404 status, specifically returning null or empty instead of throwing {@link FeignException}.
*
* <p/>
* All first-party (ex gson) decoders return well-known empty values defined by
* {@link Util#emptyValueOf}. To customize further, wrap an existing {@link #decoder(Decoder)
* decoder} or make your own.
*
* <p/>
* This flag only works with 404, as opposed to all or arbitrary status codes. This was an
* explicit decision: 404 -> empty is safe, common and doesn't complicate redirection, retry or
* fallback policy. If your server returns a different status for not-found, correct via a
* custom {@link #client(Client) client}.
*
* @since 11.9
*/
public Builder dismiss404() {
this.dismiss404 = true;
return this;
}

Expand Down Expand Up @@ -285,7 +308,7 @@ public Feign build() {

SynchronousMethodHandler.Factory synchronousMethodHandlerFactory =
new SynchronousMethodHandler.Factory(client, retryer, requestInterceptors, logger,
logLevel, decode404, closeAfterDecode, propagationPolicy, forceDecoding);
logLevel, dismiss404, closeAfterDecode, propagationPolicy, forceDecoding);
ParseHandlersByName handlersByName =
new ParseHandlersByName(contract, options, encoder, decoder, queryMapEncoder,
errorDecoder, synchronousMethodHandlerFactory);
Expand Down
12 changes: 6 additions & 6 deletions core/src/main/java/feign/SynchronousMethodHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ private SynchronousMethodHandler(Target<?> target, Client client, Retryer retrye
List<RequestInterceptor> requestInterceptors, Logger logger,
Logger.Level logLevel, MethodMetadata metadata,
RequestTemplate.Factory buildTemplateFromArgs, Options options,
Decoder decoder, ErrorDecoder errorDecoder, boolean decode404,
Decoder decoder, ErrorDecoder errorDecoder, boolean dismiss404,
boolean closeAfterDecode, ExceptionPropagationPolicy propagationPolicy,
boolean forceDecoding) {

Expand All @@ -75,7 +75,7 @@ private SynchronousMethodHandler(Target<?> target, Client client, Retryer retrye
} else {
this.decoder = null;
this.asyncResponseHandler = new AsyncResponseHandler(logLevel, logger, decoder, errorDecoder,
decode404, closeAfterDecode);
dismiss404, closeAfterDecode);
}
}

Expand Down Expand Up @@ -181,20 +181,20 @@ static class Factory {
private final List<RequestInterceptor> requestInterceptors;
private final Logger logger;
private final Logger.Level logLevel;
private final boolean decode404;
private final boolean dismiss404;
private final boolean closeAfterDecode;
private final ExceptionPropagationPolicy propagationPolicy;
private final boolean forceDecoding;

Factory(Client client, Retryer retryer, List<RequestInterceptor> requestInterceptors,
Logger logger, Logger.Level logLevel, boolean decode404, boolean closeAfterDecode,
Logger logger, Logger.Level logLevel, boolean dismiss404, boolean closeAfterDecode,
ExceptionPropagationPolicy propagationPolicy, boolean forceDecoding) {
this.client = checkNotNull(client, "client");
this.retryer = checkNotNull(retryer, "retryer");
this.requestInterceptors = checkNotNull(requestInterceptors, "requestInterceptors");
this.logger = checkNotNull(logger, "logger");
this.logLevel = checkNotNull(logLevel, "logLevel");
this.decode404 = decode404;
this.dismiss404 = dismiss404;
this.closeAfterDecode = closeAfterDecode;
this.propagationPolicy = propagationPolicy;
this.forceDecoding = forceDecoding;
Expand All @@ -208,7 +208,7 @@ public MethodHandler create(Target<?> target,
ErrorDecoder errorDecoder) {
return new SynchronousMethodHandler(target, client, retryer, requestInterceptors, logger,
logLevel, md, buildTemplateFromArgs, options, decoder,
errorDecoder, decode404, closeAfterDecode, propagationPolicy, forceDecoding);
errorDecoder, dismiss404, closeAfterDecode, propagationPolicy, forceDecoding);
}
}
}
2 changes: 1 addition & 1 deletion core/src/main/java/feign/Util.java
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ public static Type resolveLastTypeParameter(Type genericContext, Class<?> supert
* </ul>
*
* <p/>
* When {@link Feign.Builder#decode404() decoding HTTP 404 status}, you'll need to teach decoders
* When {@link Feign.Builder#dismiss404() decoding HTTP 404 status}, you'll need to teach decoders
* a default empty value for a type. This method cheaply supports typical types by only looking at
* the raw type (vs type hierarchy). Decorate for sophistication.
*/
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/feign/codec/Decoder.java
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
* List<Foo>}. <br/>
* <h3>Note on exception propagation</h3> Exceptions thrown by {@link Decoder}s get wrapped in a
* {@link DecodeException} unless they are a subclass of {@link FeignException} already, and unless
* the client was configured with {@link Feign.Builder#decode404()}.
* the client was configured with {@link Feign.Builder#dismiss404()}.
*/
public interface Decoder {

Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/feign/codec/ErrorDecoder.java
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
* <p/>
* It is commonly the case that 404 (Not Found) status has semantic value in HTTP apis. While the
* default behavior is to raise exeception, users can alternatively enable 404 processing via
* {@link feign.Feign.Builder#decode404()}.
* {@link feign.Feign.Builder#dismiss404()}.
*/
public interface ErrorDecoder {

Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/feign/codec/StringDecoder.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public class StringDecoder implements Decoder {
@Override
public Object decode(Response response, Type type) throws IOException {
Response.Body body = response.body();
if (body == null) {
if (response.status() == 404 || response.status() == 204 || body == null) {
return null;
}
if (String.class.equals(type)) {
Expand Down
12 changes: 6 additions & 6 deletions core/src/test/java/feign/AsyncFeignTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -583,12 +583,12 @@ public Object decode(Response response, Type type) throws IOException {
}

@Test
public void decodingExceptionGetWrappedInDecode404Mode() throws Throwable {
public void decodingExceptionGetWrappedInDismiss404Mode() throws Throwable {
server.enqueue(new MockResponse().setResponseCode(404));
thrown.expect(DecodeException.class);
thrown.expectCause(isA(NoSuchElementException.class));;

TestInterfaceAsync api = new TestInterfaceAsyncBuilder().decode404().decoder(new Decoder() {
TestInterfaceAsync api = new TestInterfaceAsyncBuilder().dismiss404().decoder(new Decoder() {
@Override
public Object decode(Response response, Type type) throws IOException {
assertEquals(404, response.status());
Expand All @@ -600,11 +600,11 @@ public Object decode(Response response, Type type) throws IOException {
}

@Test
public void decodingDoesNotSwallow404ErrorsInDecode404Mode() throws Throwable {
public void decodingDoesNotSwallow404ErrorsInDismiss404Mode() throws Throwable {
server.enqueue(new MockResponse().setResponseCode(404));
thrown.expect(IllegalArgumentException.class);

TestInterfaceAsync api = new TestInterfaceAsyncBuilder().decode404()
TestInterfaceAsync api = new TestInterfaceAsyncBuilder().dismiss404()
.errorDecoder(new IllegalArgumentExceptionOn404())
.target("http://localhost:" + server.getPort());

Expand Down Expand Up @@ -1010,8 +1010,8 @@ TestInterfaceAsyncBuilder errorDecoder(ErrorDecoder errorDecoder) {
return this;
}

TestInterfaceAsyncBuilder decode404() {
delegate.decode404();
TestInterfaceAsyncBuilder dismiss404() {
delegate.dismiss404();
return this;
}

Expand Down
4 changes: 2 additions & 2 deletions core/src/test/java/feign/FeignBuilderTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public void testDefaults() throws Exception {

/** Shows exception handling isn't required to coerce 404 to null or empty */
@Test
public void testDecode404() {
public void testDismiss404() {
server.enqueue(new MockResponse().setResponseCode(404));
server.enqueue(new MockResponse().setResponseCode(404));
server.enqueue(new MockResponse().setResponseCode(404));
Expand All @@ -75,7 +75,7 @@ public void testDecode404() {
server.enqueue(new MockResponse().setResponseCode(400));

String url = "http://localhost:" + server.getPort();
TestInterface api = Feign.builder().decode404().target(TestInterface.class, url);
TestInterface api = Feign.builder().dismiss404().target(TestInterface.class, url);

assertThat(api.getQueues("/")).isEmpty(); // empty, not null!
assertThat(api.decodedLazyPost().hasNext()).isFalse(); // empty, not null!
Expand Down
12 changes: 6 additions & 6 deletions core/src/test/java/feign/FeignTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -723,13 +723,13 @@ public Object decode(Response response, Type type) throws IOException {
}

@Test
public void decodingExceptionGetWrappedInDecode404Mode() throws Exception {
public void decodingExceptionGetWrappedInDismiss404Mode() throws Exception {
server.enqueue(new MockResponse().setResponseCode(404));
thrown.expect(DecodeException.class);
thrown.expectCause(isA(NoSuchElementException.class));;

TestInterface api = new TestInterfaceBuilder()
.decode404()
.dismiss404()
.decoder(new Decoder() {
@Override
public Object decode(Response response, Type type) throws IOException {
Expand All @@ -741,12 +741,12 @@ public Object decode(Response response, Type type) throws IOException {
}

@Test
public void decodingDoesNotSwallow404ErrorsInDecode404Mode() throws Exception {
public void decodingDoesNotSwallow404ErrorsInDismiss404Mode() throws Exception {
server.enqueue(new MockResponse().setResponseCode(404));
thrown.expect(IllegalArgumentException.class);

TestInterface api = new TestInterfaceBuilder()
.decode404()
.dismiss404()
.errorDecoder(new IllegalArgumentExceptionOn404())
.target("http://localhost:" + server.getPort());
api.queryMap(Collections.<String, Object>emptyMap());
Expand Down Expand Up @@ -1168,8 +1168,8 @@ TestInterfaceBuilder errorDecoder(ErrorDecoder errorDecoder) {
return this;
}

TestInterfaceBuilder decode404() {
delegate.decode404();
TestInterfaceBuilder dismiss404() {
delegate.dismiss404();
return this;
}

Expand Down
12 changes: 6 additions & 6 deletions core/src/test/java/feign/FeignUnderAsyncTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -582,13 +582,13 @@ public Object decode(Response response, Type type) throws IOException {
}

@Test
public void decodingExceptionGetWrappedInDecode404Mode() throws Exception {
public void decodingExceptionGetWrappedInDismiss404Mode() throws Exception {
server.enqueue(new MockResponse().setResponseCode(404));
thrown.expect(DecodeException.class);
thrown.expectCause(isA(NoSuchElementException.class));;

TestInterface api = new TestInterfaceBuilder()
.decode404()
.dismiss404()
.decoder(new Decoder() {
@Override
public Object decode(Response response, Type type) throws IOException {
Expand All @@ -600,12 +600,12 @@ public Object decode(Response response, Type type) throws IOException {
}

@Test
public void decodingDoesNotSwallow404ErrorsInDecode404Mode() throws Exception {
public void decodingDoesNotSwallow404ErrorsInDismiss404Mode() throws Exception {
server.enqueue(new MockResponse().setResponseCode(404));
thrown.expect(IllegalArgumentException.class);

TestInterface api = new TestInterfaceBuilder()
.decode404()
.dismiss404()
.errorDecoder(new IllegalArgumentExceptionOn404())
.target("http://localhost:" + server.getPort());
api.queryMap(Collections.<String, Object>emptyMap());
Expand Down Expand Up @@ -1000,8 +1000,8 @@ TestInterfaceBuilder errorDecoder(ErrorDecoder errorDecoder) {
return this;
}

TestInterfaceBuilder decode404() {
delegate.decode404();
TestInterfaceBuilder dismiss404() {
delegate.dismiss404();
return this;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public void simple404OptionalTest() throws IOException, InterruptedException {
server.enqueue(new MockResponse().setBody("foo"));

final OptionalInterface api = Feign.builder()
.decode404()
.dismiss404()
.decoder(new OptionalDecoder(new Decoder.Default()))
.target(OptionalInterface.class, server.url("/").toString());

Expand Down
3 changes: 3 additions & 0 deletions gson/src/main/java/feign/gson/GsonDecoder.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.lang.reflect.Type;
import java.util.Collections;
import feign.Response;
import feign.Util;
import feign.codec.Decoder;
import static feign.Util.UTF_8;
import static feign.Util.ensureClosed;
Expand All @@ -43,6 +44,8 @@ public GsonDecoder(Gson gson) {

@Override
public Object decode(Response response, Type type) throws IOException {
if (response.status() == 404 || response.status() == 204)
return Util.emptyValueOf(type);
if (response.body() == null)
return null;
Reader reader = response.body().asReader(UTF_8);
Expand Down
6 changes: 3 additions & 3 deletions gson/src/test/java/feign/gson/GsonCodecTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -226,15 +226,15 @@ public void customEncoder() {
+ "]");
}

/** Enabled via {@link feign.Feign.Builder#decode404()} */
/** Enabled via {@link feign.Feign.Builder#dismiss404()} */
@Test
public void notFoundDecodesToNull() throws Exception {
public void notFoundDecodesToEmpty() throws Exception {
Response response = Response.builder()
.status(404)
.reason("NOT FOUND")
.headers(Collections.emptyMap())
.request(Request.create(HttpMethod.GET, "/api", Collections.emptyMap(), null, Util.UTF_8))
.build();
assertThat((byte[]) new GsonDecoder().decode(response, byte[].class)).isNull();
assertThat((byte[]) new GsonDecoder().decode(response, byte[].class)).isEmpty();
}
}
Loading