Visitar URL original
fix: don't drop handlers that collide with a Mount()/Route() pattern … · go-chi/chi@29164f0 · GitHub
Skip to content

Commit 29164f0

Browse files
VojtechVitekVojtechVitek
authored
fix: don't drop handlers that collide with a Mount()/Route() pattern (#1148)
* fix: don't drop handlers that collide with a Mount()/Route() pattern Mount() (and Route(), which is sugar for it) registers a synthetic "stub" handler on its exact mount pattern to connect it to the subrouter. node.routes() used that stub as a signal to hide the whole node from Routes()/Walk() — but if a real handler was also registered on that same pattern (e.g. r.Get("/bar", h) next to r.Route("/bar", ...)), it got hidden too, even though the route is perfectly routable. routes() now compares each handler against the stub by identity (sameHandler, using reflection since http.HandlerFunc isn't == comparable) and only hides the stub itself, keeping any real handler that shares the node. The "*" slot is only filtered on leaf nodes, since Walk() also reads Handlers["*"] to detect a *ChainHandler and propagate With() middleware into a subrouter — that entry has to survive even when it's the same value as the stub. Fixes #830. Also adds a regression test for #750 (middlewares registered on a Group not reaching handlers on a Route() mounted inside it), which turns out to already be fixed on master — this just locks it in. * Trim comments in routes() to just the why * Rename sameHandler to equalHandlers; narrow the reflect switch to Func Chan, Pointer, and UnsafePointer are already == comparable in Go — only Func, Map, and Slice aren't, and Map/Slice identity was never a real case here (the stub is always http.HandlerFunc or *ChainHandler). * Explain why Routes() reports /api/* and not /api/{id} in TestRoutesHidesMountStub * Trim comment in TestRoutesHidesMountStub
1 parent bc02284 commit 29164f0

2 files changed

Lines changed: 176 additions & 4 deletions

File tree

‎tree.go‎

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ package chi
77
import (
88
"fmt"
99
"net/http"
10+
"reflect"
1011
"regexp"
1112
"slices"
1213
"sort"
@@ -629,8 +630,10 @@ func (n *node) routes() []Route {
629630
rts := []Route{}
630631

631632
n.walk(func(eps endpoints, subroutes Routes) bool {
632-
if eps[mSTUB] != nil && eps[mSTUB].handler != nil && subroutes == nil {
633-
return false
633+
// Hide Mount()'s stub handler, but not a real handler sharing its pattern.
634+
var stubHandler http.Handler
635+
if eps[mSTUB] != nil {
636+
stubHandler = eps[mSTUB].handler
634637
}
635638

636639
// Group methodHandlers by unique patterns
@@ -650,19 +653,29 @@ func (n *node) routes() []Route {
650653

651654
for p, mh := range pats {
652655
hs := make(map[string]http.Handler)
656+
657+
// Walk() reads Handlers["*"] for With() middleware when recursing
658+
// into a subroute, so keep it there even if it's also the stub.
653659
if mh[mALL] != nil && mh[mALL].handler != nil {
654-
hs["*"] = mh[mALL].handler
660+
if subroutes != nil || !equalHandlers(mh[mALL].handler, stubHandler) {
661+
hs["*"] = mh[mALL].handler
662+
}
655663
}
656664

657665
for mt, h := range mh {
658-
if h.handler == nil {
666+
if h.handler == nil || equalHandlers(h.handler, stubHandler) {
659667
continue
660668
}
661669
if m, ok := reverseMethodMap[mt]; ok {
662670
hs[m] = h.handler
663671
}
664672
}
665673

674+
// Keep subroute nodes so Walk() can recurse; a stub-only leaf has nothing to report.
675+
if len(hs) == 0 && subroutes == nil {
676+
continue
677+
}
678+
666679
rt := Route{subroutes, hs, p}
667680
rts = append(rts, rt)
668681
}
@@ -673,6 +686,29 @@ func (n *node) routes() []Route {
673686
return rts
674687
}
675688

689+
// equalHandlers reports whether a and b are the same handler value. Handlers
690+
// are commonly funcs (e.g. http.HandlerFunc), and a direct == on those
691+
// panics at runtime, so funcs are compared by pointer instead.
692+
func equalHandlers(a, b http.Handler) bool {
693+
if a == nil || b == nil {
694+
return a == b
695+
}
696+
697+
av := reflect.ValueOf(a)
698+
bv := reflect.ValueOf(b)
699+
if av.Type() != bv.Type() {
700+
return false
701+
}
702+
703+
if av.Kind() == reflect.Func {
704+
return av.Pointer() == bv.Pointer()
705+
}
706+
if av.Type().Comparable() {
707+
return a == b
708+
}
709+
return false
710+
}
711+
676712
func (n *node) walk(fn func(eps endpoints, subroutes Routes) bool) bool {
677713
// Visit the leaf values if any
678714
if (n.endpoints != nil || n.subroutes != nil) && fn(n.endpoints, n.subroutes) {

‎tree_test.go‎

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ import (
44
"fmt"
55
"log"
66
"net/http"
7+
"reflect"
8+
"slices"
79
"testing"
810
)
911

@@ -577,3 +579,137 @@ func TestWalkInlineMiddlewaresAcrossSubrouter(t *testing.T) {
577579
})
578580
}
579581
}
582+
583+
// https://github.com/go-chi/chi/issues/830
584+
func TestWalkRouteWithHandlerAndSubrouter(t *testing.T) {
585+
r := NewRouter()
586+
handler := func(w http.ResponseWriter, r *http.Request) {}
587+
588+
r.Route("/foo", func(r Router) {
589+
r.Route("/bar", func(r Router) {
590+
r.Get("/{id}", handler)
591+
})
592+
r.Get("/bar", handler)
593+
})
594+
595+
routes := map[string]bool{}
596+
if err := Walk(r, func(method, route string, handler http.Handler, middlewares ...func(http.Handler) http.Handler) error {
597+
routes[method+" "+route] = true
598+
return nil
599+
}); err != nil {
600+
t.Fatal(err)
601+
}
602+
603+
for _, route := range []string{"GET /foo/bar", "GET /foo/bar/{id}"} {
604+
if !routes[route] {
605+
t.Fatalf("expected Walk to include %s, got %v", route, routes)
606+
}
607+
}
608+
}
609+
610+
// https://github.com/go-chi/chi/issues/830
611+
//
612+
// Routes() must not leak the synthetic stub handler Mount() installs on
613+
// its mount pattern: a plain Mount()/Route() should report exactly its
614+
// "/pattern/*" entry, and a handler that collides with a mount pattern
615+
// must be reported without also surfacing the internal stub under "*".
616+
func TestRoutesHidesMountStub(t *testing.T) {
617+
handler := func(w http.ResponseWriter, r *http.Request) {}
618+
619+
t.Run("plain mount reports a single subroute entry", func(t *testing.T) {
620+
r := NewRouter()
621+
r.Route("/api", func(r Router) {
622+
r.Get("/{id}", handler)
623+
})
624+
625+
// Routes() isn't recursive: /api mounts a separate subrouter, so only
626+
// its "/api/*" connector shows here, not "/api/{id}". SubRoutes can be
627+
// walked further, the same way Walk() does internally.
628+
routes := r.Routes()
629+
if len(routes) != 1 {
630+
t.Fatalf("expected exactly 1 route, got %d: %+v", len(routes), routes)
631+
}
632+
if routes[0].Pattern != "/api/*" || routes[0].SubRoutes == nil {
633+
t.Fatalf("expected the single route to be the /api/* subroute, got %+v", routes[0])
634+
}
635+
})
636+
637+
t.Run("handler colliding with a mount pattern hides the stub", func(t *testing.T) {
638+
r := NewRouter()
639+
r.Route("/bar", func(r Router) {
640+
r.Get("/{id}", handler)
641+
})
642+
r.Get("/bar", handler)
643+
644+
for _, rt := range r.Routes() {
645+
if rt.Pattern != "/bar" {
646+
continue
647+
}
648+
if rt.SubRoutes != nil {
649+
t.Fatalf("expected /bar to have no subroutes of its own, got %+v", rt)
650+
}
651+
if len(rt.Handlers) != 1 {
652+
t.Fatalf("expected /bar to report exactly the GET handler, got %v", rt.Handlers)
653+
}
654+
if _, ok := rt.Handlers[http.MethodGet]; !ok {
655+
t.Fatalf("expected /bar to report a GET handler, got %v", rt.Handlers)
656+
}
657+
}
658+
})
659+
}
660+
661+
// https://github.com/go-chi/chi/issues/750
662+
//
663+
// Middlewares registered on a Group must still reach handlers defined on a
664+
// Route() mounted inside that group.
665+
func TestWalkMiddlewaresAcrossGroupAndRoute(t *testing.T) {
666+
requestID := func(next http.Handler) http.Handler { return next }
667+
timeout := func(next http.Handler) http.Handler { return next }
668+
handler := func(w http.ResponseWriter, r *http.Request) {}
669+
670+
r := NewRouter()
671+
r.Use(requestID)
672+
r.Get("/A", handler)
673+
674+
r.Group(func(r Router) {
675+
r.Use(timeout)
676+
r.Get("/B", handler)
677+
r.Route("/C", func(r Router) {
678+
r.Get("/D", handler)
679+
})
680+
})
681+
682+
mwName := func(f func(http.Handler) http.Handler) string {
683+
switch reflect.ValueOf(f).Pointer() {
684+
case reflect.ValueOf(requestID).Pointer():
685+
return "requestID"
686+
case reflect.ValueOf(timeout).Pointer():
687+
return "timeout"
688+
default:
689+
return "unknown"
690+
}
691+
}
692+
693+
got := map[string][]string{}
694+
if err := Walk(r, func(method, route string, handler http.Handler, mws ...func(http.Handler) http.Handler) error {
695+
names := make([]string, len(mws))
696+
for i, m := range mws {
697+
names[i] = mwName(m)
698+
}
699+
got[method+" "+route] = names
700+
return nil
701+
}); err != nil {
702+
t.Fatal(err)
703+
}
704+
705+
want := map[string][]string{
706+
"GET /A": {"requestID"},
707+
"GET /B": {"requestID", "timeout"},
708+
"GET /C/D": {"requestID", "timeout"},
709+
}
710+
for route, wantMws := range want {
711+
if gotMws := got[route]; !slices.Equal(gotMws, wantMws) {
712+
t.Fatalf("%s: expected middlewares %v, got %v", route, wantMws, gotMws)
713+
}
714+
}
715+
}

0 commit comments

Comments
 (0)