5

In my request handler, if the passed-in accountId cannot be converted to a valid ObjectId I want to catch the error and send back a meaningful message; however, doing so causes the return type to be incompatible, and I cannot figure out how to achieve this pretty trivial use case.

My code:

  @GetMapping("/{accountId}")
  public Mono<ResponseEntity<Account>> get(@PathVariable String accountId) {
      log.debug(GETTING_DATA_FOR_ACCOUNT, accountId);

      try {
        ObjectId id = new ObjectId(accountId);
        return repository.findById(id)
            .map(ResponseEntity::ok)
            .switchIfEmpty(Mono.just(ResponseEntity.notFound().build()));
      } catch (IllegalArgumentException ex) {
        log.error(MALFORMED_OBJECT_ID, accountId);
        // TODO(marco): find a way to return the custom error message. This seems to be currently
        //  impossible with the Reactive API, as using body(message) changes the return type to
        //  be incompatible (and Mono<ResponseEntity<?>> does not seem to cut it).
        return Mono.just(ResponseEntity.badRequest().build());
      }
  }

The body(T body) method changes the type of the returned Mono so that it is (assuming one just sends a String) a Mono<ResponseEntity<String>>; however, changing the method's return type to Mono<ResponseEntity<?>> does not work either:

        ...
        return Mono.just(ResponseEntity.badRequest().body(
            MALFORMED_OBJECT_ID.replace("{}", accountId)));

as it gives an "incompatible type" error on the other return statement:

error: incompatible types: Mono<ResponseEntity<Account>> cannot be converted to Mono<ResponseEntity<?>>
            .switchIfEmpty(Mono.just(ResponseEntity.notFound().build()));

Obviously, changing the return type of the method to Mono<?> would work, but the response then is the serialized JSON of the ResponseEntity which is NOT what I want.

I have also tried using the onErrorXxxx() methods, but they do not work here either, as the conversion error happens even before the Flux is computed, and I just get a "vanilla" 400 error with an empty message.

The only way I can think of working around this would be to add a message field to my Account object and return that one, but it's genuinely a horrible hack.

Toerktumlare
  • 12,548
  • 3
  • 35
  • 54
Marco Massenzio
  • 2,822
  • 1
  • 25
  • 37
  • you need to read up on the basics of reactive programming. You should not use `try/catch`, if you wish to return an error to the client should return a `Mono.error` containing a `ResponseStatusException`. I suggest reading the getting started guide in the reactor documentation. https://projectreactor.io/docs/core/release/reference/#getting-started – Toerktumlare Sep 01 '20 at 08:33

1 Answers1

2

@thomas-andolf's answer helped me figure out the actual solution.

For anyone stumbling upon this in future, here is how I actually solved the puzzle (and, yes, you still need the try/catch to intercept the error thrown by the ObjectId constructor):

  @GetMapping("/{accountId}")
  public Mono<ResponseEntity<Account>> get(@PathVariable String accountId) {
    return Mono.just(accountId)
        .map(acctId -> {
          try {
            return new ObjectId(accountId);
          } catch (IllegalArgumentException ex) {
            throw new ResponseStatusException(HttpStatus.BAD_REQUEST,
                MALFORMED_OBJECT_ID));
          }
        })
        .flatMap(repository::findById)
        .map(ResponseEntity::ok)
        .switchIfEmpty(Mono.just(ResponseEntity.notFound().build()));
  }

To actually see the message in the returned body, you will need to add server.error.include-message=always in application.properties (see here).

Using onError() won't work here (I did try that, in all its variants) as it requires a Mono<ResponseEntity<Account>> and there is no way to generate one from the error status (when adding the message body).

Marco Massenzio
  • 2,822
  • 1
  • 25
  • 37
  • Not sure why someone down-voted this, I literally tried the code in the "accepted" answer and it doesn't compile (nor it does what I intended to do in my original question). This one compiles and does return the error message to the caller. – Marco Massenzio Sep 20 '20 at 06:58
  • I had the same issue and your question helped to solve mine (https://stackoverflow.com/a/67413913/2096986). your solution with try/catch works although it is not using the functional style which is the best choice since you are using reactive programming. – Felipe May 06 '21 at 07:57
  • 1
    glad it helped, @Felipe - if it did, please do upvote it, so it will help others: someone (who didn't even bother to comment and explain themselves) down-voted it, so it shows lower than the (upvoted, but incorrect) one. As I mention also above, the `try/catch` is *necessary* here because the library method I'm calling does not use functional/reactive style: that method is given and cannot be modified; _not_ using a `try/catch` here causes the exception to propagate and breaks the reactive functional chain. – Marco Massenzio May 14 '21 at 07:20