spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.76k stars 38.16k forks source link

Kotlin Serialization support does not take null-safety into account #33016

Closed fkneier-bikeleasing closed 4 months ago

fkneier-bikeleasing commented 5 months ago

The json deserialization is broken if kotlinx-serialization-json is in classpath. It seems that type nullability information is lost when using KotlinSerializationStringDecoder.

@RestController
@SpringBootApplication
open class Application {

    @PostMapping
    fun get(
        @RequestBody body: Map<String, String?>,
    ) = run {
        body
    }

}
POST http://localhost:8080
Content-Type: application/json

{
  "value": null
}
kotlinx.serialization.json.internal.JsonDecodingException: Unexpected JSON token at offset 13: Expected string literal but 'null' literal was found at path: $['value']
Use 'coerceInputValues = true' in 'Json {}' builder to coerce nulls if property has a default value.
JSON input: {
  "value": null
}
    at kotlinx.serialization.json.internal.JsonExceptionsKt.JsonDecodingException(JsonExceptions.kt:24) ~[kotlinx-serialization-json-jvm-1.7.0.jar:na]
    Suppressed: reactor.core.publisher.FluxOnAssembly$OnAssemblyException: 
Error has been observed at the following site(s):
    *__checkpoint ⇢ HTTP POST "/" [ExceptionHandlingWebHandler]
