diff --git a/gin_test.go b/gin_test.go index 71a817a73f..0311955483 100644 --- a/gin_test.go +++ b/gin_test.go @@ -1195,3 +1195,78 @@ func TestRouterQueryMethod(t *testing.T) { router.ServeHTTP(w2, req2) assert.Equal(t, http.StatusNotFound, w2.Code) } + +func TestRouterParamRouteWithTrailingSlashSibling(t *testing.T) { + for _, redirectTrailingSlash := range []bool{false, true} { + // Case 1: Route /b/ and /:name, request /b + router1 := New() + router1.RedirectTrailingSlash = redirectTrailingSlash + + var matchedRoute string + var matchedParams Params + + router1.GET("/b/", func(c *Context) { + matchedRoute = "/b/" + matchedParams = c.Params + c.String(http.StatusOK, "b_slash") + }) + router1.GET("/:name", func(c *Context) { + matchedRoute = "/:name" + matchedParams = c.Params + c.String(http.StatusOK, "name") + }) + + // GET /b -> should match /:name with name=b + w := httptest.NewRecorder() + req, _ := http.NewRequest(http.MethodGet, "/b", nil) + router1.ServeHTTP(w, req) + assert.Equal(t, http.StatusOK, w.Code) + assert.Equal(t, "/:name", matchedRoute) + assert.Equal(t, Params{Param{Key: "name", Value: "b"}}, matchedParams) + + // GET /b/ -> should match /b/ + w = httptest.NewRecorder() + req, _ = http.NewRequest(http.MethodGet, "/b/", nil) + router1.ServeHTTP(w, req) + assert.Equal(t, http.StatusOK, w.Code) + assert.Equal(t, "/b/", matchedRoute) + + // GET /c -> should match /:name with name=c + w = httptest.NewRecorder() + req, _ = http.NewRequest(http.MethodGet, "/c", nil) + router1.ServeHTTP(w, req) + assert.Equal(t, http.StatusOK, w.Code) + assert.Equal(t, "/:name", matchedRoute) + assert.Equal(t, Params{Param{Key: "name", Value: "c"}}, matchedParams) + + // Case 2: Mirror case: /a/b and /:p/b/, request /a/b/ + router2 := New() + router2.RedirectTrailingSlash = redirectTrailingSlash + + router2.GET("/a/b", func(c *Context) { + matchedRoute = "/a/b" + matchedParams = c.Params + c.String(http.StatusOK, "ab") + }) + router2.GET("/:p/b/", func(c *Context) { + matchedRoute = "/:p/b/" + matchedParams = c.Params + c.String(http.StatusOK, "pb_slash") + }) + + // GET /a/b/ -> should match /:p/b/ with p=a + w = httptest.NewRecorder() + req, _ = http.NewRequest(http.MethodGet, "/a/b/", nil) + router2.ServeHTTP(w, req) + assert.Equal(t, http.StatusOK, w.Code) + assert.Equal(t, "/:p/b/", matchedRoute) + assert.Equal(t, Params{Param{Key: "p", Value: "a"}}, matchedParams) + + // GET /a/b -> should match /a/b + w = httptest.NewRecorder() + req, _ = http.NewRequest(http.MethodGet, "/a/b", nil) + router2.ServeHTTP(w, req) + assert.Equal(t, http.StatusOK, w.Code) + assert.Equal(t, "/a/b", matchedRoute) + } +} diff --git a/go.mod b/go.mod index 99a8d5bb45..34770a69ab 100644 --- a/go.mod +++ b/go.mod @@ -12,7 +12,7 @@ require ( github.com/mattn/go-isatty v0.0.20 github.com/modern-go/reflect2 v1.0.2 github.com/pelletier/go-toml/v2 v2.2.4 - github.com/quic-go/quic-go v0.60.0 + github.com/quic-go/quic-go v0.63.0 github.com/stretchr/testify v1.12.1 github.com/ugorji/go/codec v1.3.1 go.mongodb.org/mongo-driver/v2 v2.5.0 diff --git a/go.sum b/go.sum index dc6ef902ec..6ebe4d14df 100644 --- a/go.sum +++ b/go.sum @@ -50,6 +50,8 @@ github.com/quic-go/qpack v0.6.0 h1:g7W+BMYynC1LbYLSqRt8PBg5Tgwxn214ZZR34VIOjz8= github.com/quic-go/qpack v0.6.0/go.mod h1:lUpLKChi8njB4ty2bFLX2x4gzDqXwUpaO1DP9qMDZII= github.com/quic-go/quic-go v0.60.0 h1:xcQioE8OM66UQLeUMHltK1CCcOu3JbVB4JAQdDQSB+0= github.com/quic-go/quic-go v0.60.0/go.mod h1:wpKpjmPpftl30sL6pFh7REVpjbcCVy4zt2vDyK1TuJk= +github.com/quic-go/quic-go v0.63.0 h1:LIFGHI4PFUhhw2dDD1ARHdCff143ffMHwZtbnbuJ78A= +github.com/quic-go/quic-go v0.63.0/go.mod h1:RAro2j2yN9a9EiPACLHT9IB2NXCvGQmmo/alT0yYI0w= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo= diff --git a/tree.go b/tree.go index 6842d225fd..8750ff0c3d 100644 --- a/tree.go +++ b/tree.go @@ -461,28 +461,28 @@ walk: // Outer loop for walking the tree } 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 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 the current node has no child nodes, + // roll back to last valid skippedNode + cTsr := path == "/" && n.handlers != nil + 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 + value.tsr = value.tsr || cTsr + continue walk } } // Nothing found. // We can recommend to redirect to the same URL without a // trailing slash if a leaf exists for that path. - value.tsr = path == "/" && n.handlers != nil + value.tsr = value.tsr || cTsr return value } @@ -537,20 +537,52 @@ walk: // Outer loop for walking the tree } // ... but we can't - value.tsr = len(path) == end+1 + cTsr := len(path) == end+1 + 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 + value.tsr = value.tsr || cTsr + continue walk + } + } + value.tsr = value.tsr || cTsr return value } if value.handlers = n.handlers; value.handlers != nil { value.fullPath = n.fullPath + value.tsr = false return value } + cTsr := false if len(n.children) == 1 { // No handle found. Check if a handle for this path + a // trailing slash exists for TSR recommendation n = n.children[0] - value.tsr = (n.path == "/" && n.handlers != nil) || (n.path == "" && n.indices == "/") + cTsr = (n.path == "/" && n.handlers != nil) || (n.path == "" && n.indices == "/") } + 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 + value.tsr = value.tsr || cTsr + continue walk + } + } + value.tsr = value.tsr || cTsr return value case catchAll: @@ -583,6 +615,7 @@ walk: // Outer loop for walking the tree value.handlers = n.handlers value.fullPath = n.fullPath + value.tsr = false return value default: @@ -592,9 +625,9 @@ walk: // Outer loop for walking the tree } if path == prefix { - // 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 + // If the current node does not have a registered handle, // the current node needs to roll back to last valid skippedNode - if n.handlers == nil && path != "/" { + if n.handlers == nil { for length := len(*skippedNodes); length > 0; length-- { skippedNode := (*skippedNodes)[length-1] *skippedNodes = (*skippedNodes)[:length-1] @@ -608,50 +641,32 @@ walk: // Outer loop for walking the tree continue walk } } - // n = latestNode.children[len(latestNode.children)-1] } // We should have reached the node containing the handle. // Check if this node has a handle registered. if value.handlers = n.handlers; value.handlers != nil { value.fullPath = n.fullPath + value.tsr = false return value } // If there is no handle for this route, but this route has a // wildcard child, there must be a handle for this path with an // additional trailing slash - if path == "/" && n.wildChild && n.nType != root { - value.tsr = true - return value - } - - if path == "/" && n.nType == static { - value.tsr = true - return value - } - - // No handle found. Check if a handle for this path + a - // trailing slash exists for trailing slash recommendation - for i, c := range []byte(n.indices) { - if c == '/' { - n = n.children[i] - value.tsr = (len(n.path) == 1 && n.handlers != nil) || - (n.nType == catchAll && n.children[0].handlers != nil) - return value + cTsr := (path == "/" && n.wildChild && n.nType != root) || (path == "/" && n.nType == static) + if !cTsr { + // No handle found. Check if a handle for this path + a + // trailing slash exists for trailing slash recommendation + for i, c := range []byte(n.indices) { + if c == '/' { + n = n.children[i] + cTsr = (len(n.path) == 1 && n.handlers != nil) || + (n.nType == catchAll && n.children[0].handlers != nil) + break + } } } - return value - } - - // 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 length := len(*skippedNodes); length > 0; length-- { skippedNode := (*skippedNodes)[length-1] *skippedNodes = (*skippedNodes)[:length-1] @@ -662,11 +677,38 @@ walk: // Outer loop for walking the tree *value.params = (*value.params)[:skippedNode.paramsCount] } globalParamsCount = skippedNode.paramsCount + value.tsr = value.tsr || cTsr continue walk } } + + value.tsr = value.tsr || cTsr + return value + } + + // Nothing found. We can recommend to redirect to the same URL with an + // extra trailing slash if a leaf exists for that path + cTsr := path == "/" || + (len(prefix) == len(path)+1 && prefix[len(path)] == '/' && + path == prefix[:len(prefix)-1] && n.handlers != nil) + + // roll back to last valid skippedNode + 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 + value.tsr = value.tsr || cTsr + continue walk + } } + value.tsr = value.tsr || cTsr return value } } diff --git a/tree_test.go b/tree_test.go index 23339af4bb..50e71bca4e 100644 --- a/tree_test.go +++ b/tree_test.go @@ -1111,3 +1111,77 @@ func TestTreeFindCaseInsensitivePathWildcardParamAndStaticChild(t *testing.T) { t.Errorf("Wrong result for '/prefix/something': %s", string(out)) } } + +func TestTreeTrailingSlashAndParamRoute(t *testing.T) { + // Case 1: Route /b/ and /:name, request /b + // GET /b should match /:name with param name="b", rather than redirecting to /b/ + tree := &node{} + routes := []string{ + "/b/", + "/:name", + } + for _, route := range routes { + tree.addRoute(route, fakeHandler(route)) + } + + checkRequests(t, tree, testRequests{ + {"/b", false, "/:name", Params{Param{Key: "name", Value: "b"}}}, + {"/c", false, "/:name", Params{Param{Key: "name", Value: "c"}}}, + {"/b/", false, "/b/", nil}, + }) + checkPriorities(t, tree) + + // Case 2: Mirror case: /a/b and /:p/b/, request /a/b/ + // GET /a/b/ should match /:p/b/ with param p="a", rather than redirecting to /a/b + tree2 := &node{} + routes2 := []string{ + "/a/b", + "/:p/b/", + } + for _, route := range routes2 { + tree2.addRoute(route, fakeHandler(route)) + } + + checkRequests(t, tree2, testRequests{ + {"/a/b/", false, "/:p/b/", Params{Param{Key: "p", Value: "a"}}}, + {"/a/b", false, "/a/b", nil}, + }) + checkPriorities(t, tree2) + + // Case 3: When no wildcard route matches the exact request, + // TSR should still be recommended for candidate static route + tree3 := &node{} + routes3 := []string{ + "/b/", + "/:name/x", + } + for _, route := range routes3 { + tree3.addRoute(route, fakeHandler(route)) + } + + val := tree3.getValue("/b", nil, getSkippedNodes(), false) + if val.handlers != nil { + t.Errorf("Expected nil handler for '/b', got: %v", val.handlers) + } + if !val.tsr { + t.Errorf("Expected TSR=true for '/b' when '/b/' exists and '/:name/x' does not match") + } + + // Case 4: Mirror TSR fallback when no wildcard route matches + tree4 := &node{} + routes4 := []string{ + "/a/b", + "/:p/b/x", + } + for _, route := range routes4 { + tree4.addRoute(route, fakeHandler(route)) + } + + val4 := tree4.getValue("/a/b/", nil, getSkippedNodes(), false) + if val4.handlers != nil { + t.Errorf("Expected nil handler for '/a/b/', got: %v", val4.handlers) + } + if !val4.tsr { + t.Errorf("Expected TSR=true for '/a/b/' when '/a/b' exists and '/:p/b/x' does not match") + } +}