Skip to content

Commit 24885fe

Browse files
author
Adrian Cole
committed
Adds Feign.Builder.decode404() to reduce boilerplate for empty semantics
This adds the `Feign.Builder.decode404()` flag which indicates decoders should process responses with 404 status. It also changes all first-party decoders (like gson) to return well-known empty values by default. Further customization is possible by wrapping or creating a custom decoder. Prior to this change, we used custom invocation handlers as the way to add fallback values based on exception or return status. `feign-hystrix` uses this to return `HystrixCommand<X>`, but the general pattern applies to anything that has a type representing both success and failure, such as `Try<X>` or `Observable<X>`. As we define it here, 404 status is not a retry or fallback policy, it is just empty semantics. By limiting Feign's special processing to 404, we gain a lot with very little supporting code. If instead we opened all codes, Feign could easily turn bad request, redirect, or server errors silently to null. This sort of configuration issue is hard to troubleshoot. 404 -> empty is a very safe policy vs all codes. Moreover, we don't create a cliff, where folks seeking fallback policy eventually realize they can't if only given a response code. Fallback systems like Hystrix address exceptions that occur before or in lieu of a response. By special-casing 404, we avoid a slippery slope of half- implementing Hystrix. Finally, 404 handling has been commonly requested: it has a clear use- case, and through that value. This design supports that without breaking compatibility, or impacting existing integrations such as Hystrix or Ribbon. See OpenFeign#238 OpenFeign#287
1 parent c9c5e31 commit 24885fe

19 files changed

Lines changed: 244 additions & 59 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
### Version 8.12
2+
* Adds `Feign.Builder.decode404()` to reduce boilerplate for empty semantics.
3+
14
### Version 8.11
25
* Adds support for Hystrix via a `HystrixFeign` builder.
36

