From d6783a6f1ec8dd08fafe76ecd072913d4e6f66b9 Mon Sep 17 00:00:00 2001 From: Olga MaciaszekSharma Date: Mon, 11 Oct 2021 15:14:46 +0200 Subject: [PATCH] Block clas-level request mapping on Feign clients. --- README.adoc | 18 +--- .../main/asciidoc/spring-cloud-openfeign.adoc | 5 +- .../openfeign/support/SpringMvcContract.java | 43 ++------- .../support/SpringMvcContractTests.java | 90 ++++++------------- 4 files changed, 40 insertions(+), 116 deletions(-) diff --git a/README.adoc b/README.adoc index 12399cd6..1faffeb8 100644 --- a/README.adoc +++ b/README.adoc @@ -66,23 +66,9 @@ the `.mvn` configuration, so if you find you have to do it to make a build succeed, please raise a ticket to get the settings added to source control. -For hints on how to build the project look in `.travis.yml` if there -is one. There should be a "script" and maybe "install" command. Also -look at the "services" section to see if any services need to be -running locally (e.g. mongo or rabbit). Ignore the git-related bits -that you might find in "before_install" since they're related to setting git -credentials and you already have those. +The projects that require middleware (i.e. Redis) for testing generally +require that a local instance of [Docker](https://www.docker.com/get-started) is installed and running. -The projects that require middleware generally include a -`docker-compose.yml`, so consider using -https://docs.docker.com/compose/[Docker Compose] to run the middeware servers -in Docker containers. See the README in the -https://github.com/spring-cloud-samples/scripts[scripts demo -repository] for specific instructions about the common cases of mongo, -rabbit and redis. - -NOTE: If all else fails, build with the command from `.travis.yml` (usually -`./mvnw install`). === Documentation diff --git a/docs/src/main/asciidoc/spring-cloud-openfeign.adoc b/docs/src/main/asciidoc/spring-cloud-openfeign.adoc index daf4c129..21a1a524 100644 --- a/docs/src/main/asciidoc/spring-cloud-openfeign.adoc +++ b/docs/src/main/asciidoc/spring-cloud-openfeign.adoc @@ -523,10 +523,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 also actually -doesn't work with Spring MVC in its current form (method parameter -mapping is not inherited). +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 a589eb69..11cd1b77 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; @@ -83,6 +85,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"; @@ -181,49 +185,20 @@ public class SpringMvcContract extends Contract.BaseContract @Override protected void processAnnotationOnClass(MethodMetadata data, Class clz) { - if (clz.getInterfaces().length == 0) { RequestMapping classAnnotation = findMergedAnnotation(clz, RequestMapping.class); if (classAnnotation != null) { - // Prepend path from class annotation if specified - if (classAnnotation.value().length > 0) { - String pathValue = emptyToNull(classAnnotation.value()[0]); - pathValue = resolve(pathValue); - if (!pathValue.startsWith("/")) { - pathValue = "/" + pathValue; - } - data.template().uri(pathValue); - if (data.template().decodeSlash() != decodeSlash) { - data.template().decodeSlash(decodeSlash); - } - } + 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"); } - } } @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 757bcc69..ff16f723 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 @@ -62,6 +62,7 @@ import static com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility.ANY; import static com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility.NONE; 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,28 +183,23 @@ 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 public void testProcessAnnotations_Class_AnnotationsGetAllTests() throws Exception { Method method = TestTemplate_Class_Annotations.class - .getDeclaredMethod("getAllTests", String.class); + .getDeclaredMethod("getAllTests", String.class); MethodMetadata data = contract - .parseAndValidateMetadata(method.getDeclaringClass(), method); + .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"); @@ -211,22 +207,6 @@ public class SpringMvcContractTests { 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", @@ -249,27 +229,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", @@ -306,7 +265,7 @@ public class SpringMvcContractTests { .parseAndValidateMetadata(method.getDeclaringClass(), method); assertThat(data.template().url()) - .isEqualTo("/advanced/test/{id}?amount=" + "{amount}"); + .isEqualTo("/test/{id}?amount=" + "{amount}"); assertThat(data.template().method()).isEqualTo("PUT"); assertThat(data.template().headers().get("Accept").iterator().next()) .isEqualTo(MediaType.APPLICATION_JSON_VALUE); @@ -342,7 +301,7 @@ public class SpringMvcContractTests { .parseAndValidateMetadata(method.getDeclaringClass(), method); assertThat(data.template().url()) - .isEqualTo("/advanced/test/{id}?amount=" + "{amount}"); + .isEqualTo("/test/{id}?amount=" + "{amount}"); assertThat(data.template().method()).isEqualTo("PUT"); assertThat(data.template().headers().get("Accept").iterator().next()) .isEqualTo(MediaType.APPLICATION_JSON_VALUE); @@ -367,7 +326,7 @@ public class SpringMvcContractTests { .parseAndValidateMetadata(method.getDeclaringClass(), method); assertThat(data.template().url()) - .isEqualTo("/advanced/test2?amount=" + "{amount}"); + .isEqualTo("/test2?amount=" + "{amount}"); assertThat(data.template().method()).isEqualTo("PUT"); assertThat(data.template().headers().get("Accept").iterator().next()) .isEqualTo(MediaType.APPLICATION_JSON_VALUE); @@ -427,12 +386,12 @@ public class SpringMvcContractTests { public void testProcessAnnotations_Advanced2() throws Exception { Method method = TestTemplate_Advanced.class.getDeclaredMethod("getTest"); MethodMetadata data = contract - .parseAndValidateMetadata(method.getDeclaringClass(), method); + .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); + .isEqualTo(MediaType.APPLICATION_JSON_VALUE); } @Test @@ -539,7 +498,7 @@ public class SpringMvcContractTests { .parseAndValidateMetadata(method.getDeclaringClass(), method); assertThat(data.template().url()) - .isEqualTo("/advanced/testfallback/{id}?amount=" + "{amount}"); + .isEqualTo("/testfallback/{id}?amount=" + "{amount}"); assertThat(data.template().method()).isEqualTo("PUT"); assertThat(data.template().headers().get("Accept").iterator().next()) .isEqualTo(MediaType.APPLICATION_JSON_VALUE); @@ -699,7 +658,7 @@ public class SpringMvcContractTests { ResponseEntity getMappingTest(@PathVariable("id") String id); @RequestMapping(method = RequestMethod.POST, - produces = MediaType.APPLICATION_JSON_VALUE) + produces = MediaType.APPLICATION_JSON_VALUE) TestObject postTest(@RequestBody TestObject object); @PostMapping(produces = MediaType.APPLICATION_JSON_VALUE) @@ -708,11 +667,19 @@ 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 { @RequestMapping(value = "/test/{testId}", method = RequestMethod.GET) TestObject getSpecificTest(@PathVariable("classId") String classId, - @PathVariable("testId") String testId); + @PathVariable("testId") String testId); @RequestMapping(method = RequestMethod.GET) TestObject getAllTests(@PathVariable("classId") String classId); @@ -812,7 +779,6 @@ public class SpringMvcContractTests { } @JsonAutoDetect - @RequestMapping("/advanced") public interface TestTemplate_Advanced { @CollectionFormat(SSV)