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.

Sign up for GitHub

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

[play-server] handle playBodyParsers parsing errors #2084

Merged
merged 8 commits into from
Apr 29, 2022
Merged

[play-server] handle playBodyParsers parsing errors #2084

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
feef12d
failing test
jtjeferreira Apr 26, 2022
8bbd7de
use exception to propagate Result from BodyParser
jtjeferreira Apr 26, 2022
a367239
use serverOptions.playBodyParsers in all situations (StringBody, File…
jtjeferreira Apr 26, 2022
adceb28
scalafmt
jtjeferreira Apr 26, 2022
ddb688c
revert changes to ServerBasicTests, but increase maxMemoryBuffer conf
jtjeferreira Apr 28, 2022
0b83400
fix compilation in scala 2.12
jtjeferreira Apr 28, 2022
6f8c12d
better scala 2.12 code
jtjeferreira Apr 28, 2022
f655e76
rework tests
jtjeferreira Apr 28, 2022
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
27 changes: 19 additions & 8 deletions server/play-server/src/main/scala/sttp/tapir/server/play/PlayRequestBody.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
package sttp.tapir.server.play

import akka.stream.Materializer
import akka.stream.scaladsl.{FileIO, Sink, Source}
import akka.stream.scaladsl.{FileIO, Source}
import akka.util.ByteString
import play.api.mvc.Request
import play.api.mvc.{Request, Result}
import play.core.parsers.Multipart
import sttp.capabilities.akka.AkkaStreams
import sttp.model.{Header, MediaType, Part}
Expand Down Expand Up @@ -40,10 +40,16 @@ private[play] class PlayRequestBody(serverOptions: PlayServerOptions)(implicit
)(implicit
mat: Materializer
): Future[RawValue[R]] = {
def bodyAsByteString() = body().runWith(Sink.fold(ByteString.newBuilder)(_ append _)).map(_.result())
// playBodyParsers is used, so that the maxLength limits from Play configuration are applied
def bodyAsByteString(): Future[ByteString] = {
serverOptions.playBodyParsers.byteString.apply(request).run(body()).flatMap {
case Left(result) => Future.failed(new PlayBodyParserException(result))
case Right(value) => Future.successful(value)
}
}
bodyType match {
case RawBodyType.StringBody(defaultCharset) =>
bodyAsByteString().map(b => RawValue(new String(b.toArray, charset.getOrElse(defaultCharset))))
bodyAsByteString().map(b => RawValue(b.decodeString(charset.getOrElse(defaultCharset))))
case RawBodyType.ByteArrayBody => bodyAsByteString().map(b => RawValue(b.toArray))
case RawBodyType.ByteBufferBody => bodyAsByteString().map(b => RawValue(b.toByteBuffer))
case RawBodyType.InputStreamBody => bodyAsByteString().map(b => RawValue(new ByteArrayInputStream(b.toArray)))
Expand All @@ -54,7 +60,10 @@ private[play] class PlayRequestBody(serverOptions: PlayServerOptions)(implicit
Future.successful(RawValue(tapirFile, Seq(tapirFile)))
case None =>
val file = FileRange(serverOptions.temporaryFileCreator.create().toFile)
body().runWith(FileIO.toPath(file.file.toPath)).map(_ => RawValue(file, Seq(file)))
serverOptions.playBodyParsers.file(file.file).apply(request).run(body()).flatMap {
case Left(result) => Future.failed(new PlayBodyParserException(result))
case Right(_) => Future.successful(RawValue(file, Seq(file)))
}
}
case m: RawBodyType.MultipartBody => multiPartRequestToRawBody(request, m, body)
}
Expand All @@ -71,16 +80,16 @@ private[play] class PlayRequestBody(serverOptions: PlayServerOptions)(implicit
Multipart.handleFilePartAsTemporaryFile(serverOptions.temporaryFileCreator)
)
bodyParser.apply(request).run(body()).flatMap {
case Left(_) =>
Future.failed(new IllegalArgumentException("Unable to parse multipart form data.")) // TODO
case Left(r) =>
Future.failed(new PlayBodyParserException(r))
case Right(value) =>
val dataParts: Seq[Future[Option[Part[Any]]]] = value.dataParts.flatMap { case (key, value) =>
m.partType(key).map { partType =>
toRaw(
request,
partType,
charset(partType),
() => Source.single(ByteString(value.flatMap(_.getBytes).toArray)),
() => Source(value.map(ByteString.apply)),
None
).map(body => Some(Part(key, body.value)))
}
Expand Down Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I make this exception private/internal?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's keep this solution for now:)

59 changes: 33 additions & 26 deletions server/play-server/src/main/scala/sttp/tapir/server/play/PlayServerInterpreter.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,33 +86,40 @@ trait PlayServerInterpreter {
playServerOptions.deleteFile
)

interpreter(serverRequest).map {
case RequestResult.Failure(_) =>
Left(Result(header = ResponseHeader(StatusCode.NotFound.code), body = HttpEntity.NoEntity))
case RequestResult.Response(response: ServerResponse[PlayResponseBody]) =>
val headers: Map[String, String] = response.headers
.foldLeft(Map.empty[String, List[String]]) { (a, b) =>
if (a.contains(b.name)) a + (b.name -> (a(b.name) :+ b.value)) else a + (b.name -> List(b.value))
interpreter(serverRequest)
.map {
case RequestResult.Failure(_) =>
Left(Result(header = ResponseHeader(StatusCode.NotFound.code), body = HttpEntity.NoEntity))
case RequestResult.Response(response: ServerResponse[PlayResponseBody]) =>
val headers: Map[String, String] = response.headers
.foldLeft(Map.empty[String, List[String]]) { (a, b) =>
if (a.contains(b.name)) a + (b.name -> (a(b.name) :+ b.value)) else a + (b.name -> List(b.value))
}
.map {
// See comment in play.api.mvc.CookieHeaderEncoding
case (key, value) if key == HeaderNames.SET_COOKIE => (key, value.mkString(";;"))
case (key, value) => (key, value.mkString(", "))
}
.filterNot(allowToSetExplicitly)

val status = response.code.code
response.body match {
case Some(Left(flow)) => Right(flow)
case Some(Right(entity)) => Left(Result(ResponseHeader(status, headers), entity))
case None =>
if (serverRequest.method.is(Method.HEAD) && response.contentLength.isDefined)
Left(
Result(
ResponseHeader(status, headers),
HttpEntity.Streamed(Source.empty, response.contentLength, response.contentType)
)
)
else Left(Result(ResponseHeader(status, headers), HttpEntity.Strict(ByteString.empty, response.contentType)))
}
.map {
// See comment in play.api.mvc.CookieHeaderEncoding
case (key, value) if key == HeaderNames.SET_COOKIE => (key, value.mkString(";;"))
case (key, value) => (key, value.mkString(", "))
}
.filterNot(allowToSetExplicitly)

val status = response.code.code
response.body match {
case Some(Left(flow)) => Right(flow)
case Some(Right(entity)) => Left(Result(ResponseHeader(status, headers), entity))
case None =>
if (serverRequest.method.is(Method.HEAD) && response.contentLength.isDefined)
Left(
Result(ResponseHeader(status, headers), HttpEntity.Streamed(Source.empty, response.contentLength, response.contentType))
)
else Left(Result(ResponseHeader(status, headers), HttpEntity.Strict(ByteString.empty, response.contentType)))
}
}
}
.recover { case e: PlayBodyParserException =>
Left(e.result)
}
}
}
}
Expand Down
43 changes: 43 additions & 0 deletions server/play-server/src/test/scala/sttp/tapir/server/play/PlayServerWithContextTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import play.api.Mode
import play.api.routing.Router
import play.core.server.{DefaultAkkaHttpServerComponents, ServerConfig}
import sttp.client3._
import sttp.model.{Part, StatusCode}
import sttp.tapir._
import sttp.tapir.tests.Test

Expand All @@ -34,6 +35,48 @@ class PlayServerWithContextTest(backend: SttpBackend[IO, Any])(implicit _actorSy
}
r.onComplete(_ => s.stop())
r
},
Test("reject big body in multipart request") {
jtjeferreira marked this conversation as resolved.
Show resolved Hide resolved
import sttp.tapir.generic.auto._
case class A(part1: Part[String])
val e = endpoint.post.in("hello").in(multipartBody[A]).out(stringBody).serverLogicSuccess(_ => Future.successful("world"))
val components = new DefaultAkkaHttpServerComponents {
override lazy val serverConfig: ServerConfig = ServerConfig(port = Some(0), address = "127.0.0.1", mode = Mode.Test)
override lazy val actorSystem: ActorSystem = ActorSystem("tapir", defaultExecutionContext = Some(_actorSystem.dispatcher))
override def router: Router = Router.from(PlayServerInterpreter().toRoutes(e)).withPrefix("/test")
}
val s = components.server
val r = Future.successful(()).flatMap { _ =>
basicRequest
.post(uri"http://localhost:${s.mainAddress.getPort}/test/hello")
.body(Array.ofDim[Byte](1024 * 15000)) // 15M
.send(backend)
.map(_.code shouldBe StatusCode.PayloadTooLarge)
.unsafeToFuture()
}
r.onComplete(_ => s.stop())
r
},
Test("reject big body in normal request") {
import sttp.tapir.generic.auto._
case class A(part1: Part[String])
val e = endpoint.post.in("hello").in(stringBody).out(stringBody).serverLogicSuccess(_ => Future.successful("world"))
val components = new DefaultAkkaHttpServerComponents {
override lazy val serverConfig: ServerConfig = ServerConfig(port = Some(0), address = "127.0.0.1", mode = Mode.Test)
override lazy val actorSystem: ActorSystem = ActorSystem("tapir", defaultExecutionContext = Some(_actorSystem.dispatcher))
override def router: Router = Router.from(PlayServerInterpreter().toRoutes(e)).withPrefix("/test")
}
val s = components.server
val r = Future.successful(()).flatMap { _ =>
basicRequest
.post(uri"http://localhost:${s.mainAddress.getPort}/test/hello")
.body(Array.ofDim[Byte](1024 * 15000)) // 15M
.send(backend)
.map(_.code shouldBe StatusCode.PayloadTooLarge)
.unsafeToFuture()
}
r.onComplete(_ => s.stop())
r
}
)
}
4 changes: 2 additions & 2 deletions server/tests/src/main/scala/sttp/tapir/server/tests/ServerBasicTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,9 +496,9 @@ class ServerBasicTests[F[_], OPTIONS, ROUTE](
) { (backend, baseUri) =>
basicStringRequest
.post(uri"$baseUri/p2")
.body("a" * 1000000)
.body("a" * 100000)
.send(backend)
.map { r => r.body shouldBe "p2 1000000" }
.map { r => r.body shouldBe "p2 100000" }
},
jtjeferreira marked this conversation as resolved.
Show resolved Hide resolved
testServer(
"two endpoints with query defined as the first input, path segments as second input: should try the second endpoint if the path doesn't match",
Expand Down