WebClient API improvements

[InfoSec812]

Recently, while using the RxJava2 implementation of WebClient, I noticed something which is not intuitive.

client.get(somePath)
    .map(HttpResponse::bodyAsJsonObject)
    .map(Future::succeededFuture)
    .onErrorReturn(Future::failedFuture)
    .subscribe(handler::handle);

When the client gets a 4XX or a 5XX response from the server, that error does NOT trigger the onErrorReturn path. I think I understand this to be because of it being generated code which is not "smart" enough to understand that a 4XX or 5XX is an error. Instead, I had to create a mapToError method and put it into the mix:

client.get(somePath)
    .map(this::mapToError)
    .map(HttpResponse::bodyAsJsonObject)
    .map(Future::succeededFuture)
    .onErrorReturn(Future::failedFuture)
    .subscribe(handler::handle);

This works, but it is not intuitive.

I propose that the documentation for the WebClient API be updated to include examples for error handling. This probably needs to be done in many other places in the Vert.x documentation as well.

[vietj]

good point, I've actually proposed a new feature for the web client "expectations" that would be something like:

client.get(path)
      .expect(200)
      ...

that produces an error if the code is not 200

there could be other expectations (around content type, etc...)

[InfoSec812]

@vietj I like the basic concept, but I'm not sure about that specific implementation. Perhaps static methods on a WebClientMappers class? Something like:

WebClientMappers.expectResponseCode(HttpResponse response, HttpResponseStatus status)
WebClientMappers.expectHeader(HttpResponse response, String headerName, String headerValue)

And then you could do something like:

client.get(somePath)
    .map(r -> WebClientMappers.expectResponse(r, INTERNAL_SERVER_ERROR)
    .map(r -> WebClientMappers.expectHeader(r, "Content-Type", APPLICATION_JSON)
    .map(HttpResponse::bodyAsJsonObject)
    .map(Future::succeededFuture)
    .onErrorReturn(Future::failedFuture)
    .subscribe(handler::handle);

[vietj]

that would be on HttpRequest itself so you use it before hte map.

your code is incorrect because it should contains an rxSend call, so it would rather be:

client.get(somePath)
      .expect(200).
      .rxSend()
      ...
      .subscribe(res -> {});

[InfoSec812]

@vietj OK, you're right. I missed the call to rxSend. It should be:

client.get(somePath)
    .rxSend()
    .map(r -> WebClientMappers.expectResponse(r, INTERNAL_SERVER_ERROR)
    .map(r -> WebClientMappers.expectHeader(r, "Content-Type", APPLICATION_JSON)
    .map(HttpResponse::bodyAsJsonObject)
    .map(Future::succeededFuture)
    .onErrorReturn(Future::failedFuture)
    .subscribe(handler::handle);

[InfoSec812]

@vietj Thinking about the approach you are suggesting, perhaps something like:

client.get(somePath)
    .expectHeader(headerName, headerValue)
    .expectStatus(OK)
    .expectBody(matchesJsonPath("$.some.json.field"))
    .rxSend()
    .map(HttpResponse::bodyAsJsonObject)
    .map(Future::succeededFuture)
    .onErrorReturn(Future::failedFuture)
    .subscribe(handler::handle);

Thoughts?

[vietj]

yes that seems fine, I did not have intention for body expectations first but why not ? it can help unit testing I guess.

[InfoSec812]

Oh, wait... On the headers... We would need to support MultiMap, right?

[vietj]

I think we can support rather per header checking, I don't think MultiMap support is needed.

[tsegismont]

Closing, now tracked by vert-x3/vertx-web#1013