Handle primitive HTTP client responses with MessageBodyReader

[kevin-wise]

Fixes issue #10698

[CLAassistant]

All committers have signed the CLA.

[sdelamo]

it misses a test

[kevin-wise]

@sdelamo I'm happy to add a test. Any guidance on the best place to put the test? Should I add a test to HttpGetSpec? Or maybe that's too high level

[kevin-wise]

So as I was writing the tests, I was initially unable to reproduce the problem. It turns out it was being masked in the tests because NettyConverters has a TypeConverter from BytBuf to Object. So when http-server-netty is loaded, the tests pass without my modifications. The tests fail when http-server-netty is not loaded though.

I do think this should be fixed though, because that TypeConverter simply uses .toString() to convert, which is a bit naive even if it might be functional for boxed primitives. Also there is no guarantee that the Netty server lib will be loaded along with the HttpClient, so we shouldn't rely on that.

I was going to put the test right next to io.micronaut.http.body.ConversionTextPlainHandlerSpec, but that Gradle module includes project(":http-server-netty") as a dependency, so it always loads NettyConverters. Is there a pattern for this kind of test, which requires a specific set of dependencies? Is that what the test-suite-* modules are for?

[kevin-wise]

@sdelamo I pushed up some tests which fail on 4.3.x and pass on my branch. As I mentioned above, I had to block NettyConverters to get it to fail. Is there a better way to block NettyConverters?

I also noticed some tests which were leaking controllers into other tests, so I cleaned those up while I was at it.

@dstepanov The approach I took of creating a mock NettyConverters meant I had to make it non-final. I see you were the one that made it final. Are you ok with that change?

[kevin-wise]

Also, any chance this can make it into 4.4?
When will the 4.4 platform bom get released?

[dstepanov]

@kevin-wise Maybe you can just use @Replaces?

[kevin-wise]

Looks like @Replaces along with GroovyMock worked with NettyConverter still final. Will post shortly.

Update: it seemed to have worked, but the input/output tests don't actually fail without the bug fix, so somehow the converters are still getting registered. I'll keep looking for some combination that works.

The error with @MockBean is

Cannot apply AOP advice to final class. Class must be made non-final to support proxying: io.micronaut.http.server.netty.converters.NettyConverters

[kevin-wise]

Ok, found a combination that worked. The solution was to mock the interface instead of the class. The interface is what we care about anyway.

    @MockBean(bean = TypeConverterRegistrar, named = "NettyConverters")
    TypeConverterRegistrar nettyConverters() {
        Mock(TypeConverterRegistrar.class)
    }

Without fix:

With fix:

[kevin-wise]

@sdelamo I noticed you changed the base branch to 4.5. Any chance this could make it in 4.4.x?

[yawkat]

imo this is sufficiently bug-fixy to make it into a patch release of core

[yawkat]

@sdelamo WDYT, 4.4.x?

[sdelamo]

I will cherry picked it and port it to 4.4.x

[kevin-wise]

Thanks!

[yawkat]

Thank you for the contribution!