Make newly added Routes take precedence over old ones (zio#3066)

Author: 987Nabil

.github/workflows/ci.yml

@@ -603,7 +603,7 @@ jobs:
       matrix:
         os: [ubuntu-latest]
         scala: [2.13.16]
-        java: [temurin@8]
+        java: [zulu@8]
     runs-on: ${{ matrix.os }}
     steps:
       - uses: coursier/setup-action@v1

zio-http-testkit/src/main/scala/zio/http/TestClient.scala

@@ -72,6 +72,7 @@ final case class TestClient(
       r <- ZIO.environment[R]
       provided = route.provideEnvironment(r)
       _ <- behavior.update(_ :+ provided)
+      _ <- behavior.get.debug("Added route")
     } yield ()
 
   /**
@@ -121,7 +122,7 @@ final case class TestClient(
     proxy: Option[Proxy],
   )(implicit trace: Trace): ZIO[Any, Throwable, Response] = {
     for {
-      currentBehavior <- behavior.get.map(_ :+ Method.ANY / trailing -> handler(Response.notFound))
+      currentBehavior <- behavior.get
       request = Request(
         body = body,
         headers = headers,

zio-http-testkit/src/test/scala/zio/http/RoutesPrecedentsSpec.scala

@@ -0,0 +1,55 @@
+package zio.http
+
+import zio._
+import zio.test.TestAspect.shrinks
+import zio.test._
+
+import zio.http.endpoint.{AuthType, Endpoint}
+import zio.http.netty.NettyConfig
+import zio.http.netty.server.NettyDriver
+
+object RoutesPrecedentsSpec extends ZIOSpecDefault {
+
+  trait MyService  {
+    def code: UIO[Int]
+  }
+  object MyService {
+    def live(code: Int): ULayer[MyService] = ZLayer.succeed(new MyServiceLive(code))
+  }
+  final class MyServiceLive(_code: Int) extends MyService {
+    def code: UIO[Int] = ZIO.succeed(_code)
+  }
+
+  val endpoint: Endpoint[Unit, String, ZNothing, Int, AuthType.None] =
+    Endpoint(RoutePattern.POST / "api").in[String].out[Int]
+
+  val api = endpoint.implement(_ => ZIO.serviceWithZIO[MyService](_.code))
+
+  // when adding the same route multiple times to the server, the last one should take precedence
+  override def spec: Spec[TestEnvironment & Scope, Any] =
+    test("test") {
+      check(Gen.fromIterable(List(1, 2, 3, 4, 5))) { code =>
+        (
+          for {
+            client <- ZIO.service[Client]
+            port   <- ZIO.serviceWithZIO[Server](_.port)
+            url     = URL.root.port(port) / "api"
+            request = Request
+              .post(url = url, body = Body.fromString(""""this is some input""""))
+              .addHeader(Header.Accept(MediaType.application.json))
+            _      <- TestServer.addRoutes(api.toRoutes)
+            result <- client.batched(request)
+            output <- result.body.asString
+          } yield assertTrue(output == code.toString)
+        ).provideSome[TestServer & Client](
+          ZLayer.succeed(new MyServiceLive(code)),
+        )
+      }.provide(
+        ZLayer.succeed(Server.Config.default.onAnyOpenPort),
+        TestServer.layer,
+        Client.default,
+        NettyDriver.customized,
+        ZLayer.succeed(NettyConfig.defaultWithFastShutdown),
+      )
+    } @@ shrinks(0)
+}

zio-http-testkit/src/test/scala/zio/http/TestClientSpec.scala

@@ -22,12 +22,12 @@ object TestClientSpec extends ZIOHttpSpec {
             _             <- TestClient.addRequestResponse(request2, Response.ok)
             goodResponse2 <- client(request)
             badResponse2  <- client(request2)
-          } yield assertTrue(extractStatus(goodResponse) == Status.Ok) && assertTrue(
+          } yield assertTrue(
+            extractStatus(goodResponse) == Status.Ok,
             extractStatus(badResponse) == Status.NotFound,
-          ) &&
-            assertTrue(extractStatus(goodResponse2) == Status.Ok) && assertTrue(
-              extractStatus(badResponse2) == Status.Ok,
-            )
+            extractStatus(goodResponse2) == Status.Ok,
+            extractStatus(badResponse2) == Status.Ok,
+          )
         },
       ),
       suite("addHandler")(

zio-http/jvm/src/test/scala/zio/http/RoutePatternSpec.scala

@@ -249,7 +249,7 @@ object RoutePatternSpec extends ZIOHttpSpec {
         tree = tree.add(pattern2, 2)
         tree = tree.add(pattern3, 3)
 
-        assertTrue(tree.get(Method.OPTIONS, Path("/users")) == Chunk(2, 1, 3))
+        assertTrue(tree.get(Method.OPTIONS, Path("/users")) == Chunk(2))
       },
       test("multiple routes") {
         var tree: Tree[Unit] = RoutePattern.Tree.empty

zio-http/jvm/src/test/scala/zio/http/RoutesSpec.scala

@@ -18,7 +18,7 @@ package zio.http
 
 import zio.test._
 
-import zio.http.codec.PathCodec
+import zio.http.codec.{PathCodec, SegmentCodec}
 
 object RoutesSpec extends ZIOHttpSpec {
   def extractStatus(response: Response): Status = response.status
@@ -108,5 +108,27 @@ object RoutesSpec extends ZIOHttpSpec {
         )
       }
     },
+    test("overlapping routes with different segment types") {
+      val app = Routes(
+        Method.GET / "foo" / string("id")                                      -> Handler.status(Status.NoContent),
+        Method.GET / "foo" / string("id")                                      -> Handler.ok,
+        Method.GET / "foo" / (SegmentCodec.literal("prefix") ~ string("rest")) -> Handler.ok,
+        Method.GET / "foo" / int("id")                                         -> Handler.ok,
+      )
+
+      for {
+        stringId     <- app.runZIO(Request.get("/foo/123"))
+        stringPrefix <- app.runZIO(Request.get("/foo/prefix123"))
+        intId        <- app.runZIO(Request.get("/foo/123"))
+        notFound     <- app.runZIO(Request.get("/foo/123/456"))
+      } yield {
+        assertTrue(
+          extractStatus(stringId) == Status.Ok,
+          extractStatus(stringPrefix) == Status.Ok,
+          extractStatus(intId) == Status.Ok,
+          extractStatus(notFound) == Status.NotFound,
+        )
+      }
+    },
   )
 }

zio-http/shared/src/main/scala/zio/http/RoutePattern.scala

@@ -216,10 +216,7 @@ object RoutePattern                                                       {
         }
       }
 
-      if (anyRoot eq null) forMethod
-      else {
-        forMethod ++ anyRoot.get(path)
-      }
+      if (forMethod.isEmpty && anyRoot != null) anyRoot.get(path) else forMethod
     }
 
     def map[B](f: A => B): Tree[B] =

zio-http/shared/src/main/scala/zio/http/Routes.scala

@@ -48,9 +48,8 @@ final case class Routes[-Env, +Err](routes: Chunk[zio.http.Route[Env, Err]]) { s
     new ApplyContextAspect[Env, Err, Env0](self)
 
   /**
-   * Combines this HTTP application with the specified HTTP application. In case
-   * of route conflicts, the routes in this HTTP application take precedence
-   * over the routes in the specified HTTP application.
+   * Combines this Routes with the specified Routes. In case of route conflicts,
+   * the new Routes take precedence over the current Routes.
    */
   def ++[Env1 <: Env, Err1 >: Err](that: Routes[Env1, Err1]): Routes[Env1, Err1] =
     copy(routes = routes ++ that.routes)
@@ -252,18 +251,7 @@ final case class Routes[-Env, +Err](routes: Chunk[zio.http.Route[Env, Err]]) { s
         chunk.length match {
           case 0 => Handler.notFound
           case 1 => chunk(0)
-          case n => // TODO: Support precomputed fallback among all chunk elements
-            var acc = chunk(0)
-            var i   = 1
-            while (i < n) {
-              val h = chunk(i)
-              acc = acc.catchAll { response =>
-                if (response.status == Status.NotFound) h
-                else Handler.fail(response)
-              }
-              i += 1
-            }
-            acc
+          case _ => chunk.last
         }
       }
       .merge