Skip to content

S
spring-cloud-netflix
Commit 9e479481 by Dave Syer
Options

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<String> at least. Fixes gh-1115
parent 20db8685
Showing with 93 additions and 53 deletions
spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/annotation/RequestParamParameterProcessor.java
...@@ -22,11 +22,11 @@ import java.util.Collection; ...@@ -22,11 +22,11 @@ import java.util.Collection;
import org.springframework.cloud.netflix.feign.AnnotatedParameterProcessor; import org.springframework.cloud.netflix.feign.AnnotatedParameterProcessor;
import org.springframework.web.bind.annotation.RequestParam; import org.springframework.web.bind.annotation.RequestParam;
import feign.MethodMetadata;
import static feign.Util.checkState; import static feign.Util.checkState;
import static feign.Util.emptyToNull; import static feign.Util.emptyToNull;
import feign.MethodMetadata;
/** /**
* {@link RequestParam} parameter processor. * {@link RequestParam} parameter processor.
* *
...@@ -35,23 +35,27 @@ import static feign.Util.emptyToNull; ...@@ -35,23 +35,27 @@ import static feign.Util.emptyToNull;
*/ */
public class RequestParamParameterProcessor implements AnnotatedParameterProcessor { public class RequestParamParameterProcessor implements AnnotatedParameterProcessor {
private static final Class<RequestParam> ANNOTATION = RequestParam.class; private static final Class<RequestParam> ANNOTATION = RequestParam.class;
@Override @Override
public Class<? extends Annotation> getAnnotationType() { public Class<? extends Annotation> getAnnotationType() {
return ANNOTATION; return ANNOTATION;
} }
@Override @Override
public boolean processArgument(AnnotatedParameterContext context, Annotation annotation) { public boolean processArgument(AnnotatedParameterContext context,
String name = ANNOTATION.cast(annotation).value(); Annotation annotation) {
checkState(emptyToNull(name) != null, RequestParam requestParam = ANNOTATION.cast(annotation);
"RequestParam.value() was empty on parameter %s", context.getParameterIndex()); String name = requestParam.value();
context.setParameterName(name); checkState(emptyToNull(name) != null,
"RequestParam.value() was empty on parameter %s",
MethodMetadata data = context.getMethodMetadata(); context.getParameterIndex());
Collection<String> query = context.setTemplateParameter(name, data.template().queries().get(name)); context.setParameterName(name);
data.template().query(name, query);
return true; MethodMetadata data = context.getMethodMetadata();
} Collection<String> query = context.setTemplateParameter(name,
data.template().queries().get(name));
data.template().query(name, query);
return true;
}
} }
spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/support/SpringMvcContract.java
...@@ -45,15 +45,15 @@ import org.springframework.util.StringUtils; ...@@ -45,15 +45,15 @@ import org.springframework.util.StringUtils;
import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestMethod; 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.Contract;
import feign.Feign; import feign.Feign;
import feign.MethodMetadata; import feign.MethodMetadata;
import feign.Param; import feign.Param;
import static feign.Util.checkState;
import static feign.Util.emptyToNull;
import static org.springframework.core.annotation.AnnotatedElementUtils.findMergedAnnotation;
/** /**
* @author Spencer Gibb * @author Spencer Gibb
*/ */
...@@ -237,6 +237,7 @@ public class SpringMvcContract extends Contract.BaseContract ...@@ -237,6 +237,7 @@ public class SpringMvcContract extends Contract.BaseContract
} }
} }
if (isHttpAnnotation && data.indexToExpander().get(paramIndex) == null if (isHttpAnnotation && data.indexToExpander().get(paramIndex) == null
&& !isMultiValued(method.getParameterTypes()[paramIndex])
&& this.conversionService.canConvert( && this.conversionService.canConvert(
method.getParameterTypes()[paramIndex], String.class)) { method.getParameterTypes()[paramIndex], String.class)) {
data.indexToExpander().put(paramIndex, this.expander); data.indexToExpander().put(paramIndex, this.expander);
...@@ -244,6 +245,13 @@ public class SpringMvcContract extends Contract.BaseContract ...@@ -244,6 +245,13 @@ public class SpringMvcContract extends Contract.BaseContract
return isHttpAnnotation; 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, private void parseProduces(MethodMetadata md, Method method,
RequestMapping annotation) { RequestMapping annotation) {
checkAtMostOne(method, annotation.produces(), "produces"); checkAtMostOne(method, annotation.produces(), "produces");
......
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; ...@@ -27,13 +27,13 @@ import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
import org.springframework.test.context.web.WebAppConfiguration; import org.springframework.test.context.web.WebAppConfiguration;
import org.springframework.web.bind.annotation.RestController; import org.springframework.web.bind.annotation.RestController;
import feign.RequestTemplate;
import lombok.Data;
import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.notNullValue;
import static org.junit.Assert.assertThat; import static org.junit.Assert.assertThat;
import feign.RequestTemplate;
import lombok.Data;
/** /**
* @author Spencer Gibb * @author Spencer Gibb
*/ */
...@@ -50,7 +50,7 @@ public class SpringEncoderTests { ...@@ -50,7 +50,7 @@ public class SpringEncoderTests {
@Autowired @Autowired
@Qualifier("myHttpMessageConverter") @Qualifier("myHttpMessageConverter")
private HttpMessageConverter myConverter; private HttpMessageConverter<?> myConverter;
@Test @Test
public void testCustomHttpMessageConverter() { public void testCustomHttpMessageConverter() {
...@@ -73,14 +73,14 @@ public class SpringEncoderTests { ...@@ -73,14 +73,14 @@ public class SpringEncoderTests {
private MediaType mediaType; private MediaType mediaType;
public MediaTypeMatcher(String type, String subtype) { public MediaTypeMatcher(String type, String subtype) {
mediaType = new MediaType(type, subtype); this.mediaType = new MediaType(type, subtype);
} }
@Override @Override
public boolean matches(Object argument) { public boolean matches(Object argument) {
if (argument instanceof MediaType) { if (argument instanceof MediaType) {
MediaType other = (MediaType) argument; MediaType other = (MediaType) argument;
return mediaType.equals(other); return this.mediaType.equals(other);
} }
return false; return false;
} }
...@@ -88,7 +88,7 @@ public class SpringEncoderTests { ...@@ -88,7 +88,7 @@ public class SpringEncoderTests {
@Override @Override
public String toString() { public String toString() {
final StringBuffer sb = new StringBuffer("MediaTypeMatcher{"); final StringBuffer sb = new StringBuffer("MediaTypeMatcher{");
sb.append("mediaType=").append(mediaType); sb.append("mediaType=").append(this.mediaType);
sb.append('}'); sb.append('}');
return sb.toString(); return sb.toString();
} }
...@@ -109,18 +109,19 @@ public class SpringEncoderTests { ...@@ -109,18 +109,19 @@ public class SpringEncoderTests {
protected static class Application implements TestClient { protected static class Application implements TestClient {
@Bean @Bean
HttpMessageConverter myHttpMessageConverter() { HttpMessageConverter<?> myHttpMessageConverter() {
return new MyHttpMessageConverter(); return new MyHttpMessageConverter();
} }
private static class MyHttpMessageConverter extends AbstractGenericHttpMessageConverter<Object> { private static class MyHttpMessageConverter
extends AbstractGenericHttpMessageConverter<Object> {
public MyHttpMessageConverter() { public MyHttpMessageConverter() {
super(new MediaType("application", "mytype")); super(new MediaType("application", "mytype"));
} }
@Override @Override
protected boolean supports(Class clazz) { protected boolean supports(Class<?> clazz) {
return false; return false;
} }
...@@ -135,17 +136,22 @@ public class SpringEncoderTests { ...@@ -135,17 +136,22 @@ public class SpringEncoderTests {
} }
@Override @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 @Override
protected Object readInternal(Class clazz, HttpInputMessage inputMessage) throws IOException, HttpMessageNotReadableException { protected Object readInternal(Class<?> clazz, HttpInputMessage inputMessage)
throws IOException, HttpMessageNotReadableException {
return null; return null;
} }
@Override @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; return null;
} }
} }
......
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; ...@@ -18,6 +18,7 @@ package org.springframework.cloud.netflix.feign.support;
import java.lang.reflect.InvocationTargetException; import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method; import java.lang.reflect.Method;
import java.util.List;
import org.junit.Before; import org.junit.Before;
import org.junit.Test; import org.junit.Test;
...@@ -34,14 +35,16 @@ import org.springframework.web.bind.annotation.RequestParam; ...@@ -34,14 +35,16 @@ import org.springframework.web.bind.annotation.RequestParam;
import com.fasterxml.jackson.annotation.JsonAutoDetect; 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 feign.MethodMetadata;
import lombok.AllArgsConstructor; import lombok.AllArgsConstructor;
import lombok.NoArgsConstructor; import lombok.NoArgsConstructor;
import lombok.ToString; import lombok.ToString;
import static org.junit.Assert.assertEquals;
import static org.junit.Assume.assumeTrue;
/** /**
* @author chadjaros * @author chadjaros
*/ */
...@@ -97,10 +100,10 @@ public class SpringMvcContractTests { ...@@ -97,10 +100,10 @@ public class SpringMvcContractTests {
@Test @Test
public void testProcessAnnotations_Class_AnnotationsGetSpecificTest() public void testProcessAnnotations_Class_AnnotationsGetSpecificTest()
throws Exception { throws Exception {
Method method = TestTemplate_Class_Annotations.class.getDeclaredMethod( Method method = TestTemplate_Class_Annotations.class
"getSpecificTest", String.class, String.class); .getDeclaredMethod("getSpecificTest", String.class, String.class);
MethodMetadata data = this.contract.parseAndValidateMetadata( MethodMetadata data = this.contract
method.getDeclaringClass(), method); .parseAndValidateMetadata(method.getDeclaringClass(), method);
assertEquals("/prepend/{classId}/test/{testId}", data.template().url()); assertEquals("/prepend/{classId}/test/{testId}", data.template().url());
assertEquals("GET", data.template().method()); assertEquals("GET", data.template().method());
...@@ -111,10 +114,10 @@ public class SpringMvcContractTests { ...@@ -111,10 +114,10 @@ public class SpringMvcContractTests {
@Test @Test
public void testProcessAnnotations_Class_AnnotationsGetAllTests() throws Exception { public void testProcessAnnotations_Class_AnnotationsGetAllTests() throws Exception {
Method method = TestTemplate_Class_Annotations.class.getDeclaredMethod( Method method = TestTemplate_Class_Annotations.class
"getAllTests", String.class); .getDeclaredMethod("getAllTests", String.class);
MethodMetadata data = this.contract.parseAndValidateMetadata( MethodMetadata data = this.contract
method.getDeclaringClass(), method); .parseAndValidateMetadata(method.getDeclaringClass(), method);
assertEquals("/prepend/{classId}", data.template().url()); assertEquals("/prepend/{classId}", data.template().url());
assertEquals("GET", data.template().method()); assertEquals("GET", data.template().method());
...@@ -129,10 +132,10 @@ public class SpringMvcContractTests { ...@@ -129,10 +132,10 @@ public class SpringMvcContractTests {
MethodMetadata extendedData = this.contract.parseAndValidateMetadata( MethodMetadata extendedData = this.contract.parseAndValidateMetadata(
extendedMethod.getDeclaringClass(), extendedMethod); extendedMethod.getDeclaringClass(), extendedMethod);
Method method = TestTemplate_Class_Annotations.class.getDeclaredMethod( Method method = TestTemplate_Class_Annotations.class
"getAllTests", String.class); .getDeclaredMethod("getAllTests", String.class);
MethodMetadata data = this.contract.parseAndValidateMetadata( MethodMetadata data = this.contract
method.getDeclaringClass(), method); .parseAndValidateMetadata(method.getDeclaringClass(), method);
assertEquals(extendedData.template().url(), data.template().url()); assertEquals(extendedData.template().url(), data.template().url());
assertEquals(extendedData.template().method(), data.template().method()); assertEquals(extendedData.template().method(), data.template().method());
...@@ -193,6 +196,7 @@ public class SpringMvcContractTests { ...@@ -193,6 +196,7 @@ public class SpringMvcContractTests {
assertEquals("Authorization", data.indexToName().get(0).iterator().next()); assertEquals("Authorization", data.indexToName().get(0).iterator().next());
assertEquals("id", data.indexToName().get(1).iterator().next()); assertEquals("id", data.indexToName().get(1).iterator().next());
assertEquals("amount", data.indexToName().get(2).iterator().next()); assertEquals("amount", data.indexToName().get(2).iterator().next());
assertNotNull(data.indexToExpander().get(2));
assertEquals("{Authorization}", assertEquals("{Authorization}",
data.template().headers().get("Authorization").iterator().next()); data.template().headers().get("Authorization").iterator().next());
...@@ -246,6 +250,19 @@ public class SpringMvcContractTests { ...@@ -246,6 +250,19 @@ public class SpringMvcContractTests {
} }
@Test @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 { public void testProcessHeaders() throws Exception {
Method method = TestTemplate_Headers.class.getDeclaredMethod("getTest", Method method = TestTemplate_Headers.class.getDeclaredMethod("getTest",
String.class); String.class);
...@@ -339,6 +356,11 @@ public class SpringMvcContractTests { ...@@ -339,6 +356,11 @@ public class SpringMvcContractTests {
ResponseEntity<TestObject> getTest(@PathVariable("id") String id); ResponseEntity<TestObject> getTest(@PathVariable("id") String id);
} }
public interface TestTemplate_ListParams {
@RequestMapping(value = "/test", method = RequestMethod.GET)
ResponseEntity<TestObject> getTest(@RequestParam("id") List<String> id);
}
@JsonAutoDetect @JsonAutoDetect
@RequestMapping("/advanced") @RequestMapping("/advanced")
public interface TestTemplate_Advanced { public interface TestTemplate_Advanced {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment