Skip to content

Commit cd5405e

Browse files
davidmc24davidmc24
committed
Simplify Encoder/Decoder interfaces (OpenFeign#53)
This is intended as a step towards simplifying Feign. This changeset removes the generics from both interfaces, and changes their Dagger bindings from SET to UNIQUE. Additionally, in changing the signatures for Encoder/Decoder, it focuses on use of the RequestTemplate and Response objects, allowing us to extend them in the future to support binary data without needing to change the Encoder/Decoder signatures again.
1 parent ed68302 commit cd5405e

21 files changed

Lines changed: 416 additions & 293 deletions

‎CHANGES.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22
* Remove support for Observable methods.
33
* SaxDecoder now decodes multiple types.
44
* Remove pattern decoders in favor of SaxDecoder.
5+
* Use single non-generic Decoder/Encoder instead of sets of type-specific Decoders/Encoders.
6+
* Decoders/Encoders are now more flexible, having access to the Response/RequestTemplate respectively.
57

68
### Version 4.4.1
79
* Fix NullPointerException on calling equals and hashCode.

‎README.md‎

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ You can find [several examples](https://github.com/Netflix/feign/tree/master/fei
7373
### Integrations
7474
Feign intends to work well within Netflix and other Open Source communities. Modules are welcome to integrate with your favorite projects!
7575
### Gson
76-
[GsonModule](https://github.com/Netflix/feign/tree/master/feign-gson) adds default encoders and decoders so you get get started with a json api.
76+
[GsonModule](https://github.com/Netflix/feign/tree/master/feign-gson) adds default encoders and decoders so you get get started with a JSON api.
7777

7878
Integration requires you pass `new GsonModule()` to `Feign.create()`, or add it to your graph with Dagger:
7979
```java
@@ -101,46 +101,46 @@ MyService api = Feign.create(MyService.class, "https://myAppProd", new RibbonMod
101101
### Decoders
102102
The last argument to `Feign.create` allows you to specify additional configuration such as how to decode a responses, modeled in Dagger.
103103

104-
If any methods in your interface return types besides `void` or `String`, you'll need to configure a `Decoder.TextStream<T>` or a general one for all types (`Decoder.TextStream<Object>`).
104+
If any methods in your interface return types besides `Response`, `void` or `String`, you'll need to configure a `Decoder`.
105105

106-
The `GsonModule` in the `feign-gson` extension configures a (`Decoder.TextStream<Object>`) which parses objects from json using reflection.
106+
The `GsonModule` in the `feign-gson` extension configures a `Decoder` which parses objects from JSON using reflection.
107107

108108
Here's how you could write this yourself, using whatever library you prefer:
109109
```java
110110
@Module(library = true)
111111
static class JsonModule {
112-
@Provides(type = SET) Decoder decoder(final JsonParser parser) {
113-
return new Decoder.TextStream<Object>() {
112+
@Provides Decoder decoder(final JsonParser parser) {
113+
return new Decoder() {
114114

115-
@Override public Object decode(Reader reader, Type type) throws IOException {
116-
return parser.readJson(reader, type);
115+
@Override public Object decode(Response response, Type type) throws IOException {
116+
return parser.readJson(response.body().asReader(), type);
117117
}
118118

119119
};
120120
}
121121
}
122122
```
123-
#### Type-specific Decoders
124-
The generic parameter of `Decoder.TextStream<T>` designates which The type parameter is either a concrete type, or `Object`, if your decoder can handle multiple types. To add a type-specific decoder, ensure your type parameter is correct. Here's an example of an xml decoder that will only apply to methods that return `ZoneList`.
125-
126-
```
127-
@Provides(type = SET) Decoder zoneListDecoder(Provider<ListHostedZonesResponseHandler> handlers) {
128-
return new SAXDecoder<ZoneList>(handlers){};
129-
}
130-
```
131123

132124
### Advanced usage and Dagger
133125
#### Dagger
134126
Feign can be directly wired into Dagger which keeps things at compile time and Android friendly. As opposed to exposing builders for config, Feign intends users to embed their config in Dagger.
135127

136-
Where possible, Feign configuration uses normal Dagger conventions. For example, `Decoder` bindings are of `Provider.Type.SET`, meaning you can make multiple bindings for all the different types you return. Here's an example of multiple decoder bindings.
128+
Where possible, Feign configuration uses normal Dagger conventions. For example, `RequestInterceptor` bindings are of `Provider.Type.SET`, meaning you can have multiple interceptors. Here's an example of multiple interceptor bindings.
137129
```java
138-
@Provides(type = SET) Decoder recordListDecoder(Provider<RecordListHandler> handlers) {
139-
return new SAXDecoder<List<Record>>(handlers){};
130+
@Provides(type = SET) RequestInterceptor forwardedForInterceptor() {
131+
return new RequestInterceptor() {
132+
@Override public void apply(RequestTemplate template) {
133+
template.header("X-Forwarded-For", "origin.host.com");
134+
}
135+
};
140136
}
141137

142-
@Provides(type = SET) Decoder directionalRecordListDecoder(Provider<DirectionalRecordListHandler> handlers) {
143-
return new SAXDecoder<List<DirectionalRecord>>(handlers){};
138+
@Provides(type = SET) RequestInterceptor userAgentInterceptor() {
139+
return new RequestInterceptor() {
140+
@Override public void apply(RequestTemplate template) {
141+
template.header("User-Agent", "My Cool Client");
142+
}
143+
};
144144
}
145145
```
146146
#### Logging

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@
2121
import feign.Logger.NoOpLogger;
2222
import feign.Request.Options;
2323
import feign.Target.HardCodedTarget;
24+
import feign.codec.Decoder;
25+
import feign.codec.Encoder;
2426
import feign.codec.ErrorDecoder;
2527

2628
import javax.net.ssl.HostnameVerifier;
@@ -107,6 +109,16 @@ public static class Defaults {
107109
return new NoOpLogger();
108110
}
109111

112+
@Provides
113+
Encoder defaultEncoder() {
114+
return new Encoder.Default();
115+
}
116+
117+
@Provides
118+
Decoder defaultDecoder() {
119+
return new Decoder.Default();
120+
}
121+
110122
@Provides ErrorDecoder errorDecoder() {
111123
return new ErrorDecoder.Default();
112124
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,8 @@ public static FeignException errorStatus(String methodKey, Response response) {
3535
String message = format("status %s reading %s", response.status(), methodKey);
3636
try {
3737
if (response.body() != null) {
38-
String body = toString.decode(response.body().asReader(), String.class);
39-
response = Response.create(response.status(), response.reason(), response.headers(), body.toString());
38+
String body = toString.decode(response, String.class).toString();
39+
response = Response.create(response.status(), response.reason(), response.headers(), body);
4040
message += "; content:\n" + body;
4141
}
4242
} catch (IOException ignored) { // NOPMD

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,7 @@ Response logAndRebufferResponse(String configKey, Level logLevel, Response respo
191191
log(configKey, "<--- END HTTP (%s-byte body)", bodyAsString.getBytes(UTF_8).length);
192192
return Response.create(response.status(), response.reason(), response.headers(), bodyAsString);
193193
} finally {
194-
ensureClosed(response.body());
194+
ensureClosed(body);
195195
}
196196
}
197197
}

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

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ static class Factory {
5252
}
5353

5454
public MethodHandler create(Target<?> target, MethodMetadata md, BuildTemplateFromArgs buildTemplateFromArgs,
55-
Options options, Decoder.TextStream<?> decoder, ErrorDecoder errorDecoder) {
55+
Options options, Decoder decoder, ErrorDecoder errorDecoder) {
5656
return new SynchronousMethodHandler(target, client, retryer, requestInterceptors, logger, logLevel, md,
5757
buildTemplateFromArgs, options, decoder, errorDecoder);
5858
}
@@ -82,14 +82,14 @@ static final class SynchronousMethodHandler implements MethodHandler {
8282
private final Provider<Logger.Level> logLevel;
8383
private final BuildTemplateFromArgs buildTemplateFromArgs;
8484
private final Options options;
85-
private final Decoder.TextStream<?> decoder;
85+
private final Decoder decoder;
8686
private final ErrorDecoder errorDecoder;
8787

8888
private SynchronousMethodHandler(Target<?> target, Client client, Provider<Retryer> retryer,
8989
Set<RequestInterceptor> requestInterceptors, Logger logger,
9090
Provider<Logger.Level> logLevel, MethodMetadata metadata,
9191
BuildTemplateFromArgs buildTemplateFromArgs, Options options,
92-
Decoder.TextStream<?> decoder, ErrorDecoder errorDecoder) {
92+
Decoder decoder, ErrorDecoder errorDecoder) {
9393
this.target = checkNotNull(target, "target");
9494
this.client = checkNotNull(client, "client for %s", target);
9595
this.retryer = checkNotNull(retryer, "retryer for %s", target);
@@ -169,13 +169,8 @@ Request targetRequest(RequestTemplate template) {
169169
}
170170

171171
Object decode(Response response) throws Throwable {
172-
if (metadata.returnType().equals(Response.class)) {
173-
return response;
174-
} else if (metadata.returnType() == void.class || response.body() == null) {
175-
return null;
176-
}
177172
try {
178-
return decoder.decode(response.body().asReader(), metadata.returnType());
173+
return decoder.decode(response, metadata.returnType());
179174
} catch (FeignException e) {
180175
throw e;
181176
} catch (RuntimeException e) {

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

Lines changed: 13 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -21,16 +21,13 @@
2121
import feign.codec.EncodeException;
2222
import feign.codec.Encoder;
2323
import feign.codec.ErrorDecoder;
24-
import feign.codec.StringDecoder;
2524

2625
import javax.inject.Inject;
2726
import java.lang.reflect.InvocationHandler;
2827
import java.lang.reflect.Method;
2928
import java.lang.reflect.Proxy;
30-
import java.lang.reflect.Type;
3129
import java.util.Collection;
3230
import java.util.Collections;
33-
import java.util.HashMap;
3431
import java.util.LinkedHashMap;
3532
import java.util.List;
3633
import java.util.Map;
@@ -39,9 +36,6 @@
3936

4037
import static feign.Util.checkArgument;
4138
import static feign.Util.checkNotNull;
42-
import static feign.Util.checkState;
43-
import static feign.Util.resolveLastTypeParameter;
44-
import static java.lang.String.format;
4539

4640
@SuppressWarnings("rawtypes")
4741
public class ReflectiveFeign extends Feign {
@@ -122,14 +116,6 @@ public static class Module {
122116
return Collections.emptySet();
123117
}
124118

125-
@Provides(type = Provides.Type.SET_VALUES) Set<Encoder> noEncoders() {
126-
return Collections.emptySet();
127-
}
128-
129-
@Provides(type = Provides.Type.SET_VALUES) Set<Decoder> noDecoders() {
130-
return Collections.emptySet();
131-
}
132-
133119
@Provides Feign provideFeign(ReflectiveFeign in) {
134120
return in;
135121
}
@@ -138,46 +124,20 @@ public static class Module {
138124
static final class ParseHandlersByName {
139125
private final Contract contract;
140126
private final Options options;
141-
private final Map<Type, Encoder.Text<? super Object>> encoders = new HashMap<Type, Encoder.Text<? super Object>>();
142-
private final Encoder.Text<Map<String, ?>> formEncoder;
143-
private final Map<Type, Decoder.TextStream<?>> decoders = new HashMap<Type, Decoder.TextStream<?>>();
127+
private final Encoder encoder;
128+
private final Decoder decoder;
144129
private final ErrorDecoder errorDecoder;
145130
private final MethodHandler.Factory factory;
146131

147132
@SuppressWarnings("unchecked")
148-
@Inject ParseHandlersByName(Contract contract, Options options, Set<Encoder> encoders, Set<Decoder> decoders,
133+
@Inject ParseHandlersByName(Contract contract, Options options, Encoder encoder, Decoder decoder,
149134
ErrorDecoder errorDecoder, MethodHandler.Factory factory) {
150135
this.contract = contract;
151136
this.options = options;
152137
this.factory = factory;
153138
this.errorDecoder = errorDecoder;
154-
for (Encoder encoder : encoders) {
155-
checkState(encoder instanceof Encoder.Text,
156-
"Currently, only Encoder.Text is supported. Found: ", encoder);
157-
Type type = resolveLastTypeParameter(encoder.getClass(), Encoder.class);
158-
this.encoders.put(type, Encoder.Text.class.cast(encoder));
159-
}
160-
try {
161-
Type formEncoderType = getClass().getDeclaredField("formEncoder").getGenericType();
162-
Type formType = resolveLastTypeParameter(formEncoderType, Encoder.class);
163-
Encoder.Text<? super Object> formEncoder = this.encoders.get(formType);
164-
if (formEncoder == null) {
165-
formEncoder = this.encoders.get(Object.class);
166-
}
167-
this.formEncoder = (Encoder.Text) formEncoder;
168-
} catch (NoSuchFieldException e) {
169-
throw new AssertionError(e);
170-
}
171-
StringDecoder stringDecoder = new StringDecoder();
172-
this.decoders.put(void.class, stringDecoder);
173-
this.decoders.put(Response.class, stringDecoder);
174-
this.decoders.put(String.class, stringDecoder);
175-
for (Decoder decoder : decoders) {
176-
checkState(decoder instanceof Decoder.TextStream,
177-
"Currently, only Decoder.TextStream is supported. Found: ", decoder);
178-
Type type = resolveLastTypeParameter(decoder.getClass(), Decoder.class);
179-
this.decoders.put(type, Decoder.TextStream.class.cast(decoder));
180-
}
139+
this.encoder = checkNotNull(encoder, "encoder");
140+
this.decoder = checkNotNull(decoder, "decoder");
181141
}
182142

183143
public Map<String, MethodHandler> apply(Target key) {
@@ -186,32 +146,12 @@ public Map<String, MethodHandler> apply(Target key) {
186146
for (MethodMetadata md : metadata) {
187147
BuildTemplateByResolvingArgs buildTemplate;
188148
if (!md.formParams().isEmpty() && md.template().bodyTemplate() == null) {
189-
if (formEncoder == null) {
190-
throw new IllegalStateException(format("%s needs @Provides(type = Set) Encoder encoder()" +
191-
"{ // Encoder.Text<Map<String, ?>> or Encoder.Text<Object>}", md.configKey()));
192-
}
193-
buildTemplate = new BuildFormEncodedTemplateFromArgs(md, formEncoder);
149+
buildTemplate = new BuildFormEncodedTemplateFromArgs(md, encoder);
194150
} else if (md.bodyIndex() != null) {
195-
Encoder.Text<? super Object> encoder = encoders.get(md.bodyType());
196-
if (encoder == null) {
197-
encoder = encoders.get(Object.class);
198-
}
199-
if (encoder == null) {
200-
throw new IllegalStateException(format("%s needs @Provides(type = Set) Encoder encoder()" +
201-
"{ // Encoder.Text<%s> or Encoder.Text<Object>}", md.configKey(), md.bodyType()));
202-
}
203151
buildTemplate = new BuildEncodedTemplateFromArgs(md, encoder);
204152
} else {
205153
buildTemplate = new BuildTemplateByResolvingArgs(md);
206154
}
207-
Decoder.TextStream decoder = decoders.get(md.returnType());
208-
if (decoder == null) {
209-
decoder = decoders.get(Object.class);
210-
}
211-
if (decoder == null) {
212-
throw new IllegalStateException(format("%s needs @Provides(type = Set) Decoder decoder()" +
213-
"{ // Decoder.TextStream<%s> or Decoder.TextStream<Object>}", md.configKey(), md.returnType()));
214-
}
215155
result.put(md.configKey(), factory.create(key, md, buildTemplate, options, decoder, errorDecoder));
216156
}
217157
return result;
@@ -249,11 +189,11 @@ protected RequestTemplate resolve(Object[] argv, RequestTemplate mutable, Map<St
249189
}
250190

251191
private static class BuildFormEncodedTemplateFromArgs extends BuildTemplateByResolvingArgs {
252-
private final Encoder.Text<Map<String, ?>> formEncoder;
192+
private final Encoder encoder;
253193

254-
private BuildFormEncodedTemplateFromArgs(MethodMetadata metadata, Encoder.Text<Map<String, ?>> formEncoder) {
194+
private BuildFormEncodedTemplateFromArgs(MethodMetadata metadata, Encoder encoder) {
255195
super(metadata);
256-
this.formEncoder = formEncoder;
196+
this.encoder = encoder;
257197
}
258198

259199
@Override
@@ -264,7 +204,7 @@ protected RequestTemplate resolve(Object[] argv, RequestTemplate mutable, Map<St
264204
formVariables.put(entry.getKey(), entry.getValue());
265205
}
266206
try {
267-
mutable.body(formEncoder.encode(formVariables));
207+
encoder.encode(formVariables, mutable);
268208
} catch (EncodeException e) {
269209
throw e;
270210
} catch (RuntimeException e) {
@@ -275,9 +215,9 @@ protected RequestTemplate resolve(Object[] argv, RequestTemplate mutable, Map<St
275215
}
276216

277217
private static class BuildEncodedTemplateFromArgs extends BuildTemplateByResolvingArgs {
278-
private final Encoder.Text<? super Object> encoder;
218+
private final Encoder encoder;
279219

280-
private BuildEncodedTemplateFromArgs(MethodMetadata metadata, Encoder.Text<? super Object> encoder) {
220+
private BuildEncodedTemplateFromArgs(MethodMetadata metadata, Encoder encoder) {
281221
super(metadata);
282222
this.encoder = encoder;
283223
}
@@ -287,7 +227,7 @@ protected RequestTemplate resolve(Object[] argv, RequestTemplate mutable, Map<St
287227
Object body = argv[metadata.bodyIndex()];
288228
checkArgument(body != null, "Body parameter %s was null", metadata.bodyIndex());
289229
try {
290-
mutable.body(encoder.encode(body));
230+
encoder.encode(body, mutable);
291231
} catch (EncodeException e) {
292232
throw e;
293233
} catch (RuntimeException e) {

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
*/
1616
package feign;
1717

18+
import java.io.Closeable;
1819
import java.io.IOException;
1920
import java.lang.reflect.Array;
2021
import java.lang.reflect.ParameterizedType;
@@ -128,10 +129,10 @@ public static <T> Collection<T> valuesOrEmpty(Map<String, Collection<T>> map, St
128129
return map.containsKey(key) ? map.get(key) : Collections.<T>emptyList();
129130
}
130131

131-
public static void ensureClosed(Response.Body body) {
132-
if (body != null) {
132+
public static void ensureClosed(Closeable closeable) {
133+
if (closeable != null) {
133134
try {
134-
body.close();
135+
closeable.close();
135136
} catch (IOException ignored) { // NOPMD
136137
}
137138
}

0 commit comments

Comments
 (0)