From 05b58e087ed0eff7753fbeae768a6b7946fb7872 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Mon, 20 Jan 2020 17:00:56 +0100 Subject: [PATCH 1/5] Add public delegate modifiers in order to avoid reflection in SC Sleuth. --- .../FeignBlockingLoadBalancerClient.java | 16 +++++++++++----- .../ribbon/LoadBalancerFeignClient.java | 7 ++++++- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/FeignBlockingLoadBalancerClient.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/FeignBlockingLoadBalancerClient.java index 12e797ec..a6ef870e 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/FeignBlockingLoadBalancerClient.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/FeignBlockingLoadBalancerClient.java @@ -41,14 +41,14 @@ import org.springframework.util.Assert; public class FeignBlockingLoadBalancerClient implements Client { private static final Log LOG = LogFactory - .getLog(FeignBlockingLoadBalancerClient.class); - - private final Client delegate; + .getLog(FeignBlockingLoadBalancerClient.class); private final BlockingLoadBalancerClient loadBalancerClient; + private Client delegate; + public FeignBlockingLoadBalancerClient(Client delegate, - BlockingLoadBalancerClient loadBalancerClient) { + BlockingLoadBalancerClient loadBalancerClient) { this.delegate = delegate; this.loadBalancerClient = loadBalancerClient; } @@ -58,7 +58,7 @@ public class FeignBlockingLoadBalancerClient implements Client { final URI originalUri = URI.create(request.url()); String serviceId = originalUri.getHost(); Assert.state(serviceId != null, - "Request URI does not contain a valid hostname: " + originalUri); + "Request URI does not contain a valid hostname: " + originalUri); ServiceInstance instance = loadBalancerClient.choose(serviceId); if (instance == null) { String message = "Load balancer does not contain an instance for the service " @@ -78,8 +78,14 @@ public class FeignBlockingLoadBalancerClient implements Client { return delegate.execute(newRequest, options); } + // Visible for Sleuth instrumentation public Client getDelegate() { return delegate; } + // Visible for Sleuth instrumentation + public void setDelegate(Client delegate) { + this.delegate = delegate; + } + } diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/ribbon/LoadBalancerFeignClient.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/ribbon/LoadBalancerFeignClient.java index 9ed17419..19b9ea2a 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/ribbon/LoadBalancerFeignClient.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/ribbon/LoadBalancerFeignClient.java @@ -37,7 +37,7 @@ public class LoadBalancerFeignClient implements Client { static final Request.Options DEFAULT_OPTIONS = new Request.Options(); - private final Client delegate; + private Client delegate; private CachingSpringLoadBalancerFactory lbClientFactory; @@ -116,6 +116,11 @@ public class LoadBalancerFeignClient implements Client { return this.delegate; } + // Visible for Sleuth instrumentation + public void setDelegate(Client delegate) { + this.delegate = delegate; + } + private FeignLoadBalancer lbClient(String clientName) { return this.lbClientFactory.create(clientName); } From ada2cb79130dbe5fe0fda74d3a27219f67224d01 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Mon, 20 Jan 2020 17:31:13 +0100 Subject: [PATCH 2/5] Revert "Add public delegate modifiers in order to avoid reflection in SC Sleuth." This reverts commit 05b58e087ed0eff7753fbeae768a6b7946fb7872. --- .../FeignBlockingLoadBalancerClient.java | 16 +++++----------- .../ribbon/LoadBalancerFeignClient.java | 7 +------ 2 files changed, 6 insertions(+), 17 deletions(-) diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/FeignBlockingLoadBalancerClient.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/FeignBlockingLoadBalancerClient.java index a6ef870e..12e797ec 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/FeignBlockingLoadBalancerClient.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/FeignBlockingLoadBalancerClient.java @@ -41,14 +41,14 @@ import org.springframework.util.Assert; public class FeignBlockingLoadBalancerClient implements Client { private static final Log LOG = LogFactory - .getLog(FeignBlockingLoadBalancerClient.class); + .getLog(FeignBlockingLoadBalancerClient.class); + + private final Client delegate; private final BlockingLoadBalancerClient loadBalancerClient; - private Client delegate; - public FeignBlockingLoadBalancerClient(Client delegate, - BlockingLoadBalancerClient loadBalancerClient) { + BlockingLoadBalancerClient loadBalancerClient) { this.delegate = delegate; this.loadBalancerClient = loadBalancerClient; } @@ -58,7 +58,7 @@ public class FeignBlockingLoadBalancerClient implements Client { final URI originalUri = URI.create(request.url()); String serviceId = originalUri.getHost(); Assert.state(serviceId != null, - "Request URI does not contain a valid hostname: " + originalUri); + "Request URI does not contain a valid hostname: " + originalUri); ServiceInstance instance = loadBalancerClient.choose(serviceId); if (instance == null) { String message = "Load balancer does not contain an instance for the service " @@ -78,14 +78,8 @@ public class FeignBlockingLoadBalancerClient implements Client { return delegate.execute(newRequest, options); } - // Visible for Sleuth instrumentation public Client getDelegate() { return delegate; } - // Visible for Sleuth instrumentation - public void setDelegate(Client delegate) { - this.delegate = delegate; - } - } diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/ribbon/LoadBalancerFeignClient.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/ribbon/LoadBalancerFeignClient.java index 19b9ea2a..9ed17419 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/ribbon/LoadBalancerFeignClient.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/ribbon/LoadBalancerFeignClient.java @@ -37,7 +37,7 @@ public class LoadBalancerFeignClient implements Client { static final Request.Options DEFAULT_OPTIONS = new Request.Options(); - private Client delegate; + private final Client delegate; private CachingSpringLoadBalancerFactory lbClientFactory; @@ -116,11 +116,6 @@ public class LoadBalancerFeignClient implements Client { return this.delegate; } - // Visible for Sleuth instrumentation - public void setDelegate(Client delegate) { - this.delegate = delegate; - } - private FeignLoadBalancer lbClient(String clientName) { return this.lbClientFactory.create(clientName); } From 3eabb0237ecf14bbb58e13e38a51d2e163ab2da8 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Mon, 20 Jan 2020 17:32:19 +0100 Subject: [PATCH 3/5] Add comment. --- .../openfeign/loadbalancer/FeignBlockingLoadBalancerClient.java | 1 + 1 file changed, 1 insertion(+) diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/FeignBlockingLoadBalancerClient.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/FeignBlockingLoadBalancerClient.java index 12e797ec..d42fe9ef 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/FeignBlockingLoadBalancerClient.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/FeignBlockingLoadBalancerClient.java @@ -78,6 +78,7 @@ public class FeignBlockingLoadBalancerClient implements Client { return delegate.execute(newRequest, options); } + // Visible for Sleuth instrumentation public Client getDelegate() { return delegate; } From 422dda0a75cfd283479b107492017275765ff832 Mon Sep 17 00:00:00 2001 From: buildmaster Date: Tue, 21 Jan 2020 14:06:45 +0000 Subject: [PATCH 4/5] Bumping versions --- .../.flattened-pom.xml | 24 +++++++++---------- .../FeignBlockingLoadBalancerClient.java | 6 ++--- spring-cloud-openfeign-dependencies/pom.xml | 2 +- .../.flattened-pom.xml | 2 +- 4 files changed, 17 insertions(+), 17 deletions(-) diff --git a/spring-cloud-openfeign-core/.flattened-pom.xml b/spring-cloud-openfeign-core/.flattened-pom.xml index 2eb49a24..2df8945e 100644 --- a/spring-cloud-openfeign-core/.flattened-pom.xml +++ b/spring-cloud-openfeign-core/.flattened-pom.xml @@ -100,7 +100,7 @@ org.springframework.boot spring-boot-autoconfigure - 2.3.0.BUILD-SNAPSHOT + 2.2.2.RELEASE compile @@ -112,42 +112,42 @@ org.springframework.boot spring-boot-starter-actuator - 2.3.0.BUILD-SNAPSHOT + 2.2.2.RELEASE compile true org.springframework.boot spring-boot-starter-security - 2.3.0.BUILD-SNAPSHOT + 2.2.2.RELEASE compile true org.springframework.boot spring-boot-starter-web - 2.3.0.BUILD-SNAPSHOT + 2.2.2.RELEASE compile true org.springframework.boot spring-boot-starter-webflux - 2.3.0.BUILD-SNAPSHOT + 2.2.2.RELEASE compile true org.springframework.boot spring-boot-starter-reactor-netty - 2.3.0.BUILD-SNAPSHOT + 2.2.2.RELEASE compile true org.springframework.boot spring-boot-starter-hateoas - 2.3.0.BUILD-SNAPSHOT + 2.2.2.RELEASE compile true @@ -161,7 +161,7 @@ io.projectreactor reactor-core - 3.3.2.RELEASE + 3.3.1.RELEASE compile true @@ -182,7 +182,7 @@ org.springframework.boot spring-boot-starter-aop - 2.3.0.BUILD-SNAPSHOT + 2.2.2.RELEASE compile @@ -356,21 +356,21 @@ org.springframework.boot spring-boot-autoconfigure-processor - 2.3.0.BUILD-SNAPSHOT + 2.2.2.RELEASE compile true org.springframework.boot spring-boot-configuration-processor - 2.3.0.BUILD-SNAPSHOT + 2.2.2.RELEASE compile true org.springframework.data spring-data-commons - 2.3.0.M2 + 2.2.3.RELEASE compile true diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/FeignBlockingLoadBalancerClient.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/FeignBlockingLoadBalancerClient.java index d42fe9ef..af55f1c3 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/FeignBlockingLoadBalancerClient.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/FeignBlockingLoadBalancerClient.java @@ -41,14 +41,14 @@ import org.springframework.util.Assert; public class FeignBlockingLoadBalancerClient implements Client { private static final Log LOG = LogFactory - .getLog(FeignBlockingLoadBalancerClient.class); + .getLog(FeignBlockingLoadBalancerClient.class); private final Client delegate; private final BlockingLoadBalancerClient loadBalancerClient; public FeignBlockingLoadBalancerClient(Client delegate, - BlockingLoadBalancerClient loadBalancerClient) { + BlockingLoadBalancerClient loadBalancerClient) { this.delegate = delegate; this.loadBalancerClient = loadBalancerClient; } @@ -58,7 +58,7 @@ public class FeignBlockingLoadBalancerClient implements Client { final URI originalUri = URI.create(request.url()); String serviceId = originalUri.getHost(); Assert.state(serviceId != null, - "Request URI does not contain a valid hostname: " + originalUri); + "Request URI does not contain a valid hostname: " + originalUri); ServiceInstance instance = loadBalancerClient.choose(serviceId); if (instance == null) { String message = "Load balancer does not contain an instance for the service " diff --git a/spring-cloud-openfeign-dependencies/pom.xml b/spring-cloud-openfeign-dependencies/pom.xml index 3602be91..83bdcb1a 100644 --- a/spring-cloud-openfeign-dependencies/pom.xml +++ b/spring-cloud-openfeign-dependencies/pom.xml @@ -6,7 +6,7 @@ spring-cloud-dependencies-parent org.springframework.cloud - 2.2.1.RELEASE + 2.2.2.BUILD-SNAPSHOT spring-cloud-openfeign-dependencies diff --git a/spring-cloud-starter-openfeign/.flattened-pom.xml b/spring-cloud-starter-openfeign/.flattened-pom.xml index 8aa54b1b..ed9e3d52 100644 --- a/spring-cloud-starter-openfeign/.flattened-pom.xml +++ b/spring-cloud-starter-openfeign/.flattened-pom.xml @@ -112,7 +112,7 @@ org.springframework spring-web - 5.2.3.RELEASE + 5.2.2.RELEASE compile From 892e65e303e11baa259440a56d68a009da2b4cdd Mon Sep 17 00:00:00 2001 From: Aaron Whiteside Date: Fri, 24 Jan 2020 01:02:21 +1100 Subject: [PATCH 5/5] Fix GH-201: Multiple @RequestPart not Working (#258) Summary of changes follows: - Delegate ALL requestBody encoding where Content-Type is multipart/form-data to the SpringFormEncoder. - Introduce RequestPartParameterProcessor to deal with @RequestPart annotations, adds parameters to MethodMetadata.formParams(). - Wrap HttpMessageConversionException in EncodeException. - Add tests to verify expected behaviour. However there still exists a gap in functionality where any user defined pojo will be serialized as a Map, as there is currently no way for SpringFormEncoder to know about the Content-Type of the individual parts beyond MultipartFile, boxed primitive types (which are treated as text/plain) and other built-in types inherited from FormEncoder. --- .../RequestPartParameterProcessor.java | 64 +++++++ .../openfeign/support/SpringEncoder.java | 25 +-- .../openfeign/support/SpringMvcContract.java | 3 + .../openfeign/support/SpringEncoderTests.java | 2 - .../support/SpringMvcContractTests.java | 22 +++ .../openfeign/valid/FeignClientTests.java | 176 +++++++++++++++++- 6 files changed, 277 insertions(+), 15 deletions(-) create mode 100644 spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/annotation/RequestPartParameterProcessor.java diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/annotation/RequestPartParameterProcessor.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/annotation/RequestPartParameterProcessor.java new file mode 100644 index 00000000..78b7765e --- /dev/null +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/annotation/RequestPartParameterProcessor.java @@ -0,0 +1,64 @@ +/* + * Copyright 2013-2019 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.openfeign.annotation; + +import java.lang.annotation.Annotation; +import java.lang.reflect.Method; +import java.util.Collection; + +import feign.MethodMetadata; + +import org.springframework.cloud.openfeign.AnnotatedParameterProcessor; +import org.springframework.web.bind.annotation.RequestPart; + +import static feign.Util.checkState; +import static feign.Util.emptyToNull; + +/** + * {@link RequestPart} parameter processor. + * + * @author Aaron Whiteside + * @see AnnotatedParameterProcessor + */ +public class RequestPartParameterProcessor implements AnnotatedParameterProcessor { + + private static final Class ANNOTATION = RequestPart.class; + + @Override + public Class getAnnotationType() { + return ANNOTATION; + } + + @Override + public boolean processArgument(AnnotatedParameterContext context, + Annotation annotation, Method method) { + int parameterIndex = context.getParameterIndex(); + MethodMetadata data = context.getMethodMetadata(); + + String name = ANNOTATION.cast(annotation).value(); + checkState(emptyToNull(name) != null, + "RequestPart.value() was empty on parameter %s", parameterIndex); + context.setParameterName(name); + + data.formParams().add(name); + Collection names = context.setTemplateParameter(name, + data.indexToName().get(parameterIndex)); + data.indexToName().put(parameterIndex, names); + return true; + } + +} diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringEncoder.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringEncoder.java index bb82c76c..dedee915 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringEncoder.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringEncoder.java @@ -41,6 +41,7 @@ import org.springframework.http.HttpOutputMessage; import org.springframework.http.MediaType; import org.springframework.http.converter.ByteArrayHttpMessageConverter; import org.springframework.http.converter.GenericHttpMessageConverter; +import org.springframework.http.converter.HttpMessageConversionException; import org.springframework.http.converter.HttpMessageConverter; import org.springframework.http.converter.protobuf.ProtobufHttpMessageConverter; import org.springframework.web.multipart.MultipartFile; @@ -52,6 +53,7 @@ import static org.springframework.cloud.openfeign.support.FeignUtils.getHttpHead * @author Spencer Gibb * @author Scien Jus * @author Ahmad Mozafarnia + * @author Aaron Whiteside */ public class SpringEncoder implements Encoder { @@ -59,7 +61,7 @@ public class SpringEncoder implements Encoder { private final SpringFormEncoder springFormEncoder = new SpringFormEncoder(); - private ObjectFactory messageConverters; + private final ObjectFactory messageConverters; public SpringEncoder(ObjectFactory messageConverters) { this.messageConverters = messageConverters; @@ -79,16 +81,15 @@ public class SpringEncoder implements Encoder { requestContentType = MediaType.valueOf(type); } - if (bodyType != null && bodyType.equals(MultipartFile.class)) { - if (Objects.equals(requestContentType, MediaType.MULTIPART_FORM_DATA)) { - this.springFormEncoder.encode(requestBody, bodyType, request); - return; - } - else { - String message = "Content-Type \"" + MediaType.MULTIPART_FORM_DATA - + "\" not set for request body of type " - + requestBody.getClass().getSimpleName(); - throw new EncodeException(message); + if (Objects.equals(requestContentType, MediaType.MULTIPART_FORM_DATA)) { + this.springFormEncoder.encode(requestBody, bodyType, request); + return; + } + else { + if (bodyType == MultipartFile.class) { + log.warn( + "For MultipartFile to be handled correctly, the 'consumes' parameter of @RequestMapping " + + "should be specified as MediaType.MULTIPART_FORM_DATA_VALUE"); } } @@ -106,7 +107,7 @@ public class SpringEncoder implements Encoder { messageConverter, request); } } - catch (IOException ex) { + catch (IOException | HttpMessageConversionException ex) { throw new EncodeException("Error converting request body", ex); } if (outputMessage != null) { 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 7b23f957..f24c99fb 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 @@ -41,6 +41,7 @@ import org.springframework.cloud.openfeign.annotation.PathVariableParameterProce import org.springframework.cloud.openfeign.annotation.QueryMapParameterProcessor; import org.springframework.cloud.openfeign.annotation.RequestHeaderParameterProcessor; import org.springframework.cloud.openfeign.annotation.RequestParamParameterProcessor; +import org.springframework.cloud.openfeign.annotation.RequestPartParameterProcessor; import org.springframework.context.ConfigurableApplicationContext; import org.springframework.context.ResourceLoaderAware; import org.springframework.core.DefaultParameterNameDiscoverer; @@ -69,6 +70,7 @@ import static org.springframework.core.annotation.AnnotatedElementUtils.findMerg * @author Halvdan Hoem Grelland * @author Aram Peres * @author Olga Maciaszek-Sharma + * @author Aaron Whiteside */ public class SpringMvcContract extends Contract.BaseContract implements ResourceLoaderAware { @@ -357,6 +359,7 @@ public class SpringMvcContract extends Contract.BaseContract annotatedArgumentResolvers.add(new RequestParamParameterProcessor()); annotatedArgumentResolvers.add(new RequestHeaderParameterProcessor()); annotatedArgumentResolvers.add(new QueryMapParameterProcessor()); + annotatedArgumentResolvers.add(new RequestPartParameterProcessor()); return annotatedArgumentResolvers; } diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringEncoderTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringEncoderTests.java index 5e91e76b..45295f3b 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringEncoderTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringEncoderTests.java @@ -156,8 +156,6 @@ public class SpringEncoderTests { MultipartFile multipartFile = new MockMultipartFile("test_multipart_file", "hi".getBytes()); encoder.encode(multipartFile, MultipartFile.class, request); - - assertThat(request.requestCharset()).as("request charset is not null").isNull(); } // gh-105, gh-107 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 39c8a215..66f050b0 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 @@ -54,6 +54,8 @@ import org.springframework.web.bind.annotation.RequestHeader; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestMethod; import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.web.bind.annotation.RequestPart; +import org.springframework.web.multipart.MultipartFile; import static com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility.ANY; import static com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility.NONE; @@ -66,6 +68,7 @@ import static org.springframework.web.util.UriUtils.encode; * @author chadjaros * @author Halvdan Hoem Grelland * @author Aram Peres + * @author Aaron Whiteside */ public class SpringMvcContractTests { @@ -566,6 +569,16 @@ public class SpringMvcContractTests { "{Accept}"); } + @Test + public void testMultipleRequestPartAnnotations() throws NoSuchMethodException { + Method method = TestTemplate_RequestPart.class.getDeclaredMethod( + "requestWithMultipleParts", MultipartFile.class, String.class); + + MethodMetadata data = contract + .parseAndValidateMetadata(method.getDeclaringClass(), method); + assertThat(data.formParams()).contains("file", "id"); + } + public interface TestTemplate_Simple { @RequestMapping(value = "/test/{id}", method = RequestMethod.GET, @@ -670,6 +683,15 @@ public class SpringMvcContractTests { } + public interface TestTemplate_RequestPart { + + @RequestMapping(path = "/requestPart", method = RequestMethod.POST, + consumes = MediaType.MULTIPART_FORM_DATA_VALUE) + void requestWithMultipleParts(@RequestPart("file") MultipartFile file, + @RequestPart("id") String identifier); + + } + public interface TestTemplate_MatrixVariable { @RequestMapping(path = "/matrixVariable/{params}") diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/valid/FeignClientTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/valid/FeignClientTests.java index ab4502b1..0dd60ad5 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/valid/FeignClientTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/valid/FeignClientTests.java @@ -24,12 +24,19 @@ import java.time.LocalDate; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.HashMap; import java.util.List; import java.util.Locale; +import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.UUID; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; +import java.util.stream.Collectors; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.Part; import com.netflix.hystrix.HystrixCommand; import com.netflix.hystrix.HystrixCommandGroupKey; @@ -43,9 +50,12 @@ import feign.Logger; import feign.RequestInterceptor; import feign.RequestTemplate; import feign.Target; +import feign.codec.EncodeException; import feign.hystrix.FallbackFactory; import feign.hystrix.SetterFactory; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import org.junit.runner.RunWith; import rx.Observable; import rx.Single; @@ -74,7 +84,9 @@ import org.springframework.format.FormatterRegistry; import org.springframework.format.annotation.DateTimeFormat; import org.springframework.http.HttpEntity; import org.springframework.http.HttpStatus; +import org.springframework.http.MediaType; import org.springframework.http.ResponseEntity; +import org.springframework.mock.web.MockMultipartFile; import org.springframework.test.annotation.DirtiesContext; import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; import org.springframework.web.bind.annotation.RequestBody; @@ -82,15 +94,19 @@ import org.springframework.web.bind.annotation.RequestHeader; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestMethod; import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.web.bind.annotation.RequestPart; import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.multipart.MultipartFile; import static org.assertj.core.api.Assertions.assertThat; +import static org.hamcrest.core.IsInstanceOf.instanceOf; /** * @author Spencer Gibb * @author Jakub Narloch * @author Erik Kringen * @author Halvdan Hoem Grelland + * @author Aaron Whiteside */ @RunWith(SpringJUnit4ClassRunner.class) @SpringBootTest(classes = FeignClientTests.Application.class, @@ -110,6 +126,9 @@ public class FeignClientTests { public static final String MYHEADER2 = "myheader2"; + @Rule + public ExpectedException expected = ExpectedException.none(); + @Autowired HystrixClient hystrixClient; @@ -144,6 +163,9 @@ public class FeignClientTests { @Autowired private NullHystrixClientWithFallBackFactory nullHystrixClientWithFallBackFactory; + @Autowired + private MultipartClient multipartClient; + private static ArrayList getHelloList() { ArrayList hellos = new ArrayList<>(); hellos.add(new Hello(HELLO_WORLD_1)); @@ -422,6 +444,75 @@ public class FeignClientTests { assertThat(getHelloList()).as("hellos didn't match").isEqualTo(hellos); } + @Test + public void testSingleRequestPart() { + String response = this.multipartClient.singlePart("abc"); + assertThat(response).isEqualTo("abc"); + } + + @Test + public void testMultipleRequestParts() { + MockMultipartFile file = new MockMultipartFile("file", "hello.bin", null, + "hello".getBytes()); + String response = this.multipartClient.multipart("abc", "123", file); + assertThat(response).isEqualTo("abc123hello.bin"); + } + + @Test + public void testRequestPartWithListOfMultipartFiles() { + List multipartFiles = Arrays.asList( + new MockMultipartFile("file1", "hello1.bin", null, "hello".getBytes()), + new MockMultipartFile("file2", "hello2.bin", null, "hello".getBytes())); + String partNames = this.multipartClient + .requestPartListOfMultipartFilesReturnsPartNames(multipartFiles); + assertThat(partNames).isEqualTo("files,files"); + String fileNames = this.multipartClient + .requestPartListOfMultipartFilesReturnsFileNames(multipartFiles); + assertThat(fileNames).contains("hello1.bin", "hello2.bin"); + } + + @Test + public void testRequestBodyWithSingleMultipartFile() { + String partName = UUID.randomUUID().toString(); + MockMultipartFile file1 = new MockMultipartFile(partName, "hello1.bin", null, + "hello".getBytes()); + String response = this.multipartClient.requestBodySingleMultipartFile(file1); + assertThat(response).isEqualTo(partName); + } + + @Test + public void testRequestBodyWithListOfMultipartFiles() { + MockMultipartFile file1 = new MockMultipartFile("file1", "hello1.bin", null, + "hello".getBytes()); + MockMultipartFile file2 = new MockMultipartFile("file2", "hello2.bin", null, + "hello".getBytes()); + String response = this.multipartClient + .requestBodyListOfMultipartFiles(Arrays.asList(file1, file2)); + assertThat(response).contains("file1", "file2"); + } + + @Test + public void testRequestBodyWithMap() { + MockMultipartFile file1 = new MockMultipartFile("file1", "hello1.bin", null, + "hello".getBytes()); + MockMultipartFile file2 = new MockMultipartFile("file2", "hello2.bin", null, + "hello".getBytes()); + Map form = new HashMap<>(); + form.put("file1", file1); + form.put("file2", file2); + form.put("hello", "world"); + String response = this.multipartClient.requestBodyMap(form); + assertThat(response).contains("file1", "file2", "hello"); + } + + @Test + public void testInvalidMultipartFile() { + MockMultipartFile file = new MockMultipartFile("file1", "hello1.bin", null, + "hello".getBytes()); + expected.expectCause(instanceOf(EncodeException.class)); + this.multipartClient.invalid(file); + } + protected enum Arg { A, B; @@ -572,6 +663,55 @@ public class FeignClientTests { } + @FeignClient(name = "localapp8") + protected interface MultipartClient { + + @RequestMapping(method = RequestMethod.POST, path = "/singlePart", + consumes = MediaType.MULTIPART_FORM_DATA_VALUE, + produces = MediaType.TEXT_PLAIN_VALUE) + String singlePart(@RequestPart("hello") String hello); + + @RequestMapping(method = RequestMethod.POST, path = "/multipart", + consumes = MediaType.MULTIPART_FORM_DATA_VALUE, + produces = MediaType.TEXT_PLAIN_VALUE) + String multipart(@RequestPart("hello") String hello, + @RequestPart("world") String world, + @RequestPart("file") MultipartFile file); + + @RequestMapping(method = RequestMethod.POST, path = "/multipartNames", + consumes = MediaType.MULTIPART_FORM_DATA_VALUE, + produces = MediaType.TEXT_PLAIN_VALUE) + String requestPartListOfMultipartFilesReturnsPartNames( + @RequestPart("files") List files); + + @RequestMapping(method = RequestMethod.POST, path = "/multipartFilenames", + consumes = MediaType.MULTIPART_FORM_DATA_VALUE, + produces = MediaType.TEXT_PLAIN_VALUE) + String requestPartListOfMultipartFilesReturnsFileNames( + @RequestPart("files") List files); + + @RequestMapping(method = RequestMethod.POST, path = "/multipartNames", + consumes = MediaType.MULTIPART_FORM_DATA_VALUE, + produces = MediaType.TEXT_PLAIN_VALUE) + String requestBodyListOfMultipartFiles(@RequestBody List files); + + @RequestMapping(method = RequestMethod.POST, path = "/multipartNames", + consumes = MediaType.MULTIPART_FORM_DATA_VALUE, + produces = MediaType.TEXT_PLAIN_VALUE) + String requestBodySingleMultipartFile(@RequestBody MultipartFile file); + + @RequestMapping(method = RequestMethod.POST, path = "/multipartNames", + consumes = MediaType.MULTIPART_FORM_DATA_VALUE, + produces = MediaType.TEXT_PLAIN_VALUE) + String requestBodyMap(@RequestBody Map form); + + @RequestMapping(method = RequestMethod.POST, path = "/invalid", + consumes = MediaType.APPLICATION_OCTET_STREAM_VALUE, + produces = MediaType.TEXT_PLAIN_VALUE) + String invalid(@RequestBody MultipartFile file); + + } + protected static class OtherArg { public final String value; @@ -706,7 +846,7 @@ public class FeignClientTests { DecodingTestClient.class, HystrixClient.class, HystrixClientWithFallBackFactory.class, HystrixSetterFactoryClient.class, InvalidTypeHystrixClientWithFallBackFactory.class, - NullHystrixClientWithFallBackFactory.class }, + NullHystrixClientWithFallBackFactory.class, MultipartClient.class }, defaultConfiguration = TestDefaultFeignConfig.class) @RibbonClients({ @RibbonClient(name = "localapp", @@ -724,6 +864,8 @@ public class FeignClientTests { @RibbonClient(name = "localapp6", configuration = LocalRibbonClientConfiguration.class), @RibbonClient(name = "localapp7", + configuration = LocalRibbonClientConfiguration.class), + @RibbonClient(name = "localapp8", configuration = LocalRibbonClientConfiguration.class) }) @Import(NoSecurityConfiguration.class) protected static class Application { @@ -884,6 +1026,38 @@ public class FeignClientTests { return result; } + @RequestMapping(method = RequestMethod.POST, path = "/singlePart", + consumes = MediaType.MULTIPART_FORM_DATA_VALUE, + produces = MediaType.TEXT_PLAIN_VALUE) + String multipart(@RequestPart("hello") String hello) { + return hello; + } + + @RequestMapping(method = RequestMethod.POST, path = "/multipart", + consumes = MediaType.MULTIPART_FORM_DATA_VALUE, + produces = MediaType.TEXT_PLAIN_VALUE) + String multipart(@RequestPart("hello") String hello, + @RequestPart("world") String world, + @RequestPart("file") MultipartFile file) { + return hello + world + file.getOriginalFilename(); + } + + @RequestMapping(method = RequestMethod.POST, path = "/multipartNames", + consumes = MediaType.MULTIPART_FORM_DATA_VALUE, + produces = MediaType.TEXT_PLAIN_VALUE) + String multipartNames(HttpServletRequest request) throws Exception { + return request.getParts().stream().map(Part::getName) + .collect(Collectors.joining(",")); + } + + @RequestMapping(method = RequestMethod.POST, path = "/multipartFilenames", + consumes = MediaType.MULTIPART_FORM_DATA_VALUE, + produces = MediaType.TEXT_PLAIN_VALUE) + String multipartFilenames(HttpServletRequest request) throws Exception { + return request.getParts().stream().map(Part::getSubmittedFileName) + .collect(Collectors.joining(",")); + } + } public static class Hello {