fix(router): keep group auto-404 in sync with root RouteNotFound#3052
Conversation
Group.Use auto-registers nil-handler RouteNotFound routes so group
middleware still runs on 404s. Those handlers were filled from
r.notFoundHandler at registration time, which stayed the default even
after e.RouteNotFound("/*", h). Groups with middleware therefore used
the stock 404 JSON body instead of the root catch-all (labstack#2485).
When a RouteNotFound is added for /* or "", update r.notFoundHandler so
later group auto-404 routes inherit the same handler.
Fixes labstack#2485
aldas
left a comment
There was a problem hiding this comment.
I do not think adding a route should have side-effects like that to Router.
you can achieve same (global 404 handler) when you create Router with your own NotFoundHandler
so this
e := echo.New()
e.RouteNotFound("/*", func(c *echo.Context) error {
return c.NoContent(http.StatusNotFound)
})should be
e := echo.NewWithConfig(echo.Config{
Router: echo.NewRouter(echo.RouterConfig{
NotFoundHandler: func(c *echo.Context) error {
return c.NoContent(http.StatusNotFound)
},
}),
})…tFoundHandler Per maintainer guidance, adding a route via e.RouteNotFound must not have side effects on the Router. Revert the Add() mutation of r.notFoundHandler. For groups that auto-register catch-all 404 routes (i.e. groups with middleware), the supported way to customise the 404 handler is RouterConfig.NotFoundHandler, which the group's nil-handler routes resolve to at registration time and which group middleware wraps. Update the existing labstack#2485 test expectations to reflect this design and add TestGroup_RouteNotFoundUsesRouterConfig documenting the supported path.
|
@aldas agreed — a call to add a route shouldn't mutate the Router like that. I've dropped the side-effect entirely. What the reworked PR does instead:
So this PR now just documents + tests the existing intended behaviour rather than changing routing. If you think #2485 is better served purely by a docs note and no code/test changes at all, I can trim it further — let me know. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3052 +/- ##
=======================================
Coverage 93.34% 93.34%
=======================================
Files 43 43
Lines 4735 4735
=======================================
Hits 4420 4420
Misses 192 192
Partials 123 123 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary
When a group has middleware, Echo auto-registers
RouteNotFoundroutes with a nil handler so the group middleware still runs on 404s. Those handlers are filled fromDefaultRouter.notFoundHandler.e.RouteNotFound("/*", h)only installedhon the root tree node. It did not updater.notFoundHandler, so later group auto-404 routes kept the default JSON{"message":"Not Found"}instead of the root catch-all.Fix
On
DefaultRouter.Add, if method isRouteNotFoundand path is/*or"", also setr.notFoundHandler = route.Handler.Test plan
TestGroup_RouteNotFoundFallsBackToRoot(issue repro:/foo,/v0/foo,/v1/foo)TestGroup_RouteNotFound*/TestGroup_UseMultipleTimesgo test .Fixes #2485