search

spring-projects / spring-framework

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

Web controller call with invalid body resulting in 500 instead of 400 when using kotlinx-serialization #33138

Closed bcmedeiros closed 2 months ago

bcmedeiros commented 3 months ago

Consider the following controller:

import kotlinx.serialization.Serializable
import org.springframework.web.bind.annotation.PostMapping
import org.springframework.web.bind.annotation.RequestBody
import org.springframework.web.bind.annotation.RestController

@RestController
class TestController {
    @PostMapping("/test")
    fun test(
        @RequestBody body: Body
    ): String {
        return body.message
    }
}

@Serializable
data class Body(val message: String)

If I run curl -X POST --location http://localhost:8080/test -H "Content-Type: application/json" -d '{"message2": "hello"}', I get a 500 instead of a 400. If I just comment out the @Serializable from the Body data class, then Spring falls back to Jackson and I get a 400 instead.

I did some quick debugging, and it seems that KotlinSerializationJsonDecoder is at fault here; if you look at org.springframework.http.codec.json.AbstractJackson2Decoder#processException, it is catching com.fasterxml.jackson.core.JsonProcessingException and throwing org.springframework.core.codec.DecodingException, which is then caught by org.springframework.web.reactive.result.method.annotation.AbstractMessageReaderArgumentResolver#handleReadError and converted into a org.springframework.web.server.ServerWebInputException which eventually results on a 400.

KotlinSerializationJsonDecoder, on the other hand, never catches kotlinx.serialization.json.internal.JsonDecodingException (which is thrown by decodeFromString) and converts it to org.springframework.core.codec.DecodingException, so it bubbles all the way up as a 500 error:

kotlinx.serialization.json.internal.JsonDecodingException: Unexpected JSON token at offset 2: Encountered an unknown key 'message2' at path: $
Use 'ignoreUnknownKeys = true' in 'Json {}' builder to ignore unknown keys.
JSON input: {"message2": "hello"}
    at kotlinx.serialization.json.internal.JsonExceptionsKt.JsonDecodingException(JsonExceptions.kt:24) ~[kotlinx-serialization-json-jvm-1.6.3.jar:na]
    Suppressed: reactor.core.publisher.FluxOnAssembly$OnAssemblyException: 
Assembly trace from producer [reactor.core.publisher.MonoMapFuseable] :
    reactor.core.publisher.Mono.map(Mono.java:3482)
    org.springframework.http.codec.KotlinSerializationStringDecoder.lambda$decodeToMono$3(KotlinSerializationStringDecoder.java:118)
Error has been observed at the following site(s):
    *_____________Mono.map ⇢ at org.springframework.http.codec.KotlinSerializationStringDecoder.lambda$decodeToMono$3(KotlinSerializationStringDecoder.java:118)
    *___________Mono.defer ⇢ at org.springframework.http.codec.KotlinSerializationStringDecoder.decodeToMono(KotlinSerializationStringDecoder.java:111)
    |_     Mono.onErrorMap ⇢ at org.springframework.web.reactive.result.method.annotation.AbstractMessageReaderArgumentResolver.readBody(AbstractMessageReaderArgumentResolver.java:202)
    |_  Mono.switchIfEmpty ⇢ at org.springframework.web.reactive.result.method.annotation.AbstractMessageReaderArgumentResolver.readBody(AbstractMessageReaderArgumentResolver.java:204)
    |_ Mono.defaultIfEmpty ⇢ at org.springframework.web.reactive.result.method.InvocableHandlerMethod.getMethodArgumentValues(InvocableHandlerMethod.java:265)
    |_      Mono.doOnError ⇢ at org.springframework.web.reactive.result.method.InvocableHandlerMethod.getMethodArgumentValues(InvocableHandlerMethod.java:266)
    *_____________Mono.zip ⇢ at org.springframework.web.reactive.result.method.InvocableHandlerMethod.getMethodArgumentValues(InvocableHandlerMethod.java:273)
    |_        Mono.flatMap ⇢ at org.springframework.web.reactive.result.method.InvocableHandlerMethod.invoke(InvocableHandlerMethod.java:185)
    *___________Mono.defer ⇢ at org.springframework.web.reactive.result.method.annotation.RequestMappingHandlerAdapter.handle(RequestMappingHandlerAdapter.java:260)
    *____________Mono.then ⇢ at org.springframework.web.reactive.result.method.annotation.RequestMappingHandlerAdapter.handle(RequestMappingHandlerAdapter.java:260)
    |_       Mono.doOnNext ⇢ at org.springframework.web.reactive.result.method.annotation.RequestMappingHandlerAdapter.handle(RequestMappingHandlerAdapter.java:261)
    |_  Mono.onErrorResume ⇢ at org.springframework.web.reactive.result.method.annotation.RequestMappingHandlerAdapter.handle(RequestMappingHandlerAdapter.java:262)
    |_  Mono.onErrorResume ⇢ at org.springframework.web.reactive.DispatcherHandler.handleResultMono(DispatcherHandler.java:168)
    |_        Mono.flatMap ⇢ at org.springframework.web.reactive.DispatcherHandler.handleResultMono(DispatcherHandler.java:172)
    *___________Mono.error ⇢ at org.springframework.web.reactive.result.method.annotation.RequestMappingHandlerAdapter.handleException(RequestMappingHandlerAdapter.java:317)
    *___________Mono.error ⇢ at org.springframework.web.reactive.result.method.annotation.RequestMappingHandlerAdapter.handleException(RequestMappingHandlerAdapter.java:317)
    *_________Mono.flatMap ⇢ at org.springframework.web.reactive.DispatcherHandler.handle(DispatcherHandler.java:154)
    *___________Mono.defer ⇢ at org.springframework.web.server.handler.DefaultWebFilterChain.filter(DefaultWebFilterChain.java:106)
    |_      Mono.doOnError ⇢ at org.springframework.web.server.handler.ExceptionHandlingWebHandler.handle(ExceptionHandlingWebHandler.java:84)
    |_  Mono.onErrorResume ⇢ at org.springframework.web.server.handler.ExceptionHandlingWebHandler.handle(ExceptionHandlingWebHandler.java:85)
    |_      Mono.doOnError ⇢ at org.springframework.web.server.handler.ExceptionHandlingWebHandler.handle(ExceptionHandlingWebHandler.java:84)
    *___________Mono.error ⇢ at org.springframework.web.server.handler.ExceptionHandlingWebHandler$CheckpointInsertingHandler.handle(ExceptionHandlingWebHandler.java:106)
    |_          checkpoint ⇢ HTTP POST "/test" [ExceptionHandlingWebHandler]
