From 9e479481ccccfad60007d1ccecd959ce5fc2bd4f Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Fri, 17 Jun 2016 14:55:35 +0100 Subject: [PATCH] Exempt @RequestParams of type Collection from feign expander It isn't very helpful to have a Collection converted to a String anyway. We need to implement something new in Feign to apply the converter (expander) to individual elements in the collection but this change should get back to the old behaviour with List at least. Fixes gh-1115 --- .../RequestParamParameterProcessor.java | 40 +++++++------- .../feign/support/SpringMvcContract.java | 16 ++++-- .../feign/support/SpringEncoderTests.java | 32 +++++++----- .../feign/support/SpringMvcContractTests.java | 52 +++++++++++++------ 4 files changed, 90 insertions(+), 50 deletions(-) diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/annotation/RequestParamParameterProcessor.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/annotation/RequestParamParameterProcessor.java index edd2b226a..a67da38ba 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/annotation/RequestParamParameterProcessor.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/annotation/RequestParamParameterProcessor.java @@ -22,11 +22,11 @@ import java.util.Collection; import org.springframework.cloud.netflix.feign.AnnotatedParameterProcessor; import org.springframework.web.bind.annotation.RequestParam; -import feign.MethodMetadata; - import static feign.Util.checkState; import static feign.Util.emptyToNull; +import feign.MethodMetadata; + /** * {@link RequestParam} parameter processor. * @@ -35,23 +35,27 @@ import static feign.Util.emptyToNull; */ public class RequestParamParameterProcessor implements AnnotatedParameterProcessor { - private static final Class ANNOTATION = RequestParam.class; + private static final Class ANNOTATION = RequestParam.class; - @Override - public Class getAnnotationType() { - return ANNOTATION; - } + @Override + public Class getAnnotationType() { + return ANNOTATION; + } - @Override - public boolean processArgument(AnnotatedParameterContext context, Annotation annotation) { - String name = ANNOTATION.cast(annotation).value(); - checkState(emptyToNull(name) != null, - "RequestParam.value() was empty on parameter %s", context.getParameterIndex()); - context.setParameterName(name); + @Override + public boolean processArgument(AnnotatedParameterContext context, + Annotation annotation) { + RequestParam requestParam = ANNOTATION.cast(annotation); + String name = requestParam.value(); + checkState(emptyToNull(name) != null, + "RequestParam.value() was empty on parameter %s", + context.getParameterIndex()); + context.setParameterName(name); - MethodMetadata data = context.getMethodMetadata(); - Collection query = context.setTemplateParameter(name, data.template().queries().get(name)); - data.template().query(name, query); - return true; - } + MethodMetadata data = context.getMethodMetadata(); + Collection query = context.setTemplateParameter(name, + data.template().queries().get(name)); + data.template().query(name, query); + return true; + } } diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/support/SpringMvcContract.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/support/SpringMvcContract.java index 7dab129c0..34b1a643d 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/support/SpringMvcContract.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/support/SpringMvcContract.java @@ -45,15 +45,15 @@ import org.springframework.util.StringUtils; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestMethod; +import static feign.Util.checkState; +import static feign.Util.emptyToNull; +import static org.springframework.core.annotation.AnnotatedElementUtils.findMergedAnnotation; + import feign.Contract; import feign.Feign; import feign.MethodMetadata; import feign.Param; -import static feign.Util.checkState; -import static feign.Util.emptyToNull; -import static org.springframework.core.annotation.AnnotatedElementUtils.findMergedAnnotation; - /** * @author Spencer Gibb */ @@ -237,6 +237,7 @@ public class SpringMvcContract extends Contract.BaseContract } } if (isHttpAnnotation && data.indexToExpander().get(paramIndex) == null + && !isMultiValued(method.getParameterTypes()[paramIndex]) && this.conversionService.canConvert( method.getParameterTypes()[paramIndex], String.class)) { data.indexToExpander().put(paramIndex, this.expander); @@ -244,6 +245,13 @@ public class SpringMvcContract extends Contract.BaseContract return isHttpAnnotation; } + private boolean isMultiValued(Class type) { + // Feign will deal with each element in a collection individually (with no + // expander as of 8.16.2, but we'd rather have no conversion than convert a + // collection to a String (which ends up being a csv). + return Collection.class.isAssignableFrom(type); + } + private void parseProduces(MethodMetadata md, Method method, RequestMapping annotation) { checkAtMostOne(method, annotation.produces(), "produces"); diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/support/SpringEncoderTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/support/SpringEncoderTests.java index a101e6916..cfa04cac4 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/support/SpringEncoderTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/support/SpringEncoderTests.java @@ -27,13 +27,13 @@ import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; import org.springframework.test.context.web.WebAppConfiguration; import org.springframework.web.bind.annotation.RestController; -import feign.RequestTemplate; -import lombok.Data; - import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; import static org.junit.Assert.assertThat; +import feign.RequestTemplate; +import lombok.Data; + /** * @author Spencer Gibb */ @@ -50,7 +50,7 @@ public class SpringEncoderTests { @Autowired @Qualifier("myHttpMessageConverter") - private HttpMessageConverter myConverter; + private HttpMessageConverter myConverter; @Test public void testCustomHttpMessageConverter() { @@ -73,14 +73,14 @@ public class SpringEncoderTests { private MediaType mediaType; public MediaTypeMatcher(String type, String subtype) { - mediaType = new MediaType(type, subtype); + this.mediaType = new MediaType(type, subtype); } @Override public boolean matches(Object argument) { if (argument instanceof MediaType) { MediaType other = (MediaType) argument; - return mediaType.equals(other); + return this.mediaType.equals(other); } return false; } @@ -88,7 +88,7 @@ public class SpringEncoderTests { @Override public String toString() { final StringBuffer sb = new StringBuffer("MediaTypeMatcher{"); - sb.append("mediaType=").append(mediaType); + sb.append("mediaType=").append(this.mediaType); sb.append('}'); return sb.toString(); } @@ -109,18 +109,19 @@ public class SpringEncoderTests { protected static class Application implements TestClient { @Bean - HttpMessageConverter myHttpMessageConverter() { + HttpMessageConverter myHttpMessageConverter() { return new MyHttpMessageConverter(); } - private static class MyHttpMessageConverter extends AbstractGenericHttpMessageConverter { + private static class MyHttpMessageConverter + extends AbstractGenericHttpMessageConverter { public MyHttpMessageConverter() { super(new MediaType("application", "mytype")); } @Override - protected boolean supports(Class clazz) { + protected boolean supports(Class clazz) { return false; } @@ -135,17 +136,22 @@ public class SpringEncoderTests { } @Override - protected void writeInternal(Object o, Type type, HttpOutputMessage outputMessage) throws IOException, HttpMessageNotWritableException { + protected void writeInternal(Object o, Type type, + HttpOutputMessage outputMessage) + throws IOException, HttpMessageNotWritableException { } @Override - protected Object readInternal(Class clazz, HttpInputMessage inputMessage) throws IOException, HttpMessageNotReadableException { + protected Object readInternal(Class clazz, HttpInputMessage inputMessage) + throws IOException, HttpMessageNotReadableException { return null; } @Override - public Object read(Type type, Class contextClass, HttpInputMessage inputMessage) throws IOException, HttpMessageNotReadableException { + public Object read(Type type, Class contextClass, + HttpInputMessage inputMessage) + throws IOException, HttpMessageNotReadableException { return null; } } diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/support/SpringMvcContractTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/support/SpringMvcContractTests.java index 1cf2a3fde..a66163610 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/support/SpringMvcContractTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/support/SpringMvcContractTests.java @@ -18,6 +18,7 @@ package org.springframework.cloud.netflix.feign.support; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; +import java.util.List; import org.junit.Before; import org.junit.Test; @@ -34,14 +35,16 @@ import org.springframework.web.bind.annotation.RequestParam; import com.fasterxml.jackson.annotation.JsonAutoDetect; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assume.assumeTrue; + import feign.MethodMetadata; import lombok.AllArgsConstructor; import lombok.NoArgsConstructor; import lombok.ToString; -import static org.junit.Assert.assertEquals; -import static org.junit.Assume.assumeTrue; - /** * @author chadjaros */ @@ -97,10 +100,10 @@ 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 = this.contract.parseAndValidateMetadata( - method.getDeclaringClass(), method); + Method method = TestTemplate_Class_Annotations.class + .getDeclaredMethod("getSpecificTest", String.class, String.class); + MethodMetadata data = this.contract + .parseAndValidateMetadata(method.getDeclaringClass(), method); assertEquals("/prepend/{classId}/test/{testId}", data.template().url()); assertEquals("GET", data.template().method()); @@ -111,10 +114,10 @@ public class SpringMvcContractTests { @Test public void testProcessAnnotations_Class_AnnotationsGetAllTests() throws Exception { - Method method = TestTemplate_Class_Annotations.class.getDeclaredMethod( - "getAllTests", String.class); - MethodMetadata data = this.contract.parseAndValidateMetadata( - method.getDeclaringClass(), method); + Method method = TestTemplate_Class_Annotations.class + .getDeclaredMethod("getAllTests", String.class); + MethodMetadata data = this.contract + .parseAndValidateMetadata(method.getDeclaringClass(), method); assertEquals("/prepend/{classId}", data.template().url()); assertEquals("GET", data.template().method()); @@ -129,10 +132,10 @@ public class SpringMvcContractTests { MethodMetadata extendedData = this.contract.parseAndValidateMetadata( extendedMethod.getDeclaringClass(), extendedMethod); - Method method = TestTemplate_Class_Annotations.class.getDeclaredMethod( - "getAllTests", String.class); - MethodMetadata data = this.contract.parseAndValidateMetadata( - method.getDeclaringClass(), method); + Method method = TestTemplate_Class_Annotations.class + .getDeclaredMethod("getAllTests", String.class); + MethodMetadata data = this.contract + .parseAndValidateMetadata(method.getDeclaringClass(), method); assertEquals(extendedData.template().url(), data.template().url()); assertEquals(extendedData.template().method(), data.template().method()); @@ -193,6 +196,7 @@ public class SpringMvcContractTests { assertEquals("Authorization", data.indexToName().get(0).iterator().next()); assertEquals("id", data.indexToName().get(1).iterator().next()); assertEquals("amount", data.indexToName().get(2).iterator().next()); + assertNotNull(data.indexToExpander().get(2)); assertEquals("{Authorization}", data.template().headers().get("Authorization").iterator().next()); @@ -245,6 +249,19 @@ public class SpringMvcContractTests { data.template().headers().get("Accept").iterator().next()); } + @Test + public void testProcessAnnotations_ListParams() throws Exception { + Method method = TestTemplate_ListParams.class.getDeclaredMethod("getTest", + List.class); + MethodMetadata data = this.contract + .parseAndValidateMetadata(method.getDeclaringClass(), method); + + assertEquals("/test", data.template().url()); + assertEquals("GET", data.template().method()); + assertEquals("[{id}]", data.template().queries().get("id").toString()); + assertNull(data.indexToExpander().get(0)); + } + @Test public void testProcessHeaders() throws Exception { Method method = TestTemplate_Headers.class.getDeclaredMethod("getTest", @@ -339,6 +356,11 @@ public class SpringMvcContractTests { ResponseEntity getTest(@PathVariable("id") String id); } + public interface TestTemplate_ListParams { + @RequestMapping(value = "/test", method = RequestMethod.GET) + ResponseEntity getTest(@RequestParam("id") List id); + } + @JsonAutoDetect @RequestMapping("/advanced") public interface TestTemplate_Advanced {