fix(router): keep group auto-404 in sync with root RouteNotFound - #3052
Conversation
Sorry, something went wrong.
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)
},
}),
})
Sorry, something went wrong.
…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. |
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ 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.
|
Sorry, something went wrong.
aldas
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
Summary
When a group has middleware, Echo auto-registers RouteNotFound routes with a nil handler so the group middleware still runs on 404s. Those handlers are filled from DefaultRouter.notFoundHandler.
e.RouteNotFound("/*", h) only installed h on the root tree node. It did not update r.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 is RouteNotFound and path is /* or "", also set r.notFoundHandler = route.Handler.
Test plan
Fixes #2485