Fix Route merge order to fix Issue #3066

[exi]

In case of duplicate routes, the old logic gives precedence to older routes, which breaks cases where the routes are updated with new environments and re-added.

This gives precedence to the newer ones.

/claim #3066

[CLAassistant]

All committers have signed the CLA.

[exi]

@kyri-petrou

EDIT / PS: If this is indeed the solution to the issue, I think we should be fixing ++ instead and adding tests to ensure that it's left-associative

As far as understand, doing ++ on routes just concatenates them in a list. That list is then checked left-to-right during calls to search for appropriate request handlers. Since in the linked issue, we add the same route twice, with different environments, before this PR the old route takes precedence, after this PR, the new one does.

I agree that I would have expected routes to be merged route-by-route, but figured that there probably was a good reason to just put then in a list.

[exi]

Adding the test case is complicated because it depends on TestServer, which is afaik not usable in zio-http because it would lead to circular dependencies. For my local testing I added a new test top level sbt module named zio-http-regressiontests. I'm happy to include it but it seems a bit misplaced since it only contains one test.

[987Nabil]

Even if this works, this is not how ++ should work. Like on Map the right should win. If there is a bug, it is way deeper in the code

[exi]

@987Nabil Do you mean that the request handlers should be constructed from the routes iterating back to front over them or do you mean that the request handler tree building code should read front to back but override older values or at least prefer newer ones? I could implement that.

[exi]

@987Nabil @kyri-petrou

I updated the PR to change the handler merging logic in cases of multiple handlers for the same path. The old code iterated over the handler from left to right, the new one flips that and also gets rid of explicit loops in favor of a reduction.

[exi]

Since the new code is more general, i also got rid of the special casing between one handler and more than one handler.

[exi]

Lot's of tests failing, I'll check what's up with that.

[987Nabil]

This change does not look right either. Neither the place nor the fact that we replace a while loop with allocation of an interator

[exi]

@987Nabil Then i'm not sure what the intended behaviour is.

Routes uses the normal ++ behaviour, which when doing oldApp ++ newApp, will create Chunks of routes and put the new one at the end/right. This later gets dumped into then handler tree building where routes are aggregated by path, while still keeping the chunk order of the handlers. The handler tree building at the lowest level creates the real handler where in the case of the code I changed here, the old code only ever looks at the second handler in the list when the first handler returned Status.NotFound.

And since the chunks and therefore the handlers are put into "oldest left" order and the handler building iterates left-to-right, the oldest one gets preference every time.

Alternative

I could change the ++ implementation for Routes and do an actual merge instead of a concatenation, and throw away the old handler in case of duplicate paths. Some other ++ implementations seem to be doing that.

[exi]

@guizmaii @jdegoes I went through a couple of iterations in this PR already but I'm now a bit lost on what the desired solution is. I think the key issue is that ++ for Routes just appends the new routes to the list of old routes and the handler building walks this list front-to-back and uses whatever handler works first. I could either change the handler building logic to work backwards, or I could implement a different ++ for Routes where that discards old handlers during the merge. Which one is the preferred solution here?

[exi]

@kyri-petrou @987Nabil Turns out, our notion of how ++ is supposed to work is wrong for Routes. I just found this comment:

https://github.com/zio/zio-http/blob/502331c7c8ec548b326120a9596864d30fabf4a7/zio-http/shared/src/main/scala/zio/http/Routes.scala#L51-L57

It seems like "old (left) has precedence over new (right)" is the intended behaviour. Which makes addRoutes for the same route unusable during tests. I also think that many of the tests zio-http has that use addRoutes do not expect this behaviour.

[987Nabil]

@jdegoes I find this behaviour confusing. Everywhere else we give "the new thing" precedence. Should we change it?

[987Nabil]

closing in favor of #3337