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

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

* Ensure uniqueness of routes when creating look up tree

Author: 987Nabil

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[Scope, 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

@@ -21,6 +21,7 @@ import java.util.UUID
 import zio.Chunk
 import zio.test._
 
+import zio.http.codec.Doc
 import zio.http.{int => _, uuid => _}
 
 object RoutePatternSpec extends ZIOHttpSpec {
@@ -233,7 +234,6 @@ object RoutePatternSpec extends ZIOHttpSpec {
         val pattern2 = Method.GET / "users" / trailing / "123"
 
         tree = tree.add(pattern2, 2)
-        println(tree.get(Method.GET, Path("/users/bla/123")))
         tree = tree.add(pattern1, 1)
 
         assertTrue(tree.get(Method.GET, Path("/users/123")).contains(1))
@@ -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
@@ -497,11 +497,36 @@ object RoutePatternSpec extends ZIOHttpSpec {
       },
     )
 
+  def structureEquals = suite("structure equals")(
+    test("equals") {
+      val routePattern = Method.GET / "users" / int("user-id") / "posts" / string("post-id")
+
+      assertTrue(routePattern.structureEquals(routePattern))
+    },
+    test("equals with docs") {
+      val routePattern = Method.GET / "users" / int("user-id") / "posts" / string("post-id")
+
+      assertTrue(
+        routePattern.structureEquals(routePattern ?? Doc.p("docs")),
+      )
+    },
+    test("equals with mapping") {
+      val routePattern  = Method.GET / "users" / int("user-id") / "posts" / string("post-id")
+      val routePattern1 =
+        Method.GET / "users" / int("user-id").transform(_.toString())(_.toInt) / "posts" / string("post-id")
+
+      assertTrue(
+        routePattern.structureEquals(routePattern1),
+      )
+    },
+  )
+
   def spec =
     suite("RoutePatternSpec")(
       decoding,
       rendering,
       formatting,
       tree,
+      structureEquals,
     )
 }

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
@@ -112,5 +112,30 @@ 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"))
+        logs         <- ZTestLogger.logOutput.map { logs => logs.map(_.message()) }
+      } yield {
+        println(logs)
+        assertTrue(
+          logs.contains("Duplicate routes detected:\nGET /foo/{id}\nThe last route of each path will be used."),
+          extractStatus(stringId) == Status.Ok,
+          extractStatus(stringPrefix) == Status.Ok,
+          extractStatus(intId) == Status.Ok,
+          extractStatus(notFound) == Status.NotFound,
+        )
+      }
+    },
   )
 }

zio-http/jvm/src/test/scala/zio/http/security/UserDataSpec.scala

@@ -3,8 +3,6 @@ package zio.http.security
 import zio._
 import zio.test._
 
-import zio.schema._
-
 import zio.http._
 import zio.http.codec._
 import zio.http.endpoint._
