From c38542739734c15e84a28ecc5f575127f25d310a Mon Sep 17 00:00:00 2001 From: Sebastien Deleuze Date: Tue, 23 Feb 2016 10:27:39 +0100 Subject: [PATCH] Allow to specify AbstractHttpMessageConverter default charset Before this commit, specifying the charset to use with produces or consumes @RequestMapping attributes resulted in default charset loss. That was really annoying for JSON for example, where using UTF-8 charset is mandatory in a lot of use cases. This commit adds a defaultCharset property to AbstractHttpMessageConverter in order to avoid losing the default charset when specifying the charset with these @RequestMapping attributes. It changes slightly the default behavior (that's why we have waited 4.3), but it is much more error prone, and will match with most user's expectations since the charset loss was accidental in most use cases (users usually just want to limit the media type supported by a specific handler method). Issue: SPR-13631 --- .../org/springframework/util/MimeType.java | 26 ++++++++++++++++--- .../samples/standalone/AsyncTests.java | 14 +++++----- .../resultmatchers/ContentAssertionTests.java | 10 +++---- .../resultmatchers/XpathAssertionTests.java | 2 +- .../org/springframework/http/MediaType.java | 11 ++++++++ .../AbstractHttpMessageConverter.java | 24 ++++++++++++++++- .../ObjectToStringHttpMessageConverter.java | 2 +- .../converter/StringHttpMessageConverter.java | 7 ++--- .../AbstractJackson2HttpMessageConverter.java | 3 +++ .../json/GsonHttpMessageConverter.java | 5 ++-- .../MappingJackson2HttpMessageConverter.java | 5 ++-- ...appingJackson2XmlHttpMessageConverter.java | 8 +++--- .../ServletAnnotationControllerTests.java | 4 +-- ...questResponseBodyMethodProcessorTests.java | 23 ++++++++++++++++ ...nnotationControllerHandlerMethodTests.java | 12 ++++----- 15 files changed, 114 insertions(+), 42 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/util/MimeType.java b/spring-core/src/main/java/org/springframework/util/MimeType.java index 0f770998f2..2d7480eee2 100644 --- a/spring-core/src/main/java/org/springframework/util/MimeType.java +++ b/spring-core/src/main/java/org/springframework/util/MimeType.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2015 the original author or authors. + * Copyright 2002-2016 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. @@ -22,6 +22,7 @@ import java.util.BitSet; import java.util.Collections; import java.util.Comparator; import java.util.Iterator; +import java.util.LinkedHashMap; import java.util.List; import java.util.Locale; import java.util.Map; @@ -126,11 +127,28 @@ public class MimeType implements Comparable, Serializable { * Create a new {@code MimeType} for the given type, subtype, and character set. * @param type the primary type * @param subtype the subtype - * @param charSet the character set + * @param charset the character set * @throws IllegalArgumentException if any of the parameters contains illegal characters */ - public MimeType(String type, String subtype, Charset charSet) { - this(type, subtype, Collections.singletonMap(PARAM_CHARSET, charSet.name())); + public MimeType(String type, String subtype, Charset charset) { + this(type, subtype, Collections.singletonMap(PARAM_CHARSET, charset.name())); + } + + /** + * Copy-constructor that copies the type, subtype, parameters of the given {@code MimeType}, + * and allows to set the specified character set. + * @param other the other media type + * @param charset the character set + * @throws IllegalArgumentException if any of the parameters contains illegal characters + */ + public MimeType(MimeType other, Charset charset) { + this(other.getType(), other.getSubtype(), addCharsetParameter(charset, other.getParameters())); + } + + private static Map addCharsetParameter(Charset charset, Map parameters) { + Map map = new LinkedHashMap(parameters); + map.put(PARAM_CHARSET, charset.name()); + return map; } /** diff --git a/spring-test/src/test/java/org/springframework/test/web/servlet/samples/standalone/AsyncTests.java b/spring-test/src/test/java/org/springframework/test/web/servlet/samples/standalone/AsyncTests.java index 4803876321..ee8f505e51 100644 --- a/spring-test/src/test/java/org/springframework/test/web/servlet/samples/standalone/AsyncTests.java +++ b/spring-test/src/test/java/org/springframework/test/web/servlet/samples/standalone/AsyncTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2015 the original author or authors. + * Copyright 2002-2016 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. @@ -67,7 +67,7 @@ public class AsyncTests { this.mockMvc.perform(asyncDispatch(mvcResult)) .andExpect(status().isOk()) - .andExpect(content().contentType(MediaType.APPLICATION_JSON)) + .andExpect(content().contentType(MediaType.APPLICATION_JSON_UTF8_VALUE)) .andExpect(content().string("{\"name\":\"Joe\",\"someDouble\":0.0,\"someBoolean\":false}")); } @@ -81,7 +81,7 @@ public class AsyncTests { this.mockMvc.perform(asyncDispatch(mvcResult)) .andExpect(status().isOk()) - .andExpect(content().contentType(MediaType.APPLICATION_JSON)) + .andExpect(content().contentType(MediaType.APPLICATION_JSON_UTF8_VALUE)) .andExpect(content().string("{\"name\":\"Joe\",\"someDouble\":0.0,\"someBoolean\":false}")); } @@ -94,7 +94,7 @@ public class AsyncTests { this.mockMvc.perform(asyncDispatch(mvcResult)) .andExpect(status().isOk()) - .andExpect(content().contentType(MediaType.APPLICATION_JSON)) + .andExpect(content().contentType(MediaType.APPLICATION_JSON_UTF8_VALUE)) .andExpect(content().string("{\"name\":\"Joe\",\"someDouble\":0.0,\"someBoolean\":false}")); } @@ -122,7 +122,7 @@ public class AsyncTests { this.mockMvc.perform(asyncDispatch(mvcResult)) .andExpect(status().isOk()) - .andExpect(content().contentType(MediaType.APPLICATION_JSON)) + .andExpect(content().contentType(MediaType.APPLICATION_JSON_UTF8_VALUE)) .andExpect(content().string("{\"name\":\"Joe\",\"someDouble\":0.0,\"someBoolean\":false}")); } @@ -137,7 +137,7 @@ public class AsyncTests { this.mockMvc.perform(asyncDispatch(mvcResult)) .andExpect(status().isOk()) - .andExpect(content().contentType(MediaType.APPLICATION_JSON)) + .andExpect(content().contentType(MediaType.APPLICATION_JSON_UTF8_VALUE)) .andExpect(content().string("{\"name\":\"Joe\",\"someDouble\":0.0,\"someBoolean\":false}")); } @@ -161,7 +161,7 @@ public class AsyncTests { this.mockMvc.perform(asyncDispatch(mvcResult)) .andDo(print(writer)) .andExpect(status().isOk()) - .andExpect(content().contentType(MediaType.APPLICATION_JSON)) + .andExpect(content().contentType(MediaType.APPLICATION_JSON_UTF8_VALUE)) .andExpect(content().string("{\"name\":\"Joe\",\"someDouble\":0.0,\"someBoolean\":false}")); assertTrue(writer.toString().contains("Async started = false")); diff --git a/spring-test/src/test/java/org/springframework/test/web/servlet/samples/standalone/resultmatchers/ContentAssertionTests.java b/spring-test/src/test/java/org/springframework/test/web/servlet/samples/standalone/resultmatchers/ContentAssertionTests.java index 0c470ea5fb..d0d601387f 100644 --- a/spring-test/src/test/java/org/springframework/test/web/servlet/samples/standalone/resultmatchers/ContentAssertionTests.java +++ b/spring-test/src/test/java/org/springframework/test/web/servlet/samples/standalone/resultmatchers/ContentAssertionTests.java @@ -16,8 +16,6 @@ package org.springframework.test.web.servlet.samples.standalone.resultmatchers; -import java.nio.charset.Charset; - import org.junit.Before; import org.junit.Test; @@ -44,8 +42,6 @@ import static org.springframework.test.web.servlet.setup.MockMvcBuilders.*; */ public class ContentAssertionTests { - public static final MediaType TEXT_PLAIN_UTF8 = new MediaType("text", "plain", Charset.forName("UTF-8")); - private MockMvc mockMvc; @Before @@ -56,8 +52,10 @@ public class ContentAssertionTests { @Test public void testContentType() throws Exception { this.mockMvc.perform(get("/handle").accept(MediaType.TEXT_PLAIN)) - .andExpect(content().contentType(MediaType.TEXT_PLAIN)) - .andExpect(content().contentType("text/plain")); + .andExpect(content().contentType(MediaType.valueOf("text/plain;charset=ISO-8859-1"))) + .andExpect(content().contentType("text/plain;charset=ISO-8859-1")) + .andExpect(content().contentTypeCompatibleWith("text/plain")) + .andExpect(content().contentTypeCompatibleWith(MediaType.TEXT_PLAIN)); this.mockMvc.perform(get("/handleUtf8")) .andExpect(content().contentType(MediaType.valueOf("text/plain;charset=UTF-8"))) diff --git a/spring-test/src/test/java/org/springframework/test/web/servlet/samples/standalone/resultmatchers/XpathAssertionTests.java b/spring-test/src/test/java/org/springframework/test/web/servlet/samples/standalone/resultmatchers/XpathAssertionTests.java index 4ae88e7a12..c1e25791b6 100644 --- a/spring-test/src/test/java/org/springframework/test/web/servlet/samples/standalone/resultmatchers/XpathAssertionTests.java +++ b/spring-test/src/test/java/org/springframework/test/web/servlet/samples/standalone/resultmatchers/XpathAssertionTests.java @@ -158,7 +158,7 @@ public class XpathAssertionTests { standaloneSetup(new BlogFeedController()).build() .perform(get("/blog.atom").accept(MediaType.APPLICATION_ATOM_XML)) .andExpect(status().isOk()) - .andExpect(content().contentType(MediaType.APPLICATION_ATOM_XML)) + .andExpect(content().contentTypeCompatibleWith(MediaType.APPLICATION_ATOM_XML)) .andExpect(xpath("//feed/title").string("Test Feed")) .andExpect(xpath("//feed/icon").string("http://www.example.com/favicon.ico")); } diff --git a/spring-web/src/main/java/org/springframework/http/MediaType.java b/spring-web/src/main/java/org/springframework/http/MediaType.java index 79818edb2d..373c237c03 100644 --- a/spring-web/src/main/java/org/springframework/http/MediaType.java +++ b/spring-web/src/main/java/org/springframework/http/MediaType.java @@ -278,6 +278,17 @@ public class MediaType extends MimeType implements Serializable { this(type, subtype, Collections.singletonMap(PARAM_QUALITY_FACTOR, Double.toString(qualityValue))); } + /** + * Copy-constructor that copies the type, subtype and parameters of the given + * {@code MediaType}, and allows to set the specified character set. + * @param other the other media type + * @param charset the character set + * @throws IllegalArgumentException if any of the parameters contain illegal characters + */ + public MediaType(MediaType other, Charset charset) { + super(other, charset); + } + /** * Copy-constructor that copies the type and subtype of the given {@code MediaType}, * and allows for different parameter. diff --git a/spring-web/src/main/java/org/springframework/http/converter/AbstractHttpMessageConverter.java b/spring-web/src/main/java/org/springframework/http/converter/AbstractHttpMessageConverter.java index 33e9a84b62..6168fb858d 100644 --- a/spring-web/src/main/java/org/springframework/http/converter/AbstractHttpMessageConverter.java +++ b/spring-web/src/main/java/org/springframework/http/converter/AbstractHttpMessageConverter.java @@ -18,6 +18,7 @@ package org.springframework.http.converter; import java.io.IOException; import java.io.OutputStream; +import java.nio.charset.Charset; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -51,6 +52,8 @@ public abstract class AbstractHttpMessageConverter implements HttpMessageConv private List supportedMediaTypes = Collections.emptyList(); + private Charset defaultCharset; + /** * Construct an {@code AbstractHttpMessageConverter} with no supported media types. @@ -75,6 +78,11 @@ public abstract class AbstractHttpMessageConverter implements HttpMessageConv setSupportedMediaTypes(Arrays.asList(supportedMediaTypes)); } + protected AbstractHttpMessageConverter(Charset defaultCharset, MediaType... supportedMediaTypes) { + this.defaultCharset = defaultCharset; + setSupportedMediaTypes(Arrays.asList(supportedMediaTypes)); + } + /** * Set the list of {@link MediaType} objects supported by this converter. @@ -89,6 +97,16 @@ public abstract class AbstractHttpMessageConverter implements HttpMessageConv return Collections.unmodifiableList(this.supportedMediaTypes); } + /** + * Set the default character set if any. + */ + public void setDefaultCharset(Charset defaultCharset) { + this.defaultCharset = defaultCharset; + } + + public Charset getDefaultCharset() { + return defaultCharset; + } /** * This implementation checks if the given class is {@linkplain #supports(Class) supported}, @@ -200,7 +218,8 @@ public abstract class AbstractHttpMessageConverter implements HttpMessageConv /** * Add default headers to the output message. *

This implementation delegates to {@link #getDefaultContentType(Object)} if a content - * type was not provided, calls {@link #getContentLength}, and sets the corresponding headers + * type was not provided, set if necessary the default character set, calls + * {@link #getContentLength}, and sets the corresponding headers. * @since 4.2 */ protected void addDefaultHeaders(HttpHeaders headers, T t, MediaType contentType) throws IOException{ @@ -214,6 +233,9 @@ public abstract class AbstractHttpMessageConverter implements HttpMessageConv contentTypeToUse = (mediaType != null ? mediaType : contentTypeToUse); } if (contentTypeToUse != null) { + if (contentTypeToUse.getCharSet() == null && this.defaultCharset != null) { + contentTypeToUse = new MediaType(contentTypeToUse, this.defaultCharset); + } headers.setContentType(contentTypeToUse); } } diff --git a/spring-web/src/main/java/org/springframework/http/converter/ObjectToStringHttpMessageConverter.java b/spring-web/src/main/java/org/springframework/http/converter/ObjectToStringHttpMessageConverter.java index c1ef5c26dc..66c03e7cd3 100644 --- a/spring-web/src/main/java/org/springframework/http/converter/ObjectToStringHttpMessageConverter.java +++ b/spring-web/src/main/java/org/springframework/http/converter/ObjectToStringHttpMessageConverter.java @@ -74,7 +74,7 @@ public class ObjectToStringHttpMessageConverter extends AbstractHttpMessageConve * @param defaultCharset the default charset */ public ObjectToStringHttpMessageConverter(ConversionService conversionService, Charset defaultCharset) { - super(new MediaType("text", "plain", defaultCharset)); + super(defaultCharset, MediaType.TEXT_PLAIN); Assert.notNull(conversionService, "conversionService is required"); this.conversionService = conversionService; diff --git a/spring-web/src/main/java/org/springframework/http/converter/StringHttpMessageConverter.java b/spring-web/src/main/java/org/springframework/http/converter/StringHttpMessageConverter.java index 15c2693c81..9ed1d1cca8 100644 --- a/spring-web/src/main/java/org/springframework/http/converter/StringHttpMessageConverter.java +++ b/spring-web/src/main/java/org/springframework/http/converter/StringHttpMessageConverter.java @@ -42,8 +42,6 @@ public class StringHttpMessageConverter extends AbstractHttpMessageConverter availableCharsets; private boolean writeAcceptCharset = true; @@ -62,8 +60,7 @@ public class StringHttpMessageConverter extends AbstractHttpMessageConverter(Charset.availableCharsets().values()); } @@ -125,7 +122,7 @@ public class StringHttpMessageConverter extends AbstractHttpMessageConverter> converters = new ArrayList<>(); + converters.add(new MappingJackson2HttpMessageConverter()); + RequestResponseBodyMethodProcessor processor = new RequestResponseBodyMethodProcessor(converters); + + Object returnValue = new JacksonController().defaultCharset(); + processor.handleReturnValue(returnValue, methodReturnType, this.container, this.request); + + assertEquals("UTF-8", this.servletResponse.getCharacterEncoding()); + } + private void assertContentDisposition(RequestResponseBodyMethodProcessor processor, boolean expectContentDisposition, String requestURI, String comment) throws Exception { @@ -921,6 +937,13 @@ public class RequestResponseBodyMethodProcessorTests { bar.setName("bar"); return Arrays.asList(foo, bar); } + + @RequestMapping(produces = MediaType.APPLICATION_JSON_VALUE) + @ResponseBody + public String defaultCharset() { + return "foo"; + } + } private static class EmptyRequestBodyAdvice implements RequestBodyAdvice { diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java index 19fa225150..369fdf8ea0 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java @@ -1011,7 +1011,7 @@ public class ServletAnnotationControllerHandlerMethodTests extends AbstractServl request.addHeader("Accept", "application/json, text/javascript, */*"); MockHttpServletResponse response = new MockHttpServletResponse(); getServlet().service(request, response); - assertEquals("Invalid content-type", "application/json", response.getHeader("Content-Type")); + assertEquals("Invalid content-type", "application/json;charset=ISO-8859-1", response.getHeader("Content-Type")); } @Test @@ -1531,7 +1531,7 @@ public class ServletAnnotationControllerHandlerMethodTests extends AbstractServl getServlet().service(request, response); assertEquals(200, response.getStatus()); - assertEquals("application/json", response.getHeader("Content-Type")); + assertEquals("application/json;charset=ISO-8859-1", response.getHeader("Content-Type")); assertEquals("homeJson", response.getContentAsString()); } @@ -1652,7 +1652,7 @@ public class ServletAnnotationControllerHandlerMethodTests extends AbstractServl getServlet().service(request, response); assertEquals(200, response.getStatus()); - assertEquals("text/html", response.getContentType()); + assertEquals("text/html;charset=ISO-8859-1", response.getContentType()); assertEquals("inline;filename=f.txt", response.getHeader("Content-Disposition")); assertArrayEquals(content, response.getContentAsByteArray()); } @@ -1678,7 +1678,7 @@ public class ServletAnnotationControllerHandlerMethodTests extends AbstractServl getServlet().service(request, response); assertEquals(200, response.getStatus()); - assertEquals("text/html", response.getContentType()); + assertEquals("text/html;charset=ISO-8859-1", response.getContentType()); assertNull(response.getHeader("Content-Disposition")); assertArrayEquals(content, response.getContentAsByteArray()); } @@ -1704,7 +1704,7 @@ public class ServletAnnotationControllerHandlerMethodTests extends AbstractServl getServlet().service(request, response); assertEquals(200, response.getStatus()); - assertEquals("text/html", response.getContentType()); + assertEquals("text/html;charset=ISO-8859-1", response.getContentType()); assertNull(response.getHeader("Content-Disposition")); assertArrayEquals(content, response.getContentAsByteArray()); } @@ -1730,7 +1730,7 @@ public class ServletAnnotationControllerHandlerMethodTests extends AbstractServl getServlet().service(request, response); assertEquals(200, response.getStatus()); - assertEquals("text/css", response.getContentType()); + assertEquals("text/css;charset=ISO-8859-1", response.getContentType()); assertNull(response.getHeader("Content-Disposition")); assertArrayEquals(content, response.getContentAsByteArray()); }