Skip to content

Commit

Permalink
Perform type validation for QueryMap and HeaderMap in Contract (#573)
Browse files Browse the repository at this point in the history
  • Loading branch information
@EthanLozano @adriancole
EthanLozano authored and adriancole committed Jul 6, 2017
1 parent 0142f0c commit a029afd
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 36 deletions.
20 changes: 15 additions & 5 deletions core/src/main/java/feign/Contract.java
View file
Expand Up @@ -18,6 +18,8 @@
import java.lang.annotation.Annotation;
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.lang.reflect.ParameterizedType;
import java.lang.reflect.Type;
import java.net.URI;
import java.util.ArrayList;
import java.util.Collection;
Expand Down Expand Up @@ -98,6 +100,7 @@ protected MethodMetadata parseAndValidateMetadata(Class<?> targetType, Method me
"Method %s not annotated with HTTP method type (ex. GET, POST)",
method.getName());
Class<?>[] parameterTypes = method.getParameterTypes();
Type[] genericParameterTypes = method.getGenericParameterTypes();

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

if (data.headerMapIndex() != null) {
checkState(Map.class.isAssignableFrom(parameterTypes[data.headerMapIndex()]),
"HeaderMap parameter must be a Map: %s", parameterTypes[data.headerMapIndex()]);
checkMapString("HeaderMap", parameterTypes[data.headerMapIndex()], genericParameterTypes[data.headerMapIndex()]);
}

if (data.queryMapIndex() != null) {
checkState(Map.class.isAssignableFrom(parameterTypes[data.queryMapIndex()]),
"QueryMap parameter must be a Map: %s", parameterTypes[data.queryMapIndex()]);
checkMapString("QueryMap", parameterTypes[data.queryMapIndex()], genericParameterTypes[data.queryMapIndex()]);
}

return data;
}

private static void checkMapString(String name, Class<?> type, Type genericType) {
checkState(Map.class.isAssignableFrom(type),
"%s parameter must be a Map: %s", name, type);
Type[] parameterTypes = ((ParameterizedType) genericType).getActualTypeArguments();
Class<?> keyClass = (Class<?>) parameterTypes[0];
checkState(String.class.equals(keyClass),
"%s key must be a String: %s", name, keyClass.getSimpleName());
}

/**
* Called by parseAndValidateMetadata twice, first on the declaring class, then on the
* target type (unless they are the same).
Expand Down
22 changes: 8 additions & 14 deletions core/src/main/java/feign/ReflectiveFeign.java
View file
Expand Up @@ -214,11 +214,11 @@ public RequestTemplate create(Object[] argv) {
if (metadata.queryMapIndex() != null) {
// add query map parameters after initial resolve so that they take
// precedence over any predefined values
template = addQueryMapQueryParameters(argv, template);
template = addQueryMapQueryParameters((Map<String, Object>) argv[metadata.queryMapIndex()], template);
}

if (metadata.headerMapIndex() != null) {
template = addHeaderMapHeaders(argv, template);
template = addHeaderMapHeaders((Map<String, Object>) argv[metadata.headerMapIndex()], template);
}

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

@SuppressWarnings("unchecked")
private RequestTemplate addHeaderMapHeaders(Object[] argv, RequestTemplate mutable) {
Map<Object, Object> headerMap = (Map<Object, Object>) argv[metadata.headerMapIndex()];
for (Entry<Object, Object> currEntry : headerMap.entrySet()) {
checkState(currEntry.getKey().getClass() == String.class, "HeaderMap key must be a String: %s", currEntry.getKey());

private RequestTemplate addHeaderMapHeaders(Map<String, Object> headerMap, RequestTemplate mutable) {
for (Entry<String, Object> currEntry : headerMap.entrySet()) {
Collection<String> values = new ArrayList<String>();

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

mutable.header((String) currEntry.getKey(), values);
mutable.header(currEntry.getKey(), values);
}
return mutable;
}

@SuppressWarnings("unchecked")
private RequestTemplate addQueryMapQueryParameters(Object[] argv, RequestTemplate mutable) {
Map<Object, Object> queryMap = (Map<Object, Object>) argv[metadata.queryMapIndex()];
for (Entry<Object, Object> currEntry : queryMap.entrySet()) {
checkState(currEntry.getKey().getClass() == String.class, "QueryMap key must be a String: %s", currEntry.getKey());

private RequestTemplate addQueryMapQueryParameters(Map<String, Object> queryMap, RequestTemplate mutable) {
for (Entry<String, Object> currEntry : queryMap.entrySet()) {
Collection<String> values = new ArrayList<String>();

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

mutable.query(true, encoded ? (String) currEntry.getKey() : RequestTemplate.urlEncode(currEntry.getKey()), values);
mutable.query(true, encoded ? currEntry.getKey() : RequestTemplate.urlEncode(currEntry.getKey()), values);
}
return mutable;
}
Expand Down
14 changes: 14 additions & 0 deletions core/src/test/java/feign/DefaultContractTest.java
View file
Expand Up @@ -328,6 +328,16 @@ public void queryMapMustBeInstanceOfMap() throws Exception {
}
}

@Test
public void queryMapKeysMustBeStrings() throws Exception {
try {
parseAndValidateMetadata(QueryMapTestInterface.class, "nonStringKeyQueryMap", Map.class);
Fail.failBecauseExceptionWasNotThrown(IllegalStateException.class);
} catch (IllegalStateException ex) {
assertThat(ex).hasMessage("QueryMap key must be a String: Integer");
}
}

@Test
public void slashAreEncodedWhenNeeded() throws Exception {
MethodMetadata md = parseAndValidateMetadata(SlashNeedToBeEncoded.class,
Expand Down Expand Up @@ -500,6 +510,10 @@ interface QueryMapTestInterface {
// invalid
@RequestLine("POST /")
void nonMapQueryMap(@QueryMap String notAMap);

// invalid
@RequestLine("POST /")
void nonStringKeyQueryMap(@QueryMap Map<Integer, String> queryMap);
}

interface SlashNeedToBeEncoded {
Expand Down
17 changes: 0 additions & 17 deletions core/src/test/java/feign/FeignTest.java
View file
Expand Up @@ -353,23 +353,6 @@ public void queryMapWithQueryParams() throws Exception {
.hasPath("/");
}

@Test
public void queryMapKeysMustBeStrings() throws Exception {
server.enqueue(new MockResponse());

TestInterface api = new TestInterfaceBuilder().target("http://localhost:" + server.getPort());

Map<Object, String> queryMap = new LinkedHashMap<Object, String>();
queryMap.put(Integer.valueOf(42), "alice");

try {
api.queryMap((Map) queryMap);
Fail.failBecauseExceptionWasNotThrown(IllegalStateException.class);
} catch (IllegalStateException ex) {
assertThat(ex).hasMessage("QueryMap key must be a String: 42");
}
}

@Test
public void queryMapValueStartingWithBrace() throws Exception {
TestInterface api = new TestInterfaceBuilder().target("http://localhost:" + server.getPort());
Expand Down

0 comments on commit a029afd

Please sign in to comment.