Skip to content

Commit 3081f5e

Browse files
wuyangfancursoragent
andcommitted
fix: inherit parent RouteNotFound handler for groups with middleware
When a group registers middleware, reuse the most specific parent RouteNotFound handler instead of always falling back to the default JSON NotFoundHandler. Fixes #2485 Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 25685e6 commit 3081f5e

4 files changed

Lines changed: 116 additions & 6 deletions

File tree

group.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,12 @@ func (g *Group) Use(middleware ...MiddlewareFunc) {
2828
// are only executed if they are added to the Router with route.
2929
// So we register catch all route (404 is a safe way to emulate route match) for this group and now during routing the
3030
// Router would find route to match our request path and therefore guarantee the middleware(s) will get executed.
31-
g.RouteNotFound("", NotFoundHandler)
32-
g.RouteNotFound("/*", NotFoundHandler)
31+
handler := NotFoundHandler
32+
if inherited := g.echo.router.inheritedRouteNotFoundHandler(g.prefix); inherited != nil {
33+
handler = inherited
34+
}
35+
g.RouteNotFound("", handler)
36+
g.RouteNotFound("/*", handler)
3337
}
3438

3539
// CONNECT implements `Echo#CONNECT()` for sub-routes within the Group.
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
package echo
2+
3+
import (
4+
"io"
5+
"net/http"
6+
"net/http/httptest"
7+
"testing"
8+
9+
"github.com/stretchr/testify/require"
10+
)
11+
12+
func TestGroupRouteNotFoundFallsBackToRootHandler(t *testing.T) {
13+
e := New()
14+
e.HideBanner = true
15+
e.HidePort = true
16+
17+
e.RouteNotFound("/*", func(c Context) error {
18+
return c.NoContent(http.StatusNotFound)
19+
})
20+
21+
v0 := e.Group("/v0", func(next HandlerFunc) HandlerFunc {
22+
return func(c Context) error {
23+
return next(c)
24+
}
25+
})
26+
v0.POST("/resource", func(c Context) error {
27+
return c.NoContent(http.StatusOK)
28+
})
29+
30+
v1 := e.Group("/v1")
31+
v1.POST("/resource", func(c Context) error {
32+
return c.NoContent(http.StatusOK)
33+
})
34+
35+
srv := httptest.NewServer(e)
36+
t.Cleanup(srv.Close)
37+
38+
for _, path := range []string{"/foo", "/v0/foo", "/v1/foo"} {
39+
t.Run(path, func(t *testing.T) {
40+
req, err := http.NewRequest(http.MethodPost, srv.URL+path, nil)
41+
require.NoError(t, err)
42+
43+
resp, err := srv.Client().Do(req)
44+
require.NoError(t, err)
45+
defer resp.Body.Close()
46+
47+
b, err := io.ReadAll(resp.Body)
48+
require.NoError(t, err)
49+
50+
require.Equal(t, http.StatusNotFound, resp.StatusCode)
51+
require.Empty(t, b)
52+
require.Equal(t, "0", resp.Header.Get("Content-Length"))
53+
})
54+
}
55+
}

group_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -204,17 +204,17 @@ func TestGroup_RouteNotFoundWithMiddleware(t *testing.T) {
204204
expectCode: http.StatusNotFound,
205205
},
206206
{
207-
name: "ok, default group 404 handler is called with middleware",
207+
name: "ok, inherited root 404 handler is called with middleware",
208208
givenCustom404: false,
209209
whenURL: "/group/test3",
210-
expectBody: "{\"message\":\"Not Found\"}\n",
210+
expectBody: "GET /group/*",
211211
expectCode: http.StatusNotFound,
212212
},
213213
{
214-
name: "ok, (no slash) default group 404 handler is called with middleware",
214+
name: "ok, (no slash) inherited root 404 handler is called with middleware",
215215
givenCustom404: false,
216216
whenURL: "/group",
217-
expectBody: "{\"message\":\"Not Found\"}\n",
217+
expectBody: "GET /group",
218218
expectCode: http.StatusNotFound,
219219
},
220220
}

router.go

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"bytes"
88
"fmt"
99
"net/http"
10+
"strings"
1011
)
1112

1213
// Router is the registry of all registered routes for an `Echo` instance for
@@ -155,6 +156,56 @@ func (r *Router) Routes() []*Route {
155156
return routes
156157
}
157158

159+
func (r *Router) inheritedRouteNotFoundHandler(groupPrefix string) HandlerFunc {
160+
var best *routeMethod
161+
var walk func(n *node)
162+
walk = func(n *node) {
163+
if h := n.notFoundHandler; h != nil && isParentRouteNotFound(h.ppath, groupPrefix) {
164+
if best == nil || routeNotFoundSpecificity(h.ppath) > routeNotFoundSpecificity(best.ppath) {
165+
best = h
166+
}
167+
}
168+
for _, child := range n.staticChildren {
169+
walk(child)
170+
}
171+
if n.paramChild != nil {
172+
walk(n.paramChild)
173+
}
174+
if n.anyChild != nil {
175+
walk(n.anyChild)
176+
}
177+
}
178+
walk(r.tree)
179+
if best != nil {
180+
return best.handler
181+
}
182+
return nil
183+
}
184+
185+
func isParentRouteNotFound(routePath, groupPrefix string) bool {
186+
if routePath == groupPrefix || routePath == groupPrefix+"/*" {
187+
return false
188+
}
189+
if routePath == "" || routePath == "/*" {
190+
return true
191+
}
192+
if strings.HasSuffix(routePath, "/*") {
193+
base := strings.TrimSuffix(routePath, "/*")
194+
if base == "" {
195+
return true
196+
}
197+
return groupPrefix == base || strings.HasPrefix(groupPrefix, base+"/")
198+
}
199+
return groupPrefix == routePath || strings.HasPrefix(groupPrefix, routePath+"/")
200+
}
201+
202+
func routeNotFoundSpecificity(path string) int {
203+
if path == "" || path == "/*" {
204+
return 0
205+
}
206+
return len(path)
207+
}
208+
158209
// Reverse generates a URL from route name and provided parameters.
159210
func (r *Router) Reverse(name string, params ...interface{}) string {
160211
uri := new(bytes.Buffer)

0 commit comments

Comments
 (0)