search

micronaut-projects / micronaut-core

Micronaut Application Framework
http://micronaut.io
Apache License 2.0
6.04k stars 1.06k forks source link

HTTP client behavior doesn't change with the presence of `micronaut.http.client.exception-on-error-status` set to `false` #7253

Closed x80486 closed 3 weeks ago

x80486 commented 2 years ago

Expected Behavior

HTTP client shouldn't throw io.micronaut.http.client.exceptions.HttpClientResponseException: Not Found.

Actual Behaviour

It does throw :sob:

If I have this setting in the application's configuration (say: application-test.yaml) file:

micronaut:
  http:
    client:
      exception-on-error-status: false

And a test case like this:

@MicronautTest(environments = [Environment.TEST])
internal class ControllerTest {
  @Inject
  @field:Client("/api/resource")
  private lateinit var client: HttpClient

  @Test
  fun get_WhenRecordDoesNotExist() {
    val id = UUID.randomUUID()
    val request = HttpRequest.GET<Entity>("/$id").accept(MediaType.APPLICATION_JSON_TYPE)
    val response = client.toBlocking().exchange(request, String::class.java)
    Assertions.assertThat(response.status).isEqualTo(HttpStatus.NOT_FOUND)
  }
}

I should be able to check the response and everything else, without the need to handle the exception.

More info in this StackOverflow question.

Steps To Reproduce

  1. Have a test case that checks for HTTP 404 (Not Found) response — without handling exceptions
  2. Configure the HTTP client properly for this — according to this pull request
  3. Run the test

Environment Information

Example Application

N/A

Version

3.4.2

jameskleeh commented 2 years ago

Please upload an example application that reproduces the issue

x80486 commented 2 years ago

Find the example on this repository: https://github.com/x80486/kotlin-micronaut

./gradlew clean check will do it. There is only one test failing:

CustomerControllerTest > get_WhenRecordDoesNotExist() STANDARD_OUT
    2022-04-27 07:06:31,067 |- DEBUG in i.m.h.c.netty.DefaultHttpClient:2820 [default-nioEventLoopGroup-1-2] - Sending HTTP GET to http://localhost:41019/api/customers/707ccdfb-b39d-45db-9c66-618894fecc47
    2022-04-27 07:06:31,068 |- TRACE in i.m.h.c.netty.DefaultHttpClient:2859 [default-nioEventLoopGroup-1-2] - Accept: application/json
    2022-04-27 07:06:31,068 |- TRACE in i.m.h.c.netty.DefaultHttpClient:2859 [default-nioEventLoopGroup-1-2] - host: localhost:41019
    2022-04-27 07:06:31,068 |- TRACE in i.m.h.c.netty.DefaultHttpClient:2859 [default-nioEventLoopGroup-1-2] - connection: close
    2022-04-27 07:06:31,182 |-  INFO in org.acme.api.CustomerController:27 [io-executor-thread-2] - Retrieving customer by identifier [707ccdfb-b39d-45db-9c66-618894fecc47]
    2022-04-27 07:06:31,466 |- DEBUG in i.m.h.c.netty.DefaultHttpClient:2423 [default-nioEventLoopGroup-1-2] - Received response 404 from http://localhost:41019/api/customers/707ccdfb-b39d-45db-9c66-618894fecc47
    2022-04-27 07:06:31,466 |- TRACE in i.m.h.c.netty.DefaultHttpClient:2859 [default-nioEventLoopGroup-1-2] - Content-Type: application/json
    2022-04-27 07:06:31,466 |- TRACE in i.m.h.c.netty.DefaultHttpClient:2859 [default-nioEventLoopGroup-1-2] - date: Wed, 27 Apr 2022 11:06:31 GMT
    2022-04-27 07:06:31,466 |- TRACE in i.m.h.c.netty.DefaultHttpClient:2859 [default-nioEventLoopGroup-1-2] - connection: close
    2022-04-27 07:06:31,466 |- TRACE in i.m.h.c.netty.DefaultHttpClient:2859 [default-nioEventLoopGroup-1-2] - content-length: 176
    2022-04-27 07:06:31,466 |- TRACE in i.m.h.c.netty.DefaultHttpClient:2838 [default-nioEventLoopGroup-1-2] - Response Body
    2022-04-27 07:06:31,466 |- TRACE in i.m.h.c.netty.DefaultHttpClient:2839 [default-nioEventLoopGroup-1-2] - ----
    2022-04-27 07:06:31,466 |- TRACE in i.m.h.c.netty.DefaultHttpClient:2840 [default-nioEventLoopGroup-1-2] - {"message":"Not Found","_links":{"self":{"href":"/api/customers/707ccdfb-b39d-45db-9c66-618894fecc47","templated":false}},"_embedded":{"errors":[{"message":"Page Not Found"}]}}
    2022-04-27 07:06:31,467 |- TRACE in i.m.h.c.netty.DefaultHttpClient:2841 [default-nioEventLoopGroup-1-2] - ----

