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 {