Original Stack Trace:
        at kotlinx.serialization.json.internal.JsonExceptionsKt.JsonDecodingException(JsonExceptions.kt:24) ~[kotlinx-serialization-json-jvm-1.7.0.jar:na]
        at kotlinx.serialization.json.internal.JsonExceptionsKt.JsonDecodingException(JsonExceptions.kt:32) ~[kotlinx-serialization-json-jvm-1.7.0.jar:na]
        at kotlinx.serialization.json.internal.AbstractJsonLexer.fail(AbstractJsonLexer.kt:580) ~[kotlinx-serialization-json-jvm-1.7.0.jar:na]
        at kotlinx.serialization.json.internal.AbstractJsonLexer.unexpectedToken(AbstractJsonLexer.kt:221) ~[kotlinx-serialization-json-jvm-1.7.0.jar:na]
        at kotlinx.serialization.json.internal.StringJsonLexer.consumeNextToken(StringJsonLexer.kt:76) ~[kotlinx-serialization-json-jvm-1.7.0.jar:na]
        at kotlinx.serialization.json.internal.StringJsonLexer.consumeKeyString(StringJsonLexer.kt:88) ~[kotlinx-serialization-json-jvm-1.7.0.jar:na]
        at kotlinx.serialization.json.internal.AbstractJsonLexer.consumeString(AbstractJsonLexer.kt:365) ~[kotlinx-serialization-json-jvm-1.7.0.jar:na]
        at kotlinx.serialization.json.internal.StreamingJsonDecoder.decodeString(StreamingJsonDecoder.kt:339) ~[kotlinx-serialization-json-jvm-1.7.0.jar:na]
        at kotlinx.serialization.internal.StringSerializer.deserialize(Primitives.kt:160) ~[kotlinx-serialization-core-jvm-1.7.0.jar:1.7.0]
        at kotlinx.serialization.internal.StringSerializer.deserialize(Primitives.kt:156) ~[kotlinx-serialization-core-jvm-1.7.0.jar:1.7.0]
        at kotlinx.serialization.json.internal.StreamingJsonDecoder.decodeSerializableValue(StreamingJsonDecoder.kt:69) ~[kotlinx-serialization-json-jvm-1.7.0.jar:na]
        at kotlinx.serialization.encoding.AbstractDecoder.decodeSerializableValue(AbstractDecoder.kt:43) ~[kotlinx-serialization-core-jvm-1.7.0.jar:1.7.0]
        at kotlinx.serialization.encoding.AbstractDecoder.decodeSerializableElement(AbstractDecoder.kt:70) ~[kotlinx-serialization-core-jvm-1.7.0.jar:1.7.0]
        at kotlinx.serialization.json.internal.StreamingJsonDecoder.decodeSerializableElement(StreamingJsonDecoder.kt:168) ~[kotlinx-serialization-json-jvm-1.7.0.jar:na]
        at kotlinx.serialization.encoding.CompositeDecoder$DefaultImpls.decodeSerializableElement$default(Decoding.kt:538) ~[kotlinx-serialization-core-jvm-1.7.0.jar:1.7.0]
        at kotlinx.serialization.internal.MapLikeSerializer.readElement(CollectionSerializers.kt:111) ~[kotlinx-serialization-core-jvm-1.7.0.jar:1.7.0]
        at kotlinx.serialization.internal.MapLikeSerializer.readElement(CollectionSerializers.kt:84) ~[kotlinx-serialization-core-jvm-1.7.0.jar:1.7.0]
        at kotlinx.serialization.internal.AbstractCollectionSerializer.readElement$default(CollectionSerializers.kt:51) ~[kotlinx-serialization-core-jvm-1.7.0.jar:1.7.0]
        at kotlinx.serialization.internal.AbstractCollectionSerializer.merge(CollectionSerializers.kt:36) ~[kotlinx-serialization-core-jvm-1.7.0.jar:1.7.0]
        at kotlinx.serialization.internal.AbstractCollectionSerializer.deserialize(CollectionSerializers.kt:43) ~[kotlinx-serialization-core-jvm-1.7.0.jar:1.7.0]
        at kotlinx.serialization.json.internal.StreamingJsonDecoder.decodeSerializableValue(StreamingJsonDecoder.kt:69) ~[kotlinx-serialization-json-jvm-1.7.0.jar:na]
        at kotlinx.serialization.json.Json.decodeFromString(Json.kt:165) ~[kotlinx-serialization-json-jvm-1.7.0.jar:na]
        at org.springframework.http.codec.KotlinSerializationStringDecoder.lambda$decodeToMono$2(KotlinSerializationStringDecoder.java:118) ~[spring-web-6.1.8.jar:6.1.8]
        at reactor.core.publisher.FluxMapFuseable$MapFuseableSubscriber.onNext(FluxMapFuseable.java:113) ~[reactor-core-3.6.6.jar:3.6.6]
        at reactor.core.publisher.FluxMapFuseable$MapFuseableSubscriber.onNext(FluxMapFuseable.java:129) ~[reactor-core-3.6.6.jar:3.6.6]
        at reactor.core.publisher.FluxContextWrite$ContextWriteSubscriber.onNext(FluxContextWrite.java:107) ~[reactor-core-3.6.6.jar:3.6.6]
        at reactor.core.publisher.FluxMapFuseable$MapFuseableConditionalSubscriber.onNext(FluxMapFuseable.java:299) ~[reactor-core-3.6.6.jar:3.6.6]
        at reactor.core.publisher.FluxFilterFuseable$FilterFuseableConditionalSubscriber.onNext(FluxFilterFuseable.java:337) ~[reactor-core-3.6.6.jar:3.6.6]
        at reactor.core.publisher.Operators$BaseFluxToMonoOperator.completePossiblyEmpty(Operators.java:2097) ~[reactor-core-3.6.6.jar:3.6.6]
        at reactor.core.publisher.MonoCollect$CollectSubscriber.onComplete(MonoCollect.java:145) ~[reactor-core-3.6.6.jar:3.6.6]
        at reactor.core.publisher.FluxMap$MapSubscriber.onComplete(FluxMap.java:144) ~[reactor-core-3.6.6.jar:3.6.6]
        at reactor.core.publisher.FluxPeek$PeekSubscriber.onComplete(FluxPeek.java:260) ~[reactor-core-3.6.6.jar:3.6.6]
        at reactor.core.publisher.FluxMap$MapSubscriber.onComplete(FluxMap.java:144) ~[reactor-core-3.6.6.jar:3.6.6]
        at reactor.netty.channel.FluxReceive.onInboundComplete(FluxReceive.java:415) ~[reactor-netty-core-1.1.19.jar:1.1.19]
        at reactor.netty.channel.ChannelOperations.onInboundComplete(ChannelOperations.java:446) ~[reactor-netty-core-1.1.19.jar:1.1.19]
        at reactor.netty.http.server.HttpServerOperations.onInboundNext(HttpServerOperations.java:689) ~[reactor-netty-http-1.1.19.jar:1.1.19]
        at reactor.netty.channel.ChannelOperationsHandler.channelRead(ChannelOperationsHandler.java:114) ~[reactor-netty-core-1.1.19.jar:1.1.19]
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:444) ~[netty-transport-4.1.109.Final.jar:4.1.109.Final]
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) ~[netty-transport-4.1.109.Final.jar:4.1.109.Final]
        at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412) ~[netty-transport-4.1.109.Final.jar:4.1.109.Final]
        at reactor.netty.http.server.HttpTrafficHandler.channelRead(HttpTrafficHandler.java:286) ~[reactor-netty-http-1.1.19.jar:1.1.19]
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:442) ~[netty-transport-4.1.109.Final.jar:4.1.109.Final]
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) ~[netty-transport-4.1.109.Final.jar:4.1.109.Final]
        at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412) ~[netty-transport-4.1.109.Final.jar:4.1.109.Final]
        at io.netty.channel.CombinedChannelDuplexHandler$DelegatingChannelHandlerContext.fireChannelRead(CombinedChannelDuplexHandler.java:436) ~[netty-transport-4.1.109.Final.jar:4.1.109.Final]
        at io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:346) ~[netty-codec-4.1.109.Final.jar:4.1.109.Final]
        at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:318) ~[netty-codec-4.1.109.Final.jar:4.1.109.Final]
        at io.netty.channel.CombinedChannelDuplexHandler.channelRead(CombinedChannelDuplexHandler.java:251) ~[netty-transport-4.1.109.Final.jar:4.1.109.Final]
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:442) ~[netty-transport-4.1.109.Final.jar:4.1.109.Final]
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) ~[netty-transport-4.1.109.Final.jar:4.1.109.Final]
        at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412) ~[netty-transport-4.1.109.Final.jar:4.1.109.Final]
        at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410) ~[netty-transport-4.1.109.Final.jar:4.1.109.Final]
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:440) ~[netty-transport-4.1.109.Final.jar:4.1.109.Final]
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) ~[netty-transport-4.1.109.Final.jar:4.1.109.Final]
        at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919) ~[netty-transport-4.1.109.Final.jar:4.1.109.Final]
        at io.netty.channel.epoll.AbstractEpollStreamChannel$EpollStreamUnsafe.epollInReady(AbstractEpollStreamChannel.java:801) ~[netty-transport-classes-epoll-4.1.109.Final.jar:4.1.109.Final]
        at io.netty.channel.epoll.EpollEventLoop.processReady(EpollEventLoop.java:501) ~[netty-transport-classes-epoll-4.1.109.Final.jar:4.1.109.Final]
        at io.netty.channel.epoll.EpollEventLoop.run(EpollEventLoop.java:399) ~[netty-transport-classes-epoll-4.1.109.Final.jar:4.1.109.Final]
        at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997) ~[netty-common-4.1.109.Final.jar:4.1.109.Final]
        at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74) ~[netty-common-4.1.109.Final.jar:4.1.109.Final]
        at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) ~[netty-common-4.1.109.Final.jar:4.1.109.Final]
        at java.base/java.lang.Thread.run(Thread.java:1583) ~[na:na]