CustomerControllerTest > get_WhenRecordDoesNotExist() FAILED
    io.micronaut.http.client.exceptions.HttpClientResponseException: Not Found
        at app//io.micronaut.http.client.netty.DefaultHttpClient$11.channelReadInstrumented(DefaultHttpClient.java:2493)
        at app//io.micronaut.http.client.netty.DefaultHttpClient$11.channelReadInstrumented(DefaultHttpClient.java:2389)
        at app//io.micronaut.http.client.netty.DefaultHttpClient$SimpleChannelInboundHandlerInstrumented.channelRead0(DefaultHttpClient.java:3145)
        at app//io.netty.channel.SimpleChannelInboundHandler.channelRead(SimpleChannelInboundHandler.java:99)
        at app//io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
        at app//io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
        at app//io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
        at app//io.micronaut.http.netty.stream.HttpStreamsHandler.channelRead(HttpStreamsHandler.java:215)
        at app//io.micronaut.http.netty.stream.HttpStreamsClientHandler.channelRead(HttpStreamsClientHandler.java:180)
        at app//io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
        at app//io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
        at app//io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
        at app//io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:103)
        at app//io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
        at app//io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
        at app//io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
        at app//io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:103)
        at app//io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
        at app//io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
        at app//io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
        at app//io.netty.channel.CombinedChannelDuplexHandler$DelegatingChannelHandlerContext.fireChannelRead(CombinedChannelDuplexHandler.java:436)
        at app//io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:327)
        at app//io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:299)
        at app//io.netty.channel.CombinedChannelDuplexHandler.channelRead(CombinedChannelDuplexHandler.java:251)
        at app//io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
        at app//io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
        at app//io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
        at app//io.netty.handler.timeout.IdleStateHandler.channelRead(IdleStateHandler.java:286)
        at app//io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
        at app//io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
        at app//io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
        at app//io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410)
        at app//io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
        at app//io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
        at app//io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919)
        at app//io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:166)
        at app//io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:722)
        at app//io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:658)
        at app//io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:584)
        at app//io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:496)
        at app//io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:986)
        at app//io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
        at app//io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
        at java.base@11.0.13/java.lang.Thread.run(Thread.java:829)
yawkat commented 2 years ago

ok it turns out that exception-on-error-status only matters if the error type is the same as the body type, and the error type is JsonError by default. if you specify it explicitly to be String, it works: client.toBlocking().exchange(request, Argument.STRING, Argument.STRING)

It makes sense imo (no reason to assume the deserialized type for normal body and error match, otherwise) but i dont think it's documented.

