zio-http: Use Http.collectHandler

[frekw]

This is an attempt to fix #2708. Sorry about getting it wrong the first time, I'm still not completely familiar with the Middleware, Routing and Handler changes in zio-http 0.0.4.

If I understand zio/zio-http#1975 (comment) correctly, the issue with the current interpreter is that we're effectfully creating a handler here, which in turn will result in our interpreter accepting all routes (and more or less doing its internal routing later), which makes the middlewares run incorrectly from the point of view of a user.

This instead tries to adapt the pattern that the play interpreter uses and constructs a PartialFunction[Request, Handler[R, Throwable, Request, Response] which we then pass to Http.collectHandler, which if I understand it correctly, means we're doing the right thing.

I almost have it working, but a couple of tests relating to rejection are failing:

[info] - given a list of endpoints, should return 405 for unsupported methods *** FAILED *** (12 milliseconds)
[info]   404 was not equal to 405 (ServerRejectTests.scala:30)
[info] - given a list of endpoints with different paths, should return 405 for unsupported methods *** FAILED *** (10 milliseconds)
[info]   404 was not equal to 405 (ServerRejectTests.scala:37)
[info] - given a list of endpoints and a customized reject handler, should return a custom response for unsupported methods *** FAILED *** (10 milliseconds)
[info]   404 was not equal to 400 (ServerRejectTests.scala:52)
[info] - returns tapir-generated 404 when defaultHandlers(notFoundWhenRejected = true) is used *** FAILED *** (7 milliseconds)
[info]   Left("") was not equal to Left("ERROR: Not Found") (ServerOptionsTests.scala:40)

I think this is because we do as the Play interpreter does and setup a custom DecodeFailureHandler to use from the partial function, but I'm not sure. And I'm not very familiar with Tapir so could probably use a pointer or two to fix them :)

[frekw]

Wait, now that I think of it I guess it's how zio-http is handling unmatched routes in Http.collectHandler.

I'm not sure this is something we can actually control from the interpreter's side without reintroducing the previous issues (since it would require us to execute everything in order to know if we have a matching route or not).

We might need another approach to support it. I'll see if I can figure something out.

[frekw]

I tried another approach still using fromOptionalHandlerZIO but returning None in the case where we have no matches, but we get the same error. When doing some more digging, this seems to be controlled by the zio-http server implementation: https://github.com/zio/zio-http/blob/699649636ac7547deb3454e27db14d98dafa2294/zio-http/src/main/scala/zio/http/netty/server/ServerInboundHandler.scala#L258-L289 and thus not something we can control (without re-introducing the previous issues).

@adamw what do you think should be our approach here? Even if the current implementation passes more tests, how it (doesn't) work with middlewares feel pretty problematic to me. Should we disable some of these tests for zio-http?

[frekw]

Ok, I actually managed to work around by defaulting to the previous implementation in cases of unmatched routes. 🎉

I also added some tests for middleware to verify that they run before the handler.

[frekw]

Scala 3 seems to dislike the zio-http middleware so I removed those tests.

[adamw]

Thanks for your work on this! I haven't looked at the new zio-http API, so I'm not familiar with it at all, and I guess I wouldn't be of much help.

It is a bit unfortunate that you have to define a PartialFunction. In Play this is the only option (since the API is stabilized long ago), but in zio-http maybe we could lobby for sth better :). It would be a shame to repeat Play's mistakes (at least as long as it comes to integrating other routing libraries). Having a Request => ZIO[Option[Response]] instead of a PartialFunction[Request, ZIO[Response]] would do the trick.

The problem with the PartialFunction is that currently endpoint decoding happens twice: once in isDefined, and then another time in apply. That's a waste ;).

Also, why do we need both routes and default? The code seems to be duplicated there.

Would you be willing to open an issue in zio-http to suggest some improvements to the routing integration, which we can then leverage?

[frekw]

It is a bit unfortunate that you have to define a PartialFunction

