Fix pageable processing when querymap present. (#843)

Author: OlgaMaciaszek

spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringMvcContract.java

@@ -34,12 +34,14 @@
 import feign.Feign;
 import feign.MethodMetadata;
 import feign.Param;
+import feign.QueryMap;
 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;
+import org.springframework.cloud.openfeign.SpringQueryMap;
 import org.springframework.cloud.openfeign.annotation.CookieValueParameterProcessor;
 import org.springframework.cloud.openfeign.annotation.MatrixVariableParameterProcessor;
 import org.springframework.cloud.openfeign.annotation.PathVariableParameterProcessor;
@@ -67,6 +69,7 @@
 import org.springframework.util.StringUtils;
 import org.springframework.web.bind.annotation.RequestMapping;
 import org.springframework.web.bind.annotation.RequestMethod;
+import org.springframework.web.bind.annotation.RequestParam;
 
 import static feign.Util.checkState;
 import static feign.Util.emptyToNull;
@@ -158,7 +161,7 @@ private static TypeDescriptor createTypeDescriptor(Method method, int paramIndex
 
 	private static TypeDescriptor getElementTypeDescriptor(TypeDescriptor typeDescriptor) {
 		TypeDescriptor elementTypeDescriptor = typeDescriptor.getElementTypeDescriptor();
-		// that means it's not a collection but it is iterable, gh-135
+		// that means it's not a collection, but it is iterable, gh-135
 		if (elementTypeDescriptor == null && Iterable.class.isAssignableFrom(typeDescriptor.getType())) {
 			ResolvableType type = typeDescriptor.getResolvableType().as(Iterable.class).getGeneric(0);
 			if (type.resolve() == null) {
@@ -268,8 +271,12 @@ protected boolean processAnnotationsOnParameter(MethodMetadata data, Annotation[
 
 		try {
 			if (Pageable.class.isAssignableFrom(data.method().getParameterTypes()[paramIndex])) {
-				data.queryMapIndex(paramIndex);
-				return false;
+				// do not set a Pageable as QueryMap if there's an actual QueryMap param
+				// present
+				if (!queryMapParamPresent(data)) {
+					data.queryMapIndex(paramIndex);
+					return false;
+				}
 			}
 		}
 		catch (NoClassDefFoundError ignored) {
@@ -304,6 +311,20 @@ protected boolean processAnnotationsOnParameter(MethodMetadata data, Annotation[
 		return isHttpAnnotation;
 	}
 
+	private boolean queryMapParamPresent(MethodMetadata data) {
+		Annotation[][] paramsAnnotations = data.method().getParameterAnnotations();
+		for (int i = 0; i < paramsAnnotations.length; i++) {
+			Annotation[] paramAnnotations = paramsAnnotations[i];
+			Class<?> parameterType = data.method().getParameterTypes()[i];
+			if (Arrays.stream(paramAnnotations).anyMatch(
+					annotation -> Map.class.isAssignableFrom(parameterType) && annotation instanceof RequestParam
+							|| annotation instanceof SpringQueryMap || annotation instanceof QueryMap)) {
+				return true;
+			}
+		}
+		return false;
+	}
+
 	private void parseProduces(MethodMetadata md, Method method, RequestMapping annotation) {
 		String[] serverProduces = annotation.produces();
 		String clientAccepts = serverProduces.length == 0 ? null : emptyToNull(serverProduces[0]);

spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractTests.java

@@ -631,10 +631,20 @@ void testMultipleCookiesAnnotation() throws NoSuchMethodException {
 	void shouldNotFailWhenBothPageableAndRequestBodyParamsInPostRequest() {
 		List<MethodMetadata> data = contract.parseAndValidateMetadata(TestTemplate_PageablePost.class);
 
-		assertThat(data.get(0).queryMapIndex().intValue()).isEqualTo(0);
-		assertThat(data.get(0).bodyIndex().intValue()).isEqualTo(1);
-		assertThat(data.get(1).queryMapIndex().intValue()).isEqualTo(1);
-		assertThat(data.get(1).bodyIndex().intValue()).isEqualTo(0);
+		assertThat(data.get(0).queryMapIndex()).isEqualTo(0);
+		assertThat(data.get(0).bodyIndex()).isEqualTo(1);
+		assertThat(data.get(1).queryMapIndex()).isEqualTo(1);
+		assertThat(data.get(1).bodyIndex()).isEqualTo(0);
+	}
+
+	@Test
+	void shouldSetPageableAsBodyWhenQueryMapParamPresent() {
+		List<MethodMetadata> data = contract.parseAndValidateMetadata(TestTemplate_PageablePostWithQueryMap.class);
+
+		assertThat(data.get(0).queryMapIndex()).isEqualTo(0);
+		assertThat(data.get(0).bodyIndex()).isEqualTo(1);
+		assertThat(data.get(1).queryMapIndex()).isEqualTo(1);
+		assertThat(data.get(1).bodyIndex()).isEqualTo(0);
 	}
 
 	private ConversionService getConversionService() {
@@ -838,7 +848,7 @@ public interface TestTemplate_NumberFormatParameter {
 
 	}
 
-	public interface TestTemplate_PageablePost {
+	interface TestTemplate_PageablePost {
 
 		@PostMapping
 		Page<String> getPage(Pageable pageable, @RequestBody String body);
@@ -848,6 +858,16 @@ public interface TestTemplate_PageablePost {
 
 	}
 
+	interface TestTemplate_PageablePostWithQueryMap {
+
+		@PostMapping
+		Page<String> getPage(@SpringQueryMap TestObject pojo, Pageable pageable);
+
+		@PostMapping
+		Page<String> getPage(Pageable pageable, @SpringQueryMap TestObject pojo);
+
+	}
+
 	@JsonAutoDetect(fieldVisibility = ANY, getterVisibility = NONE, setterVisibility = NONE)
 	public class TestObject {