(also the test case has another bug, it should be contains not containsExactly, but when that's fixed as well, the test passes with the above exchange call)

jehrenzweig-leagueapps commented 2 years ago

Yep, running into this problem right now. When I attempt to use Micronaut's HttpClient class to make calls to the following controller method signature:

@Schema(implementation = PotatoTypeResponse::class)
@Get(uri = "/potato-type/name/{potatoTypeName}")
open fun getPotatoTypeByName(@PathVariable potatoTypeName: String): HttpResponse<PotatoTypeResponse> {
    // ...
}

The following code in a unit test throws a HttpClientResponseException, instead of returning an HttpResponse<T> object with a 422 status:

val httpRequest = HttpRequest.GET<Any>("potato-type/name/Walrus")
val httpResponse = httpClient.toBlocking().exchange(httpRequest, Argument.of(PotatoTypeResponse::class.java))

Even though the following settings are defined in both application.yml and application-test.yml:

micronaut:
  http:
    client:
      exception-on-error-status: false
    services:
      exception-on-error-status: false
jehrenzweig-leagueapps commented 2 years ago

ok it turns out that exception-on-error-status only matters if the error type is the same as the body type, and the error type is JsonError by default. if you specify it explicitly to be String, it works: client.toBlocking().exchange(request, Argument.STRING, Argument.STRING)

It makes sense imo (no reason to assume the deserialized type for normal body and error match, otherwise) but i dont think it's documented.

(also the test case has another bug, it should be contains not containsExactly, but when that's fixed as well, the test passes with the above exchange call)

The exception-on-error-status setting is conditionally applied in DefaultHttpClient.java based on the body type:

boolean convertBodyWithBodyType = statusCode < 400 ||
    (!DefaultHttpClient.this.configuration.isExceptionOnErrorStatus() && bodyType.equalsType(errorType));

Shouldn't the setting be applied regardless of body type, like so?

boolean convertBodyWithBodyType = statusCode < 400 ||
    !DefaultHttpClient.this.configuration.isExceptionOnErrorStatus();
yawkat commented 2 years ago

No, that can't work, because it might yield a body that is of the wrong type (bodyType, when it should parse as errorType). I think the current behavior is correct, just confusing.

jehrenzweig-leagueapps commented 2 years ago

Fair enough; I've switched to using Methanol instead of Micronaut's HttpClient for now to make HTTP requests, since that doesn't require wrapping code in try/catch blocks to handle >= 400 status codes.

x80486 commented 2 years ago

That's one reason I can't fully understand. The setting exception-on-error-status reads quite global, so I shouldn't expect any exceptions whatsoever — I'm talking from the user-side.

The mix around exceptions (Java concept) and just receiving certain result via HTTP is a little bit awkward.

yawkat commented 2 years ago

Yes the setting name is confusing, but I don't think it can work differently in a sensible way

MT-Jacobs commented 2 years ago

Would it be possible (albeit a breaking change) to resolve this issue by storing the success body in one field and the error body in another field of the HttpClient?

It seems like a common use case where your controller method is typed to return an HttpResponse<SomeResponse>, but if you run into a validation error, your method winds up returning HttpResponse<JsonError> before actually entering the controller method.

MT-Jacobs commented 2 years ago

Funny enough, there is a version of HttpClient#exchange that takes both a body type and error type:

https://github.com/micronaut-projects/micronaut-core/blob/08f3e7aa5ae400c1afc287b3a1dc47e7ecd4249a/http-client/src/main/java/io/micronaut/http/client/netty/DefaultHttpClient.java#L837-L843

so it surprises me that if exception-on-error-status is set to false we can't return a response with a null body & filled exception field.

yawkat commented 2 years ago

there is no exception field in HttpResponse, not sure what you mean.

You can get the response with the error type from the HttpClientResponseException.

johncurrier commented 1 year ago

This makes no sense to me. I should be able to get the HttpResponse<Foo> from httpClient.exchange(request, Foo.class) without exceptions being thrown. Then I can query the HttpResponse to determine if it was successful or not. That's how other frameworks work.

body(), of course, will be null. Why in the world would anyone try to map the failure into body? If the text of the failing response is critical it can be stored in another portion of HttpResponse.

Note that @yawkat suggested looking for details in HttpClientResponseException. I guess we're expected to walk the exception cause chain to find that when the exceptions are thrown in other threads? Nothing about this is clean or remotely obvious.

johncurrier commented 1 year ago

I came up with a workaround for this. Note that in my scenario everything eventually gets turned into a CompletableFuture, so this will obviously need to be tweaked for different approaches. I put this in an HttpClientUtils utility class:

/**
 * This deals with the issue where Micronaut's HttpClient throws an exception instead of returning
 * an {@link HttpResponse} when it detects an "exceptional" status code.<p/>
 *
 * Pass this to {@link CompletableFuture#exceptionally(Function)} on an HttpClient's response
 * to have those mapped to an {@link HttpResponse}.
 *
 * @see https://github.com/micronaut-projects/micronaut-core/issues/7253
 */
@SuppressWarnings("unchecked")
public static <T> Function<Throwable, HttpResponse<T>> exceptionToResponseMapper() {
    return exc -> {
        if (exc instanceof HttpClientResponseException) {
            HttpClientResponseException respExc = (HttpClientResponseException)exc;

            return (HttpResponse<T>)respExc.getResponse();
        }

        throw new RuntimeException(exc);
    };
}

Example usage:

    HttpRequest<?> request = HttpRequest.GET(uri).accept(MediaType.APPLICATION_JSON);

    return Mono.from(httpClient.exchange(request, Foo.class)).toFuture()
            .exceptionally(HttpClientUtils.exceptionToResponseMapper());

Edit: warning: you have to cast the HttpResponse and not create a new one if you're running an AWS Lambda behind API Gateway. If you try to create a new one (via HttpResponse.status(blah)) you'll somehow end up getting the io.micronaut.function.aws.proxy.MicronautAwsProxyResponse singleton and really screw things up. I love working with Microanut, but at times it seems to be incredibly fragile.

autocast commented 6 months ago

This makes no sense to me. I should be able to get the HttpResponse<Foo> from httpClient.exchange(request, Foo.class) without exceptions being thrown. Then I can query the HttpResponse to determine if it was successful or not. That's how other frameworks work.

body(), of course, will be null. Why in the world would anyone try to map the failure into body? If the text of the failing response is critical it can be stored in another portion of HttpResponse.

Note that @yawkat suggested looking for details in HttpClientResponseException. I guess we're expected to walk the exception cause chain to find that when the exceptions are thrown in other threads? Nothing about this is clean or remotely obvious.

I agree. This is how I expected it to work, but I am facing the same problem.