From 79c5ef88f54d025088e55c67f06479324a577a00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Deleuze?= Date: Wed, 29 Mar 2023 15:42:29 +0200 Subject: [PATCH] Refine generic type management in AbstractMessageWriterResultHandler This commit updates AbstractMessageWriterResultHandler#writeBody in order to use the declared bodyParameter instead of ResolvableType.forInstance(body) when the former has unresolvable generics. Closes gh-30215 --- spring-webflux/spring-webflux.gradle | 2 + .../AbstractMessageWriterResultHandler.java | 13 +- .../DelegatingWebFluxConfigurationTests.java | 2 +- .../WebFluxConfigurationSupportTests.java | 6 +- .../MessageWriterResultHandlerTests.java | 16 +- .../RequestMappingIntegrationTests.java | 5 +- .../KotlinMessageWriterResultHandlerTests.kt | 161 ++++++++++++++++++ 7 files changed, 195 insertions(+), 10 deletions(-) create mode 100644 spring-webflux/src/test/kotlin/org/springframework/web/reactive/result/method/annotation/KotlinMessageWriterResultHandlerTests.kt diff --git a/spring-webflux/spring-webflux.gradle b/spring-webflux/spring-webflux.gradle index b762e09c5cb..5d466280097 100644 --- a/spring-webflux/spring-webflux.gradle +++ b/spring-webflux/spring-webflux.gradle @@ -1,6 +1,7 @@ description = "Spring WebFlux" apply plugin: "kotlin" +apply plugin: "kotlinx-serialization" dependencies { api(project(":spring-beans")) @@ -45,6 +46,7 @@ dependencies { testImplementation('org.apache.httpcomponents.core5:httpcore5-reactive') testImplementation("com.squareup.okhttp3:mockwebserver") testImplementation("org.jetbrains.kotlin:kotlin-script-runtime") + testImplementation("org.jetbrains.kotlinx:kotlinx-serialization-json") testRuntimeOnly("org.jetbrains.kotlin:kotlin-scripting-jsr223") testRuntimeOnly("org.jruby:jruby") testRuntimeOnly("org.python:jython-standalone") diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/AbstractMessageWriterResultHandler.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/AbstractMessageWriterResultHandler.java index fd20d1eec38..d65ea838c34 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/AbstractMessageWriterResultHandler.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/AbstractMessageWriterResultHandler.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2021 the original author or authors. + * Copyright 2002-2023 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -139,8 +139,15 @@ public abstract class AbstractMessageWriterResultHandler extends HandlerResultHa } else { publisher = Mono.justOrEmpty(body); - actualElementType = body != null ? ResolvableType.forInstance(body) : bodyType; - elementType = (bodyType.toClass() == Object.class && body != null ? actualElementType : bodyType); + ResolvableType bodyInstanceType = ResolvableType.forInstance(body); + if (bodyType.toClass() == Object.class && body != null) { + actualElementType = bodyInstanceType; + elementType = bodyInstanceType; + } + else { + actualElementType = (body == null || bodyInstanceType.hasUnresolvableGenerics()) ? bodyType : bodyInstanceType; + elementType = bodyType; + } } if (elementType.resolve() == void.class || elementType.resolve() == Void.class) { diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/config/DelegatingWebFluxConfigurationTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/config/DelegatingWebFluxConfigurationTests.java index 6caa8c26f4b..4816ca03547 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/config/DelegatingWebFluxConfigurationTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/config/DelegatingWebFluxConfigurationTests.java @@ -112,7 +112,7 @@ public class DelegatingWebFluxConfigurationTests { boolean condition = initializer.getValidator() instanceof LocalValidatorFactoryBean; assertThat(condition).isTrue(); assertThat(initializer.getConversionService()).isSameAs(formatterRegistry.getValue()); - assertThat(codecsConfigurer.getValue().getReaders().size()).isEqualTo(14); + assertThat(codecsConfigurer.getValue().getReaders()).hasSize(15); } @Test diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/config/WebFluxConfigurationSupportTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/config/WebFluxConfigurationSupportTests.java index 059b7779e74..356f962c98f 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/config/WebFluxConfigurationSupportTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/config/WebFluxConfigurationSupportTests.java @@ -152,7 +152,7 @@ public class WebFluxConfigurationSupportTests { assertThat(adapter).isNotNull(); List> readers = adapter.getMessageReaders(); - assertThat(readers.size()).isEqualTo(14); + assertThat(readers).hasSize(15); ResolvableType multiValueMapType = forClassWithGenerics(MultiValueMap.class, String.class, String.class); @@ -207,7 +207,7 @@ public class WebFluxConfigurationSupportTests { assertThat(handler.getOrder()).isEqualTo(0); List> writers = handler.getMessageWriters(); - assertThat(writers.size()).isEqualTo(13); + assertThat(writers).hasSize(14); assertHasMessageWriter(writers, forClass(byte[].class), APPLICATION_OCTET_STREAM); assertHasMessageWriter(writers, forClass(ByteBuffer.class), APPLICATION_OCTET_STREAM); @@ -235,7 +235,7 @@ public class WebFluxConfigurationSupportTests { assertThat(handler.getOrder()).isEqualTo(100); List> writers = handler.getMessageWriters(); - assertThat(writers.size()).isEqualTo(13); + assertThat(writers).hasSize(14); assertHasMessageWriter(writers, forClass(byte[].class), APPLICATION_OCTET_STREAM); assertHasMessageWriter(writers, forClass(ByteBuffer.class), APPLICATION_OCTET_STREAM); diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/MessageWriterResultHandlerTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/MessageWriterResultHandlerTests.java index 49d12463154..c561f87a794 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/MessageWriterResultHandlerTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/MessageWriterResultHandlerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-2023 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -62,6 +62,7 @@ import static org.springframework.web.testfixture.method.ResolvableMethod.on; * Unit tests for {@link AbstractMessageWriterResultHandler}. * * @author Rossen Stoyanchev + * @author Sebastien Deleuze */ public class MessageWriterResultHandlerTests { @@ -180,6 +181,17 @@ public class MessageWriterResultHandlerTests { assertResponseBody("[{\"id\":123,\"name\":\"foo\"},{\"id\":456,\"name\":\"bar\"}]"); } + @Test + public void jacksonTypeWithSubTypeAndObjectReturnValue() { + MethodParameter returnType = on(TestController.class).resolveReturnType(Object.class); + + SimpleBean body = new SimpleBean(123L, "foo"); + this.resultHandler.writeBody(body, returnType, this.exchange).block(Duration.ofSeconds(5)); + + assertThat(this.exchange.getResponse().getHeaders().getContentType()).isEqualTo(APPLICATION_JSON); + assertResponseBody("{\"id\":123,\"name\":\"foo\"}"); + } + private void assertResponseBody(String responseBody) { StepVerifier.create(this.exchange.getResponse().getBody()) @@ -287,6 +299,8 @@ public class MessageWriterResultHandlerTests { Identifiable identifiable() { return null; } List listIdentifiable() { return null; } + + Object object() { return null; } } } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingIntegrationTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingIntegrationTests.java index f3191dbf00f..f52537e78fb 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingIntegrationTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingIntegrationTests.java @@ -47,6 +47,7 @@ import static org.assertj.core.api.Assertions.assertThat; * * @author Rossen Stoyanchev * @author Stephane Maldini + * @author Sebastien Deleuze * @since 5.0 */ class RequestMappingIntegrationTests extends AbstractRequestMappingIntegrationTests { @@ -91,8 +92,8 @@ class RequestMappingIntegrationTests extends AbstractRequestMappingIntegrationTe void stream(HttpServer httpServer) throws Exception { startServer(httpServer); - String[] expected = {"0", "1", "2", "3", "4"}; - assertThat(performGet("/stream", new HttpHeaders(), String[].class).getBody()).isEqualTo(expected); + Integer[] expected = {0, 1, 2, 3, 4}; + assertThat(performGet("/stream", new HttpHeaders(), Integer[].class).getBody()).isEqualTo(expected); } diff --git a/spring-webflux/src/test/kotlin/org/springframework/web/reactive/result/method/annotation/KotlinMessageWriterResultHandlerTests.kt b/spring-webflux/src/test/kotlin/org/springframework/web/reactive/result/method/annotation/KotlinMessageWriterResultHandlerTests.kt new file mode 100644 index 00000000000..6599d145d9c --- /dev/null +++ b/spring-webflux/src/test/kotlin/org/springframework/web/reactive/result/method/annotation/KotlinMessageWriterResultHandlerTests.kt @@ -0,0 +1,161 @@ +/* + * Copyright 2002-2023 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.web.reactive.result.method.annotation + +import kotlinx.serialization.Serializable +import org.assertj.core.api.Assertions +import org.junit.jupiter.api.Test +import org.springframework.core.ResolvableType +import org.springframework.core.io.buffer.DataBuffer +import org.springframework.http.MediaType +import org.springframework.http.ResponseEntity +import org.springframework.http.codec.EncoderHttpMessageWriter +import org.springframework.http.codec.HttpMessageWriter +import org.springframework.http.codec.json.Jackson2JsonEncoder +import org.springframework.http.codec.json.KotlinSerializationJsonEncoder +import org.springframework.util.ObjectUtils +import org.springframework.web.bind.annotation.GetMapping +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.reactive.accept.RequestedContentTypeResolverBuilder +import org.springframework.web.testfixture.http.server.reactive.MockServerHttpRequest +import org.springframework.web.testfixture.method.ResolvableMethod +import org.springframework.web.testfixture.server.MockServerWebExchange +import reactor.test.StepVerifier +import java.nio.charset.StandardCharsets +import java.time.Duration +import java.util.* + +/** + * Kotlin unit tests for {@link AbstractMessageWriterResultHandler}. + * + * @author Sebastien Deleuze + */ +class KotlinMessageWriterResultHandlerTests { + + private val resultHandler = initResultHandler() + + private val exchange = MockServerWebExchange.from(MockServerHttpRequest.get("/path")) + + + private fun initResultHandler(vararg writers: HttpMessageWriter<*>): AbstractMessageWriterResultHandler { + val writerList = if (ObjectUtils.isEmpty(writers)) { + listOf( + EncoderHttpMessageWriter(KotlinSerializationJsonEncoder()), + EncoderHttpMessageWriter(Jackson2JsonEncoder()) + ) + } else { + listOf(*writers) + } + val resolver = RequestedContentTypeResolverBuilder().build() + return object : AbstractMessageWriterResultHandler(writerList, resolver) {} + } + + @Test + fun nonSuspendWithoutResponseEntity() { + val returnType = ResolvableMethod.on(SampleController::class.java) + .resolveReturnType(List::class.java, Person::class.java) + val body = listOf(Person(UserId(1), "John")) + resultHandler.writeBody(body, returnType, exchange).block(Duration.ofSeconds(5)) + + Assertions.assertThat(exchange.response.headers.contentType).isEqualTo(MediaType.APPLICATION_JSON) + assertResponseBody("[{\"userId\":1,\"name\":\"John\"}]") + } + + @Test + fun nonSuspendWithResponseEntity() { + val returnType = ResolvableMethod.on(SampleController::class.java) + .returning(ResolvableType.forClassWithGenerics(ResponseEntity::class.java, + ResolvableType.forClassWithGenerics(List::class.java, Person::class.java))) + .build().returnType() + val body = ResponseEntity.ok(listOf(Person(UserId(1), "John"))) + resultHandler.writeBody(body.body, returnType.nested(), returnType, exchange).block(Duration.ofSeconds(5)) + + Assertions.assertThat(exchange.response.headers.contentType).isEqualTo(MediaType.APPLICATION_JSON) + assertResponseBody("[{\"userId\":1,\"name\":\"John\"}]") + } + + @Test + fun suspendWithoutResponseEntity() { + val returnType = ResolvableMethod.on(CoroutinesSampleController::class.java) + .resolveReturnType(List::class.java, Person::class.java) + val body = listOf(Person(UserId(1), "John")) + resultHandler.writeBody(body, returnType, exchange).block(Duration.ofSeconds(5)) + + Assertions.assertThat(exchange.response.headers.contentType).isEqualTo(MediaType.APPLICATION_JSON) + assertResponseBody("[{\"userId\":1,\"name\":\"John\"}]") + } + + @Test + fun suspendWithResponseEntity() { + val returnType = ResolvableMethod.on(CoroutinesSampleController::class.java) + .returning(ResolvableType.forClassWithGenerics(ResponseEntity::class.java, + ResolvableType.forClassWithGenerics(List::class.java, Person::class.java))) + .build().returnType() + val body = ResponseEntity.ok(listOf(Person(UserId(1), "John"))) + resultHandler.writeBody(body.body, returnType.nested(), returnType, exchange).block(Duration.ofSeconds(5)) + + Assertions.assertThat(exchange.response.headers.contentType).isEqualTo(MediaType.APPLICATION_JSON) + assertResponseBody("[{\"userId\":1,\"name\":\"John\"}]") + } + + private fun assertResponseBody(responseBody: String) { + StepVerifier.create(exchange.response.body) + .consumeNextWith { buf: DataBuffer -> + Assertions.assertThat( + buf.toString(StandardCharsets.UTF_8) + ).isEqualTo(responseBody) + } + .expectComplete() + .verify() + } + + @RestController + class SampleController { + + @GetMapping("/non-suspend-with-response-entity") + fun withResponseEntity(): ResponseEntity> = + TODO() + + @GetMapping("/non-suspend-without-response-entity") + fun withoutResponseEntity(): List = + TODO() + } + + @RestController + class CoroutinesSampleController { + + @GetMapping("/suspend-with-response-entity") + suspend fun suspendAndResponseEntity(): ResponseEntity> = + TODO() + + @GetMapping("/suspend-without-response-entity") + suspend fun suspendWithoutResponseEntity(): List = + TODO() + + } + + @Serializable + data class Person( + val userId: UserId, + val name: String, + ) + + @JvmInline + @Serializable + value class UserId(val id: Int) + +}