diff --git a/core/src/main/java/feign/Contract.java b/core/src/main/java/feign/Contract.java index 54e9a39e0..129f2204b 100644 --- a/core/src/main/java/feign/Contract.java +++ b/core/src/main/java/feign/Contract.java @@ -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; @@ -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; @@ -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). diff --git a/core/src/main/java/feign/ReflectiveFeign.java b/core/src/main/java/feign/ReflectiveFeign.java index f39eeb030..d7c99c851 100644 --- a/core/src/main/java/feign/ReflectiveFeign.java +++ b/core/src/main/java/feign/ReflectiveFeign.java @@ -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) argv[metadata.queryMapIndex()], template); } if (metadata.headerMapIndex() != null) { - template = addHeaderMapHeaders(argv, template); + template = addHeaderMapHeaders((Map) argv[metadata.headerMapIndex()], template); } return template; @@ -242,11 +242,8 @@ private List expandIterable(Expander expander, Iterable value) { } @SuppressWarnings("unchecked") - private RequestTemplate addHeaderMapHeaders(Object[] argv, RequestTemplate mutable) { - Map headerMap = (Map) argv[metadata.headerMapIndex()]; - for (Entry currEntry : headerMap.entrySet()) { - checkState(currEntry.getKey().getClass() == String.class, "HeaderMap key must be a String: %s", currEntry.getKey()); - + private RequestTemplate addHeaderMapHeaders(Map headerMap, RequestTemplate mutable) { + for (Entry currEntry : headerMap.entrySet()) { Collection values = new ArrayList(); Object currValue = currEntry.getValue(); @@ -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 queryMap = (Map) argv[metadata.queryMapIndex()]; - for (Entry currEntry : queryMap.entrySet()) { - checkState(currEntry.getKey().getClass() == String.class, "QueryMap key must be a String: %s", currEntry.getKey()); - + private RequestTemplate addQueryMapQueryParameters(Map queryMap, RequestTemplate mutable) { + for (Entry currEntry : queryMap.entrySet()) { Collection values = new ArrayList(); boolean encoded = metadata.queryMapEncoded(); @@ -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; } diff --git a/core/src/test/java/feign/DefaultContractTest.java b/core/src/test/java/feign/DefaultContractTest.java index 911058f80..c73c60d19 100644 --- a/core/src/test/java/feign/DefaultContractTest.java +++ b/core/src/test/java/feign/DefaultContractTest.java @@ -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, @@ -500,6 +510,10 @@ interface QueryMapTestInterface { // invalid @RequestLine("POST /") void nonMapQueryMap(@QueryMap String notAMap); + + // invalid + @RequestLine("POST /") + void nonStringKeyQueryMap(@QueryMap Map queryMap); } interface SlashNeedToBeEncoded { diff --git a/core/src/test/java/feign/FeignTest.java b/core/src/test/java/feign/FeignTest.java index 2d3e55cb0..13a95efac 100644 --- a/core/src/test/java/feign/FeignTest.java +++ b/core/src/test/java/feign/FeignTest.java @@ -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 queryMap = new LinkedHashMap(); - 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());