From 0afb60a72a680182b53afccacb16a42dd4e8a585 Mon Sep 17 00:00:00 2001 From: nekkiy Date: Fri, 27 Mar 2020 13:58:26 +0300 Subject: [PATCH] Fix double @RequestMapping with empty value (#245) * Fix double @RequestMapping with empty value * Fix code style * Fix for review * add author to SpringMvcContract * split SpringMvcContractTests#testDoubleRequestMapping() test --- spring-cloud-openfeign-core/pom.xml | 6 ++ .../openfeign/support/SpringMvcContract.java | 18 ++-- .../support/SpringMvcContractTests.java | 86 +++++++++++++++++++ 3 files changed, 100 insertions(+), 10 deletions(-) diff --git a/spring-cloud-openfeign-core/pom.xml b/spring-cloud-openfeign-core/pom.xml index dc9d252e..4ea26409 100644 --- a/spring-cloud-openfeign-core/pom.xml +++ b/spring-cloud-openfeign-core/pom.xml @@ -177,6 +177,12 @@ spring-cloud-loadbalancer true + + pl.pragmatists + JUnitParams + 1.1.1 + test + 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 f24c99fb..02dcf832 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 @@ -71,6 +71,7 @@ import static org.springframework.core.annotation.AnnotatedElementUtils.findMerg * @author Aram Peres * @author Olga Maciaszek-Sharma * @author Aaron Whiteside + * @author Artyom Romanenko */ public class SpringMvcContract extends Contract.BaseContract implements ResourceLoaderAware { @@ -167,15 +168,14 @@ public class SpringMvcContract extends Contract.BaseContract if (clz.getInterfaces().length == 0) { RequestMapping classAnnotation = findMergedAnnotation(clz, RequestMapping.class); - if (classAnnotation != null) { + if (classAnnotation != null && classAnnotation.value().length > 0) { // Prepend path from class annotation if specified - if (classAnnotation.value().length > 0) { - String pathValue = emptyToNull(classAnnotation.value()[0]); + String pathValue = emptyToNull(classAnnotation.value()[0]); + if (pathValue != null) { pathValue = resolve(pathValue); - if (!pathValue.startsWith("/")) { - pathValue = "/" + pathValue; + if (!pathValue.equals("/")) { + data.template().uri(pathValue); } - data.template().uri(pathValue); } } } @@ -229,11 +229,9 @@ public class SpringMvcContract extends Contract.BaseContract String pathValue = emptyToNull(methodMapping.value()[0]); if (pathValue != null) { pathValue = resolve(pathValue); - // Append path from @RequestMapping if value is present on method - if (!pathValue.startsWith("/") && !data.template().path().endsWith("/")) { - pathValue = "/" + pathValue; + if (!pathValue.equals("/")) { + data.template().uri(pathValue, true); } - data.template().uri(pathValue, true); } } 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 8daf7a8c..803602e1 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 @@ -31,8 +31,11 @@ import java.util.Map; import com.fasterxml.jackson.annotation.JsonAutoDetect; import feign.MethodMetadata; import feign.Param; +import junitparams.JUnitParamsRunner; +import junitparams.Parameters; import org.junit.Before; import org.junit.Test; +import org.junit.runner.RunWith; import org.springframework.cloud.openfeign.SpringQueryMap; import org.springframework.core.convert.ConversionService; @@ -67,7 +70,9 @@ import static org.junit.Assume.assumeTrue; * @author Halvdan Hoem Grelland * @author Aram Peres * @author Aaron Whiteside + * @author Artyom Romanenko */ +@RunWith(JUnitParamsRunner.class) public class SpringMvcContractTests { private static final Class EXECUTABLE_TYPE; @@ -582,6 +587,53 @@ public class SpringMvcContractTests { "{Accept}"); } + private Class[] doubleMappingClassesProvider() { + return new Class[] { TestTemplate_RequestMapping_Empty_Class.class, + TestTemplate_RequestMapping_Empty_Method.class }; + } + + @Test + @Parameters(method = "doubleMappingClassesProvider") + public void testDoubleRequestMapping_root(Class clazz) throws NoSuchMethodException { + Method method = clazz.getDeclaredMethod("root"); + MethodMetadata data = contract + .parseAndValidateMetadata(method.getDeclaringClass(), method); + + assertThat(data.template().url()).isEqualTo("/"); + } + + @Test + @Parameters(method = "doubleMappingClassesProvider") + public void testDoubleRequestMapping_rootReverse(Class clazz) + throws NoSuchMethodException { + Method method = clazz.getDeclaredMethod("rootReverse"); + MethodMetadata data = contract + .parseAndValidateMetadata(method.getDeclaringClass(), method); + + assertThat(data.template().url()).isEqualTo("/"); + } + + @Test + @Parameters(method = "doubleMappingClassesProvider") + public void testDoubleRequestMapping_sub(Class clazz) throws NoSuchMethodException { + Method method = clazz.getDeclaredMethod("sub"); + MethodMetadata data = contract + .parseAndValidateMetadata(method.getDeclaringClass(), method); + + assertThat(data.template().url()).isEqualTo("/sub"); + } + + @Test + @Parameters(method = "doubleMappingClassesProvider") + public void testDoubleRequestMapping_subEmpty(Class clazz) + throws NoSuchMethodException { + Method method = clazz.getDeclaredMethod("subEmpty"); + MethodMetadata data = contract + .parseAndValidateMetadata(method.getDeclaringClass(), method); + + assertThat(data.template().url()).isEqualTo("/subEmpty"); + } + @Test public void testMultipleRequestPartAnnotations() throws NoSuchMethodException { Method method = TestTemplate_RequestPart.class.getDeclaredMethod( @@ -770,6 +822,40 @@ public class SpringMvcContractTests { } + @RequestMapping("") + public interface TestTemplate_RequestMapping_Empty_Class { + + @RequestMapping("/") + String root(); + + @RequestMapping("") + String rootReverse(); + + @RequestMapping("/sub") + String sub(); + + @RequestMapping("subEmpty") + String subEmpty(); + + } + + @RequestMapping("/") + public interface TestTemplate_RequestMapping_Empty_Method { + + @RequestMapping("") + String root(); + + @RequestMapping("/") + String rootReverse(); + + @RequestMapping("/sub") + String sub(); + + @RequestMapping("subEmpty") + String subEmpty(); + + } + @JsonAutoDetect(fieldVisibility = ANY, getterVisibility = NONE, setterVisibility = NONE) public class TestObject {