@@ -40,16 +38,16 @@ object UserDataSpec extends ZIOSpecDefault {
 
   val spec = suite("UserDataSpec")(
     test("No sanitation and write to server") {
+      val endpoint = Endpoint(Method.GET / "test")
+        .query(HttpCodec.query[String]("data"))
+        .out[String]
+      val route    = endpoint.implementHandler(Handler.fromFunction { (s: String) =>
+        // writeToServer or other actions
+        s
+      })
       // this is not a bug but could be a vulnerability used wrong
       check(tuples.zip(functions)) { case (_, msg, expectedResponse, _) =>
-        val endpoint = Endpoint(Method.GET / "test")
-          .query(HttpCodec.query[String]("data"))
-          .out[String]
-        val route    = endpoint.implementHandler(Handler.fromFunction { case (s: String) =>
-          // writeToServer or other actions
-          s
-        })
-        val request  =
+        val request =
           Request.get(URL(Path.root / "test", queryParams = QueryParams(("data", msg))))
         for {
           response <- route.toRoutes.runZIO(request)
@@ -58,13 +56,13 @@ object UserDataSpec extends ZIOSpecDefault {
       }
     } @@ TestAspect.failing,
     test("No sanitation using Dom") {
+      val endpoint = Endpoint(Method.GET / "test")
+        .in[Dom]
+        .out[Dom]
+      val route    = endpoint.implementHandler(Handler.fromFunction(identity))
       // this is not a bug but could be a vulnerability used wrong
       check(tuples.zip(functions)) { case (_, msg, expectedResponse, _) =>
-        val endpoint = Endpoint(Method.GET / "test")
-          .in[Dom]
-          .out[Dom]
-        val route    = endpoint.implementHandler(Handler.fromFunction(identity))
-        val request  =
+        val request =
           Request.post(URL(Path.root / "test"), Body.fromString(msg))
         for {
           response <- route.toRoutes.runZIO(request)
@@ -73,28 +71,29 @@ object UserDataSpec extends ZIOSpecDefault {
       }
     } @@ TestAspect.failing,
     test("Header injection") {
-      check(tuples.zip(functions)) { case (mediaType, msg, expectedResponse, f) =>
-        val endpoint = Endpoint(Method.GET / "test")
-          .query(HttpCodec.query[String]("data"))
-          .out[Dom]
-        val route    = endpoint.implementHandler(Handler.fromFunction { case (s: String) => f(s) })
-        val request  =
-          Request
-            .get(URL(Path.root / "test", queryParams = QueryParams(("data", msg))))
-            .addHeader(Header.Accept(mediaType))
-        for {
-          response <- route.toRoutes.runZIO(request)
-          body     <- response.body.asString
-        } yield assertTrue(body.contains(expectedResponse))
+      check(tuples.zip(functions).zip(Gen.alphaNumericStringBounded(1, 50))) {
+        case (mediaType, msg, expectedResponse, f, suffix) =>
+          val endpoint = Endpoint(Method.GET / "test" / suffix)
+            .query(HttpCodec.query[String]("data"))
+            .out[Dom]
+          val route    = endpoint.implementHandler(Handler.fromFunction { (s: String) => f(s) })
+          val request  =
+            Request
+              .get(URL(Path.root / "test" / suffix, queryParams = QueryParams(("data", msg))))
+              .addHeader(Header.Accept(mediaType))
+          for {
+            response <- route.toRoutes.runZIO(request)
+            body     <- response.body.asString
+          } yield assertTrue(body.contains(expectedResponse))
       }
     },
     test("Header injection DOM") {
+      val endpoint = Endpoint(Method.GET / "test")
+        .query(HttpCodec.query[Dom]("data"))
+        .out[Dom]
+      val route    = endpoint.implementHandler(Handler.fromFunction(s => s))
       check(tuples.zip(functions)) { case (mediaType, msg, expectedResponse, _) =>
-        val endpoint = Endpoint(Method.GET / "test")
-          .query(HttpCodec.query[Dom]("data"))
-          .out[Dom]
-        val route    = endpoint.implementHandler(Handler.fromFunction(s => s))
-        val request  =
+        val request =
           Request
             .get(URL(Path.root / "test", queryParams = QueryParams(("data", msg))))
             .addHeader(Header.Accept(mediaType))
@@ -105,52 +104,57 @@ object UserDataSpec extends ZIOSpecDefault {
       }
     } @@ TestAspect.failing,
     test("Path injection") {
-      check(tuples.zip(functions)) { case (mediaType, msg, expectedResponse, f) =>
-        val request = Request.get(URL(Path.root / "test" / msg)).addHeader(Header.Accept(mediaType))
-        val route   = Routes(
-          Endpoint(Method.GET / "test" / string("message"))
-            .out[Dom]
-            .implementHandler(Handler.fromFunction { case (s: String) =>
-              f(s)
-            }),
-        )
-        for {
-          response <- route.runZIO(request)
-          body     <- response.body.asString
-        } yield assertTrue(body.contains(expectedResponse))
+      check(tuples.zip(functions).zip(Gen.alphaNumericStringBounded(1, 50))) {
+        case (mediaType, msg, expectedResponse, f, suffix) =>
+          val request = Request.get(URL(Path.root / "test" / suffix / msg)).addHeader(Header.Accept(mediaType))
+          val route   = Routes(
+            Endpoint(Method.GET / "test" / suffix / string("message"))
+              .out[Dom]
+              .implementHandler(Handler.fromFunction { (s: String) =>
+                f(s)
+              }),
+          )
+          for {
+            response <- route.runZIO(request)
+            body     <- response.body.asString
+          } yield assertTrue(body.contains(expectedResponse))
       }
     },
     test("Body injection") {
-      check(tuples.zip(functions)) { case (mediaType, msg, expectedResponse, f) =>
-        val body    = Body.fromArray(msg.getBytes())
-        val request = Request.post("/test", body).addHeader(Header.Accept(mediaType))
-        val route   = Routes(Method.POST / "test" -> handler { (req: Request) =>
+      check(tuples.zip(functions).zip(Gen.alphaNumericStringBounded(1, 50))) {
+        case (mediaType, msg, expectedResponse, f, suffix) =>
+          val body    = Body.fromArray(msg.getBytes())
+          val request = Request.post(url"/test/$suffix", body).addHeader(Header.Accept(mediaType))
+          val route   = Routes(Method.POST / "test" / suffix -> handler { (req: Request) =>
+            for {
+              msg <- req.body.asString.orDie
+            } yield Response.text(f(msg).encode)
+          })
           for {
-            msg <- req.body.asString.orDie
-          } yield Response.text(f(msg).encode)
-        })
-        for {
-          response <- route.runZIO(request)
-          body     <- response.body.asString
-        } yield assertTrue(body.contains(expectedResponse))
+            response <- route.runZIO(request)
+            body     <- response.body.asString
+          } yield assertTrue(body.contains(expectedResponse))
       }
     },
     test("Error injection") {
-      check(tuples.zip(functions)) { case (mediaType, msg, expectedResponse, _) =>
-        val routes  = Routes(Method.POST / "test" -> handler { (req: Request) =>
-          req.body.asString.orDie.map(msg => Response.error(Status.InternalServerError, msg))
-        })
-        val body    = Body.fromString(msg)
-        val request = Request.post("/test", body).addHeader(Header.Accept(mediaType))
-        for {
-          port     <- Server.install(routes)
-          response <- ZIO.scoped {
-            Client
-              .batched(request.updateURL(_ => URL.decode(s"http://localhost:$port/test").toOption.get))
-          }
-          body     <- response.body.asString
-        } yield assertTrue(body == expectedResponse)
-      }
+      val routes = Routes(Method.POST / "test" -> handler { (req: Request) =>
+        req.body.asString.orDie.map(msg => Response.error(Status.InternalServerError, msg))
+      })
+      for {
+        port   <- Server.install(routes)
+        result <- check(tuples.zip(functions)) { case (mediaType, msg, expectedResponse, _) =>
+
+          val body    = Body.fromString(msg)
+          val request = Request.post("/test", body).addHeader(Header.Accept(mediaType))
+          for {
+            response <- ZIO.scoped {
+              Client
+                .batched(request.updateURL(_ => URL.decode(s"http://localhost:$port/test").toOption.get))
+            }
+            body     <- response.body.asString
+          } yield assertTrue(body == expectedResponse)
+        }
+      } yield result
     },
   ).provide(
     Scope.default,
@@ -160,6 +164,6 @@ object UserDataSpec extends ZIOSpecDefault {
     ),
     ZLayer.succeed(NettyConfig.defaultWithFastShutdown),
     Client.default,
-  )
+  ) @@ TestAspect.sequential
 
 }