From ae6f7a30475ec4c04eb05bbff0454c2ba4c8015e Mon Sep 17 00:00:00 2001 From: Ibraheem Ahmed Date: Sun, 31 Oct 2021 20:21:37 -0400 Subject: [PATCH] fix tsr with mixed static and wildcard paths (#2924) --- tree.go | 48 ++++++++++++++++++++++++------------------------ tree_test.go | 27 +++++++++++++++++++++++++-- 2 files changed, 49 insertions(+), 26 deletions(-) diff --git a/tree.go b/tree.go index 8cb8810a70..158a339089 100644 --- a/tree.go +++ b/tree.go @@ -448,27 +448,26 @@ walk: // Outer loop for walking the tree continue walk } } - // 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 vaild skippedNode - - if path != "/" && !n.wildChild { - for l := len(*skippedNodes); l > 0; { - skippedNode := (*skippedNodes)[l-1] - *skippedNodes = (*skippedNodes)[:l-1] - if strings.HasSuffix(skippedNode.path, path) { - path = skippedNode.path - n = skippedNode.node - if value.params != nil { - *value.params = (*value.params)[:skippedNode.paramsCount] + + if !n.wildChild { + // 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 vaild skippedNode + if path != "/" { + for l := len(*skippedNodes); l > 0; { + skippedNode := (*skippedNodes)[l-1] + *skippedNodes = (*skippedNodes)[:l-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 } - globalParamsCount = skippedNode.paramsCount - continue walk } } - } - // If there is no wildcard pattern, recommend a redirection - if !n.wildChild { // Nothing found. // We can recommend to redirect to the same URL without a // trailing slash if a leaf exists for that path. @@ -615,8 +614,14 @@ walk: // Outer loop for walking the tree return } - // roll back to last vaild skippedNode - if path != "/" { + // Nothing found. We can recommend to redirect to the same URL with an + // extra trailing slash if a leaf exists for that path + value.tsr = path == "/" || + (len(prefix) == len(path)+1 && prefix[len(path)] == '/' && + path == prefix[:len(prefix)-1] && n.handlers != nil) + + // roll back to last valid skippedNode + if !value.tsr && path != "/" { for l := len(*skippedNodes); l > 0; { skippedNode := (*skippedNodes)[l-1] *skippedNodes = (*skippedNodes)[:l-1] @@ -632,11 +637,6 @@ walk: // Outer loop for walking the tree } } - // Nothing found. We can recommend to redirect to the same URL with an - // extra trailing slash if a leaf exists for that path - value.tsr = path == "/" || - (len(prefix) == len(path)+1 && prefix[len(path)] == '/' && - path == prefix[:len(prefix)-1] && n.handlers != nil) return } } diff --git a/tree_test.go b/tree_test.go index 2a2bc6d65f..ddab6c182e 100644 --- a/tree_test.go +++ b/tree_test.go @@ -587,7 +587,14 @@ func TestTreeTrailingSlashRedirect(t *testing.T) { "/doc/go1.html", "/no/a", "/no/b", - "/api/hello/:name", + "/api/:page/:name", + "/api/hello/:name/bar/", + "/api/bar/:name", + "/api/baz/foo", + "/api/baz/foo/bar", + "/blog/:p", + "/posts/:b/:c", + "/posts/b/:c/d/", } for _, route := range routes { recv := catchPanic(func() { @@ -613,7 +620,19 @@ func TestTreeTrailingSlashRedirect(t *testing.T) { "/admin/config/", "/admin/config/permissions/", "/doc/", + "/admin/static/", + "/admin/cfg/", + "/admin/cfg/users/", + "/api/hello/x/bar", + "/api/baz/foo/", + "/api/baz/bax/", + "/api/bar/huh/", + "/api/baz/foo/bar/", + "/api/world/abc/", + "/blog/pp/", + "/posts/b/c/d", } + for _, route := range tsrRoutes { value := tree.getValue(route, nil, getSkippedNodes(), false) if value.handlers != nil { @@ -629,7 +648,11 @@ func TestTreeTrailingSlashRedirect(t *testing.T) { "/no/", "/_", "/_/", - "/api/world/abc", + "/api", + "/api/", + "/api/hello/x/foo", + "/api/baz/foo/bad", + "/foo/p/p", } for _, route := range noTsrRoutes { value := tree.getValue(route, nil, getSkippedNodes(), false)