Skip to content

Commit

Permalink
Merge pull request #2760 from softwaremill/play-zio-better-single-end…
Browse files Browse the repository at this point in the history
…point-handling

Fix the way single-endpoint routes are interpreted in Play/ZIO Http. Enable reject tests.
  • Loading branch information
adamw authored Feb 28, 2023
2 parents a33da5b + 8c45f8a commit ad201d4
Show file tree
Hide file tree
Showing 7 changed files with 37 additions and 11 deletions.
4 changes: 2 additions & 2 deletions doc/server/play.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ val countCharactersRoutes: Routes =
```eval_rst
.. note::
A single Play application can contain both tapir-managed andPlay-managed routes. However, because of the
routing implementation in Play, the shape of the paths that tapir/Play-native handlers serve should not
A single Play application can contain both tapir-managed and Play-managed routes. However, because of the
routing implementation in Play, the shape of the paths that tapir and other Play handlers serve should not
overlap. The shape of the path includes exact path segments, single- and multi-wildcards. Otherwise, request handling
will throw an exception. We don't expect users to encounter this as a problem, however the implementation here
diverges a bit comparing to other interpreters.
Expand Down
2 changes: 1 addition & 1 deletion doc/server/ziohttp.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ val countCharactersHttp: HttpApp[Any, Throwable] =
.. note::
A single ZIO-Http application can contain both tapir-managed and ZIO-Http-managed routes. However, because of the
routing implementation in ZIO Http, the shape of the paths that tapir/ZIO-Http-native handlers serve should not
routing implementation in ZIO Http, the shape of the paths that tapir and other ZIO Http handlers serve should not
overlap. The shape of the path includes exact path segments, single- and multi-wildcards. Otherwise, request handling
will throw an exception. We don't expect users to encounter this as a problem, however the implementation here
diverges a bit comparing to other interpreters.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,23 @@ trait PlayServerInterpreter {
implicit val monad: FutureMonad = new FutureMonad()

val filterServerEndpoints = FilterServerEndpoints(serverEndpoints)
val singleEndpoint = serverEndpoints.size == 1

new PartialFunction[RequestHeader, Handler] {
override def isDefinedAt(request: RequestHeader): Boolean = filterServerEndpoints(PlayServerRequest(request, request)).nonEmpty
override def isDefinedAt(request: RequestHeader): Boolean = {
val filtered = filterServerEndpoints(PlayServerRequest(request, request))
if (singleEndpoint) {
// If we are interpreting a single endpoint, we verify that the method matches as well; in case it doesn't,
// we refuse to handle the request, allowing other Play routes to handle it. Otherwise even if the method
// doesn't match, this will be handled by the RejectInterceptor
filtered.exists { e =>
val m = e.endpoint.method
m.isEmpty || m.contains(Method(request.method))
}
} else {
filtered.nonEmpty
}
}

override def apply(header: RequestHeader): Handler =
if (isWebSocket(header))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ object PlayServerOptions {
ci.decodeFailureHandler,
ci.interceptors
)
).serverLog(defaultServerLog).rejectHandler(None)
).serverLog(defaultServerLog)

def defaultDeleteFile(file: TapirFile)(implicit ec: ExecutionContext): Future[Unit] = {
Future(blocking(Defaults.deleteFile()(file)))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ import cats.data.NonEmptyList
import cats.effect.{IO, Resource}
import cats.effect.unsafe.implicits.global
import org.scalatest.matchers.should.Matchers._
import play.api.http.HttpVerbs.GET
import play.api.http.ParserConfiguration
import play.api.mvc.Result
import sttp.capabilities.akka.AkkaStreams
import sttp.client3._
import sttp.model.{MediaType, Part, StatusCode}
Expand Down Expand Up @@ -109,8 +111,7 @@ class PlayServerTest extends TestSuite {
invulnerableToUnsanitizedHeaders = false
).tests() ++
new ServerMultipartTests(createServerTest, partOtherHeaderSupport = false).tests() ++
new AllServerTests(createServerTest, interpreter, backend, basic = false, multipart = false, reject = false, options = false)
.tests() ++
new AllServerTests(createServerTest, interpreter, backend, basic = false, multipart = false, options = false).tests() ++
new ServerStreamingTests(createServerTest, AkkaStreams).tests() ++
new PlayServerWithContextTest(backend).tests() ++
new ServerWebSocketTests(createServerTest, AkkaStreams) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package sttp.tapir.server.ziohttp

import io.netty.handler.codec.http.HttpResponseStatus
import sttp.capabilities.zio.ZioStreams
import sttp.model.{HeaderNames, Header => SttpHeader}
import sttp.model.{HeaderNames, Method, Header => SttpHeader}
import sttp.monad.MonadError
import sttp.tapir.server.interceptor.RequestResult
import sttp.tapir.server.interceptor.reject.RejectInterceptor
Expand Down Expand Up @@ -72,12 +72,24 @@ trait ZioHttpInterpreter[R] {
}

val serverEndpointsFilter = FilterServerEndpoints[ZioStreams, RIO[R & R2, *]](widenedSes)
val singleEndpoint = widenedSes.size == 1

Http.fromOptionalHandlerZIO { request =>
// pre-filtering the endpoints by shape to determine, if this request should be handled by tapir
val filteredEndpoints = serverEndpointsFilter.apply(ZioHttpServerRequest(request))
filteredEndpoints match {
val filteredEndpoints2 = if (singleEndpoint) {
// If we are interpreting a single endpoint, we verify that the method matches as well; in case it doesn't,
// we refuse to handle the request, allowing other ZIO Http routes to handle it. Otherwise even if the method
// doesn't match, this will be handled by the RejectInterceptor
filteredEndpoints.filter { e =>
val m = e.endpoint.method
m.isEmpty || m.contains(Method(request.method.toString()))
}
} else filteredEndpoints

filteredEndpoints2 match {
case Nil => ZIO.fail(None)
case _ => ZIO.succeed(handleRequest(request, filteredEndpoints))
case _ => ZIO.succeed(handleRequest(request, filteredEndpoints2))
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,6 @@ class ZioHttpServerTest extends TestSuite {
staticContent = false,
multipart = false,
file = false,
reject = false,
options = false
).tests() ++
new ServerStreamingTests(createServerTest, ZioStreams).tests() ++
Expand Down

0 comments on commit ad201d4

Please sign in to comment.