[BUG] `zio-http` aspect gets run for each Tapir app, instead of only once

[godspeedelbow]

Tapir version: 1.8.4 - 1.8.10

Scala version: 3.3.0

This occurs first in Tapir version 1.8.4 and has been like this each version since. [email protected]/[email protected] is the last versions that produces the expected correct behavior.

Describe the bug

When I combine multiple Tapir apps, the aspect is executed for each route until the request is handled. The zio-http equivalent behaves as expected, logging once, regardless of how many apps.

How to reproduce?

// val zioHttpVersion = "3.0.0-RC3"
// val tapirVersion   = "1.8.4"

import sttp.tapir.*
import sttp.tapir.server.ziohttp.ZioHttpInterpreter
import zio.http.*
import zio.*

object ReproTapirIssueRC3 extends ZIOAppDefault:
  def run =
    for {
      _ <- (tapirApp @@ Foo("A")).runZIO(Request.get(URL.empty.addPath("1")))
      // Foo: A
      _ <- (tapirApp @@ Foo("B")).runZIO(Request.get(URL.empty.addPath("2")))
      // Foo: B
      // Foo: B
      _ <- (tapirApp @@ Foo("C")).runZIO(Request.get(URL.empty.addPath("3")))
      // Foo: C
      // Foo: C
      // Foo: C

      _ <- (zhttpApp @@ Foo("D")).runZIO(Request.get(URL.empty.addPath("1")))
      // Foo: D
      _ <- (zhttpApp @@ Foo("E")).runZIO(Request.get(URL.empty.addPath("2")))
      // Foo: E
      _ <- (zhttpApp @@ Foo("F")).runZIO(Request.get(URL.empty.addPath("3")))
      // Foo: F
    } yield ()

  def tapirApp = (tapirRoute("1") ++ tapirRoute("2") ++ tapirRoute("3"))
  def zhttpApp = (zhttpRoute("1") ++ zhttpRoute("2") ++ zhttpRoute("3"))

  // returns GET route, from Tapir
  def tapirRoute = (path: String) =>
    ZioHttpInterpreter().toHttp(
      sttp.tapir.endpoint.get
        .in(path)
        .serverLogic(_ => ZIO.succeed(Right(())))
    )

  // returns GET route
  def zhttpRoute = (path: String) =>
    zio.http.Routes(
      zio.http.Method.GET / path -> zio.http.Handler.ok
    ).toHttpApp

  // aspect that prints when executed
  case class Foo(i: String) extends Middleware[Any] {
    def apply[Env1 <: Any, Err](routes: Routes[Env1, Err]): Routes[Env1, Err] =
      routes.transform[Env1] {
        handler =>
          Handler.fromFunctionZIO {
            request =>
              println(s"Foo: $i")
              handler.runZIO(request)
          }
      }
  }
end ReproTapirIssueRC3

Additional information

[godspeedelbow]

This code reproduces it for

• [email protected]
• [email protected]

import sttp.tapir.*
import sttp.tapir.server.ziohttp.ZioHttpInterpreter
import zio.http.*
import zio.*

object ReproTapirIssue extends ZIOAppDefault:
  def run =
    for {
      _ <- (tapirApp @@ Foo("A")).runZIO(Request.get(URL.empty.addPath("1")))
      // Foo: A
      _ <- (tapirApp @@ Foo("B")).runZIO(Request.get(URL.empty.addPath("2")))
      // Foo: B
      // Foo: B
      _ <- (tapirApp @@ Foo("C")).runZIO(Request.get(URL.empty.addPath("3")))
      // Foo: C
      // Foo: C
      // Foo: C

      _ <- (zhttpApp @@ Foo("D")).runZIO(Request.get(URL.empty.addPath("1")))
      // Foo: D
      _ <- (zhttpApp @@ Foo("E")).runZIO(Request.get(URL.empty.addPath("2")))
      // Foo: E
      _ <- (zhttpApp @@ Foo("F")).runZIO(Request.get(URL.empty.addPath("3")))
      // Foo: F
    } yield ()

  def tapirApp = (tapirRoute("1") ++ tapirRoute("2") ++ tapirRoute("3"))
  def zhttpApp = (zhttpRoute("1") ++ zhttpRoute("2") ++ zhttpRoute("3"))

  // returns GET route, from Tapir
  def tapirRoute = (path: String) =>
    ZioHttpInterpreter().toHttp(
      sttp.tapir.endpoint.get
        .in(path)
        .serverLogic(_ => ZIO.succeed(Right(())))
    )

  // returns GET route
  def zhttpRoute = (path: String) =>
    zio.http.Routes(
      zio.http.Method.GET / path -> zio.http.Handler.ok
    )

  // aspect that prints when executed
  case class Foo(i: String) extends Middleware[Any] {
    def apply[Env1 <: Any, Err](routes: Routes[Env1, Err]): Routes[Env1, Err] =
      routes.transform[Env1] {
        handler =>
          Handler.fromFunctionZIO {
            request =>
              println(s"Foo: $i")
              handler.runZIO(request)
          }
      }
  }
end ReproTapirIssue

[adamw]

@godspeedelbow This used to be a problem before, and due to missing test coverage it got re-introduced. I added a failing test case (see PR), however I don't know how to make a proper fix. ZIO HTTP changes too fast, there's always some problems with it ;)

