Make newly added Routes take precedence over old ones (#3066) #3337
Conversation
987Nabil
requested review from
jdegoes,
vigoo and
kyri-petrou
as code owners
987Nabil
force-pushed
the
empty-collections-default
branch
3 times, most recently
from
eced14f to
66bb39e
Compare
987Nabil
force-pushed
the
empty-collections-default
branch
from
66bb39e to
3082fae
Compare
Please, explain what you mean by this! |
| @@ -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 | |||
There was a problem hiding this comment.
@jdegoes it is about this place. Before this changes we could end up with n matches under two conditions.
- You have the exact same
RoutePattern - You have the same path and the method is
ANY
We would select the first handler aka the first route that was added. The fallback acc.catchAll, is only triggered, if the user would create a response with status NotFound in the error channel of the handler.
This is afaics undocumented and in my opinion not well designed behavior.
My change now does two things.
- It selects only the last added handler for identical route patterns. This is a real change, since it was before the first added handler that was selected. I think the last makes more sense. It basically means, that routes is a "map"
- It returns the any method route handler only if there was no match for a non any method route pattern that matched the incoming request.
I expect that nobody uses the fallback, since nobody knows it exists. (I know we did not use it internally in http)
And the change from first to last I consider good since I think this is what you would expect anyway when you don't read docs. And it is the cause of the issue I am trying to solve.
What could be optimized is the tree creation. Currently we have still have all alternatives for the same route pattern in the tree. We could just keep the last one.
But idk if we really need to optimize for adding multiple routes of the same pattern. I think it is an edge case.
The simplification might tho improve performance for the single pattern path. I did not check this in detail.
There was a problem hiding this comment.
I am fine deleting the special case behavior, which I assume would mean there is only ever a single handler, because during composition, you can discard old ones. Right?
There was a problem hiding this comment.
Would it be possible to warn the user when he/she defines a Route which already exists?
During compilation would be nice, but it's probably quite complicated to implement. At least at runtime, that would help people understand their mistake.
Because it seems to me pretty unlikely someone would want to have twice the same route declared in his/her API 🤔
It's probably a programming / API composition mistake.
There was a problem hiding this comment.
@jdegoes if you would have a Routes object that has two routes with the same pattern, my change would keep both in the data structure, but always select the last added. We could probably change it, so that only the last added stays in routes. But that was more complicated and at least for now I choose the easy option.
There was a problem hiding this comment.
@guizmaii I think you are right that it is most likely a mistake. I think compile time is too complex. Too many ways to create Routes. I think the right place would be when adding it to the server or during a request. Maybe both.
There was a problem hiding this comment.
I'm very hesitant to approve this as I think it has the potential to break user applications in some edge-case usage patterns. Some time ago (during RC) I remember having issues when some of the routes where created via tapir and some "natively" and had to use specific ordering of routes in order to get things working.
I'm not sure if with the current zio-http / tapir version this would be an issue, or whether the changes in this PR would break anything in that application. Just to try to tick as many boxes as possible; once a snapshot version is out, would you be able to run tapir's zio-http test suite just to make sure we're not breaking anything there?
|
@kyri-petrou will do. Maybe we should set this up as an automation. |
987Nabil
force-pushed
the
empty-collections-default
branch
from
8075080 to
66cd66e
Compare
987Nabil
force-pushed
the
empty-collections-default
branch
from
66cd66e to
dcb6b6e
Compare
I am not 100% sure this is the right way.
I wonder, why we keep multiple routes with the same path anyway?
The old route aka fall back is taken only, if there is a response in the error channel returned in the new route by the user that has a status NotFound.
This is not only oddly specific, but also seems complicated, unexpected and does not work for the EndpointAPI.
I think we should probably get rid of duplicated Method + Path combinations.
fixes #3066
/claim #3066