Spring-cloud-netflix: ResponseEntity<Single<>> instead of Single<ResponseEntity<>>

Created on 3 Apr 2016 · 10Comments · Source: spring-cloud/spring-cloud-netflix

The new code introduced here allows the controller to return Single or Observable.

Although it has some flaws handling the http status code.

It does not honor @ResponseStatus annotations on the controller itself.
It does support ResponseEntity<Single<>> which is useless in any case where the status code depends on the reactive code, for instance: it is impossible to write a proper ResponseEntity<Single<User>> findUser() method as it will necessarily subscribe and consume the Single at the controller instead of at the response handler.
It doesn't implement support for Completable.

waiting for feedback

Source

vromero

Most helpful comment

Indeed it looks like there is no way to vary the status based on the outcome. In other words it should deal with Single<ResponseEntity<>>.

Not sure what's the issue with @ResponseStatus. That should be set regardless of the return value handler used. Perhaps it's getting overridden later when the Single completes?

rstoyanchev on 7 Apr 2016

👍3

All 10 comments

Indeed it looks like there is no way to vary the status based on the outcome. In other words it should deal with Single<ResponseEntity<>>.

Not sure what's the issue with @ResponseStatus. That should be set regardless of the return value handler used. Perhaps it's getting overridden later when the Single completes?

rstoyanchev on 7 Apr 2016

👍3

In my tests @ResponseStatus wasn't being honored although I was playing with DeferredResult<> and org.springframework.test.web.servlet.MockMvc I had problems with that combination before, so it might not be related. Let me try again and come back.

vromero on 8 Apr 2016

The annotation @ResponseStatus does work as expected. It works when instead of MockMvc I use a proper integration test.
Same results nevertheless for Single<ResponseEntity<>> and Completable (although given that is marked as @Experimental perhaps it should not necessarily be supported right now).

vromero on 10 Apr 2016

We just upgraded from M5 to RC1 and item 2 in the orginal description from @vromero is causing issues for us. In M5, we used Observable<ResponseEntity<String>> in our controller which made it possible to control the return status based on the outcome of a service call by mapping on the Observable returned from a service. In RC1, just replacing Observable with Single does not work, since the ResponseEntity inside the Single is wrapped in another ResponseEntity. Here's my test code:

Service

    public rx.Single<Optional<String>> getUserName(String id) {
        return ("none".equals(id)) ? rx.Single.just(Optional.empty()) : rx.Single.just(Optional.of(id.toUpperCase()));
    }

Controller

    @RequestMapping(method = RequestMethod.GET, value = "/user/{id}")
    public rx.Single<ResponseEntity<String>> singleTest(@PathVariable("id") String id) {
        return delegate.getUserName(id).map(uOpt -> uOpt
                .map(u -> ResponseEntity.ok().body(u))
                .orElse(ResponseEntity.status(HttpStatus.NOT_FOUND).body(null)));
    }

When requesting the resource on the existing id bob, I get a HTTP status 200 with the following JSON:

{
  "headers": {},
  "body": "BOB",
  "statusCode": "OK"
}

When requesting on the non-existing id none, I get a HTTP status 200 with the following JSON:

{
  "headers": {},
  "body": null,
  "statusCode": "NOT_FOUND"
}

The problem is that the new SingleReturnValueHandler does not handle the case where the controller return type is Single<ResponseEntity> correctly in the method convertToDeferredResult. In that case it should simply return the enclosed ResponseEntity instance instead of wrapping it in a new ResponseEntity.

tbfangel on 11 Apr 2016

+1 as mentioned returning HttpEntity<Single<>> doesn't have much use as i can't tailor my HttpEntity based on the reaction, instead have to pre-guess and rely on exception throwing/handling for alternate cases.

@jmnarloch do you agree?