Anyway ... what I need is to create a Route / Handler which would allow me to say "this request doesn't match this route, don't run it, don't run any middlewares". This used to be possible with Http.fromOptionalHandlerZIO, but I can't see that variant in Handler` now. If you of any of ZIO HTTP maintainers would know how to create such a route, please let me know here or in the PR.

[987Nabil]

@adamw From what I see, the issue is this line https://github.com/softwaremill/tapir/blob/5b85646a733db5d9ec4000b49d8b0687c4a54b6d/server/zio-http-server/src/main/scala/sttp/tapir/server/ziohttp/ZioHttpInterpreter.scala#L30

Routes.singleton is Routes(Route.route(RoutePattern.any)(h)). So you basically create a route that matches any method and path. You should use Method.<method> / <path> -> handler and wrap this in Routes. So that only one handler is selected based on the method/path. I guess then there should also only be one middleware be executed, since the middleware added to the handler only runs if the path matches.

[guersam]

I'd prefer @987Nabil 's approach, because currently it's impossible to concatenate two or more Tapir -> ZIO HTTP routes together due to the wildcarded path matching.

[adamw]

Hm I'm that proficient in the new ZIO HTTP APIs, maybe you could create a PR for this, or suggest a change to this one?

[987Nabil]

@adamw My issue is, that I don't know tapir 😅 I would hope that one could just extract the method and path out of the Tapir endpoint definition. The parsed results of path variables could maybe just get thrown away, if it is to complicated to hand them over. I would expect that the creation of Routes is the only necessary change here. I am generally happy to help, but I am working currently on finishing of 3.0 final.

Maybe @guersam has some time to offer help. He knows for sure zio-http well and knows tapir better than me, since he used it 🙂

[adamw]

Can you create a pattern with wildcards? How to do that? If we have a tapir endpoint, let's say, with .get.in("a" / "b").in(path[String]("c")), this corresponds to a route GET /a/b/*, for example.

[guersam]

@adamw That route pattern can be encoded in ZIO HTTP like this:

import zio.http.Method
import zio.http.codec.PathCodec

Method.GET / "a" / "b" / PathCodec.string("c")

// results in

import zio.http.RoutePattern
import zio.http.codec.SegmentCodec

RoutePattern(
  Method.GET,
  PathCodec.Concat(
    PathCodec.Concat(
      PathCodec.Segment(
        SegmentCodec.Literal("a"),
      ),
      PathCodec.Segment(
        SegmentCodec.Literal("b")
      ),
      ???: Combiner.WithOut[Unit, Unit, Unit]
    ),
    PathCodec.Text("c"),
    ???: Combiner.WithOut[Unit, String, String]
  )
)

There are some major obstacles when converting Tapir path input to ZIO HTTP route pattern:

1. PathCodec.Concat requires a Combiner.WithOut[A, B, C], which has the similar purpose to Tapir's ParamConcat.Aux[A, B, C]

   a. But they work in completely different ways. Combiner deals with tuples directly, whereas ParamConcat exposes left & right arities and let the caller handle the actual tuples.

   b. Tapir EndpointInput's responsibility is much broader than ZIO HTTP's PathCodec. It makes it more difficult to extract and modify the ParamConcat properly.

2. ~~SegmentCodec is a sealed trait, whereas Tapir's path[T] accepts an arbitrary type T that has an implicit Codec[String, *, TextPlain].~~ EDIT: path[T](name) can be converted using PathCodec.string(name).transformOrFail[T].

After some research, I realized that building RoutePattern from Tapir endpoints is more difficult than I expected.

Besides the mismatch between the path encodings, the sealed SegmentCodec might be resolved from ZIO HTTP's side, allowing users to bring their own segment codec.

@987Nabil What do you think?

[987Nabil]

I think we don't need to change things in zio http, since we have transformOrFail on the codecs. Not on the segment, but on the path codec

[adamw]

@guersam @987Nabil hm I'm still not sure how/if this can be done, is it possible to generate zio-http code which will run a route given e.g. a path representation such as /a/b/*/c/*/d? I think this should be good enough to provide the integration necessary. If you could share some code snippets which would generate such a route instead of our current usage of Routes.singleton that would be very helpful :)

[987Nabil]

@adamw like this?

Routes(
  Method.GET / "a" / "b" / string("param1") / "c" / string("param2") / "d" -> handler((param1: String, param2: String, req: Request) => ZIO.succeed(Response.ok))
)

Note, the string("param1") is a wildcard for exactly one segment, where "param1" is only a name for generating docs. We support multiple path segment wild card only at the end of the route for now via / trailing.

[adamw]

@987Nabil thanks, but I'd need this in a generic way - sth like given a List[String | Wildcard], generate a path matcher. I don't need the typed handler function, as the entire extraction of parameters is handled by tapir

[987Nabil]

@adamw I did not try to run it, but I think this should work

import zio.http._

case class Wildcard(name: String)

val segments: List[Any] = List("api", "v1", "users", Wildcard("id"))
val pattern: RoutePattern[Any] = segments.map {
  case s: String => PathCodec.literal(s)
  case w: Wildcard => PathCodec.string(w.name)
}.foldLeft(RoutePattern(Method.GET, PathCodec.empty).asInstanceOf[RoutePattern[Any]]){
  case (codec, next) => (codec / next).asInstanceOf[RoutePattern[Any]] 
}
val routes = Routes(
  pattern -> handler((req: Request) => ZIO.succeed(Response.text("Hello World")))
)

[adamw]

@987Nabil thanks, something similar indeed did work :)

[987Nabil]

Glad I could help :)