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

spring-web/src/main/java/org/springframework/http/codec/FormHttpMessageWriter.java

@@ -27,7 +27,6 @@
 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.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<String, String>}
+ * as HTML form data, i.e. {@code "application/x-www-form-urlencoded"}, to the
+ * body of a request.
+ *
+ * <p>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.
+ *
+ * <p>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<MultiValueMap<String, String>> {
 
@@ -53,6 +64,9 @@ public class FormHttpMessageWriter implements HttpMessageWriter<MultiValueMap<St
 	private static final ResolvableType MULTIVALUE_TYPE =
 			ResolvableType.forClassWithGenerics(MultiValueMap.class, String.class, String.class);
 
+	private static final List<MediaType> MEDIA_TYPES =
+			Collections.singletonList(MediaType.APPLICATION_FORM_URLENCODED);
+
 
 	private Charset defaultCharset = DEFAULT_CHARSET;
 
@@ -75,12 +89,27 @@ public Charset getDefaultCharset() {
 	}
 
 
+	@Override
+	public List<MediaType> 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 Mono<Void> write(Publisher<? extends MultiValueMap<String, String>> input
 
 		Charset charset = getMediaTypeCharset(contentType);
 
-		return Flux
-				.from(inputStream)
-				.single()
-				.map(form -> 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 @@ private Charset getMediaTypeCharset(@Nullable MediaType mediaType) {
 		}
 	}
 
-	private String generateForm(MultiValueMap<String, String> form, Charset charset) {
+	private String serializeForm(MultiValueMap<String, String> form, Charset charset) {
 		StringBuilder builder = new StringBuilder();
 		try {
 			for (Iterator<String> names = form.keySet().iterator(); names.hasNext();) {
 				String name = names.next();
-				for (Iterator<String> 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 @@ private String generateForm(MultiValueMap<String, String> form, Charset charset)
 		return builder.toString();
 	}
 
-	@Override
-	public List<MediaType> getWritableMediaTypes() {
-		return Collections.singletonList(MediaType.APPLICATION_FORM_URLENCODED);
-	}
-
 }

spring-web/src/main/java/org/springframework/http/codec/multipart/MultipartHttpMessageWriter.java

@@ -18,6 +18,7 @@
 
 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.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.MultiValueMap;
 
 /**
- * {@code HttpMessageWriter} for {@code "multipart/form-data"} requests.
+ * {@link HttpMessageWriter} for writing a {@code MultiValueMap<String, ?>}
+ * as multipart form data, i.e. {@code "multipart/form-data"}, to the body
+ * of a request.
  *
- * <p>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.
+ * <p>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.
+ *
+ * <p>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<MultiValueMap<String, ?>> {
 
 	public static final Charset DEFAULT_CHARSET = StandardCharsets.UTF_8;
 
 
-	private final DataBufferFactory bufferFactory = new DefaultDataBufferFactory();
-
 	private final List<HttpMessageWriter<?>> partWriters;
 
+	@Nullable
+	private final HttpMessageWriter<MultiValueMap<String, String>> formWriter;
+
 	private Charset charset = DEFAULT_CHARSET;
 
+	private final List<MediaType> 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<HttpMessageWriter<?>> 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<HttpMessageWriter<?>> partWriters,
+			@Nullable  HttpMessageWriter<MultiValueMap<String, String>> formWriter) {
+
 		this.partWriters = partWriters;
+		this.formWriter = formWriter;
+		this.supportedMediaTypes = initMediaTypes(formWriter);
+	}
+
+	private static List<MediaType> initMediaTypes(@Nullable HttpMessageWriter<?> formWriter) {
+		List<MediaType> result = new ArrayList<>();
+		result.add(MediaType.MULTIPART_FORM_DATA);
+		if (formWriter != null) {
+			result.addAll(formWriter.getWritableMediaTypes());
+		}
+		return Collections.unmodifiableList(result);
 	}
 
 
@@ -106,34 +153,63 @@ public Charset getCharset() {
 
 	@Override
 	public List<MediaType> 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
 	public Mono<Void> write(Publisher<? extends MultiValueMap<String, ?>> inputStream,
 			ResolvableType elementType, @Nullable MediaType mediaType, ReactiveHttpOutputMessage outputMessage,
 			Map<String, Object> hints) {
 
+		return Mono.from(inputStream).flatMap(map -> {
+			if (this.formWriter == null || isMultipart(map, mediaType)) {
+				return writeMultipart(map, outputMessage);
+			}
+			else {
+				@SuppressWarnings("unchecked")
+				MultiValueMap<String, String> formData = (MultiValueMap<String, String>) map;
+				return this.formWriter.write(Mono.just(formData), elementType, mediaType, outputMessage, hints);
+			}
+
+		});
+	}
+
+	private boolean isMultipart(MultiValueMap<String, ?> 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<Void> writeMultipart(MultiValueMap<String, ?> map, ReactiveHttpOutputMessage outputMessage) {
 		byte[] boundary = generateMultipartBoundary();
 
 		Map<String, String> 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<DataBuffer> body = Flux.fromIterable(map.entrySet())
-					.concatMap(entry -> encodePartValues(boundary, entry.getKey(), entry.getValue()))
-					.concatWith(Mono.just(generateLastLine(boundary)));
-			return outputMessage.writeWith(body);
-		});
+		Flux<DataBuffer> body = Flux.fromIterable(map.entrySet())
+				.concatMap(entry -> encodePartValues(boundary, entry.getKey(), entry.getValue()))
+				.concatWith(Mono.just(generateLastLine(boundary)));
+
+		return outputMessage.writeWith(body);
 	}
 
 	/**

spring-web/src/main/java/org/springframework/http/codec/support/DefaultClientCodecConfigurer.java

@@ -99,12 +99,11 @@ List<HttpMessageWriter<?>> getTypedWriters() {
 				return Collections.emptyList();
 			}
 			List<HttpMessageWriter<?>> result = super.getTypedWriters();
-			result.add(new FormHttpMessageWriter());
-			result.add(getMultipartHttpMessageWriter());
+			result.add(new MultipartHttpMessageWriter(getPartWriters(), new FormHttpMessageWriter()));
 			return result;
 		}
 
-		private MultipartHttpMessageWriter getMultipartHttpMessageWriter() {
+		private List<HttpMessageWriter<?>> getPartWriters() {
 			List<HttpMessageWriter<?>> partWriters;
 			if (this.multipartCodecs != null) {
 				partWriters = this.multipartCodecs.getWriters();
@@ -122,7 +121,7 @@ private MultipartHttpMessageWriter getMultipartHttpMessageWriter() {
 				}
 				partWriters.addAll(super.getCatchAllWriters());
 			}
-			return new MultipartHttpMessageWriter(partWriters);
+			return partWriters;
 		}
 	}
 

spring-web/src/test/java/org/springframework/http/codec/FormHttpMessageWriterTests.java

@@ -27,7 +27,9 @@
 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 void canWrite() {
 				ResolvableType.forClassWithGenerics(MultiValueMap.class, String.class, String.class),
 				MediaType.APPLICATION_FORM_URLENCODED));
 
+		// No generic information
 		assertTrue(this.writer.canWrite(
 				ResolvableType.forInstance(new LinkedMultiValueMap<String, String>()),
 				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),

spring-web/src/test/java/org/springframework/http/codec/multipart/MultipartHttpMessageWriterTests.java

@@ -70,7 +70,7 @@ public void canWrite() {
 		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));
 	}