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<String,String>, 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.

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<RequestPart> ANNOTATION = RequestPart.class;
+
+	@Override
+	public Class<? extends Annotation> 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<String> names = context.setTemplateParameter(name,
+				data.indexToName().get(parameterIndex));
+		data.indexToName().put(parameterIndex, names);
+		return true;
+	}
+
+}

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<HttpMessageConverters> messageConverters;
+	private final ObjectFactory<HttpMessageConverters> messageConverters;
 
 	public SpringEncoder(ObjectFactory<HttpMessageConverters> 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) {

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;
 	}

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

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}")

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<Hello> getHelloList() {
 		ArrayList<Hello> 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<MultipartFile> 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<String, Object> 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<MultipartFile> files);
+
+		@RequestMapping(method = RequestMethod.POST, path = "/multipartFilenames",
+				consumes = MediaType.MULTIPART_FORM_DATA_VALUE,
+				produces = MediaType.TEXT_PLAIN_VALUE)
+		String requestPartListOfMultipartFilesReturnsFileNames(
+				@RequestPart("files") List<MultipartFile> files);
+
+		@RequestMapping(method = RequestMethod.POST, path = "/multipartNames",
+				consumes = MediaType.MULTIPART_FORM_DATA_VALUE,
+				produces = MediaType.TEXT_PLAIN_VALUE)
+		String requestBodyListOfMultipartFiles(@RequestBody List<MultipartFile> 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<String, ?> 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 {