From 8083eaae54233b84edeeaf253234503c2d7f7256 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Tue, 31 Oct 2017 08:54:19 -0400 Subject: [PATCH] syncBody better differentiates plain vs multipart forms FromHttpMessageWriter and MultipartHttpMessageWriter both support MultiValueMap except the former supports String values only. This presents an issue since either full generic type information must be provided, which is cumbersome on the client side, or if left out there is no good way to order the writers to make a proper decision. This commit: - refines the canWrite behavior of to not a accept MultiValueMap without proper generic information unless the MediaType is explicitly set providing a strong hint. - modifies MultipartHttpMessageWriter to be configured with a FormHttpMessageWriter so it can write both plan and multipart data with the ability to properly differentiate based on actual map values. Issue: SPR-16131 --- .../http/codec/FormHttpMessageWriter.java | 68 +++++++---- .../multipart/MultipartHttpMessageWriter.java | 110 +++++++++++++++--- .../support/DefaultClientCodecConfigurer.java | 7 +- .../codec/FormHttpMessageWriterTests.java | 9 +- .../MultipartHttpMessageWriterTests.java | 2 +- .../support/ClientCodecConfigurerTests.java | 11 +- .../function/MultipartIntegrationTests.java | 6 +- .../annotation/MultipartIntegrationTests.java | 13 +-- 8 files changed, 161 insertions(+), 65 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/http/codec/FormHttpMessageWriter.java b/spring-web/src/main/java/org/springframework/http/codec/FormHttpMessageWriter.java index ffee0fb8513..fa4ed289c31 100644 --- a/spring-web/src/main/java/org/springframework/http/codec/FormHttpMessageWriter.java +++ b/spring-web/src/main/java/org/springframework/http/codec/FormHttpMessageWriter.java @@ -27,7 +27,6 @@ import java.util.List; import java.util.Map; import org.reactivestreams.Publisher; -import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import org.springframework.core.ResolvableType; @@ -39,12 +38,24 @@ import org.springframework.util.Assert; import org.springframework.util.MultiValueMap; /** - * Implementation of an {@link HttpMessageWriter} to write HTML form data, i.e. - * response body with media type {@code "application/x-www-form-urlencoded"}. + * {@link HttpMessageWriter} for writing a {@code MultiValueMap} + * as HTML form data, i.e. {@code "application/x-www-form-urlencoded"}, to the + * body of a request. + * + *

Note that unless the media type is explicitly set to + * {@link MediaType#APPLICATION_FORM_URLENCODED}, the {@link #canWrite} method + * will need generic type information to confirm the target map has String values. + * This is because a MultiValueMap with non-String values can be used to write + * multipart requests. + * + *

To support both form data and multipart requests, consider using + * {@link org.springframework.http.codec.multipart.MultipartHttpMessageWriter} + * configured with this writer as the fallback for writing plain form data. * * @author Sebastien Deleuze * @author Rossen Stoyanchev * @since 5.0 + * @see org.springframework.http.codec.multipart.MultipartHttpMessageWriter */ public class FormHttpMessageWriter implements HttpMessageWriter> { @@ -53,6 +64,9 @@ public class FormHttpMessageWriter implements HttpMessageWriter MEDIA_TYPES = + Collections.singletonList(MediaType.APPLICATION_FORM_URLENCODED); + private Charset defaultCharset = DEFAULT_CHARSET; @@ -75,12 +89,27 @@ public class FormHttpMessageWriter implements HttpMessageWriter getWritableMediaTypes() { + return MEDIA_TYPES; + } + + @Override public boolean canWrite(ResolvableType elementType, @Nullable MediaType mediaType) { - return (MULTIVALUE_TYPE.isAssignableFrom(elementType) || - (elementType.hasUnresolvableGenerics() && - MultiValueMap.class.isAssignableFrom(elementType.resolve(Object.class)))) && - (mediaType == null || MediaType.APPLICATION_FORM_URLENCODED.isCompatibleWith(mediaType)); + Class rawClass = elementType.getRawClass(); + if (rawClass == null || !MultiValueMap.class.isAssignableFrom(rawClass)) { + return false; + } + if (MediaType.APPLICATION_FORM_URLENCODED.isCompatibleWith(mediaType)) { + // Optimistically, any MultiValueMap with or without generics + return true; + } + if (mediaType == null) { + // Only String-based MultiValueMap + return MULTIVALUE_TYPE.isAssignableFrom(elementType); + } + return false; } @Override @@ -96,11 +125,8 @@ public class FormHttpMessageWriter implements HttpMessageWriter generateForm(form, charset)) - .flatMap(value -> { + return Mono.from(inputStream).flatMap(form -> { + String value = serializeForm(form, charset); ByteBuffer byteBuffer = charset.encode(value); DataBuffer buffer = message.bufferFactory().wrap(byteBuffer); message.getHeaders().setContentLength(byteBuffer.remaining()); @@ -118,17 +144,20 @@ public class FormHttpMessageWriter implements HttpMessageWriter form, Charset charset) { + private String serializeForm(MultiValueMap form, Charset charset) { StringBuilder builder = new StringBuilder(); try { for (Iterator names = form.keySet().iterator(); names.hasNext();) { String name = names.next(); - for (Iterator values = form.get(name).iterator(); values.hasNext();) { - String value = values.next(); + for (Iterator values = form.get(name).iterator(); values.hasNext();) { + Object rawValue = values.next(); builder.append(URLEncoder.encode(name, charset.name())); - if (value != null) { + if (rawValue != null) { builder.append('='); - builder.append(URLEncoder.encode(value, charset.name())); + Assert.isInstanceOf(String.class, rawValue, + "FormHttpMessageWriter supports String values only. " + + "Use MultipartHttpMessageWriter for multipart requests."); + builder.append(URLEncoder.encode((String) rawValue, charset.name())); if (values.hasNext()) { builder.append('&'); } @@ -145,9 +174,4 @@ public class FormHttpMessageWriter implements HttpMessageWriter getWritableMediaTypes() { - return Collections.singletonList(MediaType.APPLICATION_FORM_URLENCODED); - } - } diff --git a/spring-web/src/main/java/org/springframework/http/codec/multipart/MultipartHttpMessageWriter.java b/spring-web/src/main/java/org/springframework/http/codec/multipart/MultipartHttpMessageWriter.java index ec73736ffb2..03008459a6e 100644 --- a/spring-web/src/main/java/org/springframework/http/codec/multipart/MultipartHttpMessageWriter.java +++ b/spring-web/src/main/java/org/springframework/http/codec/multipart/MultipartHttpMessageWriter.java @@ -18,6 +18,7 @@ package org.springframework.http.codec.multipart; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; @@ -44,6 +45,7 @@ import org.springframework.http.HttpHeaders; import org.springframework.http.MediaType; import org.springframework.http.ReactiveHttpOutputMessage; import org.springframework.http.codec.EncoderHttpMessageWriter; +import org.springframework.http.codec.FormHttpMessageWriter; import org.springframework.http.codec.HttpMessageWriter; import org.springframework.http.codec.ResourceHttpMessageWriter; import org.springframework.lang.Nullable; @@ -52,37 +54,82 @@ import org.springframework.util.MimeTypeUtils; import org.springframework.util.MultiValueMap; /** - * {@code HttpMessageWriter} for {@code "multipart/form-data"} requests. + * {@link HttpMessageWriter} for writing a {@code MultiValueMap} + * as multipart form data, i.e. {@code "multipart/form-data"}, to the body + * of a request. * - *

This writer delegates to other message writers to write the respective - * parts. By default basic writers are registered for {@code String}, and - * {@code Resources}. These can be overridden through the provided constructors. + *

The serialization of individual parts is delegated to other writers. + * By default only {@link String} and {@link Resource} parts are supported but + * you can configure others through a constructor argument. + * + *

This writer can be configured with a {@link FormHttpMessageWriter} to + * delegate to. It is the preferred way of supporting both form data and + * multipart data (as opposed to registering each writer separately) so that + * when the {@link MediaType} is not specified and generics are not present on + * the target element type, we can inspect the values in the actual map and + * decide whether to write plain form data (String values only) or otherwise. * * @author Sebastien Deleuze * @author Rossen Stoyanchev * @since 5.0 + * @see FormHttpMessageWriter */ public class MultipartHttpMessageWriter implements HttpMessageWriter> { public static final Charset DEFAULT_CHARSET = StandardCharsets.UTF_8; - private final DataBufferFactory bufferFactory = new DefaultDataBufferFactory(); - private final List> partWriters; + @Nullable + private final HttpMessageWriter> formWriter; + private Charset charset = DEFAULT_CHARSET; + private final List supportedMediaTypes; + private final DataBufferFactory bufferFactory = new DefaultDataBufferFactory(); + + + /** + * Constructor with a default list of part writers (String and Resource). + */ public MultipartHttpMessageWriter() { - this.partWriters = Arrays.asList( + this(Arrays.asList( new EncoderHttpMessageWriter<>(CharSequenceEncoder.textPlainOnly()), new ResourceHttpMessageWriter() - ); + )); } + /** + * Constructor with explicit list of writers for serializing parts. + */ public MultipartHttpMessageWriter(List> partWriters) { + this(partWriters, new FormHttpMessageWriter()); + } + + /** + * Constructor with explicit list of writers for serializing parts and a + * writer for plain form data to fall back when no media type is specified + * and the actual map consists of String values only. + * @param partWriters the writers for serializing parts + * @param formWriter the fallback writer for form data, {@code null} by default + */ + public MultipartHttpMessageWriter(List> partWriters, + @Nullable HttpMessageWriter> formWriter) { + this.partWriters = partWriters; + this.formWriter = formWriter; + this.supportedMediaTypes = initMediaTypes(formWriter); + } + + private static List initMediaTypes(@Nullable HttpMessageWriter formWriter) { + List result = new ArrayList<>(); + result.add(MediaType.MULTIPART_FORM_DATA); + if (formWriter != null) { + result.addAll(formWriter.getWritableMediaTypes()); + } + return Collections.unmodifiableList(result); } @@ -106,14 +153,15 @@ public class MultipartHttpMessageWriter implements HttpMessageWriter getWritableMediaTypes() { - return Collections.singletonList(MediaType.MULTIPART_FORM_DATA); + return this.supportedMediaTypes; } @Override public boolean canWrite(ResolvableType elementType, @Nullable MediaType mediaType) { Class rawClass = elementType.getRawClass(); - return (rawClass != null && MultiValueMap.class.isAssignableFrom(rawClass) && - (mediaType == null || MediaType.MULTIPART_FORM_DATA.isCompatibleWith(mediaType))); + return rawClass != null && MultiValueMap.class.isAssignableFrom(rawClass) && + (mediaType == null || + this.supportedMediaTypes.stream().anyMatch(m -> m.isCompatibleWith(mediaType))); } @Override @@ -121,19 +169,47 @@ public class MultipartHttpMessageWriter implements HttpMessageWriter hints) { + return Mono.from(inputStream).flatMap(map -> { + if (this.formWriter == null || isMultipart(map, mediaType)) { + return writeMultipart(map, outputMessage); + } + else { + @SuppressWarnings("unchecked") + MultiValueMap formData = (MultiValueMap) map; + return this.formWriter.write(Mono.just(formData), elementType, mediaType, outputMessage, hints); + } + + }); + } + + private boolean isMultipart(MultiValueMap map, @Nullable MediaType contentType) { + if (contentType != null) { + return MediaType.MULTIPART_FORM_DATA.includes(contentType); + } + for (String name : map.keySet()) { + for (Object value : map.get(name)) { + if (value != null && !(value instanceof String)) { + return true; + } + } + } + return false; + } + + private Mono writeMultipart(MultiValueMap map, ReactiveHttpOutputMessage outputMessage) { byte[] boundary = generateMultipartBoundary(); Map params = new HashMap<>(2); params.put("boundary", new String(boundary, StandardCharsets.US_ASCII)); params.put("charset", getCharset().name()); + outputMessage.getHeaders().setContentType(new MediaType(MediaType.MULTIPART_FORM_DATA, params)); - return Mono.from(inputStream).flatMap(map -> { - Flux body = Flux.fromIterable(map.entrySet()) - .concatMap(entry -> encodePartValues(boundary, entry.getKey(), entry.getValue())) - .concatWith(Mono.just(generateLastLine(boundary))); - return outputMessage.writeWith(body); - }); + Flux body = Flux.fromIterable(map.entrySet()) + .concatMap(entry -> encodePartValues(boundary, entry.getKey(), entry.getValue())) + .concatWith(Mono.just(generateLastLine(boundary))); + + return outputMessage.writeWith(body); } /** diff --git a/spring-web/src/main/java/org/springframework/http/codec/support/DefaultClientCodecConfigurer.java b/spring-web/src/main/java/org/springframework/http/codec/support/DefaultClientCodecConfigurer.java index 6abe9a2a2b4..15a265bb3b6 100644 --- a/spring-web/src/main/java/org/springframework/http/codec/support/DefaultClientCodecConfigurer.java +++ b/spring-web/src/main/java/org/springframework/http/codec/support/DefaultClientCodecConfigurer.java @@ -99,12 +99,11 @@ public class DefaultClientCodecConfigurer extends AbstractCodecConfigurer implem return Collections.emptyList(); } List> result = super.getTypedWriters(); - result.add(new FormHttpMessageWriter()); - result.add(getMultipartHttpMessageWriter()); + result.add(new MultipartHttpMessageWriter(getPartWriters(), new FormHttpMessageWriter())); return result; } - private MultipartHttpMessageWriter getMultipartHttpMessageWriter() { + private List> getPartWriters() { List> partWriters; if (this.multipartCodecs != null) { partWriters = this.multipartCodecs.getWriters(); @@ -122,7 +121,7 @@ public class DefaultClientCodecConfigurer extends AbstractCodecConfigurer implem } partWriters.addAll(super.getCatchAllWriters()); } - return new MultipartHttpMessageWriter(partWriters); + return partWriters; } } diff --git a/spring-web/src/test/java/org/springframework/http/codec/FormHttpMessageWriterTests.java b/spring-web/src/test/java/org/springframework/http/codec/FormHttpMessageWriterTests.java index 38b77a5aec2..0b024adc9d8 100644 --- a/spring-web/src/test/java/org/springframework/http/codec/FormHttpMessageWriterTests.java +++ b/spring-web/src/test/java/org/springframework/http/codec/FormHttpMessageWriterTests.java @@ -27,7 +27,9 @@ import org.springframework.mock.http.server.reactive.test.MockServerHttpResponse import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; -import static org.junit.Assert.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; /** * @author Sebastien Deleuze @@ -43,17 +45,18 @@ public class FormHttpMessageWriterTests { ResolvableType.forClassWithGenerics(MultiValueMap.class, String.class, String.class), MediaType.APPLICATION_FORM_URLENCODED)); + // No generic information assertTrue(this.writer.canWrite( ResolvableType.forInstance(new LinkedMultiValueMap()), MediaType.APPLICATION_FORM_URLENCODED)); assertFalse(this.writer.canWrite( ResolvableType.forClassWithGenerics(MultiValueMap.class, String.class, Object.class), - MediaType.APPLICATION_FORM_URLENCODED)); + null)); assertFalse(this.writer.canWrite( ResolvableType.forClassWithGenerics(MultiValueMap.class, Object.class, String.class), - MediaType.APPLICATION_FORM_URLENCODED)); + null)); assertFalse(this.writer.canWrite( ResolvableType.forClassWithGenerics(Map.class, String.class, String.class), diff --git a/spring-web/src/test/java/org/springframework/http/codec/multipart/MultipartHttpMessageWriterTests.java b/spring-web/src/test/java/org/springframework/http/codec/multipart/MultipartHttpMessageWriterTests.java index 54845da5d00..5a8bed83633 100644 --- a/spring-web/src/test/java/org/springframework/http/codec/multipart/MultipartHttpMessageWriterTests.java +++ b/spring-web/src/test/java/org/springframework/http/codec/multipart/MultipartHttpMessageWriterTests.java @@ -70,7 +70,7 @@ public class MultipartHttpMessageWriterTests { assertFalse(this.writer.canWrite( ResolvableType.forClassWithGenerics(Map.class, String.class, Object.class), MediaType.MULTIPART_FORM_DATA)); - assertFalse(this.writer.canWrite( + assertTrue(this.writer.canWrite( ResolvableType.forClassWithGenerics(MultiValueMap.class, String.class, Object.class), MediaType.APPLICATION_FORM_URLENCODED)); } diff --git a/spring-web/src/test/java/org/springframework/http/codec/support/ClientCodecConfigurerTests.java b/spring-web/src/test/java/org/springframework/http/codec/support/ClientCodecConfigurerTests.java index 98393d309c2..ff59c9b79d7 100644 --- a/spring-web/src/test/java/org/springframework/http/codec/support/ClientCodecConfigurerTests.java +++ b/spring-web/src/test/java/org/springframework/http/codec/support/ClientCodecConfigurerTests.java @@ -42,7 +42,6 @@ import org.springframework.http.MediaType; import org.springframework.http.codec.ClientCodecConfigurer; import org.springframework.http.codec.DecoderHttpMessageReader; import org.springframework.http.codec.EncoderHttpMessageWriter; -import org.springframework.http.codec.FormHttpMessageWriter; import org.springframework.http.codec.HttpMessageReader; import org.springframework.http.codec.HttpMessageWriter; import org.springframework.http.codec.ResourceHttpMessageWriter; @@ -56,8 +55,11 @@ import org.springframework.http.codec.xml.Jaxb2XmlDecoder; import org.springframework.http.codec.xml.Jaxb2XmlEncoder; import org.springframework.util.MimeTypeUtils; -import static org.junit.Assert.*; -import static org.springframework.core.ResolvableType.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; +import static org.springframework.core.ResolvableType.forClass; /** * Unit tests for {@link ClientCodecConfigurer}. @@ -90,13 +92,12 @@ public class ClientCodecConfigurerTests { @Test public void defaultWriters() { List> writers = this.configurer.getWriters(); - assertEquals(11, writers.size()); + assertEquals(10, writers.size()); assertEquals(ByteArrayEncoder.class, getNextEncoder(writers).getClass()); assertEquals(ByteBufferEncoder.class, getNextEncoder(writers).getClass()); assertEquals(DataBufferEncoder.class, getNextEncoder(writers).getClass()); assertEquals(ResourceHttpMessageWriter.class, writers.get(index.getAndIncrement()).getClass()); assertStringEncoder(getNextEncoder(writers), true); - assertEquals(FormHttpMessageWriter.class, writers.get(this.index.getAndIncrement()).getClass()); assertEquals(MultipartHttpMessageWriter.class, writers.get(this.index.getAndIncrement()).getClass()); assertEquals(Jackson2JsonEncoder.class, getNextEncoder(writers).getClass()); assertEquals(Jackson2SmileEncoder.class, getNextEncoder(writers).getClass()); diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/function/MultipartIntegrationTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/function/MultipartIntegrationTests.java index ffefb840360..1ed3236276f 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/function/MultipartIntegrationTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/function/MultipartIntegrationTests.java @@ -52,8 +52,7 @@ public class MultipartIntegrationTests extends AbstractRouterFunctionIntegration Mono result = webClient .post() .uri("http://localhost:" + this.port + "/multipartData") - .contentType(MediaType.MULTIPART_FORM_DATA) - .body(BodyInserters.fromMultipartData(generateBody())) + .syncBody(generateBody()) .exchange(); StepVerifier @@ -67,8 +66,7 @@ public class MultipartIntegrationTests extends AbstractRouterFunctionIntegration Mono result = webClient .post() .uri("http://localhost:" + this.port + "/parts") - .contentType(MediaType.MULTIPART_FORM_DATA) - .body(BodyInserters.fromMultipartData(generateBody())) + .syncBody(generateBody()) .exchange(); StepVerifier diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/MultipartIntegrationTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/MultipartIntegrationTests.java index f3dae0600d5..7f3155050c5 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/MultipartIntegrationTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/MultipartIntegrationTests.java @@ -50,7 +50,6 @@ import org.springframework.web.bind.annotation.RequestPart; import org.springframework.web.bind.annotation.RestController; import org.springframework.web.reactive.DispatcherHandler; import org.springframework.web.reactive.config.EnableWebFlux; -import org.springframework.web.reactive.function.BodyInserters; import org.springframework.web.reactive.function.client.ClientResponse; import org.springframework.web.reactive.function.client.WebClient; import org.springframework.web.server.adapter.WebHttpHandlerBuilder; @@ -83,8 +82,7 @@ public class MultipartIntegrationTests extends AbstractHttpHandlerIntegrationTes Mono result = webClient .post() .uri("/requestPart") - .contentType(MediaType.MULTIPART_FORM_DATA) - .body(BodyInserters.fromMultipartData(generateBody())) + .syncBody(generateBody()) .exchange(); StepVerifier @@ -98,8 +96,7 @@ public class MultipartIntegrationTests extends AbstractHttpHandlerIntegrationTes Mono result = webClient .post() .uri("/requestBodyMap") - .contentType(MediaType.MULTIPART_FORM_DATA) - .body(BodyInserters.fromMultipartData(generateBody())) + .syncBody(generateBody()) .retrieve() .bodyToMono(String.class); @@ -114,8 +111,7 @@ public class MultipartIntegrationTests extends AbstractHttpHandlerIntegrationTes Mono result = webClient .post() .uri("/requestBodyFlux") - .contentType(MediaType.MULTIPART_FORM_DATA) - .body(BodyInserters.fromMultipartData(generateBody())) + .syncBody(generateBody()) .retrieve() .bodyToMono(String.class); @@ -130,8 +126,7 @@ public class MultipartIntegrationTests extends AbstractHttpHandlerIntegrationTes Mono result = webClient .post() .uri("/modelAttribute") - .contentType(MediaType.MULTIPART_FORM_DATA) - .body(BodyInserters.fromMultipartData(generateBody())) + .syncBody(generateBody()) .retrieve() .bodyToMono(String.class);