From 7e9173350270377354d5518a4604da695fed33d0 Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Wed, 15 Aug 2018 10:58:41 +0200 Subject: [PATCH] Infer HTTP 404 from empty Optional/Publisher types This commit handles "empty" cases for `ResponseEntity` controller handler return types when wrapped with a `java.util.Optional` in Spring MVC or a single `Publisher` like `Mono`. Given the following example for Spring MVC: ``` @GetMapping("/user") public Optional> fetchUser() { Optional user = //... return user.map(ResponseEntity::ok); } ``` If the resulting `Optional` is empty, Spring MVC will infer a `ResponseEntity` with an empty body and a 404 HTTP response status. The same reasoning is applied to Spring WebFlux with Publisher types: ``` @GetMapping("/user") public Mono> fetchUser() { Mono user = //... return user.map(ResponseEntity::ok); } ``` This feature is only valid for `HttpEntity` return types and does not apply to `@ResponseBody` controller handlers. Issue: SPR-13281 --- .../ResponseEntityResultHandler.java | 5 +- .../ResponseEntityResultHandlerTests.java | 14 ++ .../annotation/HttpEntityMethodProcessor.java | 16 +- .../HttpEntityMethodProcessorMockTests.java | 167 ++++++++++-------- src/docs/asciidoc/web/webflux.adoc | 21 ++- src/docs/asciidoc/web/webmvc.adoc | 18 +- 6 files changed, 158 insertions(+), 83 deletions(-) diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ResponseEntityResultHandler.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ResponseEntityResultHandler.java index 59b0b7d29b..eef6f953ff 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ResponseEntityResultHandler.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ResponseEntityResultHandler.java @@ -55,6 +55,8 @@ public class ResponseEntityResultHandler extends AbstractMessageWriterResultHand private static final Set SAFE_METHODS = EnumSet.of(HttpMethod.GET, HttpMethod.HEAD); + private static final ResponseEntity notFound = new ResponseEntity<>(HttpStatus.NOT_FOUND); + /** * Basic constructor with a default {@link ReactiveAdapterRegistry}. @@ -119,7 +121,8 @@ public class ResponseEntityResultHandler extends AbstractMessageWriterResultHand if (adapter != null) { Assert.isTrue(!adapter.isMultiValue(), "Only a single ResponseEntity supported"); - returnValueMono = Mono.from(adapter.toPublisher(result.getReturnValue())); + returnValueMono = Mono.from(adapter.toPublisher(result.getReturnValue())) + .defaultIfEmpty(notFound); bodyParameter = actualParameter.nested().nested(); } else { diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ResponseEntityResultHandlerTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ResponseEntityResultHandlerTests.java index 7be305b999..c14dc55e02 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ResponseEntityResultHandlerTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ResponseEntityResultHandlerTests.java @@ -350,6 +350,20 @@ public class ResponseEntityResultHandlerTests { assertResponseBodyIsEmpty(exchange); } + @Test // SPR-13281 + public void handleEmptyMonoShouldResultInNotFoundStatus() { + MockServerWebExchange exchange = MockServerWebExchange.from(get("/path")); + exchange.getAttributes().put(PRODUCIBLE_MEDIA_TYPES_ATTRIBUTE, Collections.singleton(APPLICATION_JSON)); + + MethodParameter type = on(TestController.class).resolveReturnType(Mono.class, ResponseEntity.class); + HandlerResult result = new HandlerResult(new TestController(), Mono.empty(), type); + + this.resultHandler.handleResult(exchange, result).block(Duration.ofSeconds(5)); + + assertEquals(HttpStatus.NOT_FOUND, exchange.getResponse().getStatusCode()); + assertResponseBodyIsEmpty(exchange); + } + private void testHandle(Object returnValue, MethodParameter returnType) { diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessor.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessor.java index cb91f07d56..4c2a31b9d6 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessor.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessor.java @@ -24,6 +24,7 @@ import java.util.Collections; import java.util.EnumSet; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -33,6 +34,7 @@ import org.springframework.core.ResolvableType; import org.springframework.http.HttpEntity; import org.springframework.http.HttpHeaders; import org.springframework.http.HttpMethod; +import org.springframework.http.HttpStatus; import org.springframework.http.RequestEntity; import org.springframework.http.ResponseEntity; import org.springframework.http.converter.HttpMessageConverter; @@ -121,8 +123,9 @@ public class HttpEntityMethodProcessor extends AbstractMessageConverterMethodPro @Override public boolean supportsReturnType(MethodParameter returnType) { - return (HttpEntity.class.isAssignableFrom(returnType.getParameterType()) && - !RequestEntity.class.isAssignableFrom(returnType.getParameterType())); + Class parameterType = returnType.nestedIfOptional().getNestedParameterType(); + return (HttpEntity.class.isAssignableFrom(parameterType) && + !RequestEntity.class.isAssignableFrom(parameterType)); } @Override @@ -150,7 +153,7 @@ public class HttpEntityMethodProcessor extends AbstractMessageConverterMethodPro @Nullable private Type getHttpEntityType(MethodParameter parameter) { - Assert.isAssignable(HttpEntity.class, parameter.getParameterType()); + Assert.isAssignable(HttpEntity.class, parameter.getNestedParameterType()); Type parameterType = parameter.getGenericParameterType(); if (parameterType instanceof ParameterizedType) { ParameterizedType type = (ParameterizedType) parameterType; @@ -169,6 +172,7 @@ public class HttpEntityMethodProcessor extends AbstractMessageConverterMethodPro } @Override + @SuppressWarnings("unchecked") public void handleReturnValue(@Nullable Object returnValue, MethodParameter returnType, ModelAndViewContainer mavContainer, NativeWebRequest webRequest) throws Exception { @@ -180,6 +184,12 @@ public class HttpEntityMethodProcessor extends AbstractMessageConverterMethodPro ServletServerHttpRequest inputMessage = createInputMessage(webRequest); ServletServerHttpResponse outputMessage = createOutputMessage(webRequest); + if (returnType.getParameterType() == Optional.class) { + Optional> optionalEntity = (Optional>) returnValue; + returnValue = optionalEntity.orElseGet(() -> new ResponseEntity<>(HttpStatus.NOT_FOUND)); + returnType = returnType.nestedIfOptional(); + } + Assert.isInstanceOf(HttpEntity.class, returnValue); HttpEntity responseEntity = (HttpEntity) returnValue; diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorMockTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorMockTests.java index c698f50a76..6ccd9e23e7 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorMockTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorMockTests.java @@ -19,7 +19,6 @@ package org.springframework.web.servlet.mvc.method.annotation; import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.InputStream; -import java.lang.reflect.Method; import java.net.URI; import java.nio.charset.StandardCharsets; import java.time.ZoneId; @@ -28,6 +27,7 @@ import java.time.temporal.ChronoUnit; import java.util.Arrays; import java.util.Collections; import java.util.Date; +import java.util.Optional; import org.hamcrest.Matchers; import org.junit.Before; @@ -56,14 +56,17 @@ import org.springframework.web.HttpMediaTypeNotAcceptableException; import org.springframework.web.HttpMediaTypeNotSupportedException; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.context.request.ServletWebRequest; +import org.springframework.web.method.ResolvableMethod; import org.springframework.web.method.support.ModelAndViewContainer; -import static java.time.Instant.*; -import static java.time.format.DateTimeFormatter.*; +import static java.time.Instant.ofEpochMilli; +import static java.time.format.DateTimeFormatter.RFC_1123_DATE_TIME; import static org.junit.Assert.*; import static org.mockito.BDDMockito.*; -import static org.springframework.http.MediaType.*; -import static org.springframework.web.servlet.HandlerMapping.*; +import static org.springframework.http.MediaType.APPLICATION_OCTET_STREAM; +import static org.springframework.http.MediaType.TEXT_PLAIN; +import static org.springframework.web.method.ResolvableMethod.on; +import static org.springframework.web.servlet.HandlerMapping.PRODUCIBLE_MEDIA_TYPES_ATTRIBUTE; /** * Test fixture for {@link HttpEntityMethodProcessor} delegating to a mock @@ -91,26 +94,6 @@ public class HttpEntityMethodProcessorMockTests { private HttpMessageConverter resourceRegionMessageConverter; - private MethodParameter paramHttpEntity; - - private MethodParameter paramRequestEntity; - - private MethodParameter paramResponseEntity; - - private MethodParameter paramInt; - - private MethodParameter returnTypeResponseEntity; - - private MethodParameter returnTypeResponseEntityProduces; - - private MethodParameter returnTypeResponseEntityResource; - - private MethodParameter returnTypeHttpEntity; - - private MethodParameter returnTypeHttpEntitySubclass; - - private MethodParameter returnTypeInt; - private ModelAndViewContainer mavContainer; private MockHttpServletRequest servletRequest; @@ -119,6 +102,12 @@ public class HttpEntityMethodProcessorMockTests { private ServletWebRequest webRequest; + private MethodParameter returnTypeResponseEntity = + on(TestHandler.class).resolveReturnType(ResponseEntity.class, String.class); + + private MethodParameter returnTypeResponseEntityResource = + on(TestHandler.class).resolveReturnType(ResponseEntity.class, Resource.class); + @Before @SuppressWarnings("unchecked") @@ -139,20 +128,6 @@ public class HttpEntityMethodProcessorMockTests { processor = new HttpEntityMethodProcessor(Arrays.asList( stringHttpMessageConverter, resourceMessageConverter, resourceRegionMessageConverter)); - Method handle1 = getClass().getMethod("handle1", HttpEntity.class, ResponseEntity.class, - Integer.TYPE, RequestEntity.class); - - paramHttpEntity = new MethodParameter(handle1, 0); - paramRequestEntity = new MethodParameter(handle1, 3); - paramResponseEntity = new MethodParameter(handle1, 1); - paramInt = new MethodParameter(handle1, 2); - returnTypeResponseEntity = new MethodParameter(handle1, -1); - returnTypeResponseEntityProduces = new MethodParameter(getClass().getMethod("handle4"), -1); - returnTypeHttpEntity = new MethodParameter(getClass().getMethod("handle2", HttpEntity.class), -1); - returnTypeHttpEntitySubclass = new MethodParameter(getClass().getMethod("handle2x", HttpEntity.class), -1); - returnTypeInt = new MethodParameter(getClass().getMethod("handle3"), -1); - returnTypeResponseEntityResource = new MethodParameter(getClass().getMethod("handle5"), -1); - mavContainer = new ModelAndViewContainer(); servletRequest = new MockHttpServletRequest("GET", "/foo"); servletResponse = new MockHttpServletResponse(); @@ -162,25 +137,39 @@ public class HttpEntityMethodProcessorMockTests { @Test public void supportsParameter() { - assertTrue("HttpEntity parameter not supported", processor.supportsParameter(paramHttpEntity)); - assertTrue("RequestEntity parameter not supported", processor.supportsParameter(paramRequestEntity)); - assertFalse("ResponseEntity parameter supported", processor.supportsParameter(paramResponseEntity)); - assertFalse("non-entity parameter supported", processor.supportsParameter(paramInt)); + ResolvableMethod method = on(TestHandler.class).named("entityParams").build(); + assertTrue("HttpEntity parameter not supported", + processor.supportsParameter(method.arg(HttpEntity.class, String.class))); + assertTrue("RequestEntity parameter not supported", + processor.supportsParameter(method.arg(RequestEntity.class, String.class))); + assertFalse("ResponseEntity parameter supported", + processor.supportsParameter(method.arg(ResponseEntity.class, String.class))); + assertFalse("non-entity parameter supported", + processor.supportsParameter(method.arg(Integer.class))); } @Test public void supportsReturnType() { - assertTrue("ResponseEntity return type not supported", processor.supportsReturnType(returnTypeResponseEntity)); - assertTrue("HttpEntity return type not supported", processor.supportsReturnType(returnTypeHttpEntity)); - assertTrue("Custom HttpEntity subclass not supported", processor.supportsReturnType(returnTypeHttpEntitySubclass)); - assertFalse("RequestEntity parameter supported", - processor.supportsReturnType(paramRequestEntity)); - assertFalse("non-ResponseBody return type supported", processor.supportsReturnType(returnTypeInt)); + assertTrue("ResponseEntity return type not supported", + processor.supportsReturnType(on(TestHandler.class).resolveReturnType(ResponseEntity.class, String.class))); + assertTrue("HttpEntity return type not supported", + processor.supportsReturnType(on(TestHandler.class).resolveReturnType(HttpEntity.class))); + assertTrue("Custom HttpEntity subclass not supported", + processor.supportsReturnType(on(TestHandler.class).resolveReturnType(CustomHttpEntity.class))); + assertTrue("Optional ResponseEntity not supported", + processor.supportsReturnType(on(TestHandler.class).resolveReturnType(Optional.class, ResponseEntity.class))); + assertFalse("RequestEntity return type supported", + processor.supportsReturnType(on(TestHandler.class) + .named("entityParams").build().arg(RequestEntity.class, String.class))); + assertFalse("non-ResponseBody return type supported", + processor.supportsReturnType(on(TestHandler.class).resolveReturnType(Integer.class))); } @Test public void shouldResolveHttpEntityArgument() throws Exception { String body = "Foo"; + MethodParameter paramHttpEntity = on(TestHandler.class).named("entityParams") + .build().arg(HttpEntity.class, String.class); MediaType contentType = TEXT_PLAIN; servletRequest.addHeader("Content-Type", contentType.toString()); @@ -199,6 +188,8 @@ public class HttpEntityMethodProcessorMockTests { @Test public void shouldResolveRequestEntityArgument() throws Exception { String body = "Foo"; + MethodParameter paramRequestEntity = on(TestHandler.class).named("entityParams") + .build().arg(RequestEntity.class, String.class); MediaType contentType = TEXT_PLAIN; servletRequest.addHeader("Content-Type", contentType.toString()); @@ -225,6 +216,8 @@ public class HttpEntityMethodProcessorMockTests { @Test public void shouldFailResolvingWhenConverterCannotRead() throws Exception { + MethodParameter paramHttpEntity = on(TestHandler.class).named("entityParams") + .build().arg(HttpEntity.class, String.class); MediaType contentType = TEXT_PLAIN; servletRequest.setMethod("POST"); servletRequest.addHeader("Content-Type", contentType.toString()); @@ -238,6 +231,8 @@ public class HttpEntityMethodProcessorMockTests { @Test public void shouldFailResolvingWhenContentTypeNotSupported() throws Exception { + MethodParameter paramHttpEntity = on(TestHandler.class).named("entityParams") + .build().arg(HttpEntity.class, String.class); servletRequest.setMethod("POST"); servletRequest.setContent("some content".getBytes(StandardCharsets.UTF_8)); this.thrown.expect(HttpMediaTypeNotSupportedException.class); @@ -260,13 +255,14 @@ public class HttpEntityMethodProcessorMockTests { @Test public void shouldHandleReturnValueWithProducibleMediaType() throws Exception { + MethodParameter returnType = on(TestHandler.class).resolveReturnType(ResponseEntity.class, CharSequence.class); String body = "Foo"; ResponseEntity returnValue = new ResponseEntity<>(body, HttpStatus.OK); servletRequest.addHeader("Accept", "text/*"); servletRequest.setAttribute(PRODUCIBLE_MEDIA_TYPES_ATTRIBUTE, Collections.singleton(MediaType.TEXT_HTML)); given(stringHttpMessageConverter.canWrite(String.class, MediaType.TEXT_HTML)).willReturn(true); - processor.handleReturnValue(returnValue, returnTypeResponseEntityProduces, mavContainer, webRequest); + processor.handleReturnValue(returnValue, returnType, mavContainer, webRequest); assertTrue(mavContainer.isRequestHandled()); verify(stringHttpMessageConverter).write(eq(body), eq(MediaType.TEXT_HTML), isA(HttpOutputMessage.class)); @@ -311,6 +307,7 @@ public class HttpEntityMethodProcessorMockTests { @Test public void shouldFailHandlingWhenConverterCannotWrite() throws Exception { + MethodParameter returnType = on(TestHandler.class).resolveReturnType(ResponseEntity.class, CharSequence.class); String body = "Foo"; ResponseEntity returnValue = new ResponseEntity<>(body, HttpStatus.OK); MediaType accepted = TEXT_PLAIN; @@ -322,7 +319,7 @@ public class HttpEntityMethodProcessorMockTests { given(stringHttpMessageConverter.canWrite(String.class, accepted)).willReturn(false); this.thrown.expect(HttpMediaTypeNotAcceptableException.class); - processor.handleReturnValue(returnValue, returnTypeResponseEntityProduces, mavContainer, webRequest); + processor.handleReturnValue(returnValue, returnType, mavContainer, webRequest); } @Test // SPR-9142 @@ -361,6 +358,16 @@ public class HttpEntityMethodProcessorMockTests { assertEquals("headerValue", outputMessage.getValue().getHeaders().get("header").get(0)); } + @Test // SPR-13281 + public void shouldReturnNotFoundWhenEmptyOptional() throws Exception { + MethodParameter returnType = on(TestHandler.class).resolveReturnType(Optional.class, ResponseEntity.class); + Optional returnValue = Optional.empty(); + + processor.handleReturnValue(returnValue, returnType, mavContainer, webRequest); + assertTrue(mavContainer.isRequestHandled()); + assertEquals(HttpStatus.NOT_FOUND.value(), servletResponse.getStatus()); + } + @Test public void shouldHandleLastModifiedWithHttp304() throws Exception { long currentTime = new Date().getTime(); @@ -697,38 +704,46 @@ public class HttpEntityMethodProcessorMockTests { } } + private static class TestHandler { - @SuppressWarnings("unused") - public ResponseEntity handle1(HttpEntity httpEntity, ResponseEntity entity, - int i, RequestEntity requestEntity) { + @SuppressWarnings("unused") + public ResponseEntity entityParams(HttpEntity httpEntity, ResponseEntity entity, + Integer i, RequestEntity requestEntity) { - return entity; - } + return entity; + } - @SuppressWarnings("unused") - public HttpEntity handle2(HttpEntity entity) { - return entity; - } + @SuppressWarnings("unused") + public HttpEntity httpEntity(HttpEntity entity) { + return entity; + } - @SuppressWarnings("unused") - public CustomHttpEntity handle2x(HttpEntity entity) { - return new CustomHttpEntity(); - } + @SuppressWarnings("unused") + public CustomHttpEntity customHttpEntity(HttpEntity entity) { + return new CustomHttpEntity(); + } - @SuppressWarnings("unused") - public int handle3() { - return 42; - } + @SuppressWarnings("unused") + public Optional handleOptionalEntity() { + return null; + } - @SuppressWarnings("unused") - @RequestMapping(produces = {"text/html", "application/xhtml+xml"}) - public ResponseEntity handle4() { - return null; - } + @SuppressWarnings("unused") + @RequestMapping(produces = {"text/html", "application/xhtml+xml"}) + public ResponseEntity produces() { + return null; + } + + @SuppressWarnings("unused") + public ResponseEntity resourceResponseEntity() { + return null; + } + + @SuppressWarnings("unused") + public Integer integer() { + return 42; + } - @SuppressWarnings("unused") - public ResponseEntity handle5() { - return null; } @SuppressWarnings("unused") diff --git a/src/docs/asciidoc/web/webflux.adoc b/src/docs/asciidoc/web/webflux.adoc index a896253cc9..73800c2084 100644 --- a/src/docs/asciidoc/web/webflux.adoc +++ b/src/docs/asciidoc/web/webflux.adoc @@ -1749,10 +1749,10 @@ controller. The following code snippet shows the usage: [TIP] ==== -Unlike the Servlet API "request paramater" concept that conflate query parameters, form +Unlike the Servlet API "request parameter" concept that conflate query parameters, form data, and multiparts into one, in WebFlux each is accessed individually through the `ServerWebExchange`. While `@RequestParam` binds to query parameters only, you can -use data binding to apply query paramerters, form data, and multiparts to a +use data binding to apply query parameters, form data, and multiparts to a <>. ==== @@ -2275,6 +2275,23 @@ on a container object that specifies request headers and body. Below is an examp } ---- +As many return values, `ResponseEntity` can be wrapped by a reactive type such as `Mono`. +In case of a `Mono` return type, the empty `Mono` case will be +automatically converted to a `ResponseEntity` with an empty body and an HTTP 404 status, +so you don't need to chain your `Mono` with an `switchIfEmpty` / `defaultIfEmpty` for simple +HTTP 404 responses. +Here's an example of this: + +[source,java,indent=0] +[subs="verbatim,quotes"] +---- +@GetMapping("/user") +public Mono> fetchUser() { + Mono user = //... + return user.map(ResponseEntity::ok); +} +---- + [[webflux-ann-jackson]] ==== Jackson JSON diff --git a/src/docs/asciidoc/web/webmvc.adoc b/src/docs/asciidoc/web/webmvc.adoc index 3b0ea7e2bf..a39d1b0a0a 100644 --- a/src/docs/asciidoc/web/webmvc.adoc +++ b/src/docs/asciidoc/web/webmvc.adoc @@ -1816,7 +1816,7 @@ supported for all return values, see below for more details. | The return value is converted through ``HttpMessageConverter``s and written to the response. See <>. -| `HttpEntity`, `ResponseEntity` +| `HttpEntity`, `ResponseEntity`, `Optional>`, `Optional>` | The return value specifies the full response including HTTP headers and body be converted through ``HttpMessageConverter``s and written to the response. See <>. @@ -2660,6 +2660,22 @@ on a container object that specifies request headers and body. Below is an examp } ---- +In case of a `java.util.Optional` return type, the `Optional.empty()` +case will be automatically converted to a `ResponseEntity` with an empty body +and an HTTP 404 status, so you don't need to chain your optional with +an `orElse` / `orElseGet` for simple HTTP 404 responses. +Here's an example of this: + +[source,java,indent=0] +[subs="verbatim,quotes"] +---- +@GetMapping("/user") +public Optional> fetchUser() { + Optional user = //... + return user.map(ResponseEntity::ok); +} +---- + [[mvc-ann-jackson]] ==== Jackson JSON