From bd6e1c031e03c8e14ae141251c24aba85237e613 Mon Sep 17 00:00:00 2001 From: Ralf Ueberfuhr Date: Thu, 2 Feb 2023 12:16:13 +0100 Subject: [PATCH 1/3] Fix RFC-7807 content negotiation - prefer RFC-7807 content-types in case of returning a ProblemDetail (error response) - exclude RFC-7807 content-types in case of returning all other types (success response) --- ...stractMessageConverterMethodProcessor.java | 39 +++- .../HttpEntityMethodProcessorTests.java | 168 ++++++++++++++++++ 2 files changed, 199 insertions(+), 8 deletions(-) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractMessageConverterMethodProcessor.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractMessageConverterMethodProcessor.java index d00b084c0d49..df2075803e44 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractMessageConverterMethodProcessor.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractMessageConverterMethodProcessor.java @@ -19,13 +19,15 @@ import java.io.IOException; import java.lang.reflect.Type; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collections; import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; import java.util.Locale; +import java.util.Map; +import java.util.Objects; import java.util.Set; +import java.util.stream.Collectors; import jakarta.servlet.ServletRequest; import jakarta.servlet.http.HttpServletRequest; @@ -73,6 +75,7 @@ * @author Rossen Stoyanchev * @author Brian Clozel * @author Juergen Hoeller + * @author Ralf Ueberfuhr * @since 3.1 */ public abstract class AbstractMessageConverterMethodProcessor extends AbstractMessageConverterMethodArgumentResolver @@ -96,8 +99,11 @@ public abstract class AbstractMessageConverterMethodProcessor extends AbstractMe private final ContentNegotiationManager contentNegotiationManager; - private final List problemMediaTypes = - Arrays.asList(MediaType.APPLICATION_PROBLEM_JSON, MediaType.APPLICATION_PROBLEM_XML); + private final Map problemMediaTypesByNormalType = Map.of( + MediaType.APPLICATION_JSON_VALUE, MediaType.APPLICATION_PROBLEM_JSON, + MediaType.APPLICATION_XML_VALUE, MediaType.APPLICATION_PROBLEM_XML + ); + private final Set problemMediaTypes = new HashSet<>(problemMediaTypesByNormalType.values()); private final Set safeExtensions = new HashSet<>(); @@ -240,14 +246,31 @@ protected void writeWithMessageConverters(@Nullable T value, MethodParameter "No converter found for return value of type: " + valueType); } - List compatibleMediaTypes = new ArrayList<>(); - determineCompatibleMediaTypes(acceptableTypes, producibleTypes, compatibleMediaTypes); - // For ProblemDetail, fall back on RFC 7807 format - if (compatibleMediaTypes.isEmpty() && ProblemDetail.class.isAssignableFrom(valueType)) { - determineCompatibleMediaTypes(this.problemMediaTypes, producibleTypes, compatibleMediaTypes); + if(ProblemDetail.class.isAssignableFrom(valueType)) { + acceptableTypes = acceptableTypes.stream() + .map( + mediaType -> this.problemMediaTypes.stream() + .noneMatch(problemType -> problemType.isCompatibleWith(mediaType)) + ? + this.problemMediaTypesByNormalType.get(mediaType.toString()) + : + mediaType + ) + .filter(Objects::nonNull) + .collect(Collectors.toList()); + producibleTypes = producibleTypes.stream() + .map(mediaType -> this.problemMediaTypesByNormalType.get(mediaType.toString())) + .filter(Objects::nonNull) + .collect(Collectors.toList()); + } else { + acceptableTypes.removeAll(this.problemMediaTypes); + producibleTypes.removeAll(this.problemMediaTypes); } + List compatibleMediaTypes = new ArrayList<>(); + determineCompatibleMediaTypes(acceptableTypes, producibleTypes, compatibleMediaTypes); + if (compatibleMediaTypes.isEmpty()) { if (logger.isDebugEnabled()) { logger.debug("No match for " + acceptableTypes + ", supported: " + producibleTypes); diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorTests.java index ebcbc87f7cdb..0cb75fd2ab41 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorTests.java @@ -21,27 +21,36 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.Stream; import com.fasterxml.jackson.annotation.JsonTypeInfo; import com.fasterxml.jackson.annotation.JsonTypeName; import jakarta.servlet.FilterChain; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; +import jakarta.xml.bind.annotation.XmlRootElement; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; import org.springframework.core.MethodParameter; import org.springframework.http.HttpEntity; import org.springframework.http.HttpHeaders; import org.springframework.http.MediaType; +import org.springframework.http.ProblemDetail; import org.springframework.http.ResponseEntity; import org.springframework.http.converter.ByteArrayHttpMessageConverter; import org.springframework.http.converter.HttpMessageConverter; import org.springframework.http.converter.StringHttpMessageConverter; import org.springframework.http.converter.json.MappingJackson2HttpMessageConverter; +import org.springframework.http.converter.xml.Jaxb2RootElementHttpMessageConverter; import org.springframework.lang.Nullable; import org.springframework.validation.beanvalidation.LocalValidatorFactoryBean; import org.springframework.web.bind.WebDataBinder; +import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.ResponseBody; import org.springframework.web.bind.support.WebDataBinderFactory; @@ -206,6 +215,157 @@ public void handleReturnValueCharSequence() throws Exception { assertThat(servletResponse.getContentAsString()).isEqualTo("Foo"); } + @Nested + class ContentNegotiationTests { + private static class ContentNegotiationCase { + private final List givenAccept; + private final String expectedContentType; + + public ContentNegotiationCase(List givenAccept, String expectedContentType) { + this.givenAccept = givenAccept; + this.expectedContentType = expectedContentType; + } + + public List getGivenAccept() { + return givenAccept; + } + + public String getExpectedContentType() { + return expectedContentType; + } + + @Override + public String toString() { + return givenAccept + "->" + expectedContentType; + } + } + + private static Stream handleContentNegotiationForProblem() { + var result = Stream.of( + new ContentNegotiationCase( + List.of(MediaType.APPLICATION_JSON_VALUE), + MediaType.APPLICATION_PROBLEM_JSON_VALUE), + new ContentNegotiationCase( + List.of(MediaType.APPLICATION_PROBLEM_JSON_VALUE, MediaType.APPLICATION_JSON_VALUE), + MediaType.APPLICATION_PROBLEM_JSON_VALUE), + new ContentNegotiationCase( + List.of("*/*"), + MediaType.APPLICATION_PROBLEM_JSON_VALUE), + new ContentNegotiationCase( + List.of("application/*+json"), + MediaType.APPLICATION_PROBLEM_JSON_VALUE), + new ContentNegotiationCase( + List.of(MediaType.TEXT_PLAIN_VALUE, MediaType.APPLICATION_JSON_VALUE), + MediaType.APPLICATION_PROBLEM_JSON_VALUE) + ); + // if ProblemDetail is supported by the XML message converter, add further test cases + if (new Jaxb2RootElementHttpMessageConverter() + .canWrite(ProblemDetail.class, MediaType.APPLICATION_XML)) { + result = Stream.concat(result, Stream.of( + new ContentNegotiationCase( + List.of(MediaType.APPLICATION_XML_VALUE), + MediaType.APPLICATION_PROBLEM_XML_VALUE), + new ContentNegotiationCase( + List.of(MediaType.APPLICATION_PROBLEM_XML_VALUE, MediaType.APPLICATION_XML_VALUE), + MediaType.APPLICATION_PROBLEM_XML_VALUE), + new ContentNegotiationCase( + List.of("application/*+xml"), + MediaType.APPLICATION_PROBLEM_XML_VALUE) + )); + } + return result; + } + + @ParameterizedTest // gh-29588 + @MethodSource + public void handleContentNegotiationForProblem(ContentNegotiationCase contentNegotiationCase) throws Exception { + List> converters = new ArrayList<>(); + converters.add(new MappingJackson2HttpMessageConverter()); + converters.add(new Jaxb2RootElementHttpMessageConverter()); + + Method method = JacksonController.class.getDeclaredMethod("handleException"); + MethodParameter returnType = new MethodParameter(method, -1); + + servletRequest.addHeader( + "Accept", + contentNegotiationCase.getGivenAccept() + .stream() + .collect(Collectors.joining(",")) + ); + + HttpEntityMethodProcessor processor = new HttpEntityMethodProcessor(converters); + processor.writeWithMessageConverters( + ProblemDetail.forStatus(400), + returnType, + webRequest); + + assertThat(servletResponse.getHeader("Content-Type")) + .isEqualTo(contentNegotiationCase.getExpectedContentType()); + } + + private static Stream handleContentNegotiationForProblemAcceptedButNoProblem() { + return Stream.of( + new ContentNegotiationCase( + List.of(MediaType.APPLICATION_JSON_VALUE), + MediaType.APPLICATION_JSON_VALUE), + new ContentNegotiationCase( + List.of(MediaType.APPLICATION_PROBLEM_JSON_VALUE, MediaType.APPLICATION_JSON_VALUE), + MediaType.APPLICATION_JSON_VALUE), + new ContentNegotiationCase( + List.of("*/*"), + MediaType.APPLICATION_JSON_VALUE), + new ContentNegotiationCase( + List.of(MediaType.APPLICATION_PROBLEM_JSON_VALUE, "*/*"), + MediaType.APPLICATION_JSON_VALUE), + new ContentNegotiationCase( + List.of("application/*+json"), + MediaType.APPLICATION_JSON_VALUE), + new ContentNegotiationCase( + List.of(MediaType.APPLICATION_XML_VALUE), + MediaType.APPLICATION_XML_VALUE), + new ContentNegotiationCase( + List.of(MediaType.APPLICATION_PROBLEM_XML_VALUE, MediaType.APPLICATION_XML_VALUE), + MediaType.APPLICATION_XML_VALUE), + new ContentNegotiationCase( + List.of("application/*+xml"), + MediaType.APPLICATION_XML_VALUE), + new ContentNegotiationCase( + List.of(MediaType.APPLICATION_XML_VALUE, MediaType.APPLICATION_JSON_VALUE), + MediaType.APPLICATION_XML_VALUE) + ); + } + + @ParameterizedTest // gh-29588 + @MethodSource + public void handleContentNegotiationForProblemAcceptedButNoProblem(ContentNegotiationCase contentNegotiationCase) throws Exception { + List> converters = new ArrayList<>(); + converters.add(new MappingJackson2HttpMessageConverter()); + converters.add(new Jaxb2RootElementHttpMessageConverter()); + + Method method = HttpEntityMethodProcessorTests.this.getClass().getDeclaredMethod("handle"); + MethodParameter returnType = new MethodParameter(method, -1); + + servletRequest.addHeader( + "Accept", + contentNegotiationCase.getGivenAccept() + .stream() + .collect(Collectors.joining(",")) + ); + + HttpEntityMethodProcessor processor = new HttpEntityMethodProcessor(converters); + processor.writeWithMessageConverters( + new Foo("foo"), + returnType, + webRequest); + + assertThat(servletResponse.getStatus()) + .isEqualTo(200); + assertThat(servletResponse.getHeader("Content-Type")) + .isEqualTo(contentNegotiationCase.getExpectedContentType()); + } + + } + @Test // SPR-13423 public void handleReturnValueWithETagAndETagFilter() throws Exception { String eTagValue = "\"deadb33f8badf00d\""; @@ -354,6 +514,7 @@ public void setParentProperty(String parentProperty) { @JsonTypeName("foo") + @XmlRootElement private static class Foo extends ParentClass { public Foo() { @@ -387,6 +548,13 @@ public HttpEntity> handleList() { list.add(new Bar("bar")); return new HttpEntity<>(list); } + + @ExceptionHandler + public ResponseEntity handleException() { + return ResponseEntity.internalServerError() + .body(ProblemDetail.forStatus(500)); + } + } } From 6b5e7fb9d6b6952f68d29bd22ce6ede55943dae2 Mon Sep 17 00:00:00 2001 From: Ralf Ueberfuhr Date: Thu, 2 Feb 2023 13:27:44 +0100 Subject: [PATCH 2/3] Fix checkstyle issues --- .../annotation/AbstractMessageConverterMethodProcessor.java | 5 +++-- .../method/annotation/HttpEntityMethodProcessorTests.java | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractMessageConverterMethodProcessor.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractMessageConverterMethodProcessor.java index df2075803e44..8fb93fe4b684 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractMessageConverterMethodProcessor.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractMessageConverterMethodProcessor.java @@ -103,7 +103,7 @@ public abstract class AbstractMessageConverterMethodProcessor extends AbstractMe MediaType.APPLICATION_JSON_VALUE, MediaType.APPLICATION_PROBLEM_JSON, MediaType.APPLICATION_XML_VALUE, MediaType.APPLICATION_PROBLEM_XML ); - private final Set problemMediaTypes = new HashSet<>(problemMediaTypesByNormalType.values()); + private final Set problemMediaTypes = new HashSet<>(this.problemMediaTypesByNormalType.values()); private final Set safeExtensions = new HashSet<>(); @@ -263,7 +263,8 @@ protected void writeWithMessageConverters(@Nullable T value, MethodParameter .map(mediaType -> this.problemMediaTypesByNormalType.get(mediaType.toString())) .filter(Objects::nonNull) .collect(Collectors.toList()); - } else { + } + else { acceptableTypes.removeAll(this.problemMediaTypes); producibleTypes.removeAll(this.problemMediaTypes); } diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorTests.java index 0cb75fd2ab41..9756ae880077 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorTests.java @@ -33,9 +33,9 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; - import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.MethodSource; + import org.springframework.core.MethodParameter; import org.springframework.http.HttpEntity; import org.springframework.http.HttpHeaders; From 5a5a59c37900e355a0853ac79a4554235cdccdda Mon Sep 17 00:00:00 2001 From: Ralf Ueberfuhr Date: Thu, 2 Feb 2023 15:06:35 +0100 Subject: [PATCH 3/3] Fix further test issues --- ...stractMessageConverterMethodProcessor.java | 45 ++++++++++++------- .../HttpEntityMethodProcessorTests.java | 6 +++ ...questResponseBodyMethodProcessorTests.java | 2 +- 3 files changed, 35 insertions(+), 18 deletions(-) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractMessageConverterMethodProcessor.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractMessageConverterMethodProcessor.java index 8fb93fe4b684..b9ee9a1dce9a 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractMessageConverterMethodProcessor.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/AbstractMessageConverterMethodProcessor.java @@ -19,6 +19,7 @@ import java.io.IOException; import java.lang.reflect.Type; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.HashSet; import java.util.LinkedHashSet; @@ -248,25 +249,21 @@ protected void writeWithMessageConverters(@Nullable T value, MethodParameter // For ProblemDetail, fall back on RFC 7807 format if(ProblemDetail.class.isAssignableFrom(valueType)) { - acceptableTypes = acceptableTypes.stream() - .map( - mediaType -> this.problemMediaTypes.stream() - .noneMatch(problemType -> problemType.isCompatibleWith(mediaType)) - ? - this.problemMediaTypesByNormalType.get(mediaType.toString()) - : - mediaType - ) - .filter(Objects::nonNull) - .collect(Collectors.toList()); - producibleTypes = producibleTypes.stream() - .map(mediaType -> this.problemMediaTypesByNormalType.get(mediaType.toString())) - .filter(Objects::nonNull) - .collect(Collectors.toList()); + acceptableTypes = this.replaceNonProblemCompliantMediaTypes(acceptableTypes); + if(acceptableTypes.isEmpty()) { + acceptableTypes = List.of(MediaType.APPLICATION_PROBLEM_JSON); + } + producibleTypes = this.replaceNonProblemCompliantMediaTypes(producibleTypes); } else { - acceptableTypes.removeAll(this.problemMediaTypes); - producibleTypes.removeAll(this.problemMediaTypes); + var statusCode = outputMessage.getServletResponse().getStatus(); + var status = HttpStatus.resolve(statusCode); + // For error status code, leave RFC 7807 media types, if requested + // otherwise, remove them + if(null == status || !status.isError()) { + acceptableTypes.removeAll(this.problemMediaTypes); + producibleTypes.removeAll(this.problemMediaTypes); + } } List compatibleMediaTypes = new ArrayList<>(); @@ -347,6 +344,20 @@ else if (mediaType.isPresentIn(ALL_APPLICATION_MEDIA_TYPES)) { } } + private List replaceNonProblemCompliantMediaTypes(Collection types) { + return types.stream() + .map( + mediaType -> this.problemMediaTypes.stream() + .noneMatch(problemType -> problemType.isCompatibleWith(mediaType)) + ? + this.problemMediaTypesByNormalType.get(mediaType.toString()) + : + mediaType + ) + .filter(Objects::nonNull) + .collect(Collectors.toList()); + } + /** * Return the type of the value to be written to the response. Typically this is * a simple check via getClass on the value but if the value is null, then the diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorTests.java index 9756ae880077..efa3781e0ba9 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorTests.java @@ -245,12 +245,18 @@ private static Stream handleContentNegotiationForProblem new ContentNegotiationCase( List.of(MediaType.APPLICATION_JSON_VALUE), MediaType.APPLICATION_PROBLEM_JSON_VALUE), + new ContentNegotiationCase( + List.of(MediaType.APPLICATION_PROBLEM_JSON_VALUE), + MediaType.APPLICATION_PROBLEM_JSON_VALUE), new ContentNegotiationCase( List.of(MediaType.APPLICATION_PROBLEM_JSON_VALUE, MediaType.APPLICATION_JSON_VALUE), MediaType.APPLICATION_PROBLEM_JSON_VALUE), new ContentNegotiationCase( List.of("*/*"), MediaType.APPLICATION_PROBLEM_JSON_VALUE), + new ContentNegotiationCase( + List.of(MediaType.APPLICATION_PDF_VALUE), + MediaType.APPLICATION_PROBLEM_JSON_VALUE), new ContentNegotiationCase( List.of("application/*+json"), MediaType.APPLICATION_PROBLEM_JSON_VALUE), diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestResponseBodyMethodProcessorTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestResponseBodyMethodProcessorTests.java index e7018c11e101..c048747b2126 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestResponseBodyMethodProcessorTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestResponseBodyMethodProcessorTests.java @@ -403,7 +403,7 @@ void problemDetailDefaultMediaType() throws Exception { @Test void problemDetailWhenJsonRequested() throws Exception { this.servletRequest.addHeader("Accept", MediaType.APPLICATION_JSON_VALUE); - testProblemDetailMediaType(MediaType.APPLICATION_JSON_VALUE); + testProblemDetailMediaType(MediaType.APPLICATION_PROBLEM_JSON_VALUE); } @Test