fkneier-bikeleasing commented 5 months ago

This really is a major problem since it can not be disabled easily. But for now I found this hacky workaround:

    @Bean
    fun disableKotlinSerializationJsonCustomizer() = CodecCustomizer {
        if (ClassUtils.isPresent("kotlinx.serialization.json.Json", javaClass.classLoader)) {
            it.defaultCodecs().kotlinSerializationJsonDecoder(
                object : KotlinSerializationJsonDecoder() {
                    override fun canDecode(elementType: ResolvableType, mimeType: MimeType?): Boolean = false
                }
            )
            it.defaultCodecs().kotlinSerializationJsonEncoder(
                object : KotlinSerializationJsonEncoder() {
                    override fun canEncode(elementType: ResolvableType, mimeType: MimeType?): Boolean = false
                }
            )
        }
    }
sdeleuze commented 5 months ago

Kotlin Serialization does not work exactly like Jackson, so I suspect what you are looking for is to disable the default codec registration which on purpose uses Kotlin Serialization when it is on the classpath, and just enable the ones you want like Jackson and potentially a few others. If you need more help, Stack Overflow is probably a better place to ask, since we would like to keep the bug tracker focused on Spring Framework issues.

As a consequence, I close this issue as invalid. If you think otherwise, please describe the Spring Framework issue in detail. Just please take in account that automatic registration of Kotlin Serialization which takes precedence over Jackson when present on the classpath is the expected behavior and is not a bug.

fkneier-bikeleasing commented 5 months ago

In certain cases Kotlin Serialization does not work at all. It is broken, which is clearly stated in the exception. I'm actually stunned that this ticket has been closed as invalid. This is not a matter of preference.

Either the type resolution has to be fixed (which DOES NOT correctly resolve nullability) or Kotlin Serialization must not be used in these cases (which DOES check nullability).

But currently a controller method with a valid body parameter has become unusable when Kotlin Serialization is used instead of Jackson, which is the default handling if Kotlin Serialization is found in the classpath.

sdeleuze commented 5 months ago

Kotlin Serialization sometimes by design works differently (for example you have to annotate the classes with @Serializable) so I thought initially that was just another difference, but after another look it looks like indeed that we are indeed losing the null-safety information probably before ResolvableType don't have this information. A related hint could be pass via AbstractMessageReaderArgumentResolver#readBody(MethodParameter, MethodParameter, boolean, BindingContext, ServerWebExchange) but it is not obvious how to do that in a non intrusive way. I need to discuss that with the team.

sdeleuze commented 5 months ago

ResolvableType#getSource should allow to get the KType with the relevant null-safety information on WebFlux. For WebMVC, that's less obvious.

fkneier-bikeleasing commented 5 months ago

Isn't this considered a bug? Maybe even a serious one? I think it breaks existing code just because of the existence of a dependency and there even isn't an easy workaround.

sdeleuze commented 5 months ago

If you do not want to use Kotlin Serialization, you can easily bring back Jackson as explain above.

sdeleuze commented 5 months ago

Depends on #33118.