[Bug] The passed Env is not the good one

[guizmaii]

Reproducer:

import zio.http.*
import zio.http.endpoint.{AuthType, Endpoint}
import zio.http.netty.NettyConfig
import zio.http.netty.server.NettyDriver
import zio.test.TestAspect.shrinks
import zio.test.{Gen, Spec, TestEnvironment, ZIOSpecDefault, assertTrue, check}
import zio.{Scope, UIO, ULayer, ZIO, ZLayer, ZNothing}

object MyApiSpec 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))

  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.json("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 == s"$code")
        ).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)
}

The test will fail on the second iteration of the check with the following assertion error:

Assertion failed:
Expected :"2"
Actual   :"1"

The Env of the api handler is never updated. It always contains the instance of MyService returning 1

This is quite a nasty bug.

Cc @jdegoes @987Nabil

[kyri-petrou]

Do you happen to know if this an issue only of the TestServer or does it also affect the "real" server implementation?

[guizmaii]

Do you happen to know if this an issue only of the TestServer or does it also affect the "real" server implementation?

I don't know. I can try to reproduce it in my app 🤔

[kyri-petrou]

I might be wrong but I'm thinking that in an app the usage pattern would be a bit different, probably it'd require a middleware to provide the env? Unless you're adding apps to the server dynamically that's it

[jdegoes]

/bounty $250

[algora-pbc]

💎 $250 bounty • ZIO

Steps to solve:

1. Start working: Comment /attempt #3066 with your implementation plan
2. Submit work: Create a pull request including /claim #3066 in the PR body to claim the bounty
3. Receive payment: 100% of the bounty is received 2-5 days post-reward. Make sure you are eligible for payouts

Thank you for contributing to zio/zio-http!

Add a bounty • Share on socials

Attempt	Started (GMT+0)	Solution
🟢 @exi	Sep 20, 2024, 7:21:09 AM	#3160

[exi]

/attempt #3066

I'd like to take a look at this

Algora profile	Completed bounties	Tech	Active attempts	Options
@exi	2 bounties from 2 projects	JavaScript, Nix, Clojure & more		Cancel attempt

[exi]

This Bug is not technically a bug but an unfortunate behaviour of the addRoutes call.
During the TestServer.addRoutes call, the routes are bound to the current environment here:

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

Line 94 in 502331c

provided: Routes[Any, Response] = routes.provideEnvironment(r)

Which in itself is not wrong, because the second call to addRoutes actually contains the correct MyServer service.
However it then goes and merges the old routes with the new routes, giving precedence to the old routes with the old environment:

zio-http/zio-http/jvm/src/main/scala/zio/http/netty/server/NettyDriver.scala

Line 70 in 502331c

val updatedApp = (oldApp ++ newApp).asInstanceOf[Routes[Any, Response]]

If we flip this here to say (newApp ++ oldApp) instead of (oldApp ++ newApp), the behaviour is as expected.

@jdegoes I'd be happy to submit a tiny PR to flip this and make it work as expected.

[guizmaii]

@exi Go for it :)

[algora-pbc]

💡 @exi submitted a pull request that claims the bounty. You can visit your bounty board to reward.

[exi]

I just found this comment:

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

Lines 50 to 57 in 502331c

	/**
	* 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.
	*/
	def ++[Env1 <: Env, Err1 >: Err](that: Routes[Env1, Err1]): Routes[Env1, Err1] =
	copy(routes = routes ++ that.routes)

It seems like it's the intended behaviour to have old routes take precendence. So this test case here is (sadly) working as intended as long as you are not replacing the TestServer for each test run. If you pull the whole environment including the TestServer into the inner environment, it works as expected.

[exi]

Gentle ping.
I'm not quite sure what else there is to do after I figured out why this behaviour is like it is and there has been no meaningful feedback for 2 weeks now.
I'd be happy to implement whatever change is best, but as it stands, this is working as intended.

[exi]

@guizmaii do you have any idea what's going on with this bounty? It has not been responded to for months.