diff --git a/AUTHORS.md b/AUTHORS.md index a477611bff..c634e6be05 100644 --- a/AUTHORS.md +++ b/AUTHORS.md @@ -190,6 +190,8 @@ People and companies, who have contributed, in alphabetical order. **@rogierlommers (Rogier Lommers)** - Add updated static serve example +**@rw-access (Ross Wolf)** +- Added support to mix exact and param routes **@se77en (Damon Zhao)** - Improve color logging diff --git a/CHANGELOG.md b/CHANGELOG.md index ddf30e189b..5766f4ef10 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # Gin ChangeLog +## Gin v1.7 + +### ENHANCEMENTS + +* Support params and exact routes without creating conflicts [#2663](https://github.com/gin-gonic/gin/pull/2663) + + ## Gin v1.6.3 ### ENHANCEMENTS diff --git a/README.md b/README.md index 119f9452a3..02d5dbbc34 100644 --- a/README.md +++ b/README.md @@ -228,6 +228,7 @@ func main() { name := c.Param("name") c.String(http.StatusOK, "Hello %s", name) }) + // However, this one will match /user/john/ and also /user/john/send // If no other routers match /user/john, it will redirect to /user/john/ @@ -243,7 +244,15 @@ func main() { c.FullPath() == "/user/:name/*action" // true }) - router.Run(":8080") + // This handler will add a new router for /user/groups. + // Exact routes are resolved before param routes, regardless of the order they were defined. + // Routes starting with /user/groups are never interpreted as /user/:name/... routes + router.GET("/user/groups", func(c *gin.Context) { + c.String(http.StatusOK, "The available groups are [...]", name) + }) + + +router.Run(":8080") } ``` diff --git a/tree.go b/tree.go index 74e07e84f3..ca753e6d27 100644 --- a/tree.go +++ b/tree.go @@ -80,6 +80,16 @@ func longestCommonPrefix(a, b string) int { return i } +// addChild will add a child node, keeping wildcards at the end +func (n *node) addChild(child *node) { + if n.wildChild && len(n.children) > 0 { + wildcardChild := n.children[len(n.children)-1] + n.children = append(n.children[:len(n.children)-1], child, wildcardChild) + } else { + n.children = append(n.children, child) + } +} + func countParams(path string) uint16 { var n uint16 s := bytesconv.StringToBytes(path) @@ -103,7 +113,7 @@ type node struct { wildChild bool nType nodeType priority uint32 - children []*node + children []*node // child nodes, at most 1 :param style node at the end of the array handlers HandlersChain fullPath string } @@ -177,36 +187,9 @@ walk: // Make new node a child of this node if i < len(path) { path = path[i:] - - if n.wildChild { - parentFullPathIndex += len(n.path) - n = n.children[0] - n.priority++ - - // Check if the wildcard matches - if len(path) >= len(n.path) && n.path == path[:len(n.path)] && - // Adding a child to a catchAll is not possible - n.nType != catchAll && - // Check for longer wildcard, e.g. :name and :names - (len(n.path) >= len(path) || path[len(n.path)] == '/') { - continue walk - } - - pathSeg := path - if n.nType != catchAll { - pathSeg = strings.SplitN(path, "/", 2)[0] - } - prefix := fullPath[:strings.Index(fullPath, pathSeg)] + n.path - panic("'" + pathSeg + - "' in new path '" + fullPath + - "' conflicts with existing wildcard '" + n.path + - "' in existing prefix '" + prefix + - "'") - } - c := path[0] - // slash after param + // '/' after param if n.nType == param && c == '/' && len(n.children) == 1 { parentFullPathIndex += len(n.path) n = n.children[0] @@ -225,21 +208,47 @@ walk: } // Otherwise insert it - if c != ':' && c != '*' { + if c != ':' && c != '*' && n.nType != catchAll { // []byte for proper unicode char conversion, see #65 n.indices += bytesconv.BytesToString([]byte{c}) child := &node{ fullPath: fullPath, } - n.children = append(n.children, child) + n.addChild(child) n.incrementChildPrio(len(n.indices) - 1) n = child + } else if n.wildChild { + // inserting a wildcard node, need to check if it conflicts with the existing wildcard + n = n.children[len(n.children)-1] + n.priority++ + + // Check if the wildcard matches + if len(path) >= len(n.path) && n.path == path[:len(n.path)] && + // Adding a child to a catchAll is not possible + n.nType != catchAll && + // Check for longer wildcard, e.g. :name and :names + (len(n.path) >= len(path) || path[len(n.path)] == '/') { + continue walk + } + + // Wildcard conflict + pathSeg := path + if n.nType != catchAll { + pathSeg = strings.SplitN(pathSeg, "/", 2)[0] + } + prefix := fullPath[:strings.Index(fullPath, pathSeg)] + n.path + panic("'" + pathSeg + + "' in new path '" + fullPath + + "' conflicts with existing wildcard '" + n.path + + "' in existing prefix '" + prefix + + "'") } + n.insertChild(path, fullPath, handlers) return } - // Otherwise and handle to current node + // Otherwise add handle to current node if n.handlers != nil { panic("handlers are already registered for path '" + fullPath + "'") } @@ -293,13 +302,6 @@ func (n *node) insertChild(path string, fullPath string, handlers HandlersChain) panic("wildcards must be named with a non-empty name in path '" + fullPath + "'") } - // Check if this node has existing children which would be - // unreachable if we insert the wildcard here - if len(n.children) > 0 { - panic("wildcard segment '" + wildcard + - "' conflicts with existing children in path '" + fullPath + "'") - } - if wildcard[0] == ':' { // param if i > 0 { // Insert prefix before the current wildcard @@ -307,13 +309,13 @@ func (n *node) insertChild(path string, fullPath string, handlers HandlersChain) path = path[i:] } - n.wildChild = true child := &node{ nType: param, path: wildcard, fullPath: fullPath, } - n.children = []*node{child} + n.addChild(child) + n.wildChild = true n = child n.priority++ @@ -326,7 +328,7 @@ func (n *node) insertChild(path string, fullPath string, handlers HandlersChain) priority: 1, fullPath: fullPath, } - n.children = []*node{child} + n.addChild(child) n = child continue } @@ -360,7 +362,7 @@ func (n *node) insertChild(path string, fullPath string, handlers HandlersChain) fullPath: fullPath, } - n.children = []*node{child} + n.addChild(child) n.indices = string('/') n = child n.priority++ @@ -404,18 +406,18 @@ walk: // Outer loop for walking the tree if len(path) > len(prefix) { if path[:len(prefix)] == prefix { path = path[len(prefix):] - // If this node does not have a wildcard (param or catchAll) - // child, we can just look up the next child node and continue - // to walk down the tree - if !n.wildChild { - idxc := path[0] - for i, c := range []byte(n.indices) { - if c == idxc { - n = n.children[i] - continue walk - } + + // Try all the non-wildcard children first by matching the indices + idxc := path[0] + for i, c := range []byte(n.indices) { + if c == idxc { + n = n.children[i] + 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. @@ -423,8 +425,9 @@ walk: // Outer loop for walking the tree return } - // Handle wildcard child - n = n.children[0] + // Handle wildcard child, which is always at the end of the array + n = n.children[len(n.children)-1] + switch n.nType { case param: // Find param end (either '/' or path end) diff --git a/tree_test.go b/tree_test.go index 1cb4f5598d..d7c4fb0b9c 100644 --- a/tree_test.go +++ b/tree_test.go @@ -137,6 +137,8 @@ func TestTreeWildcard(t *testing.T) { "/", "/cmd/:tool/:sub", "/cmd/:tool/", + "/cmd/whoami", + "/cmd/whoami/root/", "/src/*filepath", "/search/", "/search/:query", @@ -155,8 +157,12 @@ func TestTreeWildcard(t *testing.T) { checkRequests(t, tree, testRequests{ {"/", false, "/", nil}, - {"/cmd/test/", false, "/cmd/:tool/", Params{Param{Key: "tool", Value: "test"}}}, - {"/cmd/test", true, "", Params{Param{Key: "tool", Value: "test"}}}, + {"/cmd/test", true, "/cmd/:tool/", Params{Param{"tool", "test"}}}, + {"/cmd/test/", false, "/cmd/:tool/", Params{Param{"tool", "test"}}}, + {"/cmd/whoami", false, "/cmd/whoami", nil}, + {"/cmd/whoami/", true, "/cmd/whoami", nil}, + {"/cmd/whoami/root/", false, "/cmd/whoami/root/", nil}, + {"/cmd/whoami/root", true, "/cmd/whoami/root/", nil}, {"/cmd/test/3", false, "/cmd/:tool/:sub", Params{Param{Key: "tool", Value: "test"}, Param{Key: "sub", Value: "3"}}}, {"/src/", false, "/src/*filepath", Params{Param{Key: "filepath", Value: "/"}}}, {"/src/some/file.png", false, "/src/*filepath", Params{Param{Key: "filepath", Value: "/some/file.png"}}}, @@ -245,20 +251,38 @@ func testRoutes(t *testing.T, routes []testRoute) { func TestTreeWildcardConflict(t *testing.T) { routes := []testRoute{ {"/cmd/:tool/:sub", false}, - {"/cmd/vet", true}, + {"/cmd/vet", false}, + {"/foo/bar", false}, + {"/foo/:name", false}, + {"/foo/:names", true}, + {"/cmd/*path", true}, + {"/cmd/:badvar", true}, + {"/cmd/:tool/names", false}, + {"/cmd/:tool/:badsub/details", true}, {"/src/*filepath", false}, + {"/src/:file", true}, + {"/src/static.json", true}, {"/src/*filepathx", true}, {"/src/", true}, + {"/src/foo/bar", true}, {"/src1/", false}, {"/src1/*filepath", true}, {"/src2*filepath", true}, + {"/src2/*filepath", false}, {"/search/:query", false}, - {"/search/invalid", true}, + {"/search/valid", false}, {"/user_:name", false}, - {"/user_x", true}, + {"/user_x", false}, {"/user_:name", false}, {"/id:id", false}, - {"/id/:id", true}, + {"/id/:id", false}, + } + testRoutes(t, routes) +} + +func TestCatchAllAfterSlash(t *testing.T) { + routes := []testRoute{ + {"/non-leading-*catchall", true}, } testRoutes(t, routes) } @@ -266,14 +290,17 @@ func TestTreeWildcardConflict(t *testing.T) { func TestTreeChildConflict(t *testing.T) { routes := []testRoute{ {"/cmd/vet", false}, - {"/cmd/:tool/:sub", true}, + {"/cmd/:tool", false}, + {"/cmd/:tool/:sub", false}, + {"/cmd/:tool/misc", false}, + {"/cmd/:tool/:othersub", true}, {"/src/AUTHORS", false}, {"/src/*filepath", true}, {"/user_x", false}, - {"/user_:name", true}, + {"/user_:name", false}, {"/id/:id", false}, - {"/id:id", true}, - {"/:id", true}, + {"/id:id", false}, + {"/:id", false}, {"/*filepath", true}, } testRoutes(t, routes) @@ -688,8 +715,7 @@ func TestTreeWildcardConflictEx(t *testing.T) { {"/who/are/foo", "/foo", `/who/are/\*you`, `/\*you`}, {"/who/are/foo/", "/foo/", `/who/are/\*you`, `/\*you`}, {"/who/are/foo/bar", "/foo/bar", `/who/are/\*you`, `/\*you`}, - {"/conxxx", "xxx", `/con:tact`, `:tact`}, - {"/conooo/xxx", "ooo", `/con:tact`, `:tact`}, + {"/con:nection", ":nection", `/con:tact`, `:tact`}, } for _, conflict := range conflicts {