‎core/src/main/java/feign/Feign.java‎

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,7 @@ public static String configKey(Method method) {
8282

8383
public static class Builder {
8484

85-
private final List<RequestInterceptor>
86-
requestInterceptors =
85+
private final List<RequestInterceptor> requestInterceptors =
8786
new ArrayList<RequestInterceptor>();
8887
private Logger.Level logLevel = Logger.Level.NONE;
8988
private Contract contract = new Contract.Default();
@@ -94,9 +93,9 @@ public static class Builder {
9493
private Decoder decoder = new Decoder.Default();
9594
private ErrorDecoder errorDecoder = new ErrorDecoder.Default();
9695
private Options options = new Options();
97-
private InvocationHandlerFactory
98-
invocationHandlerFactory =
96+
private InvocationHandlerFactory invocationHandlerFactory =
9997
new InvocationHandlerFactory.Default();
98+
private boolean decode404;
10099

101100
public Builder logLevel(Logger.Level logLevel) {
102101
this.logLevel = logLevel;
@@ -133,6 +132,26 @@ public Builder decoder(Decoder decoder) {
133132
return this;
134133
}
135134

135+
/**
136+
* This flag indicates that the {@link #decoder(Decoder) decoder} should process responses with
137+
* 404 status, specifically returning null or empty instead of throwing {@link FeignException}.
138+
*
139+
* <p/> All first-party (ex gson) decoders return well-known empty values defined by
140+
* {@link Util#emptyValueOf}. To customize further, wrap an existing
141+
* {@link #decoder(Decoder) decoder} or make your own.
142+
*
143+
* <p/> This flag only works with 404, as opposed to all or arbitrary status codes. This was an
144+
* explicit decision: 404 -> empty is safe, common and doesn't complicate redirection, retry or
145+
* fallback policy. If your server returns a different status for not-found, correct via a
146+
* custom {@link #client(Client) client}.
147+
*
148+
* @since 8.12
149+
*/
150+
public Builder decode404() {
151+
this.decode404 = true;
152+
return this;
153+
}
154+
136155
public Builder errorDecoder(ErrorDecoder errorDecoder) {
137156
this.errorDecoder = errorDecoder;
138157
return this;
@@ -182,9 +201,8 @@ public <T> T target(Target<T> target) {
182201
public Feign build() {
183202
SynchronousMethodHandler.Factory synchronousMethodHandlerFactory =
184203
new SynchronousMethodHandler.Factory(client, retryer, requestInterceptors, logger,
185-
logLevel);
186-
ParseHandlersByName
187-
handlersByName =
204+
logLevel, decode404);
205+
ParseHandlersByName handlersByName =
188206
new ParseHandlersByName(contract, options, encoder, decoder,
189207
errorDecoder, synchronousMethodHandlerFactory);
190208
return new ReflectiveFeign(handlersByName, invocationHandlerFactory);

‎core/src/main/java/feign/SynchronousMethodHandler.java‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -43,11 +43,13 @@ final class SynchronousMethodHandler implements MethodHandler {
4343
private final Options options;
4444
private final Decoder decoder;
4545
private final ErrorDecoder errorDecoder;
46+
private final boolean decode404;
47+
4648
private SynchronousMethodHandler(Target<?> target, Client client, Retryer retryer,
4749
List<RequestInterceptor> requestInterceptors, Logger logger,
4850
Logger.Level logLevel, MethodMetadata metadata,
4951
RequestTemplate.Factory buildTemplateFromArgs, Options options,
50-
Decoder decoder, ErrorDecoder errorDecoder) {
52+
Decoder decoder, ErrorDecoder errorDecoder, boolean decode404) {
5153
this.target = checkNotNull(target, "target");
5254
this.client = checkNotNull(client, "client for %s", target);
5355
this.retryer = checkNotNull(retryer, "retryer for %s", target);
@@ -60,6 +62,7 @@ private SynchronousMethodHandler(Target<?> target, Client client, Retryer retrye
6062
this.options = checkNotNull(options, "options for %s", target);
6163
this.errorDecoder = checkNotNull(errorDecoder, "errorDecoder for %s", target);
6264
this.decoder = checkNotNull(decoder, "decoder for %s", target);
65+
this.decode404 = decode404;
6366
}
6467

6568
@Override
@@ -117,6 +120,8 @@ Object executeAndDecode(RequestTemplate template) throws Throwable {
117120
} else {
118121
return decode(response);
119122
}
123+
} else if (decode404 && response.status() == 404) {
124+
return decode(response);
120125
} else {
121126
throw errorDecoder.decode(metadata.configKey(), response);
122127
}
@@ -158,22 +163,24 @@ static class Factory {
158163
private final List<RequestInterceptor> requestInterceptors;
159164
private final Logger logger;
160165
private final Logger.Level logLevel;
166+
private final boolean decode404;
161167

162168
Factory(Client client, Retryer retryer, List<RequestInterceptor> requestInterceptors,
163-
Logger logger, Logger.Level logLevel) {
169+
Logger logger, Logger.Level logLevel, boolean decode404) {
164170
this.client = checkNotNull(client, "client");
165171
this.retryer = checkNotNull(retryer, "retryer");
166172
this.requestInterceptors = checkNotNull(requestInterceptors, "requestInterceptors");
167173
this.logger = checkNotNull(logger, "logger");
168174
this.logLevel = checkNotNull(logLevel, "logLevel");
175+
this.decode404 = decode404;
169176
}
170177

171178
public MethodHandler create(Target<?> target, MethodMetadata md,
172179
RequestTemplate.Factory buildTemplateFromArgs,
173180
Options options, Decoder decoder, ErrorDecoder errorDecoder) {
174181
return new SynchronousMethodHandler(target, client, retryer, requestInterceptors, logger,
175-
logLevel, md,
176-
buildTemplateFromArgs, options, decoder, errorDecoder);
182+
logLevel, md, buildTemplateFromArgs, options, decoder,
183+
errorDecoder, decode404);
177184
}
178185
}
179186
}

‎core/src/main/java/feign/Util.java‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,12 @@
3232
import java.util.ArrayList;
3333
import java.util.Collection;
3434
import java.util.Collections;
35+
import java.util.Iterator;
36+
import java.util.LinkedHashMap;
37+
import java.util.List;
3538
import java.util.Map;
39+
import java.util.NoSuchElementException;
40+
import java.util.Set;
3641

3742
import static java.lang.String.format;
3843

@@ -183,6 +188,54 @@ public static Type resolveLastTypeParameter(Type genericContext, Class<?> supert
183188
return types[types.length - 1];
184189
}
185190

191+
/**
192+
* This returns well known empty values for well-known java types. This returns null for types not
193+
* in the following list.
194+
*
195+
* <ul>
196+
* <li>{@code [Bb]oolean}</li>
197+
* <li>{@code byte[]}</li>
198+
* <li>{@code Collection}</li>
199+
* <li>{@code Iterator}</li>
200+
* <li>{@code List}</li>
201+
* <li>{@code Map}</li>
202+
* <li>{@code Set}</li>
203+
* </ul>
204+
*
205+
* <p/> When {@link Feign.Builder#decode404() decoding HTTP 404 status}, you'll need to teach
206+
* decoders a default empty value for a type. This method cheaply supports typical types by only
207+
* looking at the raw type (vs type hierarchy). Decorate for sophistication.
208+
*/
209+
public static Object emptyValueOf(Type type) {
210+
return EMPTIES.get(Types.getRawType(type));
211+
}
212+
213+
private static final Map<Class<?>, Object> EMPTIES;
214+
static {
215+
Map<Class<?>, Object> empties = new LinkedHashMap<Class<?>, Object>();
216+
empties.put(boolean.class, false);
217+
empties.put(Boolean.class, false);
218+
empties.put(byte[].class, new byte[0]);
219+
empties.put(Collection.class, Collections.emptyList());
220+
empties.put(Iterator.class, new Iterator<Object>() { // Collections.emptyIterator is a 1.7 api
221+
public boolean hasNext() {
222+
return false;
223+
}
224+
225+
public Object next() {
226+
throw new NoSuchElementException();
227+
}
228+
229+
public void remove() {
230+
throw new IllegalStateException();
231+
}
232+
});
233+
empties.put(List.class, Collections.emptyList());
234+
empties.put(Map.class, Collections.emptyMap());
235+
empties.put(Set.class, Collections.emptySet());
236+
EMPTIES = Collections.unmodifiableMap(empties);
237+
}
238+
186239
/**
187240
* Adapted from {@code com.google.common.io.CharStreams.toString()}.
188241
*/

‎core/src/main/java/feign/codec/Decoder.java‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -67,19 +67,15 @@ public interface Decoder {
6767
*/
6868
Object decode(Response response, Type type) throws IOException, DecodeException, FeignException;
6969

70-
/**
71-
* Default implementation of {@code Decoder}.
72-
*/
70+
/** Default implementation of {@code Decoder}. */
7371
public class Default extends StringDecoder {
7472

7573
@Override
7674
public Object decode(Response response, Type type) throws IOException {
77-
Response.Body body = response.body();
78-
if (body == null) {
79-
return null;
80-
}
75+
if (response.status() == 404) return Util.emptyValueOf(type);
76+
if (response.body() == null) return null;
8177
if (byte[].class.equals(type)) {
82-
return Util.toByteArray(body.asInputStream());
78+
return Util.toByteArray(response.body().asInputStream());
8379
}
8480
return super.decode(response, type);
8581
}

‎core/src/main/java/feign/codec/ErrorDecoder.java‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,25 +35,35 @@
3535

3636
/**
3737
* Allows you to massage an exception into a application-specific one. Converting out to a throttle
38-
* exception are examples of this in use. <br> Ex. <br>
38+
* exception are examples of this in use.
39+
*
40+
* <p/>Ex:
3941
* <pre>
4042
* class IllegalArgumentExceptionOn404Decoder extends ErrorDecoder {
4143
*
4244
* &#064;Override
4345
* public Exception decode(String methodKey, Response response) {
44-
* if (response.status() == 404)
45-
* throw new IllegalArgumentException(&quot;zone not found&quot;);
46+
* if (response.status() == 400)
47+
* throw new IllegalArgumentException(&quot;bad zone name&quot;);
4648
* return ErrorDecoder.DEFAULT.decode(methodKey, request, response);
4749
* }
4850
*
4951
* }
5052
* </pre>
51-
* <br> <b>Error handling</b><br> <br> Responses where {@link Response#status()} is not in the 2xx
53+
*
54+
* <p/><b>Error handling</b>
55+
*
56+
* <p/>Responses where {@link Response#status()} is not in the 2xx
5257
* range are classified as errors, addressed by the {@link ErrorDecoder}. That said, certain RPC
5358
* apis return errors defined in the {@link Response#body()} even on a 200 status. For example, in
5459
* the DynECT api, a job still running condition is returned with a 200 status, encoded in json.
5560
* When scenarios like this occur, you should raise an application-specific exception (which may be
5661
* {@link feign.RetryableException retryable}).
62+
*
63+
* <p/><b>Not Found Semantics</b>
64+
* <p/> It is commonly the case that 404 (Not Found) status has semantic value in HTTP apis. While
65+
* the default behavior is to raise exeception, users can alternatively enable 404 processing via
66+
* {@link feign.Feign.Builder#decode404()}.
5767
*/
5868
public interface ErrorDecoder {
5969

‎core/src/test/java/feign/FeignBuilderTest.java‎

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,9 @@
3333
import feign.codec.Encoder;
3434

3535
import static feign.assertj.MockWebServerAssertions.assertThat;
36+
import static org.assertj.core.api.Assertions.failBecauseExceptionWasNotThrown;
3637
import static org.junit.Assert.assertEquals;
38+
import static org.junit.Assert.fail;
3739

3840
public class FeignBuilderTest {
3941

@@ -54,6 +56,27 @@ public void testDefaults() throws Exception {
5456
.hasBody("request data");
5557
}
5658

59+
/** Shows exception handling isn't required to coerce 404 to null or empty */
60+
@Test
61+
public void testDecode404() throws Exception {
62+
server.enqueue(new MockResponse().setResponseCode(404));
63+
server.enqueue(new MockResponse().setResponseCode(404));
64+
server.enqueue(new MockResponse().setResponseCode(400));
65+
66+
String url = "http://localhost:" + server.getPort();
67+
TestInterface api = Feign.builder().decode404().target(TestInterface.class, url);
68+
69+
assertThat(api.getQueues("/")).isEmpty(); // empty, not null!
70+
assertThat(api.decodedPost()).isNull(); // null, not empty!
71+
72+
try { // ensure other 400 codes are not impacted.
73+
api.decodedPost();
74+
failBecauseExceptionWasNotThrown(FeignException.class);
75+
} catch (FeignException e) {
76+
assertThat(e.status()).isEqualTo(400);
77+
}
78+
}
79+
5780
@Test
5881
public void testUrlPathConcatUrlTrailingSlash() throws Exception {
5982
server.enqueue(new MockResponse().setBody("response data"));
@@ -147,8 +170,7 @@ public void apply(RequestTemplate template) {
147170
}
148171
};
149172

150-
TestInterface
151-
api =
173+
TestInterface api =
152174
Feign.builder().requestInterceptor(requestInterceptor).target(TestInterface.class, url);
153175
Response response = api.codecPost("request data");
154176
assertEquals(Util.toString(response.body().asReader()), "response data");
@@ -175,8 +197,7 @@ public InvocationHandler create(Target target, Map<Method, MethodHandler> dispat
175197
}
176198
};
177199

178-
TestInterface
179-
api =
200+
TestInterface api =
180201
Feign.builder().invocationHandlerFactory(factory).target(TestInterface.class, url);
181202
Response response = api.codecPost("request data");
182203
assertEquals("response data", Util.toString(response.body().asReader()));
@@ -192,9 +213,7 @@ public void testSlashIsEncodedInPathParams() throws Exception {
192213

193214
String url = "http://localhost:" + server.getPort();
194215

195-
TestInterface
196-
api =
197-
Feign.builder().target(TestInterface.class, url);
216+
TestInterface api = Feign.builder().target(TestInterface.class, url);
198217
api.getQueues("/");
199218

200219
assertThat(server.takeRequest())
@@ -218,6 +237,6 @@ interface TestInterface {
218237
String decodedPost();
219238

220239
@RequestLine(value = "GET /api/queues/{vhost}", decodeSlash = false)
221-
String getQueues(@Param("vhost") String vhost);
240+
byte[] getQueues(@Param("vhost") String vhost);
222241
}
223242
}

‎core/src/test/java/feign/FeignTest.java‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -231,12 +231,12 @@ public void configKeyUsesChildType() throws Exception {
231231

232232
@Test
233233
public void canOverrideErrorDecoder() throws Exception {
234-
server.enqueue(new MockResponse().setResponseCode(404).setBody("foo"));
234+
server.enqueue(new MockResponse().setResponseCode(400).setBody("foo"));
235235
thrown.expect(IllegalArgumentException.class);
236-
thrown.expectMessage("zone not found");
236+
thrown.expectMessage("bad zone name");
237237

238238
TestInterface api = new TestInterfaceBuilder()
239-
.errorDecoder(new IllegalArgumentExceptionOn404())
239+
.errorDecoder(new IllegalArgumentExceptionOn400())
240240
.target("http://localhost:" + server.getPort());
241241

242242
api.post();
@@ -548,12 +548,12 @@ public void apply(RequestTemplate template) {
548548
}
549549
}
550550

551-
static class IllegalArgumentExceptionOn404 extends ErrorDecoder.Default {
551+
static class IllegalArgumentExceptionOn400 extends ErrorDecoder.Default {
552552

553553
@Override
554554
public Exception decode(String methodKey, Response response) {
555-
if (response.status() == 404) {
556-
return new IllegalArgumentException("zone not found");
555+
if (response.status() == 400) {
556+
return new IllegalArgumentException("bad zone name");
557557
}
558558
return super.decode(methodKey, response);
559559
}

0 commit comments

Comments
 (0)