Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ResponseEntity factory method inferring FOUND / NOT_FOUND from Optional [SPR-13281] #17871

Closed
spring-projects-issues opened this issue Jul 27, 2015 · 6 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

spring-projects-issues commented Jul 27, 2015

Thomas Darimont opened SPR-13281 and commented

It would be helpful if org.springframework.http.ResponseEntity<T> would support Optional<T> as constructor parameter since it could be used to automatically derive a HttpStatus.FOUND status if Optional.isPresent is true or HttpStatus.NOT_FOUND otherwise.

interface FooRepository extends Repository<Foo,String> {
  Optional<Foo> findById(String id);
}
@RestController
class FooController {

  @Autowired
  FooRepository repository;

    @RequestMapping("/foo/{id}")
    ResponseEntity<Foo> getById(@PathVariable("id") String id) {
      return new ResponseEntity<Foo>(repository.getById(id));
    }
}

Currently one has to write a bit more boiler-plate:

@RequestMapping("/foo/{id}")
public ResponseEntity<Foo> getById(@PathVariable("id") String id) {

   return Optional.ofNullable(repository.getById(id))
        .map(result -> new ResponseEntity<Foo>(result, HttpStatus.FOUND))
        .orElseGet(() -> new ResponseEntity<Foo>(HttpStatus.NOT_FOUND));
}

Issue Links:

Referenced from: commits 7e91733

2 votes, 8 watchers

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Jul 27, 2015

Juergen Hoeller commented

This is a fine idea but unfortunately we can only properly apply it once we go Java 8+ with the core codebase itself, since a declared Optional argument drags in a mandatory dependency on JDK 8 at runtime. Exposing Optional or Stream in our own core APIs is unfortunatelly one of the few limitations we still have with Java 8 support in Spring :-(

I'm therefore scheduling this for Spring 5.0. For the time being, a Java-8-only subclass of ResponseEntity could provide that convenience, of course... I just wouldn't want to introduce it to the core web module at this point, when 5.0 is around the corner already.

Juergen

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Aug 23, 2017

Romain Dequidt commented

Thomas Darimont, the boiler-plate example doesn't work

java.util.NoSuchElementException: No value present

while

result.get()

Try

@RequestMapping("/foo/{id}")
public ResponseEntity<Foo> getById(@PathVariable("id") String id) {
   return Optional.ofNullable(repository.getById(id))
        .map(result -> new ResponseEntity<Foo>(result, HttpStatus.FOUND))
        .orElseGet(() -> new ResponseEntity<Foo>(HttpStatus.NOT_FOUND));
}

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Aug 23, 2017

Oliver Drotbohm commented

Good catch, Romain Dequidt. I've updated the description accordingly and moved to …orElseGet(…) to avoid instance creation in case there's a value present.

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Aug 15, 2018

Oliver Drotbohm commented

I am a bit torn the the support for Optional<ResponseEntity> because it expresses something that in HTTP cannot exist: the absence of a response. With Optional<Foo> you describe a resource that either exposes a Foo or not, i.e. deriving the status codes is fine, because you do it for both sides (presence and non-presence), the user doesn't have control over anything HTTP, as she opts out of that. The method signature is all about the resource model that's going to be the representation, not the request/response.

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. So conceptually it's an invalid method signature. In case you'd like to deal with ResponseEntity I think it's fine to give the developer the responsibility to rake care of the creation of a response:

// line breaks for legibility
return repo.findById(…)
  .map(ResponseEntity::ok)
  .orElseGet(ResponseEntity::notFound())
  .build();

looks sufficiently concise and expressive at the same time to me.

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Aug 15, 2018

Brian Clozel commented

Now that Spring Data Kay supports consistently Optional<User> as repository return types, Spring web frameworks should support consistently empty values as Controller handler return types as well.

Current behavior 

// 1)
@GetMapping("/hello")
public Optional<String> hello() {
return Optional.empty();
}
// 2)
@GetMapping("/entityempty")
public ResponseEntity entityEmpty() {
return ResponseEntity.ok(Optional.empty());
}
// Both will return:
// HTTP 200 OK
// body: null

// 3)
@GetMapping("/hello")
public Optional<String> hello() {
return Optional.of("Hello Spring");
}
// 4)
@GetMapping("/entity")
public ResponseEntity entity() {
return ResponseEntity.ok(Optional.of("Hello Spring"));
}
// Both will return:
// HTTP 200 OK
// body: Hello Spring

// 5)
@GetMapping("/optionalentity")
public Optional<ResponseEntity> optionalEntity() {
Optional<String> opt = Optional.of("Spring Boot");
return opt.map(ResponseEntity::ok);
}
/*
will return HTTP 200
{
  "headers": {
    
  },
  "body": "Spring Boot",
  "statusCodeValue": 200,
  "statusCode": "OK"
}
*/

// 6)
@GetMapping("/optionalentityempty")
public Optional<ResponseEntity> optionalEntityEmpty() {
Optional<String> opt = Optional.empty();
return opt.map(ResponseEntity::ok);
}
// HTTP 200 OK
// body: null

Possible improvements

Cases 1) to 4) should behave the same to stay consistent in our programming model.

Jackson already supports Optional during the serialization process, writing the value itself if present or null if empty, which is legal even for a simple JSON value. With that in mind, we can't really map empty @ResponseBody return values to HTTP 404 without breaking existing use cases.

Also, returning a ResponseEntity<Optional<String>> means that the body is optional, but the provided status and headers information should arguably stay the same. Otherwise, the only safe option there would be to ignore all response headers in the provided entity and just issue an HTTP 404.

Cases 5) and 6) are just wrong - we could use those cases, which are aligning nicely with the Optional.map operator. We could only deal with Optional<ResponseEntity> and handle the empty case as an HTTP NOT_FOUND, aligning the happy path to regular ResponseEntity return values.

The Publisher case

The same reasoning applies to Mono and ResponseEntity<Mono> - this is already handled by Jackson and we shouldn't change that.

 Also, we could improve the Mono<ResponseEntity> use case, by returning HTTP NOT_FOUND in case of empty Mono<ResponseEntity, leaving developers to use switchIfEmpty if they want to handle this more precisely.

@GetMapping("/mono")
public Mono<ResponseEntity> mono() {
return Mono.just(ResponseEntity.ok("Spring"));
}
// HTTP 200 OK
// body: Spring

@GetMapping("/monoempty")
public Mono<ResponseEntity> monoempty() {
return Mono.empty();
}
// HTTP 200 OK
// body:

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Aug 15, 2018

Brian Clozel commented

After chatting with Oliver Drotbohm, I've reverted the initial change described in the previous comment and went with a simple static shortcut on ResponseEntity to help with this particular case.

As a separate matter, I've opened #21722 to decide whether we want to do something about the Optional<ResponseEntity> case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants