Skip to content

Commit 2d761cb

Browse files
kdavisk6kdavisk6
authored
Refactoring RequestTemplate to RFC6570 (OpenFeign#778)
* Refactoring RequestTemplate to RFC6570 This change refactors `RequestTemplate` in an attempt to adhere to the [RFC-6570 - URI Template](https://tools.ietf.org/html/rfc6570) specification more closely. The reason for this is to reduce the amount of inconsistency between `@Param`, `@QueryMap`, `@Header`, `@HeaderMap`, and `@Body` template expansion. First, `RequestTemplate` now delegates uri, header, query, and body template parsing to `UriTemplate`, `HeaderTemplate`, `QueryTemplate`, and `BodyTemplate` respectively. These components are all variations on a `Template`. `UriTemplate` adheres to RFC 6570 explicitly and supports Level 1 (Simple String) variable expansion. Unresolved variables are ignored and removed from the uri. This includes query parameter pairs. All literal and expanded variables are pct-encoded according to the Charset provided in the `RequestTemplate`. `HeaderTemplate` supports Level 1 (Simple String) variable expansion. Unresolved variables are ignored. Empty headers are removed. No encoding is performed. `QueryTemplate` is a subset of a `UriTemplate` and reacts in the same way. Unresolved pairs are ignored and not present on the final template. All literals and expanded variables are pct-encoded according to the Charset provided. `BodyTemplate` supports Level 1 (Simple String) variable expansion. Unresolved variables produce empty strings. Values are not encoded. All remaining customizations, including custom encoders, collection format expansion and charset encoding are still supportted and made backward compatible. Finally, a number of inconsistent methods on `RequestTemplate` have been deprecated for public use and all deprecated usage throughout the library has been replaced.
1 parent d7cc9b6 commit 2d761cb

61 files changed

Lines changed: 3039 additions & 1014 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎README.md‎

Lines changed: 296 additions & 72 deletions
Large diffs are not rendered by default.

‎benchmark/src/main/java/feign/benchmark/DecoderIteratorsBenchmark.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
import com.fasterxml.jackson.core.type.TypeReference;
1717
import feign.Request;
18+
import feign.Request.HttpMethod;
1819
import feign.Response;
1920
import feign.Util;
2021
import feign.codec.Decoder;
@@ -79,7 +80,7 @@ public void buildResponse() {
7980
response = Response.builder()
8081
.status(200)
8182
.reason("OK")
82-
.request(Request.create("GET", "/", Collections.emptyMap(), null, Util.UTF_8))
83+
.request(Request.create(HttpMethod.GET, "/", Collections.emptyMap(), null, Util.UTF_8))
8384
.headers(Collections.emptyMap())
8485
.body(carsJson(Integer.valueOf(size)), Util.UTF_8)
8586
.build();

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

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@
1313
*/
1414
package feign;
1515

16+
import feign.template.UriUtils;
17+
import java.nio.charset.Charset;
1618
import java.util.Collection;
1719

1820
/**
@@ -61,31 +63,32 @@ public enum CollectionFormat {
6163
*
6264
* @param field The field name corresponding to these values.
6365
* @param values A collection of value strings for the given field.
66+
* @param charset to encode the sequence
6467
* @return The formatted char sequence of the field and joined values. If the value collection is
6568
* empty, an empty char sequence will be returned.
6669
*/
67-
CharSequence join(String field, Collection<String> values) {
70+
public CharSequence join(String field, Collection<String> values, Charset charset) {
6871
StringBuilder builder = new StringBuilder();
6972
int valueCount = 0;
7073
for (String value : values) {
7174
if (separator == null) {
7275
// exploded
7376
builder.append(valueCount++ == 0 ? "" : "&");
74-
builder.append(field);
77+
builder.append(UriUtils.queryEncode(field, charset));
7578
if (value != null) {
7679
builder.append('=');
77-
builder.append(value);
80+
builder.append(UriUtils.queryEncode(value, charset));
7881
}
7982
} else {
8083
// delimited with a separator character
8184
if (builder.length() == 0) {
82-
builder.append(field);
85+
builder.append(UriUtils.queryEncode(field, charset));
8386
}
8487
if (value == null) {
8588
continue;
8689
}
8790
builder.append(valueCount++ == 0 ? "=" : separator);
88-
builder.append(value);
91+
builder.append(UriUtils.queryEncode(value, charset));
8992
}
9093
}
9194
return builder;

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

Lines changed: 18 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,9 @@
1313
*/
1414
package feign;
1515

16+
import static feign.Util.checkState;
17+
import static feign.Util.emptyToNull;
18+
import feign.Request.HttpMethod;
1619
import java.lang.annotation.Annotation;
1720
import java.lang.reflect.Method;
1821
import java.lang.reflect.Modifier;
@@ -24,8 +27,8 @@
2427
import java.util.LinkedHashMap;
2528
import java.util.List;
2629
import java.util.Map;
27-
import static feign.Util.checkState;
28-
import static feign.Util.emptyToNull;
30+
import java.util.regex.Matcher;
31+
import java.util.regex.Pattern;
2932

3033
/**
3134
* Defines what annotations and values are valid on interfaces.
@@ -65,7 +68,7 @@ public List<MethodMetadata> parseAndValidatateMetadata(Class<?> targetType) {
6568
metadata.configKey());
6669
result.put(metadata.configKey(), metadata);
6770
}
68-
return new ArrayList<MethodMetadata>(result.values());
71+
return new ArrayList<>(result.values());
6972
}
7073

7174
/**
@@ -209,6 +212,9 @@ protected void nameParam(MethodMetadata data, String name, int i) {
209212
}
210213

211214
class Default extends BaseContract {
215+
216+
static final Pattern REQUEST_LINE_PATTERN = Pattern.compile("^([A-Z]+)[ ]*(.*)$");
217+
212218
@Override
213219
protected void processAnnotationOnClass(MethodMetadata data, Class<?> targetType) {
214220
if (targetType.isAnnotationPresent(Headers.class)) {
@@ -231,23 +237,16 @@ protected void processAnnotationOnMethod(MethodMetadata data,
231237
String requestLine = RequestLine.class.cast(methodAnnotation).value();
232238
checkState(emptyToNull(requestLine) != null,
233239
"RequestLine annotation was empty on method %s.", method.getName());
234-
if (requestLine.indexOf(' ') == -1) {
235-
checkState(requestLine.indexOf('/') == -1,
236-
"RequestLine annotation didn't start with an HTTP verb on method %s.",
237-
method.getName());
238-
data.template().method(requestLine);
239-
return;
240-
}
241-
data.template().method(requestLine.substring(0, requestLine.indexOf(' ')));
242-
if (requestLine.indexOf(' ') == requestLine.lastIndexOf(' ')) {
243-
// no HTTP version is ok
244-
data.template().append(requestLine.substring(requestLine.indexOf(' ') + 1));
240+
241+
Matcher requestLineMatcher = REQUEST_LINE_PATTERN.matcher(requestLine);
242+
if (!requestLineMatcher.find()) {
243+
throw new IllegalStateException(String.format(
244+
"RequestLine annotation didn't start with an HTTP verb on method %s",
245+
method.getName()));
245246
} else {
246-
// skip HTTP version
247-
data.template().append(
248-
requestLine.substring(requestLine.indexOf(' ') + 1, requestLine.lastIndexOf(' ')));
247+
data.template().method(HttpMethod.valueOf(requestLineMatcher.group(1)));
248+
data.template().uri(requestLineMatcher.group(2));
249249
}
250-
251250
data.template().decodeSlash(RequestLine.class.cast(methodAnnotation).decodeSlash());
252251
data.template()
253252
.collectionFormat(RequestLine.class.cast(methodAnnotation).collectionFormat());
@@ -288,10 +287,7 @@ protected boolean processAnnotationsOnParameter(MethodMetadata data,
288287
}
289288
data.indexToEncoded().put(paramIndex, paramAnnotation.encoded());
290289
isHttpAnnotation = true;
291-
String varName = '{' + name + '}';
292-
if (!data.template().url().contains(varName) &&
293-
!searchMapValuesContainsSubstring(data.template().queries(), varName) &&
294-
!searchMapValuesContainsSubstring(data.template().headers(), varName)) {
290+
if (!data.template().hasRequestVariable(name)) {
295291
data.formParams().add(name);
296292
}
297293
} else if (annotationType == QueryMap.class) {
@@ -310,24 +306,6 @@ protected boolean processAnnotationsOnParameter(MethodMetadata data,
310306
return isHttpAnnotation;
311307
}
312308

313-
private static <K, V> boolean searchMapValuesContainsSubstring(Map<K, Collection<String>> map,
314-
String search) {
315-
Collection<Collection<String>> values = map.values();
316-
if (values == null) {
317-
return false;
318-
}
319-
320-
for (Collection<String> entry : values) {
321-
for (String value : entry) {
322-
if (value.contains(search)) {
323-
return true;
324-
}
325-
}
326-
}
327-
328-
return false;
329-
}
330-
331309
private static Map<String, Collection<String>> toMap(String[] input) {
332310
Map<String, Collection<String>> result =
333311
new LinkedHashMap<String, Collection<String>>(input.length);

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

Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
*/
1414
package feign;
1515

16+
import feign.template.UriUtils;
1617
import java.lang.reflect.InvocationHandler;
1718
import java.lang.reflect.Method;
1819
import java.lang.reflect.Proxy;
@@ -199,11 +200,11 @@ private BuildTemplateByResolvingArgs(MethodMetadata metadata, QueryMapEncoder qu
199200

200201
@Override
201202
public RequestTemplate create(Object[] argv) {
202-
RequestTemplate mutable = new RequestTemplate(metadata.template());
203+
RequestTemplate mutable = RequestTemplate.from(metadata.template());
203204
if (metadata.urlIndex() != null) {
204205
int urlIndex = metadata.urlIndex();
205206
checkArgument(argv[urlIndex] != null, "URI parameter %s was null", urlIndex);
206-
mutable.insert(0, String.valueOf(argv[urlIndex]));
207+
mutable.target(String.valueOf(argv[urlIndex]));
207208
}
208209
Map<String, Object> varBuilder = new LinkedHashMap<String, Object>();
209210
for (Entry<Integer, Collection<String>> entry : metadata.indexToName().entrySet()) {
@@ -300,31 +301,22 @@ private RequestTemplate addQueryMapQueryParameters(Map<String, Object> queryMap,
300301
Object nextObject = iter.next();
301302
values.add(nextObject == null ? null
302303
: encoded ? nextObject.toString()
303-
: RequestTemplate.urlEncode(nextObject.toString()));
304+
: UriUtils.encode(nextObject.toString()));
304305
}
305306
} else {
306307
values.add(currValue == null ? null
307-
: encoded ? currValue.toString() : RequestTemplate.urlEncode(currValue.toString()));
308+
: encoded ? currValue.toString() : UriUtils.encode(currValue.toString()));
308309
}
309310

310-
mutable.query(true,
311-
encoded ? currEntry.getKey() : RequestTemplate.urlEncode(currEntry.getKey()), values);
311+
mutable.query(encoded ? currEntry.getKey() : UriUtils.encode(currEntry.getKey()), values);
312312
}
313313
return mutable;
314314
}
315315

316316
protected RequestTemplate resolve(Object[] argv,
317317
RequestTemplate mutable,
318318
Map<String, Object> variables) {
319-
// Resolving which variable names are already encoded using their indices
320-
Map<String, Boolean> variableToEncoded = new LinkedHashMap<String, Boolean>();
321-
for (Entry<Integer, Boolean> entry : metadata.indexToEncoded().entrySet()) {
322-
Collection<String> names = metadata.indexToName().get(entry.getKey());
323-
for (String name : names) {
324-
variableToEncoded.put(name, entry.getValue());
325-
}
326-
}
327-
return mutable.resolve(variables, variableToEncoded);
319+
return mutable.resolve(variables);
328320
}
329321
}
330322

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

Lines changed: 4 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -18,54 +18,10 @@
1818
import static java.lang.annotation.RetentionPolicy.RUNTIME;
1919

2020
/**
21-
* Expands the request-line supplied in the {@code value}, permitting path and query variables, or
22-
* just the http method. <br>
23-
*
24-
* <pre>
25-
* ...
26-
* &#64;RequestLine("POST /servers")
27-
* ...
28-
*
29-
* &#64;RequestLine("GET /servers/{serverId}?count={count}")
30-
* void get(&#64;Param("serverId") String serverId, &#64;Param("count") int count);
31-
* ...
32-
*
33-
* &#64;RequestLine("GET")
34-
* Response getNext(URI nextLink);
35-
* ...
36-
* </pre>
37-
*
38-
* HTTP version suffix is optional, but permitted. There are no guarantees this version will impact
39-
* that sent by the client. <br>
40-
*
41-
* <pre>
42-
* &#64;RequestLine("POST /servers HTTP/1.1")
43-
* ...
44-
* </pre>
45-
*
46-
* <br>
47-
* <strong>Note:</strong> Query params do not overwrite each other. All queries with the same name
48-
* will be included in the request. <br>
49-
* <br>
50-
* <b>Relationship to JAXRS</b><br>
51-
* <br>
52-
* The following two forms are identical. <br>
53-
* Feign:
54-
*
55-
* <pre>
56-
* &#64;RequestLine("GET /servers/{serverId}?count={count}")
57-
* void get(&#64;Param("serverId") String serverId, &#64;Param("count") int count);
58-
* ...
59-
* </pre>
60-
*
61-
* <br>
62-
* JAX-RS:
63-
*
64-
* <pre>
65-
* &#64;GET &#64;Path("/servers/{serverId}")
66-
* void get(&#64;PathParam("serverId") String serverId, &#64;QueryParam("count") int count);
67-
* ...
68-
* </pre>
21+
* Expands the uri template supplied in the {@code value}, permitting path and query variables, or
22+
* just the http method. Templates should conform to
23+
* <a href="https://tools.ietf.org/html/rfc6570">RFC 6570</a>. Support is limited to Simple String
24+
* expansion and Reserved Expansion (Level 1 and Level 2) expressions.
6925
*/
7026
@java.lang.annotation.Target(METHOD)
7127
@Retention(RUNTIME)

0 commit comments

Comments
 (0)