Skip to content

Commit

Permalink
JSON charset handling in StringHttpMessageConverter
Browse files Browse the repository at this point in the history 
This commit restores the interpretation of JSON as UTF-8 by default that
was removed in #bc205e0 and also ensures a charset is not appended
automatically to "application/json".

Closes gh-24123
  • Loading branch information
@rstoyanchev
rstoyanchev committed Dec 10, 2019
1 parent 70a0c93 commit 9b30d46
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 2 deletions.
16 changes: 16 additions & 0 deletions spring-web/src/main/java/org/springframework/http/converter/StringHttpMessageConverter.java
View file
Expand Up @@ -100,6 +100,18 @@ protected Long getContentLength(String str, @Nullable MediaType contentType) {
return (long) str.getBytes(charset).length;
}


@Override
protected void addDefaultHeaders(HttpHeaders headers, String s, @Nullable MediaType mediaType) throws IOException {
if (headers.getContentType() == null ) {
if (mediaType != null && mediaType.isCompatibleWith(MediaType.APPLICATION_JSON)) {
// Prevent charset parameter for JSON..
headers.setContentType(mediaType);

This comment has been minimized.

Sign in to view

Copy link
@artembilan

artembilan Dec 26, 2019

Member

I have just started to fail with this change.
My content type candidate is like this one:

if (CollectionUtils.isEmpty(acceptTypes)) {
		acceptTypes = Collections.singletonList(MediaType.ALL);
}

So, MediaType.ALL is really compatible with MediaType.APPLICATION_JSON, but it is wrong value for the Content-Type header:

java.lang.IllegalArgumentException: Content-Type cannot contain wildcard type '*'

	at org.springframework.util.Assert.isTrue(Assert.java:118)
	at org.springframework.http.HttpHeaders.setContentType(HttpHeaders.java:949)
	at org.springframework.http.converter.StringHttpMessageConverter.addDefaultHeaders(StringHttpMessageConverter.java:109)
	at org.springframework.http.converter.StringHttpMessageConverter.addDefaultHeaders(StringHttpMessageConverter.java:44)
	at org.springframework.http.converter.AbstractHttpMessageConverter.write(AbstractHttpMessageConverter.java:211)

Any clues what to use instead of MediaType.ALL when no Accept header in the request?

Or maybe this fix should be improved to skip MediaType.ALL as it is done in the super class:

if (contentType == null || contentType.isWildcardType() || contentType.isWildcardSubtype()) {
		contentTypeToUse = getDefaultContentType(t);
}

Thank you!

This comment has been minimized.

Sign in to view

Copy link
@rstoyanchev

rstoyanchev Jan 2, 2020

Author Contributor

Yes we should add a check for mediaType.isConcrete() to protect as in the base class. Can you create an issue?

I am curious where the wildcard media type comes from. At that point on the server side we've typically already checked that it is concrete or fallen back on a default one.

This comment has been minimized.

Sign in to view

Copy link
@artembilan

artembilan Jan 2, 2020

Member

Thank you, @rstoyanchev , for confirmation.
Here is an #24283.

I don't use an AbstractMessageConverterMethodProcessor in Spring Integration.
There logic in the HttpRequestHandlingMessagingGateway is like this: https://github.com/spring-projects/spring-integration/blob/master/spring-integration-http/src/main/java/org/springframework/integration/http/inbound/HttpRequestHandlingMessagingGateway.java#L171

So, instead of falling back to the MediaType.APPLICATION_OCTET_STREAM we try to rely on the Content-Type populated by the converted which fits to the payload we would like to return into the response.
}
}
super.addDefaultHeaders(headers, s, mediaType);
}

@Override
protected void writeInternal(String str, HttpOutputMessage outputMessage) throws IOException {
HttpHeaders headers = outputMessage.getHeaders();
Expand Down Expand Up @@ -130,6 +142,10 @@ private Charset getContentTypeCharset(@Nullable MediaType contentType) {
if (contentType != null && contentType.getCharset() != null) {
return contentType.getCharset();
}
else if (contentType != null && contentType.isCompatibleWith(MediaType.APPLICATION_JSON)) {
// Matching to AbstractJackson2HttpMessageConverter#DEFAULT_CHARSET
return StandardCharsets.UTF_8;
}
else {
Charset charset = getDefaultCharset();
Assert.state(charset != null, "No default charset");
Expand Down
22 changes: 22 additions & 0 deletions ...web/src/test/java/org/springframework/http/converter/StringHttpMessageConverterTests.java
View file
Expand Up @@ -70,6 +70,16 @@ public void read() throws IOException {
assertThat(result).as("Invalid result").isEqualTo(body);
}

@Test // gh-24123
public void readJson() throws IOException {
String body = "{\"result\":\"\u0414\u0410\"}";
MockHttpInputMessage inputMessage = new MockHttpInputMessage(body.getBytes(StandardCharsets.UTF_8));
inputMessage.getHeaders().setContentType(MediaType.APPLICATION_JSON);
String result = this.converter.read(String.class, inputMessage);

assertThat(result).as("Invalid result").isEqualTo(body);
}

@Test
public void writeDefaultCharset() throws IOException {
String body = "H\u00e9llo W\u00f6rld";
Expand All @@ -82,6 +92,18 @@ public void writeDefaultCharset() throws IOException {
assertThat(headers.getAcceptCharset().isEmpty()).isTrue();
}

@Test // gh-24123
public void writeJson() throws IOException {
String body = "{\"foo\":\"bar\"}";
this.converter.write(body, MediaType.APPLICATION_JSON, this.outputMessage);

HttpHeaders headers = this.outputMessage.getHeaders();
assertThat(this.outputMessage.getBodyAsString(StandardCharsets.UTF_8)).isEqualTo(body);
assertThat(headers.getContentType()).isEqualTo(MediaType.APPLICATION_JSON);
assertThat(headers.getContentLength()).isEqualTo(body.getBytes(StandardCharsets.UTF_8).length);
assertThat(headers.getAcceptCharset().isEmpty()).isTrue();
}

@Test
public void writeUTF8() throws IOException {
String body = "H\u00e9llo W\u00f6rld";
Expand Down
4 changes: 2 additions & 2 deletions ...work/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java
View file
Expand Up @@ -1011,7 +1011,7 @@ public void overlappingMessageConvertersRequestBody() throws Exception {
request.addHeader("Accept", "application/json, text/javascript, */*");
MockHttpServletResponse response = new MockHttpServletResponse();
getServlet().service(request, response);
assertThat(response.getHeader("Content-Type")).as("Invalid content-type").isEqualTo("application/json;charset=ISO-8859-1");
assertThat(response.getHeader("Content-Type")).as("Invalid content-type").isEqualTo("application/json");
}

@Test
Expand Down Expand Up @@ -1548,7 +1548,7 @@ public void testHeadersCondition() throws Exception {
getServlet().service(request, response);

assertThat(response.getStatus()).isEqualTo(200);
assertThat(response.getHeader("Content-Type")).isEqualTo("application/json;charset=ISO-8859-1");
assertThat(response.getHeader("Content-Type")).isEqualTo("application/json");
assertThat(response.getContentAsString()).isEqualTo("homeJson");
}

Expand Down

0 comments on commit 9b30d46

Please sign in to comment.