fix tsr with mixed static and wildcard paths (gin-gonic#2924)

daheige · Apr 18, 2022 · e99b03e · e99b03e
1 parent bce283a
commit e99b03e
Show file tree

Hide file tree

Showing 2 changed files with 49 additions and 26 deletions.
diff --git a/tree.go b/tree.go
@@ -447,27 +447,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.
@@ -614,8 +613,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]
@@ -631,11 +636,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
@@ -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)