Remove group.Use registering Any routes that break other routes

[segevfiner]

This can be a breaking change if this behavior is somehow important to anyone.

Fixes #1657

[codecov]

Codecov Report

Merging #1674 (7a1126f) into master (151ed6b) will decrease coverage by 0.95%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1674      +/-   ##
==========================================
- Coverage   85.28%   84.33%   -0.96%     
==========================================
  Files          28       28              
  Lines        2216     1909     -307     
==========================================
- Hits         1890     1610     -280     
+ Misses        212      189      -23     
+ Partials      114      110       -4     

Impacted Files	Coverage Δ
group.go	100.00% <ø> (ø)
middleware/proxy.go	60.97% <0.00%> (-5.37%)	⬇️
middleware/method_override.go	80.95% <0.00%> (-3.67%)	⬇️
middleware/redirect.go	85.00% <0.00%> (-3.24%)	⬇️
context.go	86.59% <0.00%> (-3.13%)	⬇️
middleware/basic_auth.go	65.62% <0.00%> (-2.95%)	⬇️
middleware/util.go	88.88% <0.00%> (-1.74%)	⬇️
response.go	85.18% <0.00%> (-1.66%)	⬇️
middleware/cors.go	77.94% <0.00%> (-1.52%)	⬇️
echo.go	84.75% <0.00%> (-1.18%)	⬇️
... and 19 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 151ed6b...7a1126f. Read the comment docs.

[lammel]

I actually expected some tests to break with your changes.
Please add tests for that behaviour (like shown in #1657), so we can ensure consistent behaviour later on.

I need to tackle some router tickets anyway, so I'll try to take a deeper look then.

[segevfiner]

I actually expected some tests to break with your changes.

I did too. I was surprised when none did. Guess the previous behavior this sought to achieve is not covered.

Please add tests for that behaviour (like shown in #1657), so we can ensure consistent behaviour later on.

I added the sample from the issue as a test, modified a bit to be slightly more consistent with the rest of them.

[lammel]

I'll need some time to review that, as there might be implications that we do not see or test currently.
Thanks for the added test, that will help a lot.

[lammel]

There really seems to be no real impact.
So I'm in favor of merging.

[pafuent]

LGTM!

[pafuent]

I found why at least one of those two removed lines were "needed". After the merge of this PR, the tests on master start failing (https://github.com/labstack/echo/runs/1561607655?check_suite_focus=true)
The use case that needs at least the g.Any("", NotFoundHandler) line is when you create a Group with only a Static Middleware attach to it. At first glance, it sounds logic to not add any route to that group because is responsibility of the middleware to handle all the requests. The issue here is that the creation of a Group and assign to it just a middleware don't do anything against the Echo Router, so any request that should get to that Group never reach it.
I still need to think in a proper solution for this, but until we find out what to do this PR needs to be removed from master. @lammel How was this situation handled in the past? (e.g. Use the GitHub Revert functionality)

[segevfiner]

Why would you add a group with only a middleware? Why not make it an handler? As in, why a static middleware and not a static handler?

[lammel]

I found why at least one of those two removed lines were "needed". After the merge of this PR, the tests on master start failing (https://github.com/labstack/echo/runs/1561607655?check_suite_focus=true)
The use case that needs at least the g.Any("", NotFoundHandler) line is when you create a Group with only a Static Middleware attach to it. At first glance, it sounds logic to not add any route to that group because is responsibility of the middleware to handle all the requests. The issue here is that the creation of a Group and assign to it just a middleware don't do anything against the Echo Router, so any request that should get to that Group never reach it.
I still need to think in a proper solution for this, but until we find out what to do this PR needs to be removed from master. @lammel How was this situation handled in the past? (e.g. Use the GitHub Revert functionality)

Why did the tests not fail on the PR?
Yes, we need to revert and check the newly found reason for that.

Sadly I do not know how that was handled before. I'd love to have an unmerge button ;-)
Otherwise we can o a manual revert on master.

[lammel]

Why would you add a group with only a middleware? Why not make it an handler? As in, why a static middleware and not a static handler?

Our master branch shall be stable, so tests must not fail there. We need to resolve that.
Of course the use case could be wrong, but that would mean we need to rethink and adjust tests, which may require changes to the static middleware too.

[segevfiner]

Note that if you revert it on master, to restore it, you need to "unrevert" the revert. That is, revert the revert commit.

[lammel]

To avoid reverting of reverted reverts, please resubmit this PR as a new one again (referencing this one).
Thanks @segevfiner !