Skip to content

Commit a029afd

Browse files
EthanLozanoadriancole
EthanLozano
authored and
adriancole
committed
Perform type validation for QueryMap and HeaderMap in Contract (OpenFeign#573)
1 parent 0142f0c commit a029afd

4 files changed

Lines changed: 37 additions & 36 deletions

File tree

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

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@
1818
import java.lang.annotation.Annotation;
1919
import java.lang.reflect.Method;
2020
import java.lang.reflect.Modifier;
21+
import java.lang.reflect.ParameterizedType;
22+
import java.lang.reflect.Type;
2123
import java.net.URI;
2224
import java.util.ArrayList;
2325
import java.util.Collection;
@@ -98,6 +100,7 @@ protected MethodMetadata parseAndValidateMetadata(Class<?> targetType, Method me
98100
"Method %s not annotated with HTTP method type (ex. GET, POST)",
99101
method.getName());
100102
Class<?>[] parameterTypes = method.getParameterTypes();
103+
Type[] genericParameterTypes = method.getGenericParameterTypes();
101104

102105
Annotation[][] parameterAnnotations = method.getParameterAnnotations();
103106
int count = parameterAnnotations.length;
@@ -113,23 +116,30 @@ protected MethodMetadata parseAndValidateMetadata(Class<?> targetType, Method me
113116
"Body parameters cannot be used with form parameters.");
114117
checkState(data.bodyIndex() == null, "Method has too many Body parameters: %s", method);
115118
data.bodyIndex(i);
116-
data.bodyType(Types.resolve(targetType, targetType, method.getGenericParameterTypes()[i]));
119+
data.bodyType(Types.resolve(targetType, targetType, genericParameterTypes[i]));
117120
}
118121
}
119122

120123
if (data.headerMapIndex() != null) {
121-
checkState(Map.class.isAssignableFrom(parameterTypes[data.headerMapIndex()]),
122-
"HeaderMap parameter must be a Map: %s", parameterTypes[data.headerMapIndex()]);
124+
checkMapString("HeaderMap", parameterTypes[data.headerMapIndex()], genericParameterTypes[data.headerMapIndex()]);
123125
}
124126

125127
if (data.queryMapIndex() != null) {
126-
checkState(Map.class.isAssignableFrom(parameterTypes[data.queryMapIndex()]),
127-
"QueryMap parameter must be a Map: %s", parameterTypes[data.queryMapIndex()]);
128+
checkMapString("QueryMap", parameterTypes[data.queryMapIndex()], genericParameterTypes[data.queryMapIndex()]);
128129
}
129130

130131
return data;
131132
}
132133

