From 2138a6256e3a23391361c5b573e20b5a8c4a4818 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johan=20Str=C3=B6m?= Date: Mon, 7 Sep 2015 09:33:49 +0200 Subject: [PATCH] Fix variable expansion in @Header class-annotation as discussed in #262 --- core/src/main/java/feign/Contract.java | 30 +++++++++++-------- .../test/java/feign/DefaultContractTest.java | 26 +++++++++++++++- .../main/java/feign/jaxrs/JAXRSContract.java | 15 ++++++---- 3 files changed, 52 insertions(+), 19 deletions(-) diff --git a/core/src/main/java/feign/Contract.java b/core/src/main/java/feign/Contract.java index dbbc64b2..3cc6d128 100644 --- a/core/src/main/java/feign/Contract.java +++ b/core/src/main/java/feign/Contract.java @@ -82,6 +82,11 @@ public interface Contract { data.returnType(Types.resolve(targetType, targetType, method.getGenericReturnType())); data.configKey(Feign.configKey(targetType, method)); + processAnnotationOnClass(data, method.getDeclaringClass()); + if (method.getDeclaringClass() != targetType) { + processAnnotationOnClass(data, targetType); + } + for (Annotation methodAnnotation : method.getAnnotations()) { processAnnotationOnMethod(data, methodAnnotation, method); } @@ -110,6 +115,15 @@ public interface Contract { return data; } + /** + * Called by parseAndValidateMetadata twice, first on the declaring class, then on the + * target type (unless they are the same). + * + * @param data metadata collected so far relating to the current java method. + * @param clz the class to process + */ + protected abstract void processAnnotationOnClass(MethodMetadata data, Class clz); + /** * @param data metadata collected so far relating to the current java method. * @param annotation annotations present on the current method annotation. @@ -152,18 +166,8 @@ public interface Contract { } class Default extends BaseContract { - @Override - protected MethodMetadata parseAndValidateMetadata(Class targetType, Method method) { - MethodMetadata data = super.parseAndValidateMetadata(targetType, method); - headersFromAnnotation(method.getDeclaringClass(), data); - if (method.getDeclaringClass() != targetType) { - headersFromAnnotation(targetType, data); - } - return data; - } - - private void headersFromAnnotation(Class targetType, MethodMetadata data) { + protected void processAnnotationOnClass(MethodMetadata data, Class targetType) { if (targetType.isAnnotationPresent(Headers.class)) { String[] headersOnType = targetType.getAnnotation(Headers.class).value(); checkState(headersOnType.length > 0, "Headers annotation was empty on type %s.", @@ -196,9 +200,9 @@ public interface Contract { data.template().append( requestLine.substring(requestLine.indexOf(' ') + 1, requestLine.lastIndexOf(' '))); } - + data.template().decodeSlash(RequestLine.class.cast(methodAnnotation).decodeSlash()); - + } else if (annotationType == Body.class) { String body = Body.class.cast(methodAnnotation).value(); checkState(emptyToNull(body) != null, "Body annotation was empty on method %s.", diff --git a/core/src/test/java/feign/DefaultContractTest.java b/core/src/test/java/feign/DefaultContractTest.java index 31cac134..b0e3eda1 100644 --- a/core/src/test/java/feign/DefaultContractTest.java +++ b/core/src/test/java/feign/DefaultContractTest.java @@ -128,7 +128,7 @@ public class DefaultContractTest { .hasQueries( entry("flag", asList(new String[]{null})), entry("NoErrors", asList(new String[]{null})) - ); + ); } @Test @@ -504,6 +504,30 @@ public class DefaultContractTest { ); } + @Headers("Authorization: {authHdr}") + interface ParameterizedHeaderExpandApi { + @RequestLine("GET /api/{zoneId}") + @Headers("Accept: application/json") + String getZone(@Param("zoneId") String vhost, @Param("authHdr") String authHdr); + } + + @Test + public void parameterizedHeaderExpandApi() throws Exception { + List md = contract.parseAndValidatateMetadata(ParameterizedHeaderExpandApi.class); + + assertThat(md).hasSize(1); + + assertThat(md.get(0).configKey()) + .isEqualTo("ParameterizedHeaderExpandApi#getZone(String,String)"); + assertThat(md.get(0).returnType()) + .isEqualTo(String.class); + assertThat(md.get(0).template()) + .hasHeaders(entry("Authorization", asList("{authHdr}")), entry("Accept", asList("application/json"))); + // Ensure that the authHdr expansion was properly detected and did not create a formParam + assertThat(md.get(0).formParams()) + .isEmpty(); + } + private MethodMetadata parseAndValidateMetadata(Class targetType, String method, Class... parameterTypes) throws NoSuchMethodException { diff --git a/jaxrs/src/main/java/feign/jaxrs/JAXRSContract.java b/jaxrs/src/main/java/feign/jaxrs/JAXRSContract.java index 1e7759f8..3346ec18 100644 --- a/jaxrs/src/main/java/feign/jaxrs/JAXRSContract.java +++ b/jaxrs/src/main/java/feign/jaxrs/JAXRSContract.java @@ -43,13 +43,19 @@ public final class JAXRSContract extends Contract.BaseContract { static final String ACCEPT = "Accept"; static final String CONTENT_TYPE = "Content-Type"; + // Protected so unittest can call us + // XXX: Should parseAndValidateMetadata(Class, Method) be public instead? The old deprecated parseAndValidateMetadata(Method) was public.. @Override protected MethodMetadata parseAndValidateMetadata(Class targetType, Method method) { - MethodMetadata md = super.parseAndValidateMetadata(targetType, method); - Path path = targetType.getAnnotation(Path.class); + return super.parseAndValidateMetadata(targetType, method); + } + + @Override + protected void processAnnotationOnClass(MethodMetadata data, Class clz) { + Path path = clz.getAnnotation(Path.class); if (path != null) { String pathValue = emptyToNull(path.value()); - checkState(pathValue != null, "Path.value() was empty on type %s", targetType.getName()); + checkState(pathValue != null, "Path.value() was empty on type %s", clz.getName()); if (!pathValue.startsWith("/")) { pathValue = "/" + pathValue; } @@ -57,9 +63,8 @@ public final class JAXRSContract extends Contract.BaseContract { // Strip off any trailing slashes, since the template has already had slashes appropriately added pathValue = pathValue.substring(0, pathValue.length()-1); } - md.template().insert(0, pathValue); + data.template().insert(0, pathValue); } - return md; } @Override