Skip to content

Commit

Permalink
Exempt @RequestParams of type Collection from feign expander
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
Dave Syer committed Jun 17, 2016
1 parent 20db868 commit 9e47948
Show file tree
Hide file tree
Showing 4 changed files with 93 additions and 53 deletions.
46 changes: 25 additions & 21 deletions ...va/org/springframework/cloud/netflix/feign/annotation/RequestParamParameterProcessor.java
View file
Expand Up @@ -22,11 +22,11 @@
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.
* *
Expand All @@ -35,23 +35,27 @@
*/ */
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;
}
} }
16 changes: 12 additions & 4 deletions ...core/src/main/java/org/springframework/cloud/netflix/feign/support/SpringMvcContract.java
View file
Expand Up @@ -45,15 +45,15 @@
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;

This comment has been minimized.

Sign in to view

Copy link
@spencergibb

spencergibb Jun 17, 2016

Member

This seems odd that static imports are in the middle. Where should they be? I've always put them at the bottom?
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
*/ */
Expand Down Expand Up @@ -237,13 +237,21 @@ protected boolean processAnnotationsOnParameter(MethodMetadata data,
} }
} }
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);
} }
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");
Expand Down
32 changes: 19 additions & 13 deletions ...ore/src/test/java/org/springframework/cloud/netflix/feign/support/SpringEncoderTests.java
View file
Expand Up @@ -27,13 +27,13 @@
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
*/ */
Expand All @@ -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() {
Expand All @@ -73,22 +73,22 @@ class MediaTypeMatcher extends ArgumentMatcher<MediaType> {
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;
} }


@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();
} }
Expand All @@ -109,18 +109,19 @@ protected interface TestClient {
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;
} }


Expand All @@ -135,17 +136,22 @@ public boolean canWrite(Class<?> clazz, MediaType mediaType) {
} }


@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;
} }
} }
Expand Down
52 changes: 37 additions & 15 deletions ...src/test/java/org/springframework/cloud/netflix/feign/support/SpringMvcContractTests.java
View file
Expand Up @@ -18,6 +18,7 @@


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;
Expand All @@ -34,14 +35,16 @@


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
*/ */
Expand Down Expand Up @@ -97,10 +100,10 @@ public void testProcessAnnotations_Simple() throws Exception {
@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());
Expand All @@ -111,10 +114,10 @@ public void testProcessAnnotations_Class_AnnotationsGetSpecificTest()


@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());
Expand All @@ -129,10 +132,10 @@ public void testProcessAnnotations_ExtendedInterface() throws Exception {
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());
Expand Down Expand Up @@ -193,6 +196,7 @@ public void testProcessAnnotations_Advanced() throws Exception {
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());
Expand Down Expand Up @@ -245,6 +249,19 @@ public void testProcessAnnotations_Advanced3() throws Exception {
data.template().headers().get("Accept").iterator().next()); 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 @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",
Expand Down Expand Up @@ -339,6 +356,11 @@ public interface TestTemplate_Headers {
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 {
Expand Down

0 comments on commit 9e47948

Please sign in to comment.