Skip to content

Commit deda96a

Browse files
parkeeadriancole
parkee
authored and
adriancole
committed
Implements possibility to disable @Param url encoding (OpenFeign#465)
1 parent 0e4a1c8 commit deda96a

9 files changed

Lines changed: 91 additions & 29 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
### Version 9.3
22
* Adds `FallbackFactory`, allowing access to the cause of a Hystrix fallback
3+
* Adds support for encoded parameters via `@Param(encoded = true)`
34

45
### Version 9.2
56
* Adds Hystrix `SetterFactory` to customize group and command keys

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

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -249,19 +249,18 @@ protected boolean processAnnotationsOnParameter(MethodMetadata data, Annotation[
249249
for (Annotation annotation : annotations) {
250250
Class<? extends Annotation> annotationType = annotation.annotationType();
251251
if (annotationType == Param.class) {
252-
String name = ((Param) annotation).value();
253-
checkState(emptyToNull(name) != null, "Param annotation was empty on param %s.",
254-
paramIndex);
252+
Param paramAnnotation = (Param) annotation;
253+
String name = paramAnnotation.value();
254+
checkState(emptyToNull(name) != null, "Param annotation was empty on param %s.", paramIndex);
255255
nameParam(data, name, paramIndex);
256-
if (annotationType == Param.class) {
257-
Class<? extends Param.Expander> expander = ((Param) annotation).expander();
258-
if (expander != Param.ToStringExpander.class) {
259-
data.indexToExpanderClass().put(paramIndex, expander);
260-
}
256+
Class<? extends Param.Expander> expander = paramAnnotation.expander();
257+
if (expander != Param.ToStringExpander.class) {
258+
data.indexToExpanderClass().put(paramIndex, expander);
261259
}
260+
data.indexToEncoded().put(paramIndex, paramAnnotation.encoded());
262261
isHttpAnnotation = true;
263262
String varName = '{' + name + '}';
264-
if (data.template().url().indexOf(varName) == -1 &&
263+
if (!data.template().url().contains(varName) &&
265264
!searchMapValuesContainsSubstring(data.template().queries(), varName) &&
266265
!searchMapValuesContainsSubstring(data.template().headers(), varName)) {
267266
data.formParams().add(name);
@@ -289,7 +288,7 @@ private static <K, V> boolean searchMapValuesContainsSubstring(Map<K, Collection
289288

290289
for (Collection<String> entry : values) {
291290
for (String value : entry) {
292-
if (value.indexOf(search) != -1) {
291+
if (value.contains(search)) {
293292
return true;
294293
}
295294
}

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ public final class MethodMetadata implements Serializable {
4242
new LinkedHashMap<Integer, Collection<String>>();
4343
private Map<Integer, Class<? extends Expander>> indexToExpanderClass =
4444
new LinkedHashMap<Integer, Class<? extends Expander>>();
45+
private Map<Integer, Boolean> indexToEncoded = new LinkedHashMap<Integer, Boolean>();
4546
private transient Map<Integer, Expander> indexToExpander;
4647

4748
MethodMetadata() {
@@ -140,6 +141,10 @@ public Map<Integer, Collection<String>> indexToName() {
140141
return indexToName;
141142
}
142143

144+
public Map<Integer, Boolean> indexToEncoded() {
145+
return indexToEncoded;
146+
}
147+
143148
/**
144149
* If {@link #indexToExpander} is null, classes here will be instantiated by newInstance.
145150
*/

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,14 @@
3838
*/
3939
Class<? extends Expander> expander() default ToStringExpander.class;
4040

41+
/**
42+
* Specifies whether argument is already encoded
43+
* The value is ignored for headers (headers are never encoded)
44+
*
45+
* @see QueryMap#encoded
46+
*/
47+
boolean encoded() default false;
48+
4149
interface Expander {
4250

4351
/**

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,10 @@
6060
@Retention(RUNTIME)
6161
@java.lang.annotation.Target(PARAMETER)
6262
public @interface QueryMap {
63-
/** Specifies whether parameter names and values are already encoded. */
63+
/**
64+
* Specifies whether parameter names and values are already encoded.
65+
*
66+
* @see Param#encoded
67+
*/
6468
boolean encoded() default false;
6569
}

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

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -291,7 +291,15 @@ private RequestTemplate addQueryMapQueryParameters(Object[] argv, RequestTemplat
291291

292292
protected RequestTemplate resolve(Object[] argv, RequestTemplate mutable,
293293
Map<String, Object> variables) {
294-
return mutable.resolve(variables);
294+
// Resolving which variable names are already encoded using their indices
295+
Map<String, Boolean> variableToEncoded = new LinkedHashMap<String, Boolean>();
296+
for (Entry<Integer, Boolean> entry : metadata.indexToEncoded().entrySet()) {
297+
Collection<String> names = metadata.indexToName().get(entry.getKey());
298+
for (String name : names) {
299+
variableToEncoded.put(name, entry.getValue());
300+
}
301+
}
302+
return mutable.resolve(variables, variableToEncoded);
295303
}
296304
}
297305

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

Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ public static String expand(String template, Map<String, ?> variables) {
117117
// skip expansion if there's no valid variables set. ex. {a} is the
118118
// first valid
119119
if (checkNotNull(template, "template").length() < 3) {
120-
return template.toString();
120+
return template;
121121
}
122122
checkNotNull(variables, "variables for %s", template);
123123

@@ -200,21 +200,29 @@ private static void putKV(String stringToParse, Map<String, Collection<String>>
200200
map.put(key, values);
201201
}
202202

203+
/** {@link #resolve(Map, Map)}, which assumes no parameter is encoded */
204+
public RequestTemplate resolve(Map<String, ?> unencoded) {
205+
return resolve(unencoded, Collections.<String, Boolean>emptyMap());
206+
}
207+
203208
/**
204209
* Resolves any template parameters in the requests path, query, or headers against the supplied
205210
* unencoded arguments. <br> <br><br><b>relationship to JAXRS 2.0</b><br> <br> This call is
206211
* similar to {@code javax.ws.rs.client.WebTarget.resolveTemplates(templateValues, true)} , except
207212
* that the template values apply to any part of the request, not just the URL
208213
*/
209-
public RequestTemplate resolve(Map<String, ?> unencoded) {
210-
replaceQueryValues(unencoded);
214+
RequestTemplate resolve(Map<String, ?> unencoded, Map<String, Boolean> alreadyEncoded) {
215+
replaceQueryValues(unencoded, alreadyEncoded);
211216
Map<String, String> encoded = new LinkedHashMap<String, String>();
212217
for (Entry<String, ?> entry : unencoded.entrySet()) {
213-
encoded.put(entry.getKey(), urlEncode(String.valueOf(entry.getValue())));
218+
final String key = entry.getKey();
219+
final Object objectValue = entry.getValue();
220+
String encodedValue = encodeValueIfNotEncoded(key, objectValue, alreadyEncoded);
221+
encoded.put(key, encodedValue);
214222
}
215223
String resolvedUrl = expand(url.toString(), encoded).replace("+", "%20");
216224
if (decodeSlash) {
217-
resolvedUrl = resolvedUrl.replace("%2F", "/");
225+
resolvedUrl = resolvedUrl.replace("%2F", "/");
218226
}
219227
url = new StringBuilder(resolvedUrl);
220228

@@ -235,13 +243,21 @@ public RequestTemplate resolve(Map<String, ?> unencoded) {
235243
return this;
236244
}
237245

246+
private String encodeValueIfNotEncoded(String key, Object objectValue, Map<String, Boolean> alreadyEncoded) {
247+
String value = String.valueOf(objectValue);
248+
final Boolean isEncoded = alreadyEncoded.get(key);
249+
if (isEncoded == null || !isEncoded) {
250+
value = urlEncode(value);
251+
}
252+
return value;
253+
}
254+
238255
/* roughly analogous to {@code javax.ws.rs.client.Target.request()}. */
239256
public Request request() {
240257
Map<String, Collection<String>> safeCopy = new LinkedHashMap<String, Collection<String>>();
241258
safeCopy.putAll(headers);
242259
return Request.create(
243-
method,
244-
new StringBuilder(url).append(queryLine()).toString(),
260+
method, url + queryLine(),
245261
Collections.unmodifiableMap(safeCopy),
246262
body, charset
247263
);
@@ -589,11 +605,16 @@ public String toString() {
589605
return request().toString();
590606
}
591607

608+
/** {@link #replaceQueryValues(Map, Map)}, which assumes no parameter is encoded */
609+
public void replaceQueryValues(Map<String, ?> unencoded) {
610+
replaceQueryValues(unencoded, Collections.<String, Boolean>emptyMap());
611+
}
612+
592613
/**
593614
* Replaces query values which are templated with corresponding values from the {@code unencoded}
594615
* map. Any unresolved queries are removed.
595616
*/
596-
public void replaceQueryValues(Map<String, ?> unencoded) {
617+
void replaceQueryValues(Map<String, ?> unencoded, Map<String, Boolean> alreadyEncoded) {
597618
Iterator<Entry<String, Collection<String>>> iterator = queries.entrySet().iterator();
598619
while (iterator.hasNext()) {
599620
Entry<String, Collection<String>> entry = iterator.next();
@@ -610,10 +631,12 @@ public void replaceQueryValues(Map<String, ?> unencoded) {
610631
}
611632
if (variableValue instanceof Iterable) {
612633
for (Object val : Iterable.class.cast(variableValue)) {
613-
values.add(urlEncode(String.valueOf(val)));
634+
String encodedValue = encodeValueIfNotEncoded(entry.getKey(), val, alreadyEncoded);
635+
values.add(encodedValue);
614636
}
615637
} else {
616-
values.add(urlEncode(String.valueOf(variableValue)));
638+
String encodedValue = encodeValueIfNotEncoded(entry.getKey(), variableValue, alreadyEncoded);
639+
values.add(encodedValue);
617640
}
618641
} else {
619642
values.add(value);

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -646,6 +646,18 @@ public void encodeLogicSupportsByteArray() throws Exception {
646646
.hasBody(expectedRequest);
647647
}
648648

649+
@Test
650+
public void encodedQueryParam() throws Exception {
651+
server.enqueue(new MockResponse());
652+
653+
TestInterface api = new TestInterfaceBuilder().target("http://localhost:" + server.getPort());
654+
655+
api.encodedQueryParam("5.2FSi+");
656+
657+
assertThat(server.takeRequest())
658+
.hasPath("/?trim=5.2FSi+");
659+
}
660+
649661
interface TestInterface {
650662

651663
@RequestLine("POST /")
@@ -704,6 +716,9 @@ void form(
704716
@RequestLine("GET /?name={name}")
705717
void queryMapWithQueryParams(@Param("name") String name, @QueryMap Map<String, Object> queryMap);
706718

719+
@RequestLine("GET /?trim={trim}")
720+
void encodedQueryParam(@Param(value = "trim", encoded = true) String trim);
721+
707722
class DateToMillis implements Param.Expander {
708723

709724
@Override

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

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -36,21 +36,20 @@ public class RequestTemplateTest {
3636
/**
3737
* Avoid depending on guava solely for map literals.
3838
*/
39-
private static Map<String, Object> mapOf(String key, Object val) {
40-
Map<String, Object> result = new LinkedHashMap<String, Object>();
39+
private static <K, V> Map<K, V> mapOf(K key, V val) {
40+
Map<K, V> result = new LinkedHashMap<K, V>();
4141
result.put(key, val);
4242
return result;
4343
}
4444

45-
private static Map<String, Object> mapOf(String k1, Object v1, String k2, Object v2) {
46-
Map<String, Object> result = mapOf(k1, v1);
45+
private static <K, V> Map<K, V> mapOf(K k1, V v1, K k2, V v2) {
46+
Map<K, V> result = mapOf(k1, v1);
4747
result.put(k2, v2);
4848
return result;
4949
}
5050

51-
private static Map<String, Object> mapOf(String k1, Object v1, String k2, Object v2, String k3,
52-
Object v3) {
53-
Map<String, Object> result = mapOf(k1, v1, k2, v2);
51+
private static <K, V> Map<K, V> mapOf(K k1, V v1, K k2, V v2, K k3, V v3) {
52+
Map<K, V> result = mapOf(k1, v1, k2, v2);
5453
result.put(k3, v3);
5554
return result;
5655
}

0 commit comments

Comments
 (0)