Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix controller @Body Object parameters #11267

Merged
merged 2 commits into from
Oct 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/gradle.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,6 @@ Optional<T> transform(NettyHttpRequest<?> nhr, ArgumentConversionContext<T> 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);
Expand Down Expand Up @@ -202,9 +198,8 @@ Optional<T> transform(NettyHttpRequest<?> nhr, ArgumentConversionContext<T> 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);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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) {
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -253,6 +266,14 @@ Publisher<HttpResponse<?>> emptySingleResult() {
CompletableFuture<HttpResponse<?>> 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,6 @@ public interface TypedMessageBodyReader<T> extends MessageBodyReader<T> {

@Override
default boolean isReadable(Argument<T> type, MediaType mediaType) {
return type.isAssignableFrom(getType());
return type.isAssignableFrom(getType()) && !type.getType().equals(Object.class);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this a correct fix, typed reader should only add a type and nothing else

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to suggestions. The new test cases cover the issue pretty well.

}
}
Loading