Original Stack Trace:
        at kotlinx.serialization.json.internal.JsonExceptionsKt.JsonDecodingException(JsonExceptions.kt:24) ~[kotlinx-serialization-json-jvm-1.6.3.jar:na]
        at kotlinx.serialization.json.internal.JsonExceptionsKt.JsonDecodingException(JsonExceptions.kt:32) ~[kotlinx-serialization-json-jvm-1.6.3.jar:na]
        at kotlinx.serialization.json.internal.AbstractJsonLexer.fail(AbstractJsonLexer.kt:598) ~[kotlinx-serialization-json-jvm-1.6.3.jar:na]
        at kotlinx.serialization.json.internal.AbstractJsonLexer.failOnUnknownKey(AbstractJsonLexer.kt:593) ~[kotlinx-serialization-json-jvm-1.6.3.jar:na]
        at kotlinx.serialization.json.internal.StreamingJsonDecoder.handleUnknown(StreamingJsonDecoder.kt:257) ~[kotlinx-serialization-json-jvm-1.6.3.jar:na]
        at kotlinx.serialization.json.internal.StreamingJsonDecoder.decodeObjectIndex(StreamingJsonDecoder.kt:243) ~[kotlinx-serialization-json-jvm-1.6.3.jar:na]
        at kotlinx.serialization.json.internal.StreamingJsonDecoder.decodeElementIndex(StreamingJsonDecoder.kt:178) ~[kotlinx-serialization-json-jvm-1.6.3.jar:na]
        at dev.bcmedeiros.app.Body$$serializer.deserialize(TestController.kt:18) ~[main/:na]
        at dev.bcmedeiros.app.Body$$serializer.deserialize(TestController.kt:18) ~[main/:na]
        at kotlinx.serialization.json.internal.StreamingJsonDecoder.decodeSerializableValue(StreamingJsonDecoder.kt:69) ~[kotlinx-serialization-json-jvm-1.6.3.jar:na]
        at kotlinx.serialization.json.Json.decodeFromString(Json.kt:107) ~[kotlinx-serialization-json-jvm-1.6.3.jar:na]
        at org.springframework.http.codec.KotlinSerializationStringDecoder.lambda$decodeToMono$2(KotlinSerializationStringDecoder.java:118) ~[spring-web-6.1.10.jar:6.1.10]
        at reactor.core.publisher.FluxMapFuseable$MapFuseableSubscriber.onNext(FluxMapFuseable.java:113) ~[reactor-core-3.6.7.jar:3.6.7]
        at reactor.core.publisher.FluxMapFuseable$MapFuseableSubscriber.onNext(FluxMapFuseable.java:129) ~[reactor-core-3.6.7.jar:3.6.7]
        at reactor.core.publisher.FluxContextWrite$ContextWriteSubscriber.onNext(FluxContextWrite.java:107) ~[reactor-core-3.6.7.jar:3.6.7]
        at reactor.core.publisher.FluxMapFuseable$MapFuseableConditionalSubscriber.onNext(FluxMapFuseable.java:299) ~[reactor-core-3.6.7.jar:3.6.7]
        at reactor.core.publisher.FluxFilterFuseable$FilterFuseableConditionalSubscriber.onNext(FluxFilterFuseable.java:337) ~[reactor-core-3.6.7.jar:3.6.7]
        at reactor.core.publisher.Operators$BaseFluxToMonoOperator.completePossiblyEmpty(Operators.java:2097) ~[reactor-core-3.6.7.jar:3.6.7]
        at reactor.core.publisher.MonoCollect$CollectSubscriber.onComplete(MonoCollect.java:145) ~[reactor-core-3.6.7.jar:3.6.7]
        at reactor.core.publisher.FluxMapFuseable$MapFuseableSubscriber.onComplete(FluxMapFuseable.java:152) ~[reactor-core-3.6.7.jar:3.6.7]
        at reactor.core.publisher.FluxPeekFuseable$PeekFuseableSubscriber.onComplete(FluxPeekFuseable.java:277) ~[reactor-core-3.6.7.jar:3.6.7]
        at reactor.core.publisher.FluxMap$MapSubscriber.onComplete(FluxMap.java:144) ~[reactor-core-3.6.7.jar:3.6.7]
        at reactor.netty.channel.FluxReceive.onInboundComplete(FluxReceive.java:415) ~[reactor-netty-core-1.1.20.jar:1.1.20]
        at reactor.netty.channel.ChannelOperations.onInboundComplete(ChannelOperations.java:446) ~[reactor-netty-core-1.1.20.jar:1.1.20]
        at reactor.netty.http.server.HttpServerOperations.onInboundNext(HttpServerOperations.java:818) ~[reactor-netty-http-1.1.20.jar:1.1.20]
        at reactor.netty.channel.ChannelOperationsHandler.channelRead(ChannelOperationsHandler.java:114) ~[reactor-netty-core-1.1.20.jar:1.1.20]
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:444) ~[netty-transport-4.1.111.Final.jar:4.1.111.Final]
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) ~[netty-transport-4.1.111.Final.jar:4.1.111.Final]
        at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412) ~[netty-transport-4.1.111.Final.jar:4.1.111.Final]
        at reactor.netty.http.server.HttpTrafficHandler.channelRead(HttpTrafficHandler.java:305) ~[reactor-netty-http-1.1.20.jar:1.1.20]
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:442) ~[netty-transport-4.1.111.Final.jar:4.1.111.Final]
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) ~[netty-transport-4.1.111.Final.jar:4.1.111.Final]
        at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412) ~[netty-transport-4.1.111.Final.jar:4.1.111.Final]
        at io.netty.channel.CombinedChannelDuplexHandler$DelegatingChannelHandlerContext.fireChannelRead(CombinedChannelDuplexHandler.java:436) ~[netty-transport-4.1.111.Final.jar:4.1.111.Final]
        at io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:346) ~[netty-codec-4.1.111.Final.jar:4.1.111.Final]
        at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:318) ~[netty-codec-4.1.111.Final.jar:4.1.111.Final]
        at io.netty.channel.CombinedChannelDuplexHandler.channelRead(CombinedChannelDuplexHandler.java:251) ~[netty-transport-4.1.111.Final.jar:4.1.111.Final]
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:442) ~[netty-transport-4.1.111.Final.jar:4.1.111.Final]
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) ~[netty-transport-4.1.111.Final.jar:4.1.111.Final]
        at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412) ~[netty-transport-4.1.111.Final.jar:4.1.111.Final]
        at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1407) ~[netty-transport-4.1.111.Final.jar:4.1.111.Final]
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:440) ~[netty-transport-4.1.111.Final.jar:4.1.111.Final]
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) ~[netty-transport-4.1.111.Final.jar:4.1.111.Final]
        at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:918) ~[netty-transport-4.1.111.Final.jar:4.1.111.Final]
        at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:166) ~[netty-transport-4.1.111.Final.jar:4.1.111.Final]
        at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:788) ~[netty-transport-4.1.111.Final.jar:4.1.111.Final]
        at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:724) ~[netty-transport-4.1.111.Final.jar:4.1.111.Final]
        at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:650) ~[netty-transport-4.1.111.Final.jar:4.1.111.Final]
        at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:562) ~[netty-transport-4.1.111.Final.jar:4.1.111.Final]
        at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:994) ~[netty-common-4.1.111.Final.jar:4.1.111.Final]
        at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74) ~[netty-common-4.1.111.Final.jar:4.1.111.Final]
        at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) ~[netty-common-4.1.111.Final.jar:4.1.111.Final]
        at java.base/java.lang.Thread.run(Thread.java:1583) ~[na:na]
bcmedeiros commented 3 months ago

In case you need a quick repro. issue-33138.zip

bcmedeiros commented 2 months ago

@sdeleuze are you sure catching IllegalArgumentException is fine? In my local tests I had to catch kotlinx.serialization.json.internal.JsonDecodingException for things to work as expected.

sdeleuze commented 2 months ago

Yes, JsonDecodingException inherit from IllegalArgumentException, we have tests tests checking that now, I checked with your repro and that's what Kotlin Serialization advises to do.

bcmedeiros commented 2 months ago

Yes, JsonDecodingException inherit from IllegalArgumentException, we have tests tests checking that now, I checked with your repro

True, I didn't go up far enough in the class hierarchy, I guess.

and that's what Kotlin Serialization advises to do.

Also true, for reference: https://kotlinlang.org/api/kotlinx.serialization/kotlinx-serialization-core/kotlinx.serialization/-serialization-exception/ That would also catch exceptions thrown in the constructor for validation purposes, which makes a lot of sense.