Skip to content

Commit

Permalink
Revert "Do not read Map in FormHttpMessageConverter"
Browse files Browse the repository at this point in the history 
This reverts commit 726ac91 and
80faa94.

See gh-32826
  • Loading branch information
@poutsma
poutsma committed Jun 6, 2024
1 parent c127421 commit 859b97c
Show file tree
Hide file tree
Showing 5 changed files with 47 additions and 174 deletions.
3 changes: 1 addition & 2 deletions ...-test/src/main/java/org/springframework/test/web/client/match/ContentRequestMatchers.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,13 +174,12 @@ public RequestMatcher formDataContains(Map<String, String> expected) {
return formData(multiValueMap, false);
}

@SuppressWarnings("unchecked")
private RequestMatcher formData(MultiValueMap<String, String> expectedMap, boolean containsExactly) {
return request -> {
MockClientHttpRequest mockRequest = (MockClientHttpRequest) request;
MockHttpInputMessage message = new MockHttpInputMessage(mockRequest.getBodyAsBytes());
message.getHeaders().putAll(mockRequest.getHeaders());
MultiValueMap<String, String> actualMap = (MultiValueMap<String, String>) new FormHttpMessageConverter().read(null, message);
MultiValueMap<String, String> actualMap = new FormHttpMessageConverter().read(null, message);
if (containsExactly) {
assertEquals("Form data", expectedMap, actualMap);
}
Expand Down
2 changes: 1 addition & 1 deletion ...a/org/springframework/test/web/servlet/request/AbstractMockHttpServletRequestBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -902,7 +902,7 @@ public HttpHeaders getHeaders() {
};

try {
return (MultiValueMap<String, String>) new FormHttpMessageConverter().read(null, message);
return new FormHttpMessageConverter().read(null, message);
}
catch (IOException ex) {
throw new IllegalStateException("Failed to parse form data in request body", ex);
Expand Down
113 changes: 39 additions & 74 deletions spring-web/src/main/java/org/springframework/http/converter/FormHttpMessageConverter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,24 +50,12 @@
* Implementation of {@link HttpMessageConverter} to read and write 'normal' HTML
* forms and also to write (but not read) multipart data (e.g. file uploads).
*
* <p>
* The following table shows an overview of the supported media and class types.
* <table border="1">
* <tr><th>Media type</th><th>Read</th><th>Write</th></tr>
* <tr>
* <td>{@code "application/x-www-form-urlencoded"}</td>
* <td>{@link MultiValueMap MultiValueMap&lt;String, String&gt;}</td>
* <td>{@link Map Map&lt;String, String&gt;}<br>
* {@link MultiValueMap MultiValueMap&lt;String, String&gt;}</td>
* </tr>
* <tr>
* <td>{@code "multipart/form-data"}<br>
* {@code "multipart/mixed"}</td>
* <td>Unsupported</td>
* <td>{@link Map Map&lt;String, Object&gt;}<br>
* {@link MultiValueMap MultiValueMap&lt;String, Object&gt;}</td>
* </tr>
* </table>
* <p>In other words, this converter can read and write the
* {@code "application/x-www-form-urlencoded"} media type as
* {@link MultiValueMap MultiValueMap&lt;String, String&gt;}, and it can also
* write (but not read) the {@code "multipart/form-data"} and
* {@code "multipart/mixed"} media types as
* {@link MultiValueMap MultiValueMap&lt;String, Object&gt;}.
*
* <h3>Multipart Data</h3>
*
Expand Down Expand Up @@ -167,10 +155,6 @@
* <p>Some methods in this class were inspired by
* {@code org.apache.commons.httpclient.methods.multipart.MultipartRequestEntity}.
*
* <p>As of 6.2, the {@code FormHttpMessageConverter} is parameterized over
* {@code Map<String, ?>} in order to support writing single-value maps.
* Before 6.2, this class was parameterized over {@code MultiValueMap<String, ?>}.
*
* @author Arjen Poutsma
* @author Rossen Stoyanchev
* @author Juergen Hoeller
Expand All @@ -179,7 +163,7 @@
* @see org.springframework.http.converter.support.AllEncompassingFormHttpMessageConverter
* @see org.springframework.util.MultiValueMap
*/
public class FormHttpMessageConverter implements HttpMessageConverter<Map<String, ?>> {
public class FormHttpMessageConverter implements HttpMessageConverter<MultiValueMap<String, ?>> {

/** The default charset used by the converter. */
public static final Charset DEFAULT_CHARSET = StandardCharsets.UTF_8;
Expand Down Expand Up @@ -342,7 +326,7 @@ public boolean canRead(Class<?> clazz, @Nullable MediaType mediaType) {

@Override
public boolean canWrite(Class<?> clazz, @Nullable MediaType mediaType) {
if (!Map.class.isAssignableFrom(clazz)) {
if (!MultiValueMap.class.isAssignableFrom(clazz)) {
return false;
}
if (mediaType == null || MediaType.ALL.equals(mediaType)) {
Expand All @@ -357,7 +341,7 @@ public boolean canWrite(Class<?> clazz, @Nullable MediaType mediaType) {
}

@Override
public Map<String, ?> read(@Nullable Class<? extends Map<String, ?>> clazz,
public MultiValueMap<String, String> read(@Nullable Class<? extends MultiValueMap<String, ?>> clazz,
HttpInputMessage inputMessage) throws IOException, HttpMessageNotReadableException {

MediaType contentType = inputMessage.getHeaders().getContentType();
Expand All @@ -383,38 +367,33 @@ public boolean canWrite(Class<?> clazz, @Nullable MediaType mediaType) {

@Override
@SuppressWarnings("unchecked")
public void write(Map<String, ?> map, @Nullable MediaType contentType, HttpOutputMessage outputMessage)
public void write(MultiValueMap<String, ?> map, @Nullable MediaType contentType, HttpOutputMessage outputMessage)
throws IOException, HttpMessageNotWritableException {

if (isMultipart(map, contentType)) {
writeMultipart((Map<String, Object>) map, contentType, outputMessage);
writeMultipart((MultiValueMap<String, Object>) map, contentType, outputMessage);
}
else {
writeForm((Map<String, Object>) map, contentType, outputMessage);
writeForm((MultiValueMap<String, Object>) map, contentType, outputMessage);
}
}


private boolean isMultipart(Map<String, ?> map, @Nullable MediaType contentType) {
private boolean isMultipart(MultiValueMap<String, ?> map, @Nullable MediaType contentType) {
if (contentType != null) {
return contentType.getType().equalsIgnoreCase("multipart");
}
for (Object value : map.values()) {
if (value instanceof List<?> values) {
for (Object v : values) {
if (v != null && !(v instanceof String)) {
return true;
}
for (List<?> values : map.values()) {
for (Object value : values) {
if (value != null && !(value instanceof String)) {
return true;
}
}
else if (value != null && !(value instanceof String)) {
return true;
}
}
return false;
}

private void writeForm(Map<String, Object> formData, @Nullable MediaType mediaType,
private void writeForm(MultiValueMap<String, Object> formData, @Nullable MediaType mediaType,
HttpOutputMessage outputMessage) throws IOException {

mediaType = getFormContentType(mediaType);
Expand Down Expand Up @@ -462,36 +441,30 @@ protected MediaType getFormContentType(@Nullable MediaType contentType) {
return contentType;
}

protected String serializeForm(Map<String, Object> formData, Charset charset) {
protected String serializeForm(MultiValueMap<String, Object> formData, Charset charset) {
StringBuilder builder = new StringBuilder();
formData.forEach((name, value) -> {
if (value instanceof List<?> values) {
formData.forEach((name, values) -> {
if (name == null) {
Assert.isTrue(CollectionUtils.isEmpty(values), () -> "Null name in form data: " + formData);
return;
}
values.forEach(v -> appendFormValue(builder, name, v, charset));
}
else {
appendFormValue(builder, name, value, charset);
}
values.forEach(value -> {
if (builder.length() != 0) {
builder.append('&');
}
builder.append(URLEncoder.encode(name, charset));
if (value != null) {
builder.append('=');
builder.append(URLEncoder.encode(String.valueOf(value), charset));
}
});
});
return builder.toString();
}

private static void appendFormValue(StringBuilder builder, String name, @Nullable Object value, Charset charset) {
if (!builder.isEmpty()) {
builder.append('&');
}
builder.append(URLEncoder.encode(name, charset));
if (value != null) {
builder.append('=');
builder.append(URLEncoder.encode(String.valueOf(value), charset));
}
return builder.toString();
}

private void writeMultipart(
Map<String, Object> parts, @Nullable MediaType contentType, HttpOutputMessage outputMessage)
MultiValueMap<String, Object> parts, @Nullable MediaType contentType, HttpOutputMessage outputMessage)
throws IOException {

// If the supplied content type is null, fall back to multipart/form-data.
Expand Down Expand Up @@ -538,24 +511,16 @@ private boolean isFilenameCharsetSet() {
return (this.multipartCharset != null);
}

private void writeParts(OutputStream os, Map<String, Object> parts, byte[] boundary) throws IOException {
for (Map.Entry<String, Object> entry : parts.entrySet()) {
private void writeParts(OutputStream os, MultiValueMap<String, Object> parts, byte[] boundary) throws IOException {
for (Map.Entry<String, List<Object>> entry : parts.entrySet()) {
String name = entry.getKey();
Object value = entry.getValue();
if (value instanceof List<?> values) {
for (Object part : values) {
if (part != null) {
writeBoundary(os, boundary);
writePart(name, getHttpEntity(part), os);
writeNewLine(os);
}
for (Object part : entry.getValue()) {
if (part != null) {
writeBoundary(os, boundary);
writePart(name, getHttpEntity(part), os);
writeNewLine(os);
}
}
else if (value != null) {
writeBoundary(os, boundary);
writePart(name, getHttpEntity(value), os);
writeNewLine(os);
}
}
}

Expand Down
5 changes: 2 additions & 3 deletions spring-web/src/main/java/org/springframework/web/filter/FormContentFilter.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2024 the original author or authors.
* Copyright 2002-2021 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.
Expand Down Expand Up @@ -95,7 +95,6 @@ protected void doFilterInternal(
}

@Nullable
@SuppressWarnings("unchecked")
private MultiValueMap<String, String> parseIfNecessary(HttpServletRequest request) throws IOException {
if (!shouldParse(request)) {
return null;
Expand All @@ -107,7 +106,7 @@ public InputStream getBody() throws IOException {
return request.getInputStream();
}
};
return (MultiValueMap<String, String>) this.formConverter.read(null, inputMessage);
return this.formConverter.read(null, inputMessage);
}

private boolean shouldParse(HttpServletRequest request) {
Expand Down
98 changes: 4 additions & 94 deletions ...g-web/src/test/java/org/springframework/http/converter/FormHttpMessageConverterTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,13 +118,12 @@ void addSupportedMediaTypes() {
}

@Test
@SuppressWarnings("unchecked")
void readForm() throws Exception {
String body = "name+1=value+1&name+2=value+2%2B1&name+2=value+2%2B2&name+3";
MockHttpInputMessage inputMessage = new MockHttpInputMessage(body.getBytes(StandardCharsets.ISO_8859_1));
inputMessage.getHeaders().setContentType(
new MediaType("application", "x-www-form-urlencoded", StandardCharsets.ISO_8859_1));
MultiValueMap<String, String> result = (MultiValueMap<String, String>) this.converter.read(null, inputMessage);
MultiValueMap<String, String> result = this.converter.read(null, inputMessage);

assertThat(result).as("Invalid result").hasSize(3);
assertThat(result.getFirst("name 1")).as("Invalid result").isEqualTo("value 1");
Expand Down Expand Up @@ -152,24 +151,7 @@ void writeForm() throws IOException {
}

@Test
void writeFormSingleValue() throws IOException {
Map<String, String> body = new LinkedHashMap<>();
body.put("name 1", "value 1");
body.put("name 2", "value 2");
body.put("name 3", null);
MockHttpOutputMessage outputMessage = new MockHttpOutputMessage();
this.converter.write(body, APPLICATION_FORM_URLENCODED, outputMessage);

assertThat(outputMessage.getBodyAsString(UTF_8))
.as("Invalid result").isEqualTo("name+1=value+1&name+2=value+2&name+3");
assertThat(outputMessage.getHeaders().getContentType())
.as("Invalid content-type").isEqualTo(APPLICATION_FORM_URLENCODED);
assertThat(outputMessage.getHeaders().getContentLength())
.as("Invalid content-length").isEqualTo(outputMessage.getBodyAsBytes().length);
}

@Test
void writeMultipartMultiValue() throws Exception {
void writeMultipart() throws Exception {

MultiValueMap<String, Object> parts = new LinkedMultiValueMap<>();
parts.add("name 1", "value 1");
Expand Down Expand Up @@ -246,78 +228,6 @@ public String getFilename() {
assertThat(item.getContentType()).isEqualTo("application/json");
}

@Test
void writeMultipartSingleValue() throws Exception {

Map<String, Object> parts = new LinkedHashMap<>();
parts.put("name 1", "value 1");
parts.put("name 2", "value 2");
parts.put("name 3", null);

Resource logo = new ClassPathResource("/org/springframework/http/converter/logo.jpg");
parts.put("logo", logo);

// SPR-12108
Resource utf8 = new ClassPathResource("/org/springframework/http/converter/logo.jpg") {
@Override
public String getFilename() {
return "Hall\u00F6le.jpg";
}
};
parts.put("utf8", utf8);

MyBean myBean = new MyBean();
myBean.setString("foo");
HttpHeaders entityHeaders = new HttpHeaders();
entityHeaders.setContentType(APPLICATION_JSON);
HttpEntity<MyBean> entity = new HttpEntity<>(myBean, entityHeaders);
parts.put("json", entity);

Map<String, String> parameters = new LinkedHashMap<>(2);
parameters.put("charset", UTF_8.name());
parameters.put("foo", "bar");

MockHttpOutputMessage outputMessage = new MockHttpOutputMessage();
this.converter.write(parts, new MediaType("multipart", "form-data", parameters), outputMessage);

final MediaType contentType = outputMessage.getHeaders().getContentType();
assertThat(contentType.getParameters()).containsKeys("charset", "boundary", "foo"); // gh-21568, gh-25839

// see if Commons FileUpload can read what we wrote
FileUpload fileUpload = new FileUpload();
fileUpload.setFileItemFactory(new DiskFileItemFactory());
RequestContext requestContext = new MockHttpOutputMessageRequestContext(outputMessage);
List<FileItem> items = fileUpload.parseRequest(requestContext);
assertThat(items).hasSize(5);
FileItem item = items.get(0);
assertThat(item.isFormField()).isTrue();
assertThat(item.getFieldName()).isEqualTo("name 1");
assertThat(item.getString()).isEqualTo("value 1");

item = items.get(1);
assertThat(item.isFormField()).isTrue();
assertThat(item.getFieldName()).isEqualTo("name 2");
assertThat(item.getString()).isEqualTo("value 2");

item = items.get(2);
assertThat(item.isFormField()).isFalse();
assertThat(item.getFieldName()).isEqualTo("logo");
assertThat(item.getName()).isEqualTo("logo.jpg");
assertThat(item.getContentType()).isEqualTo("image/jpeg");
assertThat(item.getSize()).isEqualTo(logo.getFile().length());

item = items.get(3);
assertThat(item.isFormField()).isFalse();
assertThat(item.getFieldName()).isEqualTo("utf8");
assertThat(item.getName()).isEqualTo("Hall\u00F6le.jpg");
assertThat(item.getContentType()).isEqualTo("image/jpeg");
assertThat(item.getSize()).isEqualTo(logo.getFile().length());

item = items.get(4);
assertThat(item.getFieldName()).isEqualTo("json");
assertThat(item.getContentType()).isEqualTo("application/json");
}

@Test
void writeMultipartWithSourceHttpMessageConverter() throws Exception {

Expand Down Expand Up @@ -491,8 +401,8 @@ private void assertCannotRead(Class<?> clazz, MediaType mediaType) {
}

private void assertCanWrite(MediaType mediaType) {
assertThat(this.converter.canWrite(MultiValueMap.class, mediaType)).as("MultiValueMap : " + mediaType).isTrue();
assertThat(this.converter.canWrite(Map.class, mediaType)).as("Map : " + mediaType).isTrue();
Class<?> clazz = MultiValueMap.class;
assertThat(this.converter.canWrite(clazz, mediaType)).as(clazz.getSimpleName() + " : " + mediaType).isTrue();
}

private void assertCannotWrite(MediaType mediaType) {
Expand Down

0 comments on commit 859b97c

Please sign in to comment.