[play-server] handle playBodyParsers parsing errors #2084
Conversation
server/play-server/src/test/scala/sttp/tapir/server/play/PlayServerWithContextTest.scala
Outdated
Show resolved
Hide resolved
server/tests/src/main/scala/sttp/tapir/server/tests/ServerBasicTests.scala
Outdated
Show resolved
Hide resolved
| @@ -117,3 +126,5 @@ private[play] class PlayRequestBody(serverOptions: PlayServerOptions)(implicit | |||
|
|
|||
| private def playRequest(serverRequest: ServerRequest) = serverRequest.underlying.asInstanceOf[Request[Source[ByteString, Any]]] | |||
| } | |||
|
|
|||
| class PlayBodyParserException(val result: Result) extends Exception | |||
There was a problem hiding this comment.
should I make this exception private/internal?
There was a problem hiding this comment.
One thing I'm contemplating is that this mechanism escapes our response formatting. Normally, there's a way to change the format of all your error responses using CustomiseInterceptors.defaultHandlers so that errors can always be returned as json, for example.
Maybe it would make sense to introduce a top-level exception in serverCore with special handling in ExceptionInterceptor (sth like CustomErrorException but the name is a bit weird :) ), which would overwrite the default status code & error message? Maybe other interpreters can benefit from this as well.
What do you think?
There was a problem hiding this comment.
One thing I'm contemplating is that this mechanism escapes our response formatting
You can have a Play filter configured, that can immediately return a response before it reaches Tapir. I was thinking of these bodyParser errors in the same way...
so that errors can always be returned as json, for example.
you can still achieve that using the Play HttpErrorHandler... I think is a matter of deciding/documenting that some errors will not reach Tapir.
Maybe it would make sense to introduce a top-level exception in serverCore
What would this exception contain? Because in this case the only thing I have is a Play Result.
There was a problem hiding this comment.
The exception would have to contain the status code & the string message.
I guess it's a question whether we leave the error handling to play or to tapir. Tapir handles quite a lot of errors by itself - starting with arbitrary exceptions (via the ExceptionInterceptor, unless it's explicitly disabled), ending with decode failures. What I'd like to avoid is having two places where you need to configure this. So in this solution our interceptor would replace Play's HttpErrorHandler
There was a problem hiding this comment.
The exception would have to contain the status code & the string message.
What if the play Result contains a streaming response (i.e the result.body is not HttpEntity.Strict)?
I guess it's a question whether we leave the error handling to play or to tapir.
Why not both? That's why I mentioned the Play filters. There are some errors that will be handled by play infrastructure and some others that will be handled by tapir infrastructure...
There was a problem hiding this comment.
Ok, let's keep this solution for now:)
|
Thanks for straightening out a couple of problems with body parsers! :) Looks good! I left some comments in the code |
|
Thanks! :) |
Before this PR, when parsing a multiPartRequest but the request is too large (from Play perspective) instead of an
413 Payload Too Largean500 Internal Server Erroris returned (check 1st commit with a test that reproduces the issue).The 2nd commit in this PR, fixes the issue using a custom exception (
PlayBodyParserException) to propagate the PlayResultso that we return that Result instead of throwing an exception. This was the only thing I wanted to fix when I started this PR.The 3rd commit spreads the usage of the configured playBodyParser to the other BodyTypes, which I think from a security perspective makes sense.
Some questions:
serverOptions.playBodyParsersis already configurable, and one can construct a BodyParser that does not enforce size limits...