Skip to content

Commit

Permalink
Revert "Infer HTTP 404 from empty Optional/Publisher types"
Browse files Browse the repository at this point in the history 
This reverts commit 7e91733.
  • Loading branch information
@bclozel
bclozel committed Aug 15, 2018
1 parent 1b54d21 commit f2506ca
Show file tree
Hide file tree
Showing 6 changed files with 86 additions and 155 deletions.
5 changes: 1 addition & 4 deletions ...rg/springframework/web/reactive/result/method/annotation/ResponseEntityResultHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,6 @@ public class ResponseEntityResultHandler extends AbstractMessageWriterResultHand

private static final Set<HttpMethod> SAFE_METHODS = EnumSet.of(HttpMethod.GET, HttpMethod.HEAD);

private static final ResponseEntity<Object> notFound = new ResponseEntity<>(HttpStatus.NOT_FOUND);


/**
* Basic constructor with a default {@link ReactiveAdapterRegistry}.
Expand Down Expand Up @@ -121,8 +119,7 @@ public Mono<Void> handleResult(ServerWebExchange exchange, HandlerResult result)

if (adapter != null) {
Assert.isTrue(!adapter.isMultiValue(), "Only a single ResponseEntity supported");
returnValueMono = Mono.from(adapter.toPublisher(result.getReturnValue()))
.defaultIfEmpty(notFound);
returnValueMono = Mono.from(adapter.toPublisher(result.getReturnValue()));
bodyParameter = actualParameter.nested().nested();
}
else {
Expand Down
14 changes: 0 additions & 14 deletions ...ringframework/web/reactive/result/method/annotation/ResponseEntityResultHandlerTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -350,20 +350,6 @@ public void handleResponseEntityWithExistingResponseHeaders() throws Exception {
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) {
Expand Down
16 changes: 3 additions & 13 deletions ...java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
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;
Expand All @@ -34,7 +33,6 @@
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;
Expand Down Expand Up @@ -123,9 +121,8 @@ public boolean supportsParameter(MethodParameter parameter) {

@Override
public boolean supportsReturnType(MethodParameter returnType) {
Class<?> parameterType = returnType.nestedIfOptional().getNestedParameterType();
return (HttpEntity.class.isAssignableFrom(parameterType) &&
!RequestEntity.class.isAssignableFrom(parameterType));
return (HttpEntity.class.isAssignableFrom(returnType.getParameterType()) &&
!RequestEntity.class.isAssignableFrom(returnType.getParameterType()));
}

@Override
Expand Down Expand Up @@ -153,7 +150,7 @@ public Object resolveArgument(MethodParameter parameter, @Nullable ModelAndViewC

@Nullable
private Type getHttpEntityType(MethodParameter parameter) {
Assert.isAssignable(HttpEntity.class, parameter.getNestedParameterType());
Assert.isAssignable(HttpEntity.class, parameter.getParameterType());
Type parameterType = parameter.getGenericParameterType();
if (parameterType instanceof ParameterizedType) {
ParameterizedType type = (ParameterizedType) parameterType;
Expand All @@ -172,7 +169,6 @@ else if (parameterType instanceof Class) {
}

@Override
@SuppressWarnings("unchecked")
public void handleReturnValue(@Nullable Object returnValue, MethodParameter returnType,
ModelAndViewContainer mavContainer, NativeWebRequest webRequest) throws Exception {

Expand All @@ -184,12 +180,6 @@ public void handleReturnValue(@Nullable Object returnValue, MethodParameter retu
ServletServerHttpRequest inputMessage = createInputMessage(webRequest);
ServletServerHttpResponse outputMessage = createOutputMessage(webRequest);

if (returnType.getParameterType() == Optional.class) {
Optional<HttpEntity<?>> optionalEntity = (Optional<HttpEntity<?>>) returnValue;
returnValue = optionalEntity.orElseGet(() -> new ResponseEntity<>(HttpStatus.NOT_FOUND));
returnType = returnType.nestedIfOptional();
}

Assert.isInstanceOf(HttpEntity.class, returnValue);
HttpEntity<?> responseEntity = (HttpEntity<?>) returnValue;

Expand Down
167 changes: 76 additions & 91 deletions ...springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorMockTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
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;
Expand All @@ -27,7 +28,6 @@
import java.util.Arrays;
import java.util.Collections;
import java.util.Date;
import java.util.Optional;

import org.hamcrest.Matchers;
import org.junit.Before;
Expand Down Expand Up @@ -56,17 +56,14 @@
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.ofEpochMilli;
import static java.time.format.DateTimeFormatter.RFC_1123_DATE_TIME;
import static java.time.Instant.*;
import static java.time.format.DateTimeFormatter.*;
import static org.junit.Assert.*;
import static org.mockito.BDDMockito.*;
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;
import static org.springframework.http.MediaType.*;
import static org.springframework.web.servlet.HandlerMapping.*;

/**
* Test fixture for {@link HttpEntityMethodProcessor} delegating to a mock
Expand Down Expand Up @@ -94,6 +91,26 @@ public class HttpEntityMethodProcessorMockTests {

private HttpMessageConverter<Object> 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;
Expand All @@ -102,12 +119,6 @@ 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")
Expand All @@ -128,6 +139,20 @@ public void setup() throws Exception {
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();
Expand All @@ -137,39 +162,25 @@ public void setup() throws Exception {

@Test
public void supportsParameter() {
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)));
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));
}

@Test
public void supportsReturnType() {
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)));
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));
}

@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());
Expand All @@ -188,8 +199,6 @@ public void shouldResolveHttpEntityArgument() throws Exception {
@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());
Expand All @@ -216,8 +225,6 @@ public void shouldResolveRequestEntityArgument() throws Exception {

@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());
Expand All @@ -231,8 +238,6 @@ public void shouldFailResolvingWhenConverterCannotRead() throws Exception {

@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);
Expand All @@ -255,14 +260,13 @@ public void shouldHandleReturnValue() throws Exception {

@Test
public void shouldHandleReturnValueWithProducibleMediaType() throws Exception {
MethodParameter returnType = on(TestHandler.class).resolveReturnType(ResponseEntity.class, CharSequence.class);
String body = "Foo";
ResponseEntity<String> 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, returnType, mavContainer, webRequest);
processor.handleReturnValue(returnValue, returnTypeResponseEntityProduces, mavContainer, webRequest);

assertTrue(mavContainer.isRequestHandled());
verify(stringHttpMessageConverter).write(eq(body), eq(MediaType.TEXT_HTML), isA(HttpOutputMessage.class));
Expand Down Expand Up @@ -307,7 +311,6 @@ public void shouldFailHandlingWhenContentTypeNotSupported() throws Exception {

@Test
public void shouldFailHandlingWhenConverterCannotWrite() throws Exception {
MethodParameter returnType = on(TestHandler.class).resolveReturnType(ResponseEntity.class, CharSequence.class);
String body = "Foo";
ResponseEntity<String> returnValue = new ResponseEntity<>(body, HttpStatus.OK);
MediaType accepted = TEXT_PLAIN;
Expand All @@ -319,7 +322,7 @@ public void shouldFailHandlingWhenConverterCannotWrite() throws Exception {
given(stringHttpMessageConverter.canWrite(String.class, accepted)).willReturn(false);

this.thrown.expect(HttpMediaTypeNotAcceptableException.class);
processor.handleReturnValue(returnValue, returnType, mavContainer, webRequest);
processor.handleReturnValue(returnValue, returnTypeResponseEntityProduces, mavContainer, webRequest);
}

@Test // SPR-9142
Expand Down Expand Up @@ -358,16 +361,6 @@ public void shouldHandleResponseHeaderAndBody() throws Exception {
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<ResponseEntity> 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();
Expand Down Expand Up @@ -704,46 +697,38 @@ private void assertConditionalResponse(HttpStatus status, String body, String et
}
}

private static class TestHandler {

@SuppressWarnings("unused")
public ResponseEntity<String> entityParams(HttpEntity<String> httpEntity, ResponseEntity<String> entity,
Integer i, RequestEntity<String> requestEntity) {

return entity;
}

@SuppressWarnings("unused")
public HttpEntity<?> httpEntity(HttpEntity<?> entity) {
return entity;
}
@SuppressWarnings("unused")
public ResponseEntity<String> handle1(HttpEntity<String> httpEntity, ResponseEntity<String> entity,
int i, RequestEntity<String> requestEntity) {

@SuppressWarnings("unused")
public CustomHttpEntity customHttpEntity(HttpEntity<?> entity) {
return new CustomHttpEntity();
}
return entity;
}

@SuppressWarnings("unused")
public Optional<ResponseEntity> handleOptionalEntity() {
return null;
}
@SuppressWarnings("unused")
public HttpEntity<?> handle2(HttpEntity<?> entity) {
return entity;
}

@SuppressWarnings("unused")
@RequestMapping(produces = {"text/html", "application/xhtml+xml"})
public ResponseEntity<CharSequence> produces() {
return null;
}
@SuppressWarnings("unused")
public CustomHttpEntity handle2x(HttpEntity<?> entity) {
return new CustomHttpEntity();
}

@SuppressWarnings("unused")
public ResponseEntity<Resource> resourceResponseEntity() {
return null;
}
@SuppressWarnings("unused")
public int handle3() {
return 42;
}

@SuppressWarnings("unused")
public Integer integer() {
return 42;
}
@SuppressWarnings("unused")
@RequestMapping(produces = {"text/html", "application/xhtml+xml"})
public ResponseEntity<String> handle4() {
return null;
}

@SuppressWarnings("unused")
public ResponseEntity<Resource> handle5() {
return null;
}

@SuppressWarnings("unused")
Expand Down
Loading

0 comments on commit f2506ca

Please sign in to comment.