From 663f88165e5e22c54992aae93c720654d621178d Mon Sep 17 00:00:00 2001 From: Jonas Konrad Date: Thu, 24 Oct 2024 12:30:30 +0200 Subject: [PATCH] Fix controller `@Body Object` parameters (#11267) There's two bugs here. Let's consider the following cases: 1. `application/x-www-form-urlencoded`, `@Body Object` param (tested by the old FormDataDiskSpec) 2. `application/json`, `@Body Object` param (prev. no test, tested by a new TCK test) 3. `application/x-weird`, `@Body Object` param (prev. no test, tested by a new netty-specific test) 4. `application/x-weird`, `@Body MyRecord` param (prev. no test, tested by a new netty-specific test) Prior to 4.6, case 2 has a registered "normal" reader (the JSON reader) that can handle this case fine, the other three cases do not have readers and rely on fallback conversion logic. Forms have some special handling here. In 4.6, the way raw type readers (e.g. that for String) were resolved changed. This means that for `Object` params, all raw type readers were now eligible, since they can read a subtype of Object (e.g. String). For cases 1, 3, and 4, this is a breaking change, since the reading is now done by a (random) raw type reader, instead of falling back to conversion logic. This change broke the FormDataDiskSpec (case 1). To fix this, 1d09d95f033fa6270b0b187c28183e63c250ae26 introduced a check that would blanket-ignore the reader for all Object-typed params, falling back to the conversion logic. This fixed cases 1 and 3, but inadvertently broke case 2, since the JSON reader was also ignored. Case 2 is apparently common enough that there have been multiple reports about it. It also exposed the second bug, a double-claim of the body, which is a fairly simple fix. But it shows that the fallback conversion logic had no test coverage, since this bug should trigger reliably. That is what `BodyConversionSpec` is there to improve. My proposed fix is to make the raw message readers not apply to Object parameters, and to remove the band-aid fix from 1d09d95f033fa6270b0b187c28183e63c250ae26 that broke case 2. Fixes #11266 --- .github/workflows/gradle.yml | 2 +- .../binders/NettyBodyAnnotationBinder.java | 7 +- .../netty/body/BodyConversionSpec.groovy | 90 +++++++++++++++++++ .../http/server/tck/tests/BodyTest.java | 21 +++++ .../http/body/TypedMessageBodyReader.java | 2 +- 5 files changed, 114 insertions(+), 8 deletions(-) create mode 100644 http-server-netty/src/test/groovy/io/micronaut/http/server/netty/body/BodyConversionSpec.groovy diff --git a/.github/workflows/gradle.yml b/.github/workflows/gradle.yml index 909b5fd20df..bec738f30ee 100644 --- a/.github/workflows/gradle.yml +++ b/.github/workflows/gradle.yml @@ -45,7 +45,7 @@ jobs: fetch-depth: 0 - name: "🔧 Setup GraalVM CE" - uses: graalvm/setup-graalvm@v1.2.3 + uses: graalvm/setup-graalvm@v1.2.4 with: distribution: 'graalvm' java-version: ${{ matrix.java }} diff --git a/http-server-netty/src/main/java/io/micronaut/http/server/netty/binders/NettyBodyAnnotationBinder.java b/http-server-netty/src/main/java/io/micronaut/http/server/netty/binders/NettyBodyAnnotationBinder.java index b54d7b7ca8f..a8a0531adb9 100644 --- a/http-server-netty/src/main/java/io/micronaut/http/server/netty/binders/NettyBodyAnnotationBinder.java +++ b/http-server-netty/src/main/java/io/micronaut/http/server/netty/binders/NettyBodyAnnotationBinder.java @@ -157,10 +157,6 @@ Optional transform(NettyHttpRequest nhr, ArgumentConversionContext cont if (mediaType != null && (reader == null || !reader.isReadable(context.getArgument(), mediaType))) { reader = bodyHandlerRegistry.findReader(context.getArgument(), List.of(mediaType)).orElse(null); } - if (reader != null && context.getArgument().getType().equals(Object.class)) { - // Prevent random object convertors - reader = null; - } if (reader == null && nhr.isFormOrMultipartData()) { FormDataHttpContentProcessor processor = new FormDataHttpContentProcessor(nhr, httpServerConfiguration); ByteBuf buf = AvailableNettyByteBody.toByteBuf(imm); @@ -202,9 +198,8 @@ Optional transform(NettyHttpRequest nhr, ArgumentConversionContext cont nhr.setLegacyBody(converted.orElse(null)); return converted; } - ByteBuffer byteBuffer = imm.toByteBuffer(); if (reader != null) { - T result = read(context, reader, nhr.getHeaders(), mediaType, byteBuffer); + T result = read(context, reader, nhr.getHeaders(), mediaType, imm.toByteBuffer()); nhr.setLegacyBody(result); return Optional.ofNullable(result); } diff --git a/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/body/BodyConversionSpec.groovy b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/body/BodyConversionSpec.groovy new file mode 100644 index 00000000000..731c87638c9 --- /dev/null +++ b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/body/BodyConversionSpec.groovy @@ -0,0 +1,90 @@ +package io.micronaut.http.server.netty.body + +import io.micronaut.context.ApplicationContext +import io.micronaut.context.annotation.Requires +import io.micronaut.core.convert.MutableConversionService +import io.micronaut.core.convert.TypeConverterRegistrar +import io.micronaut.http.HttpRequest +import io.micronaut.http.annotation.Body +import io.micronaut.http.annotation.Consumes +import io.micronaut.http.annotation.Controller +import io.micronaut.http.annotation.Post +import io.micronaut.http.client.HttpClient +import io.micronaut.runtime.server.EmbeddedServer +import io.netty.buffer.ByteBuf +import jakarta.inject.Singleton +import spock.lang.Specification + +import java.nio.charset.StandardCharsets + +class BodyConversionSpec extends Specification { + static final String WEIRD_CONTENT_TYPE = "application/x-weird" + + def 'weird content type, object param'() { + given: + def ctx = ApplicationContext.run(['spec.name': 'BodyConversionSpec']) + def server = ctx.getBean(EmbeddedServer) + server.start() + def client = ctx.createBean(HttpClient, server.URI).toBlocking() + + when: + def response = client.retrieve(HttpRequest.POST('/body-conversion/weird-object', "foo") + .contentType(WEIRD_CONTENT_TYPE), String) + then: + response == 'body: foo' + + cleanup: + client.close() + server.stop() + } + + def 'weird content type, converted param'() { + given: + def ctx = ApplicationContext.run(['spec.name': 'BodyConversionSpec']) + def server = ctx.getBean(EmbeddedServer) + server.start() + def client = ctx.createBean(HttpClient, server.URI).toBlocking() + + when: + def response = client.retrieve(HttpRequest.POST('/body-conversion/weird-converted', "foo") + .contentType(WEIRD_CONTENT_TYPE), String) + then: + response == 'body: MyRecord[s=foo]' + + cleanup: + client.close() + server.stop() + } + + @Controller("/body-conversion") + @Requires(property = "spec.name", value = "BodyConversionSpec") + static class MyCtrl { + @Consumes(WEIRD_CONTENT_TYPE) + @Post("/weird-object") + String object(@Body Object o) { + def text = ((ByteBuf) o).toString(StandardCharsets.UTF_8) + o.release() + return "body: " + text + } + + @Consumes(WEIRD_CONTENT_TYPE) + @Post("/weird-converted") + String converted(@Body MyRecord o) { + return "body: " + o + } + } + + @Singleton + @Requires(property = "spec.name", value = "BodyConversionSpec") + static class MyConverter implements TypeConverterRegistrar { + @Override + void register(MutableConversionService conversionService) { + conversionService.addConverter(ByteBuf.class, MyRecord.class, buf -> { + return new MyRecord(buf.toString(StandardCharsets.UTF_8)) + }) + } + } + + record MyRecord(String s) { + } +} diff --git a/http-server-tck/src/main/java/io/micronaut/http/server/tck/tests/BodyTest.java b/http-server-tck/src/main/java/io/micronaut/http/server/tck/tests/BodyTest.java index 8db11424f41..5833ec35aa0 100644 --- a/http-server-tck/src/main/java/io/micronaut/http/server/tck/tests/BodyTest.java +++ b/http-server-tck/src/main/java/io/micronaut/http/server/tck/tests/BodyTest.java @@ -184,6 +184,19 @@ void testRedirectFuture() throws IOException { .run(); } + @Test + void testObjectBody() throws IOException { + TestScenario.builder() + .specName(SPEC_NAME) + .request(HttpRequest.POST("/response-body/object", Map.of("fizz", "buzz"))) + .assertion((server, request) -> AssertionUtils.assertDoesNotThrow(server, request, + HttpResponseAssertion.builder() + .status(HttpStatus.CREATED) + .body("obj: {fizz=buzz}") + .build())) + .run(); + } + @Controller("/response-body") @Requires(property = "spec.name", value = SPEC_NAME) static class BodyController { @@ -253,6 +266,14 @@ Publisher> emptySingleResult() { CompletableFuture> redirectFuture() { return CompletableFuture.completedFuture(HttpResponse.status(HttpStatus.FOUND).header("Location", "https://example.com")); } + + @SuppressWarnings("DefaultAnnotationParam") + @Post(uri = "/object", consumes = MediaType.TEXT_PLAIN) + @Status(HttpStatus.CREATED) + @Consumes(MediaType.APPLICATION_JSON) + String objectBody(@Body Object obj) { + return "obj: " + obj; + } } @Introspected diff --git a/http/src/main/java/io/micronaut/http/body/TypedMessageBodyReader.java b/http/src/main/java/io/micronaut/http/body/TypedMessageBodyReader.java index c37182f260d..31ac2706f15 100644 --- a/http/src/main/java/io/micronaut/http/body/TypedMessageBodyReader.java +++ b/http/src/main/java/io/micronaut/http/body/TypedMessageBodyReader.java @@ -38,6 +38,6 @@ public interface TypedMessageBodyReader extends MessageBodyReader { @Override default boolean isReadable(Argument type, MediaType mediaType) { - return type.isAssignableFrom(getType()); + return type.isAssignableFrom(getType()) && !type.getType().equals(Object.class); } }