Provide context to multiple routes (#2488)

[987Nabil]

This PR is not good to go, but shows that I found a way how to provide a context to multiple routes without any more explicit typing or extremely strange syntax.

RFC and I happily accept suggestions on the syntax.

/claim #2488

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 48.07692% with 27 lines in your changes are missing coverage. Please review.

Project coverage is 64.41%. Comparing base (b1da91b) to head (957a088).
Report is 6 commits behind head on main.

Files	Patch %	Lines
...io-http/shared/src/main/scala/zio/http/Route.scala	43.75%	27 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2553      +/-   ##
==========================================
- Coverage   64.56%   64.41%   -0.16%     
==========================================
  Files         148      148              
  Lines        8649     8660      +11     
  Branches     1573     1563      -10     
==========================================
- Hits         5584     5578       -6     
- Misses       3065     3082      +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[vigoo]

Some random thoughts, only by looking at the code, did not play with it:

• it can be confusing what middleware you have to pass to this Routes constructor an what not (or what happens if you pass something that is not providing any context there? etc)
• also introducing a new route type with a different operator (partial route / -->) although I understand why, I'm afraid that it makes the API hard - when to use -> and when -->? What if I already have a Route value and want to mix it with partial routes? etc. I'm sure most of these have valid answers and maybe we can make it feel seamless, but it is not clear to me yet

On the other hand, I this proposal solves one special case when you want one context-providing middleware to provide context to a set of routes - which I think is a quite common case so maybe worth to introduce something "special" like this to help that use case.

[987Nabil]

@vigoo The middleware at the beginning is already the case if you define it per route. If you provide something without a context, nothing happens. You define just the routes like it would not be there.

To the second point, I agree. That is why this is a POC. What I try to proof is, that you can construct routes without a lot of type annotations and still provide a context. The API design / syntax is something different. But I can't imagine there is no way to find a solution for that. How beautiful it will be, I can't tell. But I think we are a step closer to something usable.

[987Nabil]

@jdegoes ping! You wanted to give me some feedback.

[jdegoes]

If we are going to use --> for this, then we should make sure to be consistent across all of ZIO HTTP.

In addition: We should give this a named variant too, rather than just the symbolic operator.

[987Nabil]

Can you be more specific on being consistent with what? I don't even say we should use it, but it was not possible to simply overload ->

[987Nabil]

@jdegoes I took a look again after letting intentionally some time pass. Which was good, since I managed to remove the new Route type.

I did this by restructuring the code for the Route.Builder. Instead of taking RoutePattern and HandlerAspect to build a route later on with a Handler, it takes now a RoutePattern and a Handler to build a route with a HandlerAspect.
This leads to the fact that the pattern -> aspect -> handler syntax is gone and is now instead pattern -> handler @@ aspect.

I provided a method currently called Routes.build

def build[Env, Err, Ctx](
    route: Route.Builder[Env, _, Ctx, _, Err],
    routes: Route.Builder[Env, _, Ctx, _, Err]*,
  ): Routes.Builder[Env, Ctx, Err] =
    Routes.Builder[Env, Ctx, Err](route +: routes)

And added a builder

  case class Builder[Env, Ctx, Err](routes: Seq[Route.Builder[Env, _, Ctx, _, Err]]){
   def @@(aspect: HandlerAspect[Env, Ctx]): Routes[Env, Err] =
     new Routes(Chunk.fromIterable(routes.map(_ @@ aspect)))
 }

Which leads to the syntax

Routes.build(
         Method.GET / "a"                  -> handler((ctx: AuthContext, _: Request) => Response.text(ctx.value)),
         Method.GET / "b" / int("id")      -> handler((id: Int, ctx: AuthContext, _: Request) =>
           Response.text(s"for id: $id: ${ctx.value}"),
         ),
         Method.GET / "c" / string("name") -> handler((name: String, ctx: AuthContext, _: Request) =>
           Response.text(s"for name: $name: ${ctx.value}"),
         ),
       ) @@ basicAuthContextM

Now we have basically two choices:

We leave the code structure wise as it is right now

Which means constructing routes inside of the two constructors Routes.apply and Routes.build works just fine. You can use for both pattern -> handler with the same requirements as on the main branch, which means explicit types for the handler input.
What does not work anymore is

val route = Method.GET / "endpoint" -> handler { (_: Request) =>
  ZIO.fail(new Exception("hmm..."))
}

This would need an explicit type to resolve to the right version of ->

val route: Route[Any, Exception] = Method.GET / "endpoint" -> handler { (_: Request) =>
  ZIO.fail(new Exception("hmm..."))
}
val route: Route.Builder[Any, (Int, AuthContext), AuthContext, (Int, AuthContext, Request), Nothing] = Method.GET / "b" / int("id") ->
  handler((id: Int, ctx: AuthContext, _: Request) =>  Response.text(s"for id: $id: ${ctx.value}"),
)

Have two different operators
Constructing routes will use -> for no context and another operator (maybe --> like in the prev version) for handlers with context.

Maybe the first solution could be more attractive, if we would have a way of simplifying the type parameters of Route.Builder[-Env, PC, Ctx, In, +Err] ?

Anyhow, I am sure this is a step forward overall.

@adamgfraser @vigoo Maybe you have some thoughts on this too.