From 1e39af4f589161275558023e648444b4b6001bf1 Mon Sep 17 00:00:00 2001 From: Gates Wang <9372086+SetagGnaw@users.noreply.github.com> Date: Sun, 2 Aug 2026 19:10:04 -0400 Subject: [PATCH] fix(tree): roll back to skipped wildcards at param route dead ends getValue records skipped wildcard siblings whenever it prefers a static branch, but two dead ends inside the param case returned "not found" without consulting that stack: - more path remains but the param node has no child to descend into - the param node consumed the whole path but has no handler registered Example: with routes /:p0/:p1 and /bc/:p1/a registered, a request for /bc/bc returned 404 instead of matching /:p0/:p1. Both dead ends now roll back to the last valid skippedNode, consistent with the existing fallback at the other dead ends. The rollback loop, previously duplicated inline at three call sites, is extracted into rollbackToSkippedNode and reused at all five. --- tree.go | 82 +++++++++++++++++++++++++++++----------------------- tree_test.go | 33 +++++++++++++++++++++ 2 files changed, 79 insertions(+), 36 deletions(-) diff --git a/tree.go b/tree.go index 580abbaf..0d9cf6bd 100644 --- a/tree.go +++ b/tree.go @@ -410,6 +410,36 @@ type skippedNode struct { paramsCount int16 } +// rollbackToSkippedNode unwinds the skipped-wildcard stack after the walk hit +// a dead end. It pops entries until it finds one whose saved remaining path +// ends with the current remaining path, i.e. a branch point on the walk that +// led here; entries from abandoned branches are discarded. If one is found, +// the walk state saved there (remaining path, current node, captured params) +// is restored and true is returned; the caller must then resume the walk +// loop, which retries from the branch point with its wildcard child. +func rollbackToSkippedNode( + skippedNodes *[]skippedNode, + path *string, + n **node, + params *Params, + globalParamsCount *int16, +) bool { + for length := len(*skippedNodes); length > 0; length-- { + sn := (*skippedNodes)[length-1] + *skippedNodes = (*skippedNodes)[:length-1] + if strings.HasSuffix(sn.path, *path) { + *path = sn.path + *n = sn.node + if params != nil { + *params = (*params)[:sn.paramsCount] + } + *globalParamsCount = sn.paramsCount + return true + } + } + return false +} + // Returns the handle registered with the given path (key). The values of // wildcards are saved to a map. // If no handle can be found, a TSR (trailing slash redirect) recommendation is @@ -457,18 +487,8 @@ walk: // Outer loop for walking the tree // If the path at the end of the loop is not equal to '/' and the current node has no child nodes // the current node needs to roll back to last valid skippedNode if path != "/" { - for length := len(*skippedNodes); length > 0; length-- { - skippedNode := (*skippedNodes)[length-1] - *skippedNodes = (*skippedNodes)[:length-1] - if strings.HasSuffix(skippedNode.path, path) { - path = skippedNode.path - n = skippedNode.node - if value.params != nil { - *value.params = (*value.params)[:skippedNode.paramsCount] - } - globalParamsCount = skippedNode.paramsCount - continue walk - } + if rollbackToSkippedNode(skippedNodes, &path, &n, value.params, &globalParamsCount) { + continue walk } } @@ -531,6 +551,11 @@ walk: // Outer loop for walking the tree // ... but we can't value.tsr = len(path) == end+1 + if !value.tsr { + if rollbackToSkippedNode(skippedNodes, &path, &n, value.params, &globalParamsCount) { + continue walk + } + } return value } @@ -544,6 +569,11 @@ walk: // Outer loop for walking the tree n = n.children[0] value.tsr = (n.path == "/" && n.handlers != nil) || (n.path == "" && n.indices == "/") } + if !value.tsr { + if rollbackToSkippedNode(skippedNodes, &path, &n, value.params, &globalParamsCount) { + continue walk + } + } return value case catchAll: @@ -588,18 +618,8 @@ walk: // Outer loop for walking the tree // If the current path does not equal '/' and the node does not have a registered handle and the most recently matched node has a child node // the current node needs to roll back to last valid skippedNode if n.handlers == nil && path != "/" { - for length := len(*skippedNodes); length > 0; length-- { - skippedNode := (*skippedNodes)[length-1] - *skippedNodes = (*skippedNodes)[:length-1] - if strings.HasSuffix(skippedNode.path, path) { - path = skippedNode.path - n = skippedNode.node - if value.params != nil { - *value.params = (*value.params)[:skippedNode.paramsCount] - } - globalParamsCount = skippedNode.paramsCount - continue walk - } + if rollbackToSkippedNode(skippedNodes, &path, &n, value.params, &globalParamsCount) { + continue walk } // n = latestNode.children[len(latestNode.children)-1] } @@ -645,18 +665,8 @@ walk: // Outer loop for walking the tree // roll back to last valid skippedNode if !value.tsr && path != "/" { - for length := len(*skippedNodes); length > 0; length-- { - skippedNode := (*skippedNodes)[length-1] - *skippedNodes = (*skippedNodes)[:length-1] - if strings.HasSuffix(skippedNode.path, path) { - path = skippedNode.path - n = skippedNode.node - if value.params != nil { - *value.params = (*value.params)[:skippedNode.paramsCount] - } - globalParamsCount = skippedNode.paramsCount - continue walk - } + if rollbackToSkippedNode(skippedNodes, &path, &n, value.params, &globalParamsCount) { + continue walk } } diff --git a/tree_test.go b/tree_test.go index 23339af4..57b5672c 100644 --- a/tree_test.go +++ b/tree_test.go @@ -1111,3 +1111,36 @@ func TestTreeFindCaseInsensitivePathWildcardParamAndStaticChild(t *testing.T) { t.Errorf("Wrong result for '/prefix/something': %s", string(out)) } } + +func TestTreeParamFallbackAfterStaticDeadEnd(t *testing.T) { + tests := []struct { + name string + routes []string + request testRequests + }{ + { + name: "param has child but no handler", + routes: []string{"/:p0/:p1", "/bc/:p1/a"}, + request: testRequests{ + {"/bc/bc", false, "/:p0/:p1", Params{{Key: "p0", Value: "bc"}, {Key: "p1", Value: "bc"}}}, + }, + }, + { + name: "param has no child for remaining path", + routes: []string{"/:p0/:p1/:p2", "/bc/:p1"}, + request: testRequests{ + {"/bc/bc/bc", false, "/:p0/:p1/:p2", Params{{Key: "p0", Value: "bc"}, {Key: "p1", Value: "bc"}, {Key: "p2", Value: "bc"}}}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tree := &node{} + for _, route := range tt.routes { + tree.addRoute(route, fakeHandler(route)) + } + checkRequests(t, tree, tt.request) + }) + } +}