diff --git a/docs/src/main/asciidoc/spring-cloud-openfeign.adoc b/docs/src/main/asciidoc/spring-cloud-openfeign.adoc index 5a9306be..2f068ad9 100644 --- a/docs/src/main/asciidoc/spring-cloud-openfeign.adoc +++ b/docs/src/main/asciidoc/spring-cloud-openfeign.adoc @@ -494,10 +494,7 @@ public interface UserClient extends UserService { } ---- -NOTE: It is generally not advisable to share an interface between a -server and a client. It introduces tight coupling, and is also not supported by -all the maintained Spring MVC versions (method parameter -mapping is not inherited in some versions). +WARNING: `@FeignClient` interfaces should not be shared between server and client and annotating `@FeignClient` interfaces with `@RequestMapping` on class level is no longer supported. === Feign request/response compression diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringMvcContract.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringMvcContract.java index 70587c96..2b7586bf 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringMvcContract.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringMvcContract.java @@ -35,6 +35,8 @@ import feign.Feign; import feign.MethodMetadata; import feign.Param; import feign.Request; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.springframework.cloud.openfeign.AnnotatedParameterProcessor; import org.springframework.cloud.openfeign.CollectionFormat; @@ -84,6 +86,8 @@ import static org.springframework.core.annotation.AnnotatedElementUtils.findMerg */ public class SpringMvcContract extends Contract.BaseContract implements ResourceLoaderAware { + private static final Log LOG = LogFactory.getLog(SpringMvcContract.class); + private static final String ACCEPT = "Accept"; private static final String CONTENT_TYPE = "Content-Type"; @@ -171,51 +175,22 @@ public class SpringMvcContract extends Contract.BaseContract implements Resource @Override protected void processAnnotationOnClass(MethodMetadata data, Class clz) { - if (clz.getInterfaces().length == 0) { - RequestMapping requestMapping = findMergedAnnotation(clz, RequestMapping.class); - if (requestMapping != null) { - // Prepend path from class annotation if specified - if (requestMapping.value().length > 0) { - String pathValue = emptyToNull(requestMapping.value()[0]); - pathValue = resolve(pathValue); - if (!pathValue.startsWith("/")) { - pathValue = "/" + pathValue; - } - data.template().uri(pathValue); - if (data.template().decodeSlash() != decodeSlash) { - data.template().decodeSlash(decodeSlash); - } - } - } - CollectionFormat collectionFormat = findMergedAnnotation(clz, CollectionFormat.class); - if (collectionFormat != null) { - data.template().collectionFormat(collectionFormat.value()); - } + RequestMapping classAnnotation = findMergedAnnotation(clz, RequestMapping.class); + if (classAnnotation != null) { + LOG.error("Cannot process class: " + clz.getName() + + ". @RequestMapping annotation is not allowed on @FeignClient interfaces."); + throw new IllegalArgumentException("@RequestMapping annotation not allowed on @FeignClient interfaces"); + } + CollectionFormat collectionFormat = findMergedAnnotation(clz, CollectionFormat.class); + if (collectionFormat != null) { + data.template().collectionFormat(collectionFormat.value()); } } @Override public MethodMetadata parseAndValidateMetadata(Class targetType, Method method) { processedMethods.put(Feign.configKey(targetType, method), method); - MethodMetadata md = super.parseAndValidateMetadata(targetType, method); - - RequestMapping classAnnotation = findMergedAnnotation(targetType, RequestMapping.class); - if (classAnnotation != null) { - // produces - use from class annotation only if method has not specified this - if (!md.template().headers().containsKey(ACCEPT)) { - parseProduces(md, method, classAnnotation); - } - - // consumes -- use from class annotation only if method has not specified this - if (!md.template().headers().containsKey(CONTENT_TYPE)) { - parseConsumes(md, method, classAnnotation); - } - - // headers -- class annotation is inherited to methods, always write these if - // present - parseHeaders(md, method, classAnnotation); - } - return md; + return super.parseAndValidateMetadata(targetType, method); } @Override diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractTests.java index fa36481e..0a58826e 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractTests.java @@ -65,6 +65,7 @@ import static com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility.NONE; import static feign.CollectionFormat.CSV; import static feign.CollectionFormat.SSV; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; import static org.junit.Assume.assumeTrue; /** @@ -182,16 +183,12 @@ public class SpringMvcContractTests { } @Test - public void testProcessAnnotations_Class_AnnotationsGetSpecificTest() throws Exception { - Method method = TestTemplate_Class_Annotations.class.getDeclaredMethod("getSpecificTest", String.class, - String.class); - MethodMetadata data = contract.parseAndValidateMetadata(method.getDeclaringClass(), method); - - assertThat(data.template().url()).isEqualTo("/prepend/{classId}/test/{testId}"); - assertThat(data.template().method()).isEqualTo("GET"); - - assertThat(data.indexToName().get(0).iterator().next()).isEqualTo("classId"); - assertThat(data.indexToName().get(1).iterator().next()).isEqualTo("testId"); + public void testProcessAnnotations_Class_Annotations_RequestMapping() { + assertThatIllegalArgumentException().isThrownBy(() -> { + Method method = TestTemplate_Class_RequestMapping.class.getDeclaredMethod("getSpecificTest", String.class, + String.class); + contract.parseAndValidateMetadata(method.getDeclaringClass(), method); + }); } @Test @@ -199,25 +196,13 @@ public class SpringMvcContractTests { Method method = TestTemplate_Class_Annotations.class.getDeclaredMethod("getAllTests", String.class); MethodMetadata data = contract.parseAndValidateMetadata(method.getDeclaringClass(), method); - assertThat(data.template().url()).isEqualTo("/prepend/{classId}"); + assertThat(data.template().url()).isEqualTo("/"); assertThat(data.template().method()).isEqualTo("GET"); assertThat(data.indexToName().get(0).iterator().next()).isEqualTo("classId"); assertThat(data.template().decodeSlash()).isTrue(); } - @Test - public void testProcessAnnotations_Class_AnnotationsGetAllTests_EncodeSlash() throws Exception { - contract = new SpringMvcContract(Collections.emptyList(), getConversionService(), false); - - Method method = TestTemplate_Class_Annotations.class.getDeclaredMethod("getAllTests", String.class); - MethodMetadata data = contract.parseAndValidateMetadata(method.getDeclaringClass(), method); - - assertThat(data.template().url()).isEqualTo("/prepend/{classId}"); - - assertThat(data.template().decodeSlash()).isFalse(); - } - @Test public void testProcessAnnotations_ExtendedInterface() throws Exception { Method extendedMethod = TestTemplate_Extended.class.getMethod("getAllTests", String.class); @@ -234,22 +219,6 @@ public class SpringMvcContractTests { assertThat(data.template().decodeSlash()).isTrue(); } - @Test - public void testProcessAnnotations_ExtendedInterface_EncodeSlash() throws Exception { - contract = new SpringMvcContract(Collections.emptyList(), getConversionService(), false); - - Method extendedMethod = TestTemplate_Extended.class.getMethod("getAllTests", String.class); - MethodMetadata extendedData = contract.parseAndValidateMetadata(extendedMethod.getDeclaringClass(), - extendedMethod); - - Method method = TestTemplate_Class_Annotations.class.getDeclaredMethod("getAllTests", String.class); - MethodMetadata data = contract.parseAndValidateMetadata(method.getDeclaringClass(), method); - - assertThat(data.template().url()).isEqualTo(extendedData.template().url()); - assertThat(data.template().method()).isEqualTo(extendedData.template().method()); - assertThat(data.template().decodeSlash()).isFalse(); - } - @Test public void testProcessAnnotations_SimplePost() throws Exception { Method method = TestTemplate_Simple.class.getDeclaredMethod("postTest", TestObject.class); @@ -280,7 +249,7 @@ public class SpringMvcContractTests { Integer.class); MethodMetadata data = contract.parseAndValidateMetadata(method.getDeclaringClass(), method); - assertThat(data.template().url()).isEqualTo("/advanced/test/{id}?amount=" + "{amount}"); + assertThat(data.template().url()).isEqualTo("/test/{id}?amount=" + "{amount}"); assertThat(data.template().method()).isEqualTo("PUT"); assertThat(data.template().headers().get("Accept").iterator().next()) .isEqualTo(MediaType.APPLICATION_JSON_VALUE); @@ -319,7 +288,7 @@ public class SpringMvcContractTests { Integer.class); MethodMetadata data = contract.parseAndValidateMetadata(method.getDeclaringClass(), method); - assertThat(data.template().url()).isEqualTo("/advanced/test/{id}?amount=" + "{amount}"); + assertThat(data.template().url()).isEqualTo("/test/{id}?amount=" + "{amount}"); assertThat(data.template().method()).isEqualTo("PUT"); assertThat(data.template().headers().get("Accept").iterator().next()) .isEqualTo(MediaType.APPLICATION_JSON_VALUE); @@ -338,7 +307,7 @@ public class SpringMvcContractTests { Method method = TestTemplate_Advanced.class.getDeclaredMethod("getTest2", String.class, Integer.class); MethodMetadata data = contract.parseAndValidateMetadata(method.getDeclaringClass(), method); - assertThat(data.template().url()).isEqualTo("/advanced/test2?amount=" + "{amount}"); + assertThat(data.template().url()).isEqualTo("/test2?amount=" + "{amount}"); assertThat(data.template().method()).isEqualTo("PUT"); assertThat(data.template().headers().get("Accept").iterator().next()) .isEqualTo(MediaType.APPLICATION_JSON_VALUE); @@ -390,7 +359,7 @@ public class SpringMvcContractTests { Method method = TestTemplate_Advanced.class.getDeclaredMethod("getTest"); MethodMetadata data = contract.parseAndValidateMetadata(method.getDeclaringClass(), method); - assertThat(data.template().url()).isEqualTo("/advanced"); + assertThat(data.template().url()).isEqualTo("/"); assertThat(data.template().method()).isEqualTo("GET"); assertThat(data.template().headers().get("Accept").iterator().next()) .isEqualTo(MediaType.APPLICATION_JSON_VALUE); @@ -482,7 +451,7 @@ public class SpringMvcContractTests { MethodMetadata data = contract.parseAndValidateMetadata(method.getDeclaringClass(), method); - assertThat(data.template().url()).isEqualTo("/advanced/testfallback/{id}?amount=" + "{amount}"); + assertThat(data.template().url()).isEqualTo("/testfallback/{id}?amount=" + "{amount}"); assertThat(data.template().method()).isEqualTo("PUT"); assertThat(data.template().headers().get("Accept").iterator().next()) .isEqualTo(MediaType.APPLICATION_JSON_VALUE); @@ -646,6 +615,13 @@ public class SpringMvcContractTests { } @RequestMapping("/prepend/{classId}") + public interface TestTemplate_Class_RequestMapping { + + @RequestMapping(value = "/test/{testId}", method = RequestMethod.GET) + TestObject getSpecificTest(@PathVariable("classId") String classId, @PathVariable("testId") String testId); + + } + public interface TestTemplate_Class_Annotations { @GetMapping("/test/{testId}") @@ -754,7 +730,6 @@ public class SpringMvcContractTests { } @JsonAutoDetect - @RequestMapping("/advanced") @CollectionFormat(CSV) public interface TestTemplate_Advanced {