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/loadbalancer/FeignBlockingLoadBalancerClient.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/FeignBlockingLoadBalancerClient.java index d94c5152..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 @@ -78,6 +78,7 @@ public class FeignBlockingLoadBalancerClient implements Client { return delegate.execute(newRequest, options); } + // Visible for Sleuth instrumentation public Client getDelegate() { return delegate; } 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/beans/FeignClientTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/beans/FeignClientTests.java index ea1de38b..dd5b4e1c 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/beans/FeignClientTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/beans/FeignClientTests.java @@ -18,11 +18,18 @@ package org.springframework.cloud.openfeign.beans; import java.lang.reflect.InvocationHandler; import java.lang.reflect.Proxy; +import java.util.Arrays; +import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.UUID; +import feign.codec.EncodeException; import org.junit.Ignore; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import org.junit.runner.RunWith; import org.springframework.beans.factory.annotation.Autowired; @@ -38,16 +45,27 @@ import org.springframework.context.ApplicationContext; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Import; +import org.springframework.http.MediaType; +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; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestMethod; +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.Matchers.instanceOf; /** * @author Dave Syer + * @author Spencer Gibb + * @author Jakub Narloch + * @author Erik Kringen + * @author Halvdan Hoem Grelland + * @author Aaron Whiteside */ @RunWith(SpringJUnit4ClassRunner.class) @SpringBootTest(classes = FeignClientTests.Application.class, @@ -58,6 +76,9 @@ import static org.assertj.core.api.Assertions.assertThat; @DirtiesContext public class FeignClientTests { + @Rule + public ExpectedException expected = ExpectedException.none(); + @Value("${local.server.port}") private int port = 0; @@ -75,6 +96,9 @@ public class FeignClientTests { @Autowired private TestClient buildByBuilder; + @Autowired + private MultipartClient multipartClient; + @Test @Ignore // FIXME: 3.0.0 public void testAnnotations() { @@ -117,6 +141,82 @@ public class FeignClientTests { assertThat(invocationHandler).as("invocationHandler was null").isNotNull(); } + @Test + @Ignore // FIXME: 3.0.0 + public void testSingleRequestPart() { + String response = this.multipartClient.singlePart("abc"); + assertThat(response).isEqualTo("abc"); + } + + @Test + @Ignore // FIXME: 3.0.0 + 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 + @Ignore // FIXME: 3.0.0 + 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 + @Ignore // FIXME: 3.0.0 + 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 + @Ignore // FIXME: 3.0.0 + 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 + @Ignore // FIXME: 3.0.0 + 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 + @Ignore // FIXME: 3.0.0 + public void testInvalidMultipartFile() { + MockMultipartFile file = new MockMultipartFile("file1", "hello1.bin", null, + "hello".getBytes()); + expected.expectCause(instanceOf(EncodeException.class)); + this.multipartClient.invalid(file); + } + @Configuration(proxyBeanMethods = false) @EnableAutoConfiguration @RestController @@ -180,4 +280,53 @@ 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); + + } + } 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 614e375c..8ee29993 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 @@ -161,8 +161,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}")