From a8091b916b74dec5da5464c0b373939b727c8924 Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Tue, 8 Dec 2020 20:50:26 +0100 Subject: [PATCH] Avoid closing Jackson JsonGenerator for error cases Prior to this commit, a change introduced in gh-25910 would close the `JsonGenerator` after it's been used for JSON serialization. This would not only close it and recycle resources, but also flush the underlyning buffer to the output. In a case where the JSON serialization process would throw an exception, the buffer would be still flushed to the response output. Before the change introduced in gh-25910, the response body could be still empty at that point and error handling could write an error body instead. This commits only closes the `JsonGenerator` when serialization has been successful. Note that we're changing this in the spirit of backwards compatibility in the 5.2.x line, but change this won't be merged forward on the 5.3.x line, for several reasons: * this behavior is not consistent. If the JSON output exceeds a certain size, or if Jackson has been configured to flush after each write, the response output might still contain an incomplete JSON payload (just like before this change) * this behavior is not consistent with the WebFlux and Messaging codecs, which are flushing or closing the generator * not closing the generator for error cases prevents resources from being recycled as expected by Jackson Fixes gh-26246 --- .../AbstractJackson2HttpMessageConverter.java | 4 +- ...pingJackson2HttpMessageConverterTests.java | 39 ++++++++++++------- 2 files changed, 28 insertions(+), 15 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/http/converter/json/AbstractJackson2HttpMessageConverter.java b/spring-web/src/main/java/org/springframework/http/converter/json/AbstractJackson2HttpMessageConverter.java index 7ea688eede..33ad75dd11 100644 --- a/spring-web/src/main/java/org/springframework/http/converter/json/AbstractJackson2HttpMessageConverter.java +++ b/spring-web/src/main/java/org/springframework/http/converter/json/AbstractJackson2HttpMessageConverter.java @@ -311,7 +311,8 @@ public abstract class AbstractJackson2HttpMessageConverter extends AbstractGener JsonEncoding encoding = getJsonEncoding(contentType); OutputStream outputStream = StreamUtils.nonClosing(outputMessage.getBody()); - try (JsonGenerator generator = this.objectMapper.getFactory().createGenerator(outputStream, encoding)) { + JsonGenerator generator = this.objectMapper.getFactory().createGenerator(outputStream, encoding); + try { writePrefix(generator, object); Object value = object; @@ -346,6 +347,7 @@ public abstract class AbstractJackson2HttpMessageConverter extends AbstractGener writeSuffix(generator, object); generator.flush(); + generator.close(); } catch (InvalidDefinitionException ex) { throw new HttpMessageConversionException("Type definition error: " + ex.getType(), ex); diff --git a/spring-web/src/test/java/org/springframework/http/converter/json/MappingJackson2HttpMessageConverterTests.java b/spring-web/src/test/java/org/springframework/http/converter/json/MappingJackson2HttpMessageConverterTests.java index 79b1b8ba36..5b5c63c49d 100644 --- a/spring-web/src/test/java/org/springframework/http/converter/json/MappingJackson2HttpMessageConverterTests.java +++ b/spring-web/src/test/java/org/springframework/http/converter/json/MappingJackson2HttpMessageConverterTests.java @@ -16,11 +16,13 @@ package org.springframework.http.converter.json; +import java.io.ByteArrayOutputStream; import java.io.IOException; import java.lang.reflect.Type; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -135,13 +137,7 @@ public class MappingJackson2HttpMessageConverterTests { @Test public void write() throws IOException { MockHttpOutputMessage outputMessage = new MockHttpOutputMessage(); - MyBean body = new MyBean(); - body.setString("Foo"); - body.setNumber(42); - body.setFraction(42F); - body.setArray(new String[] {"Foo", "Bar"}); - body.setBool(true); - body.setBytes(new byte[] {0x1, 0x2}); + MyBean body = createSampleBean(); converter.write(body, null, outputMessage); String result = outputMessage.getBodyAsString(StandardCharsets.UTF_8); assertThat(result.contains("\"string\":\"Foo\"")).isTrue(); @@ -157,13 +153,7 @@ public class MappingJackson2HttpMessageConverterTests { @Test public void writeWithBaseType() throws IOException { MockHttpOutputMessage outputMessage = new MockHttpOutputMessage(); - MyBean body = new MyBean(); - body.setString("Foo"); - body.setNumber(42); - body.setFraction(42F); - body.setArray(new String[] {"Foo", "Bar"}); - body.setBool(true); - body.setBytes(new byte[] {0x1, 0x2}); + MyBean body = createSampleBean(); converter.write(body, MyBase.class, null, outputMessage); String result = outputMessage.getBodyAsString(StandardCharsets.UTF_8); assertThat(result.contains("\"string\":\"Foo\"")).isTrue(); @@ -194,6 +184,16 @@ public class MappingJackson2HttpMessageConverterTests { converter.read(MyBean.class, inputMessage)); } + @Test // See gh-26246 + public void writeInvalidJson() throws IOException { + MockHttpOutputMessage outputMessage = new MockHttpOutputMessage(); + MyBean bean = createSampleBean(); + List body = Arrays.asList(bean, new ByteArrayOutputStream()); + assertThatExceptionOfType(HttpMessageConversionException.class) + .isThrownBy(() -> converter.write(body, null, outputMessage)); + assertThat(outputMessage.getBodyAsString(StandardCharsets.UTF_8)).isEmpty(); + } + @Test public void readValidJsonWithUnknownProperty() throws IOException { String body = "{\"string\":\"string\",\"unknownProperty\":\"value\"}"; @@ -492,6 +492,17 @@ public class MappingJackson2HttpMessageConverterTests { assertThat(outputMessage.getHeaders().getContentType()).as("Invalid content-type").isEqualTo(contentType); } + private MyBean createSampleBean() { + MyBean body = new MyBean(); + body.setString("Foo"); + body.setNumber(42); + body.setFraction(42F); + body.setArray(new String[] {"Foo", "Bar"}); + body.setBool(true); + body.setBytes(new byte[] {0x1, 0x2}); + return body; + } + interface MyInterface {