134+
private static void checkMapString(String name, Class<?> type, Type genericType) {
135+
checkState(Map.class.isAssignableFrom(type),
136+
"%s parameter must be a Map: %s", name, type);
137+
Type[] parameterTypes = ((ParameterizedType) genericType).getActualTypeArguments();
138+
Class<?> keyClass = (Class<?>) parameterTypes[0];
139+
checkState(String.class.equals(keyClass),
140+
"%s key must be a String: %s", name, keyClass.getSimpleName());
141+
}
142+
133143
/**
134144
* Called by parseAndValidateMetadata twice, first on the declaring class, then on the
135145
* target type (unless they are the same).

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

Lines changed: 8 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -214,11 +214,11 @@ public RequestTemplate create(Object[] argv) {
214214
if (metadata.queryMapIndex() != null) {
215215
// add query map parameters after initial resolve so that they take
216216
// precedence over any predefined values
217-
template = addQueryMapQueryParameters(argv, template);
217+
template = addQueryMapQueryParameters((Map<String, Object>) argv[metadata.queryMapIndex()], template);
218218
}
219219

220220
if (metadata.headerMapIndex() != null) {
221-
template = addHeaderMapHeaders(argv, template);
221+
template = addHeaderMapHeaders((Map<String, Object>) argv[metadata.headerMapIndex()], template);
222222
}
223223

224224
return template;
@@ -242,11 +242,8 @@ private List<String> expandIterable(Expander expander, Iterable value) {
242242
}
243243

244244
@SuppressWarnings("unchecked")
245-
private RequestTemplate addHeaderMapHeaders(Object[] argv, RequestTemplate mutable) {
246-
Map<Object, Object> headerMap = (Map<Object, Object>) argv[metadata.headerMapIndex()];
247-
for (Entry<Object, Object> currEntry : headerMap.entrySet()) {
248-
checkState(currEntry.getKey().getClass() == String.class, "HeaderMap key must be a String: %s", currEntry.getKey());
249-
245+
private RequestTemplate addHeaderMapHeaders(Map<String, Object> headerMap, RequestTemplate mutable) {
246+
for (Entry<String, Object> currEntry : headerMap.entrySet()) {
250247
Collection<String> values = new ArrayList<String>();
251248

252249
Object currValue = currEntry.getValue();
@@ -260,17 +257,14 @@ private RequestTemplate addHeaderMapHeaders(Object[] argv, RequestTemplate mutab
260257
values.add(currValue == null ? null : currValue.toString());
261258
}
262259

263-
mutable.header((String) currEntry.getKey(), values);
260+
mutable.header(currEntry.getKey(), values);
264261
}
265262
return mutable;
266263
}
267264

268265
@SuppressWarnings("unchecked")
269-
private RequestTemplate addQueryMapQueryParameters(Object[] argv, RequestTemplate mutable) {
270-
Map<Object, Object> queryMap = (Map<Object, Object>) argv[metadata.queryMapIndex()];
271-
for (Entry<Object, Object> currEntry : queryMap.entrySet()) {
272-
checkState(currEntry.getKey().getClass() == String.class, "QueryMap key must be a String: %s", currEntry.getKey());
273-
266+
private RequestTemplate addQueryMapQueryParameters(Map<String, Object> queryMap, RequestTemplate mutable) {
267+
for (Entry<String, Object> currEntry : queryMap.entrySet()) {
274268
Collection<String> values = new ArrayList<String>();
275269

276270
boolean encoded = metadata.queryMapEncoded();
@@ -285,7 +279,7 @@ private RequestTemplate addQueryMapQueryParameters(Object[] argv, RequestTemplat
285279
values.add(currValue == null ? null : encoded ? currValue.toString() : RequestTemplate.urlEncode(currValue.toString()));
286280
}
287281

288-
mutable.query(true, encoded ? (String) currEntry.getKey() : RequestTemplate.urlEncode(currEntry.getKey()), values);
282+
mutable.query(true, encoded ? currEntry.getKey() : RequestTemplate.urlEncode(currEntry.getKey()), values);
289283
}
290284
return mutable;
291285
}

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -328,6 +328,16 @@ public void queryMapMustBeInstanceOfMap() throws Exception {
328328
}
329329
}
330330

331+
@Test
332+
public void queryMapKeysMustBeStrings() throws Exception {
333+
try {
334+
parseAndValidateMetadata(QueryMapTestInterface.class, "nonStringKeyQueryMap", Map.class);
335+
Fail.failBecauseExceptionWasNotThrown(IllegalStateException.class);
336+
} catch (IllegalStateException ex) {
337+
assertThat(ex).hasMessage("QueryMap key must be a String: Integer");
338+
}
339+
}
340+
331341
@Test
332342
public void slashAreEncodedWhenNeeded() throws Exception {
333343
MethodMetadata md = parseAndValidateMetadata(SlashNeedToBeEncoded.class,
@@ -500,6 +510,10 @@ interface QueryMapTestInterface {
500510
// invalid
501511
@RequestLine("POST /")
502512
void nonMapQueryMap(@QueryMap String notAMap);
513+
514+
// invalid
515+
@RequestLine("POST /")
516+
void nonStringKeyQueryMap(@QueryMap Map<Integer, String> queryMap);
503517
}
504518

505519
interface SlashNeedToBeEncoded {

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

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -353,23 +353,6 @@ public void queryMapWithQueryParams() throws Exception {
353353
.hasPath("/");
354354
}
355355

356-
@Test
357-
public void queryMapKeysMustBeStrings() throws Exception {
358-
server.enqueue(new MockResponse());
359-
360-
TestInterface api = new TestInterfaceBuilder().target("http://localhost:" + server.getPort());
361-
362-
Map<Object, String> queryMap = new LinkedHashMap<Object, String>();
363-
queryMap.put(Integer.valueOf(42), "alice");
364-
365-
try {
366-
api.queryMap((Map) queryMap);
367-
Fail.failBecauseExceptionWasNotThrown(IllegalStateException.class);
368-
} catch (IllegalStateException ex) {
369-
assertThat(ex).hasMessage("QueryMap key must be a String: 42");
370-
}
371-
}
372-
373356
@Test
374357
public void queryMapValueStartingWithBrace() throws Exception {
375358
TestInterface api = new TestInterfaceBuilder().target("http://localhost:" + server.getPort());

0 commit comments

Comments
 (0)