From 40bf923d7df5d4e6b592a03d4fc6b6dd6fde7aeb Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Tue, 27 Jun 2023 10:51:42 +0100 Subject: [PATCH] Polishing in MultipartFileArgumentResolver Closes gh-30728 --- .../ROOT/pages/integration/rest-clients.adoc | 4 ++ .../MultipartFileArgumentResolver.java | 29 ++++------ .../MultipartFileArgumentResolverTests.java | 54 ++++++++++--------- .../WebClientHttpServiceProxyTests.java | 2 +- 4 files changed, 44 insertions(+), 45 deletions(-) diff --git a/framework-docs/modules/ROOT/pages/integration/rest-clients.adoc b/framework-docs/modules/ROOT/pages/integration/rest-clients.adoc index e371c596cff6..8915e2f7be3a 100644 --- a/framework-docs/modules/ROOT/pages/integration/rest-clients.adoc +++ b/framework-docs/modules/ROOT/pages/integration/rest-clients.adoc @@ -448,6 +448,10 @@ method parameters: Object (entity to be encoded, e.g. as JSON), `HttpEntity` (part content and headers), a Spring `Part`, or Reactive Streams `Publisher` of any of the above. +| `MultipartFile` +| Add a request part from a `MultipartFile`, typically used in a Spring MVC controller + where it represents an uploaded file. + | `@CookieValue` | Add a cookie or multiple cookies. The argument may be a `Map` or `MultiValueMap` with multiple cookies, a `Collection` of values, or an diff --git a/spring-web/src/main/java/org/springframework/web/service/invoker/MultipartFileArgumentResolver.java b/spring-web/src/main/java/org/springframework/web/service/invoker/MultipartFileArgumentResolver.java index 68c2d427baa0..5773ebe1243d 100644 --- a/spring-web/src/main/java/org/springframework/web/service/invoker/MultipartFileArgumentResolver.java +++ b/spring-web/src/main/java/org/springframework/web/service/invoker/MultipartFileArgumentResolver.java @@ -19,7 +19,6 @@ import java.util.Optional; import org.springframework.core.MethodParameter; -import org.springframework.core.io.Resource; import org.springframework.http.HttpEntity; import org.springframework.http.HttpHeaders; import org.springframework.util.Assert; @@ -27,36 +26,29 @@ /** * {@link HttpServiceArgumentResolver} for arguments of type {@link MultipartFile}. - * The arguments should not be annotated. To allow for non-required arguments, - * the {@link MultipartFile} parameters can also be wrapped with {@link Optional}. + * The argument is recognized by type, and does not need to be annotated. To make + * it optional, declare the parameter with an {@link Optional} wrapper. * * @author Olga Maciaszek-Sharma + * @author Rossen Stoyanchev * @since 6.1 */ public class MultipartFileArgumentResolver extends AbstractNamedValueArgumentResolver { - private static final String MULTIPART_FILE_LABEL = "multipart file"; - @Override protected NamedValueInfo createNamedValueInfo(MethodParameter parameter) { - if (!parameter.nestedIfOptional().getNestedParameterType().equals(MultipartFile.class)) { - return null; - } - return new NamedValueInfo("", true, null, MULTIPART_FILE_LABEL, true); - + Class type = parameter.nestedIfOptional().getNestedParameterType(); + return (type.equals(MultipartFile.class) ? + new NamedValueInfo("", true, null, "MultipartFile", true) : null); } @Override - protected void addRequestValue(String name, Object value, MethodParameter parameter, - HttpRequestValues.Builder requestValues) { - Assert.state(value instanceof MultipartFile, - "The value has to be of type 'MultipartFile'"); + protected void addRequestValue( + String name, Object value, MethodParameter parameter, HttpRequestValues.Builder values) { + Assert.state(value instanceof MultipartFile, "Expected MultipartFile value"); MultipartFile file = (MultipartFile) value; - requestValues.addRequestPart(name, toHttpEntity(name, file)); - } - private HttpEntity toHttpEntity(String name, MultipartFile file) { HttpHeaders headers = new HttpHeaders(); if (file.getOriginalFilename() != null) { headers.setContentDispositionFormData(name, file.getOriginalFilename()); @@ -64,7 +56,8 @@ private HttpEntity toHttpEntity(String name, MultipartFile file) { if (file.getContentType() != null) { headers.add(HttpHeaders.CONTENT_TYPE, file.getContentType()); } - return new HttpEntity<>(file.getResource(), headers); + + values.addRequestPart(name, new HttpEntity<>(file.getResource(), headers)); } } diff --git a/spring-web/src/test/java/org/springframework/web/service/invoker/MultipartFileArgumentResolverTests.java b/spring-web/src/test/java/org/springframework/web/service/invoker/MultipartFileArgumentResolverTests.java index 0a9abe8e1e40..2119f4462761 100644 --- a/spring-web/src/test/java/org/springframework/web/service/invoker/MultipartFileArgumentResolverTests.java +++ b/spring-web/src/test/java/org/springframework/web/service/invoker/MultipartFileArgumentResolverTests.java @@ -21,7 +21,6 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.springframework.http.ContentDisposition; import org.springframework.http.HttpEntity; import org.springframework.http.HttpHeaders; import org.springframework.http.MediaType; @@ -35,7 +34,8 @@ /** * Unit tests for {@link MultipartFileArgumentResolver}. - * Tests for base class functionality of this resolver can be found in {@link NamedValueArgumentResolverTests}. + * Tests for base class functionality of this resolver can be found in + * {@link NamedValueArgumentResolverTests}. * * @author Olga Maciaszek-Sharma */ @@ -46,48 +46,49 @@ class MultipartFileArgumentResolverTests { private TestClient client; + @BeforeEach void setUp() { - HttpServiceProxyFactory proxyFactory = HttpServiceProxyFactory.builder(this.clientAdapter).build(); - this.client = proxyFactory.createClient(TestClient.class); + HttpServiceProxyFactory factory = HttpServiceProxyFactory.builder(this.clientAdapter).build(); + this.client = factory.createClient(TestClient.class); } + @Test void multipartFile() { String fileName = "testFileName"; String originalFileName = "originalTestFileName"; - MultipartFile testFile = new MockMultipartFile(fileName, originalFileName, - MediaType.APPLICATION_JSON_VALUE, "test".getBytes()); + MultipartFile testFile = new MockMultipartFile(fileName, originalFileName, "text/plain", "test".getBytes()); + this.client.postMultipartFile(testFile); + Object value = this.clientAdapter.getRequestValues().getBodyValue(); + + assertThat(value).isInstanceOf(MultiValueMap.class); + MultiValueMap> map = (MultiValueMap>) value; + assertThat(map).hasSize(1); - Object body = clientAdapter.getRequestValues().getBodyValue(); - - assertThat(body).isInstanceOf(MultiValueMap.class); - MultiValueMap> map = (MultiValueMap>) body; - assertThat(map.size()).isEqualTo(1); - assertThat(map.getFirst("file")).isNotNull(); - HttpEntity fileEntity = map.getFirst("file"); - assertThat(fileEntity.getBody()).isEqualTo(testFile.getResource()); - HttpHeaders headers = fileEntity.getHeaders(); - assertThat(headers.getContentType()).isEqualTo(MediaType.APPLICATION_JSON); - ContentDisposition contentDisposition = headers.getContentDisposition(); - assertThat(contentDisposition.getType()).isEqualTo("form-data"); - assertThat(contentDisposition.getName()).isEqualTo("file"); - assertThat(contentDisposition.getFilename()).isEqualTo(originalFileName); + HttpEntity entity = map.getFirst("file"); + assertThat(entity).isNotNull(); + assertThat(entity.getBody()).isEqualTo(testFile.getResource()); + + HttpHeaders headers = entity.getHeaders(); + assertThat(headers.getContentType()).isEqualTo(MediaType.TEXT_PLAIN); + assertThat(headers.getContentDisposition().getType()).isEqualTo("form-data"); + assertThat(headers.getContentDisposition().getName()).isEqualTo("file"); + assertThat(headers.getContentDisposition().getFilename()).isEqualTo(originalFileName); } @Test void optionalMultipartFile() { this.client.postOptionalMultipartFile(Optional.empty(), "anotherPart"); + Object value = clientAdapter.getRequestValues().getBodyValue(); - Object body = clientAdapter.getRequestValues().getBodyValue(); - - assertThat(body).isInstanceOf(MultiValueMap.class); - MultiValueMap> map = (MultiValueMap>) body; - assertThat(map.size()).isEqualTo(1); - assertThat(map.getFirst("anotherPart")).isNotNull(); + assertThat(value).isInstanceOf(MultiValueMap.class); + MultiValueMap> map = (MultiValueMap>) value; + assertThat(map).containsOnlyKeys("anotherPart"); } + @SuppressWarnings("OptionalUsedAsFieldOrParameterType") private interface TestClient { @@ -98,4 +99,5 @@ private interface TestClient { void postOptionalMultipartFile(Optional file, @RequestPart String anotherPart); } + } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/support/WebClientHttpServiceProxyTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/support/WebClientHttpServiceProxyTests.java index a3b841dcaa2a..3bc8fb565d15 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/support/WebClientHttpServiceProxyTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/support/WebClientHttpServiceProxyTests.java @@ -190,7 +190,7 @@ private interface TestHttpService { @PostExchange(contentType = "application/x-www-form-urlencoded") void postForm(@RequestParam MultiValueMap params); - @PostExchange(contentType = MediaType.MULTIPART_FORM_DATA_VALUE) + @PostExchange void postMultipart(MultipartFile file, @RequestPart String anotherPart); }