Skip to content

Commit e9b3200

Browse files
kdavisk6kdavisk6
authored
Refactoring RequestTemplate to RFC6570 (#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 3711d3c commit e9b3200

61 files changed

Lines changed: 2995 additions & 959 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;
@@ -78,7 +79,7 @@ public void buildResponse() {
7879
Response.builder()
7980
.status(200)
8081
.reason("OK")
81-
.request(Request.create("GET", "/", Collections.emptyMap(), null, Util.UTF_8))
82+
.request(Request.create(HttpMethod.GET, "/", Collections.emptyMap(), null, Util.UTF_8))
8283
.headers(Collections.emptyMap())
8384
.body(carsJson(Integer.valueOf(size)), Util.UTF_8)
8485
.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
/**
@@ -53,31 +55,32 @@ public enum CollectionFormat {
5355
*
5456
* @param field The field name corresponding to these values.
5557
* @param values A collection of value strings for the given field.
58+
* @param charset to encode the sequence
5659
* @return The formatted char sequence of the field and joined values. If the value collection is
5760
* empty, an empty char sequence will be returned.
5861
*/
59-
CharSequence join(String field, Collection<String> values) {
62+
public CharSequence join(String field, Collection<String> values, Charset charset) {
6063
StringBuilder builder = new StringBuilder();
6164
int valueCount = 0;
6265
for (String value : values) {
6366
if (separator == null) {
6467
// exploded
6568
builder.append(valueCount++ == 0 ? "" : "&");
66-
builder.append(field);
69+
builder.append(UriUtils.queryEncode(field, charset));
6770
if (value != null) {
6871
builder.append('=');
69-
builder.append(value);
72+
builder.append(UriUtils.queryEncode(value, charset));
7073
}
7174
} else {
7275
// delimited with a separator character
7376
if (builder.length() == 0) {
74-
builder.append(field);
77+
builder.append(UriUtils.queryEncode(field, charset));
7578
}
7679
if (value == null) {
7780
continue;
7881
}
7982
builder.append(valueCount++ == 0 ? "=" : separator);
80-
builder.append(value);
83+
builder.append(UriUtils.queryEncode(value, charset));
8184
}
8285
}
8386
return builder;

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

Lines changed: 17 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import static feign.Util.checkState;
1717
import static feign.Util.emptyToNull;
1818

19+
import feign.Request.HttpMethod;
1920
import java.lang.annotation.Annotation;
2021
import java.lang.reflect.Method;
2122
import java.lang.reflect.Modifier;
@@ -27,6 +28,8 @@
2728
import java.util.LinkedHashMap;
2829
import java.util.List;
2930
import java.util.Map;
31+
import java.util.regex.Matcher;
32+
import java.util.regex.Pattern;
3033

3134
/** Defines what annotations and values are valid on interfaces. */
3235
public interface Contract {
@@ -71,7 +74,7 @@ public List<MethodMetadata> parseAndValidatateMetadata(Class<?> targetType) {
7174
metadata.configKey());
7275
result.put(metadata.configKey(), metadata);
7376
}
74-
return new ArrayList<MethodMetadata>(result.values());
77+
return new ArrayList<>(result.values());
7578
}
7679

7780
/**
@@ -212,6 +215,9 @@ protected void nameParam(MethodMetadata data, String name, int i) {
212215
}
213216

214217
class Default extends BaseContract {
218+
219+
static final Pattern REQUEST_LINE_PATTERN = Pattern.compile("^([A-Z]+)[ ]*(.*)$");
220+
215221
@Override
216222
protected void processAnnotationOnClass(MethodMetadata data, Class<?> targetType) {
217223
if (targetType.isAnnotationPresent(Headers.class)) {
@@ -237,26 +243,17 @@ protected void processAnnotationOnMethod(
237243
emptyToNull(requestLine) != null,
238244
"RequestLine annotation was empty on method %s.",
239245
method.getName());
240-
if (requestLine.indexOf(' ') == -1) {
241-
checkState(
242-
requestLine.indexOf('/') == -1,
243-
"RequestLine annotation didn't start with an HTTP verb on method %s.",
244-
method.getName());
245-
data.template().method(requestLine);
246-
return;
247-
}
248-
data.template().method(requestLine.substring(0, requestLine.indexOf(' ')));
249-
if (requestLine.indexOf(' ') == requestLine.lastIndexOf(' ')) {
250-
// no HTTP version is ok
251-
data.template().append(requestLine.substring(requestLine.indexOf(' ') + 1));
246+
247+
Matcher requestLineMatcher = REQUEST_LINE_PATTERN.matcher(requestLine);
248+
if (!requestLineMatcher.find()) {
249+
throw new IllegalStateException(
250+
String.format(
251+
"RequestLine annotation didn't start with an HTTP verb on method %s",
252+
method.getName()));
252253
} else {
253-
// skip HTTP version
254-
data.template()
255-
.append(
256-
requestLine.substring(
257-
requestLine.indexOf(' ') + 1, requestLine.lastIndexOf(' ')));
254+
data.template().method(HttpMethod.valueOf(requestLineMatcher.group(1)));
255+
data.template().uri(requestLineMatcher.group(2));
258256
}
259-
260257
data.template().decodeSlash(RequestLine.class.cast(methodAnnotation).decodeSlash());
261258
data.template()
262259
.collectionFormat(RequestLine.class.cast(methodAnnotation).collectionFormat());
@@ -298,10 +295,7 @@ protected boolean processAnnotationsOnParameter(
298295
}
299296
data.indexToEncoded().put(paramIndex, paramAnnotation.encoded());
300297
isHttpAnnotation = true;
301-
String varName = '{' + name + '}';
302-
if (!data.template().url().contains(varName)
303-
&& !searchMapValuesContainsSubstring(data.template().queries(), varName)
304-
&& !searchMapValuesContainsSubstring(data.template().headers(), varName)) {
298+
if (!data.template().hasRequestVariable(name)) {
305299
data.formParams().add(name);
306300
}
307301
} else if (annotationType == QueryMap.class) {
@@ -322,24 +316,6 @@ protected boolean processAnnotationsOnParameter(
322316
return isHttpAnnotation;
323317
}
324318

325-
private static <K, V> boolean searchMapValuesContainsSubstring(
326-
Map<K, Collection<String>> map, String search) {
327-
Collection<Collection<String>> values = map.values();
328-
if (values == null) {
329-
return false;
330-
}
331-
332-
for (Collection<String> entry : values) {
333-
for (String value : entry) {
334-
if (value.contains(search)) {
335-
return true;
336-
}
337-
}
338-
}
339-
340-
return false;
341-
}
342-
343319
private static Map<String, Collection<String>> toMap(String[] input) {
344320
Map<String, Collection<String>> result =
345321
new LinkedHashMap<String, Collection<String>>(input.length);

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

Lines changed: 7 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import feign.codec.EncodeException;
2424
import feign.codec.Encoder;
2525
import feign.codec.ErrorDecoder;
26+
import feign.template.UriUtils;
2627
import java.lang.reflect.InvocationHandler;
2728
import java.lang.reflect.Method;
2829
import java.lang.reflect.Proxy;
@@ -204,11 +205,11 @@ private BuildTemplateByResolvingArgs(MethodMetadata metadata, QueryMapEncoder qu
204205

205206
@Override
206207
public RequestTemplate create(Object[] argv) {
207-
RequestTemplate mutable = new RequestTemplate(metadata.template());
208+
RequestTemplate mutable = RequestTemplate.from(metadata.template());
208209
if (metadata.urlIndex() != null) {
209210
int urlIndex = metadata.urlIndex();
210211
checkArgument(argv[urlIndex] != null, "URI parameter %s was null", urlIndex);
211-
mutable.insert(0, String.valueOf(argv[urlIndex]));
212+
mutable.target(String.valueOf(argv[urlIndex]));
212213
}
213214
Map<String, Object> varBuilder = new LinkedHashMap<String, Object>();
214215
for (Entry<Integer, Collection<String>> entry : metadata.indexToName().entrySet()) {
@@ -306,38 +307,23 @@ private RequestTemplate addQueryMapQueryParameters(
306307
values.add(
307308
nextObject == null
308309
? null
309-
: encoded
310-
? nextObject.toString()
311-
: RequestTemplate.urlEncode(nextObject.toString()));
310+
: encoded ? nextObject.toString() : UriUtils.encode(nextObject.toString()));
312311
}
313312
} else {
314313
values.add(
315314
currValue == null
316315
? null
317-
: encoded
318-
? currValue.toString()
319-
: RequestTemplate.urlEncode(currValue.toString()));
316+
: encoded ? currValue.toString() : UriUtils.encode(currValue.toString()));
320317
}
321318

322-
mutable.query(
323-
true,
324-
encoded ? currEntry.getKey() : RequestTemplate.urlEncode(currEntry.getKey()),
325-
values);
319+
mutable.query(encoded ? currEntry.getKey() : UriUtils.encode(currEntry.getKey()), values);
326320
}
327321
return mutable;
328322
}
329323

330324
protected RequestTemplate resolve(
331325
Object[] argv, RequestTemplate mutable, Map<String, Object> variables) {
332-
// Resolving which variable names are already encoded using their indices
333-
Map<String, Boolean> variableToEncoded = new LinkedHashMap<String, Boolean>();
334-
for (Entry<Integer, Boolean> entry : metadata.indexToEncoded().entrySet()) {
335-
Collection<String> names = metadata.indexToName().get(entry.getKey());
336-
for (String name : names) {
337-
variableToEncoded.put(name, entry.getValue());
338-
}
339-
}
340-
return mutable.resolve(variables, variableToEncoded);
326+
return mutable.resolve(variables);
341327
}
342328
}
343329

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

Lines changed: 4 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -19,54 +19,10 @@
1919
import java.lang.annotation.Retention;
2020

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

0 commit comments

Comments
 (0)