Brian Clozel opened SPR-17187 and commented

17871 showed that Optional<ResponseEntity> return types are not being considered by the return value handler in charge of ResponseEntity types. This means those types are treated as response bodies.

When serialized by Jackson, empty Optional are serialied as null JSON values (literal "null" in the response body).

This is existing behavior, but one could argue that an Optional<ResponseEntity> return type is not valid for a controller handler.

Several solutions here: * we could ensure that this type is handled by the HttpEntityMethodProcessor * just fail at startup time / runtime * do nothing, since this variant is not widely used by developers

---

Affects: 5.0.8

Issue Links: - #17871 ResponseEntity factory method inferring FOUND / NOT_FOUND from Optional

Referenced from: commits https://github.com/spring-projects/spring-framework/commit/432cdd7802b9d1109eb02925d45782b6c99b9e01

Comment From: spring-projects-issues

Rossen Stoyanchev commented

To continue the discussion from #17871 where Oliver Drotbohm wrote:

With ResponseEntity you express that you want to take care of the low level HTTP details: status codes, headers, etc. In that world, it's not an option to not produce a response.

I agreed with this at first, but after some further thought, given that controller methods support flexible method signatures, we don't typically flag such things as errors. Rather we try and adapt. Considering we handle the return value, then Optional\ doesn't have to mean "no response" but rather I want to customize the status and headers if there is data, but otherwise give me default handling (e.g. 404).

Consider also the case of Mono\, which by the way also applies to Spring MVC. Clearly Mono is legal and expected, but it can also be empty. In this context we seem to agree that a 404 response makes sense as a convenience. I think the same convenience argument can apply to Optional\.

Comment From: spring-projects-issues

Rossen Stoyanchev commented

A related case is ResponseEntity\ methods that return null for which I could see someone asking for consistent treatment. Currently we ignore those, and do nothing to the response. We could do the same for empty Optional\ and Mono\ but I do think those aren't the same.

When a repository returns null, one can choose to wrap it in a ResponseEntity, i.e. ignoring null values, and in that case the controller always returns a non-null value. One can also choose to check for null before creating the ResponseEntity but then they're forced to deal with an else clause. Not so for Optional and Mono where you can have map and return without having to have orElse and switchIfEmpty respectively.

So with those there is a stronger case for having a fallback as a convenience with Optional and Mono, but I also see nothing wrong with doing the same with null return values from RespnseEntity methods. It's unlikely anyone relies on that today.

Comment From: spring-projects-issues

Rossen Stoyanchev commented

This is an extensive topic..

We should also consider this as it relates to @ResponseBody methods. It does make more sense for those to leave it to the HTTP message converter/writer. There isn't a strong enough indication the controller method wants to do anything special. By contrast, when ResponseEntity is in the signature, there is a stronger case for a 404 fallback since the HttpStatus is required for a ResponseEntity to be created.

We could separately support a global flag somewhere to use 404 for null or empty values from @ResponseBody. I think that might be a nice complement next to automatic support for same with ResponseEntity.

Comment From: spring-projects-issues

Oliver Drotbohm commented

I don't think that in the realm of HTTP, something like Optional<ResponseEntity> makes sense. A response is not optional. It cannot be absent. I wouldn't want to start introducing support for that and then have to figure out what kind of artificial semantics we'd want to turn this into. A ResponseEntity returning null is a response with an empty payload / response body. That's a totally valid case from the HTTP point of view and it's completely orthogonal and different from Optional<ResponseEntity>.

We already impose quite a few constraints on the flexible method signatures. BindingResult parameters have to be placed next to ModelAttribute ones, if you use HttpServletResponse as method parameter you implicitly indicate you handle writing to the response directly and view resolution is skipped. I.e. it doesn't feel special at all to impose some constraints on what can be used in what way.

Comment From: spring-projects-issues

Brian Clozel commented

I agree with Oliver Drotbohm - this variant doesn't feel right, especially when we start to consider how to make things behave consistently with other variants, namely: * a @ResponseBody method returning null * a ResponseEntity<Optional> return type

Right now, we're inconsistent for two reasons. We're treating Optional<ResponseEntity> as @ResponseBody and serializing the ResponseEntity object as JSON or XML. Also, Jackson natively supports Optional and will happily serialize things wrapped by that type - but this is not the case with other serialization technologies.

Failing at runtime could be the right thing to do, but it is unusual for Spring MVC in that context. We could also align there with null return values from @ResponseBody methods and just leave the response as it when the optional is empty.

Comment From: spring-projects-issues

Rossen Stoyanchev commented

The placement of BindingResult is the only such rule that I know of. I did a quick search of all places where an exception is raised under org.springframework.web.servlet.mvc.method.annotation to confirm. Most are on the argument resolver side and have to do with things that don't make sense (e.g. using @RequestPart on request that isn't multiplart) or missing information (e.g. HttpEntity argument without a generic type). For BindingResult there is a specific reason, which is to avoid ambiguity in case of multiple @ModelAttribute arguments. We wouldn't have had such a rule otherwise. Especially on the return value side we never raise exceptions at runtime other than for processing errors (e.g. no matching converter for the return type).

Rejecting a method signature, as Brian also pointed out, is unusual but also without precedent as it seems. At the same time knowing that Jackson will try to serialize ResponseEntity means we can't simply let it fall through. Not minimizing the question about semantics, in controllers it's more about what you do in the controller method that determines how you arrived at a particular return type. So the question is, is there a reason why anyone would think of doing this and what do they likely want to do?

If I obtain Optional\ from an underlying service or repository, then I will map it ResponseEntity\ and from there I may choose to return Optional> directly. What is the expectation then and how should the framework handle it?

I don't think it should be rejected. It's too drastic and unnecessary. There are valid reasons why someone would do that. One option is to treat it like we do null return values from ResponseEntity methods, i.e. check explicitly for that and do nothing. However if we accept that falling back on 404 for Mono> is a good idea, it's fair to seek consistency between those two cases, both of which require an explicit (but entirely optional!) step to handle the case of missing data. After all speaking of semantics, 404 is a the HTTP response for a missing resource.

As for @ResponseBody returning null, there is no obligation there to do anything with the response status. And for ResonseEntity\, that requires explicit treatment of the null case unlike Optional and Mono where the framework could provide the default handling.

Comment From: bclozel

Closing this issue as declined.