Yes, and this is basically a result of zio-http 0.0.4 completely redesigning their model. The idea is to separate the routing and handling of a request into two different, but related data types. My understanding is that the reason for doing so is to be able to be more precise around what middlewares are applicable to what, and that some are only applicable to routing (because they inherently change how the routing works, such as allowing or disallowing a route). You can find more information about it here: zio/zio-http#1916

Not saying I necessarily agree with all the changes, but I think it was needed in order to have sane composition of zio-apps with middlewares.

Having a Request => ZIO[Option[Response]] instead of a PartialFunction[Request, ZIO[Response]] would do the trick.

That would also have made this update considerably easier, but I think it's unlikely to change since they spent most development time between 0.0.3 and 0.0.4 to get rid of that exact behaviour. But of course we can open and raise it :)

The problem with the PartialFunction is that currently endpoint decoding happens twice: once in isDefined, and then another time in apply. That's a waste ;).

Yeah that's an unfortunate consequence of both libraries basically wanting to control routing in this case, I think. I guess another option would be to see if we can find a cheaper way to evaluate if an endpoint should match a request or not on the tapir side?

Also, why do we need both routes and default? The code seems to be duplicated there.

Yes, the code is very similar but with subtle differences. What we're defining top-level is a routing handler function. If the PartialFunction we define and pass to Http.collectHandler doesn't match anything, our endpoints won't get executed. But since tapir wants to "own" the routing, but in case we get an error, we then need to fall back to more or less the exact same code path to handle Tapir's routing errors, but in a way that also gives us the option of not handling the request (this line). Otherwise it seems like we won't try the next part of the chain.

At first I thought val default = Handler.fromFunctionHandler[Request](req => handleRequest(req)).toHttp would work but that fails for the 404 case.

I agree that the implementation isn't ideal, but I'm not sure that we can do better without changes on either side. :/

[frekw]

But on the positive side at least with this change it's possible to use routing middleware such as auth with tapir endpoints. 🙂

[adamw]

So I took a bit of a deeper dive, and I think I now roughly understand the problem. It seems we are currently not able to efficiently integrate with zio-http, which is unfortunate, but I'll create an issue there and link it so maybe we can improve the situation.

Unfortunately tapir can't change the way it performs routing/handling, this is fixed since 1.0. The problem is that deciding if an endpoint matches a request is combined with extracting values from the request - this is a single step. So you've got sth like a Endpoint[Input] => Request => Option[Input] function; if it's a None, the endpoint failed to decode and it doesn't match (well, it could also be a bad request - this is decided by DecodeFailureHandler); if it's a Some the endpoint logic is called. But there's no separate "routing" and "handling" steps.

I've also removed the default handler, as its presence goes against the whole manouver of having the PartialFunction - we assume that tapir handles only some endpoints in the application, and if none match, we want to try the other ones. Having a default would prevent this. Unfortunately, the PartialFunction approach also disables some functionality (e.g. serving method not allowed responses). The same is true for Play.

Finally, I've fixed (by explicitly adding type params :) ) the tests and enabled them for Scala 3.

I agree that while the solution is not perfect, it's better to have an inefficient one than a broken one :) Thanks for your work on this!

[frekw]

Thanks so much for spending time on this @adamw 🙏

I've also removed the default handler, as its presence goes against the whole manouver of having the PartialFunction - we assume that tapir handles only some endpoints in the application, and if none match, we want to try the other ones. Having a default would prevent this. Unfortunately, the PartialFunction approach also disables some functionality (e.g. serving method not allowed responses). The same is true for Play.

Yeah I think this makes the most sense as well, otherwise we'll essentially be doing error handling twice. Should also make the code much clearer, which is a nice benefit.

Finally, I've fixed (by explicitly adding type params :) ) the tests and enabled them for Scala 3.

Nice! I think I need to up my Scala 3-chops 😂

I saw your issue in zio-http, will be interesting to hear what the zio-http maintainers think!