From 9c160413f6fbe73e099809368e84c293cc061ee7 Mon Sep 17 00:00:00 2001 From: Alexander Emelin Date: Mon, 1 Nov 2021 20:02:03 +0300 Subject: [PATCH] Live: adopt latest fixes from gin route tree (#41141) --- .../live/pipeline/rule_cache_segmented.go | 3 +- pkg/services/live/pipeline/tree/tree.go | 160 +++++++++++------- pkg/services/live/pipeline/tree/tree_test.go | 85 ++++++---- 3 files changed, 156 insertions(+), 92 deletions(-) diff --git a/pkg/services/live/pipeline/rule_cache_segmented.go b/pkg/services/live/pipeline/rule_cache_segmented.go index 499f13c43d2..576eeabad07 100644 --- a/pkg/services/live/pipeline/rule_cache_segmented.go +++ b/pkg/services/live/pipeline/rule_cache_segmented.go @@ -75,8 +75,7 @@ func (s *CacheSegmentedTree) Get(orgID int64, channel string) (*LiveChannelRule, if !ok { return nil, false, nil } - ps := make(tree.Params, 0, 20) - nodeValue := t.GetValue("/"+channel, &ps, true) + nodeValue := t.GetValue("/"+channel, true) if nodeValue.Handler == nil { return nil, false, nil } diff --git a/pkg/services/live/pipeline/tree/tree.go b/pkg/services/live/pipeline/tree/tree.go index 2f4d1bf960a..2fa7d7fec85 100644 --- a/pkg/services/live/pipeline/tree/tree.go +++ b/pkg/services/live/pipeline/tree/tree.go @@ -15,6 +15,7 @@ import ( var ( strColon = []byte(":") strStar = []byte("*") + strSlash = []byte("/") ) func min(a, b int) int { @@ -51,6 +52,11 @@ func countParams(path string) uint16 { return n } +func countSections(path string) uint16 { + s := StringToBytes(path) + return uint16(bytes.Count(s, strSlash)) +} + type nodeType uint8 const ( @@ -66,14 +72,22 @@ func New() *Node { } type Node struct { - path string - indices string - wildChild bool - nType nodeType - priority uint32 - children []*Node // child nodes, at most 1 :param style Node at the end of the array - handler Handler - fullPath string + path string + indices string + wildChild bool + nType nodeType + priority uint32 + children []*Node // child nodes, at most 1 :param style Node at the end of the array + handler Handler + fullPath string + maxParams uint16 + maxSections uint16 +} + +type skippedNode struct { + path string + node *Node + paramsCount int16 } // Increments priority of the given child and reorders if necessary @@ -100,6 +114,12 @@ func (n *Node) incrementChildPrio(pos int) int { } func (n *Node) AddRoute(path string, handlers Handler) { + if paramsCount := countParams(path); paramsCount > n.maxParams { + n.maxParams = paramsCount + } + if sectionsCount := countSections(path); sectionsCount > n.maxSections { + n.maxSections = sectionsCount + } n.addRoute(path, handlers) } @@ -356,8 +376,10 @@ type NodeValue struct { FullPath string } -func (n *Node) GetValue(path string, params *Params, unescape bool) (value NodeValue) { - return n.getValue(path, params, unescape) +func (n *Node) GetValue(path string, unescape bool) (value NodeValue) { + skippedNodes := make([]skippedNode, 0, n.maxSections) + ps := make(Params, 0, n.maxParams) + return n.getValue(path, &ps, &skippedNodes, unescape) } // Returns the handle registered with the given path (key). The values of @@ -366,11 +388,8 @@ func (n *Node) GetValue(path string, params *Params, unescape bool) (value NodeV // made if a handle exists with an extra (without the) trailing slash for the // given path. // nolint:gocyclo -func (n *Node) getValue(path string, params *Params, unescape bool) (value NodeValue) { - var ( - skippedPath string - latestNode = n // Caching the latest Node - ) +func (n *Node) getValue(path string, params *Params, skippedNodes *[]skippedNode, unescape bool) (value NodeValue) { + var globalParamsCount int16 walk: // Outer loop for walking the tree for { @@ -385,15 +404,20 @@ walk: // Outer loop for walking the tree if c == idxc { // strings.HasPrefix(n.children[len(n.children)-1].path, ":") == n.wildChild if n.wildChild { - skippedPath = prefix + path - latestNode = &Node{ - path: n.path, - wildChild: n.wildChild, - nType: n.nType, - priority: n.priority, - children: n.children, - handler: n.handler, - fullPath: n.fullPath, + index := len(*skippedNodes) + *skippedNodes = (*skippedNodes)[:index+1] + (*skippedNodes)[index] = skippedNode{ + path: prefix + path, + node: &Node{ + path: n.path, + wildChild: n.wildChild, + nType: n.nType, + priority: n.priority, + children: n.children, + handler: n.handler, + fullPath: n.fullPath, + }, + paramsCount: globalParamsCount, } } @@ -401,15 +425,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 be equal to the latest matching Node - matched := path != "/" && !n.wildChild - if matched { - n = latestNode - } - // If there is no wildcard pattern, recommend a redirection 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 + } + } + } + // Nothing found. // We can recommend to redirect to the same URL without a // trailing slash if a leaf exists for that path. @@ -419,18 +454,12 @@ walk: // Outer loop for walking the tree // Handle wildcard child, which is always at the end of the array n = n.children[len(n.children)-1] + globalParamsCount++ switch n.nType { case param: // fix truncate the parameter // tree_test.go line: 204 - if matched { - path = prefix + path - // The saved path is used after the prefix route is intercepted by matching - if n.indices == "/" { - path = skippedPath[1:] - } - } // Find param end (either '/' or path end) end := 0 @@ -516,9 +545,22 @@ 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 - // the current Node needs to be equal to the latest matching Node - if latestNode.wildChild && n.handler == nil && path != "/" { - n = latestNode.children[len(latestNode.children)-1] + // the current node needs to roll back to last vaild skippedNode + if n.handler == nil && 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 + } + } + // n = latestNode.children[len(latestNode.children)-1] } // We should have reached the Node containing the handle. // Check if this Node has a handle registered. @@ -549,25 +591,29 @@ walk: // Outer loop for walking the tree return } - if path != "/" && len(skippedPath) > 0 && strings.HasSuffix(skippedPath, path) { - path = skippedPath - // Reduce the number of cycles - n, latestNode = latestNode, n - // skippedPath cannot execute - // example: - // * /:cc/cc - // call /a/cc expectations:match/200 Actual:match/200 - // call /a/dd expectations:unmatch/404 Actual: panic - // call /addr/dd/aa expectations:unmatch/404 Actual: panic - // skippedPath: It can only be executed if the secondary route is not found - skippedPath = "" - continue walk - } - // 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 && n.handler != nil) + (len(prefix) == len(path)+1 && prefix[len(path)] == '/' && + path == prefix[:len(prefix)-1] && n.handler != 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] + 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 + } + } + } + return } } diff --git a/pkg/services/live/pipeline/tree/tree_test.go b/pkg/services/live/pipeline/tree/tree_test.go index 6f248b466a5..ce7c53ba026 100644 --- a/pkg/services/live/pipeline/tree/tree_test.go +++ b/pkg/services/live/pipeline/tree/tree_test.go @@ -29,11 +29,6 @@ type testRequests []struct { ps Params } -func getParams() *Params { - ps := make(Params, 0, 20) - return &ps -} - func checkRequests(t *testing.T, tree *Node, requests testRequests, unescapes ...bool) { unescape := false if len(unescapes) >= 1 { @@ -41,7 +36,7 @@ func checkRequests(t *testing.T, tree *Node, requests testRequests, unescapes .. } for _, request := range requests { - value := tree.getValue(request.path, getParams(), unescape) + value := tree.GetValue(request.path, unescape) if value.Handler == nil { if !request.nilHandler { @@ -114,7 +109,7 @@ func TestTreeAddAndGet(t *testing.T) { "/β", } for _, route := range routes { - tree.addRoute(route, fakeHandler(route)) + tree.AddRoute(route, fakeHandler(route)) } checkRequests(t, tree, testRequests{ @@ -161,6 +156,8 @@ func TestTreeWildcard(t *testing.T) { "/aa/*xx", "/ab/*xx", "/:cc", + "/c1/:dd/e", + "/c1/:dd/e1", "/:cc/cc", "/:cc/:dd/ee", "/:cc/:dd/:ee/ff", @@ -191,7 +188,7 @@ func TestTreeWildcard(t *testing.T) { "/get/abc/123abfff/:param", } for _, route := range routes { - tree.addRoute(route, fakeHandler(route)) + tree.AddRoute(route, fakeHandler(route)) } checkRequests(t, tree, testRequests{ @@ -242,6 +239,9 @@ func TestTreeWildcard(t *testing.T) { {"/alldd", false, "/:cc", Params{Param{Key: "cc", Value: "alldd"}}}, {"/all/cc", false, "/:cc/cc", Params{Param{Key: "cc", Value: "all"}}}, {"/a/cc", false, "/:cc/cc", Params{Param{Key: "cc", Value: "a"}}}, + {"/c1/d/e", false, "/c1/:dd/e", Params{Param{Key: "dd", Value: "d"}}}, + {"/c1/d/e1", false, "/c1/:dd/e1", Params{Param{Key: "dd", Value: "d"}}}, + {"/c1/d/ee", false, "/:cc/:dd/ee", Params{Param{Key: "cc", Value: "c1"}, Param{Key: "dd", Value: "d"}}}, {"/cc/cc", false, "/:cc/cc", Params{Param{Key: "cc", Value: "cc"}}}, {"/ccc/cc", false, "/:cc/cc", Params{Param{Key: "cc", Value: "ccc"}}}, {"/deedwjfs/cc", false, "/:cc/cc", Params{Param{Key: "cc", Value: "deedwjfs"}}}, @@ -328,7 +328,7 @@ func TestUnescapeParameters(t *testing.T) { "/info/:user", } for _, route := range routes { - tree.addRoute(route, fakeHandler(route)) + tree.AddRoute(route, fakeHandler(route)) } checkRequests(t, tree, testRequests{ @@ -369,7 +369,7 @@ func testRoutes(t *testing.T, routes []testRoute) { for _, route := range routes { recv := catchPanic(func() { - tree.addRoute(route.path, nil) + tree.AddRoute(route.path, nil) }) if route.conflict { @@ -452,7 +452,7 @@ func TestTreeDuplicatePath(t *testing.T) { } for _, route := range routes { recv := catchPanic(func() { - tree.addRoute(route, fakeHandler(route)) + tree.AddRoute(route, fakeHandler(route)) }) if recv != nil { t.Fatalf("panic inserting route '%s': %v", route, recv) @@ -460,7 +460,7 @@ func TestTreeDuplicatePath(t *testing.T) { // Add again recv = catchPanic(func() { - tree.addRoute(route, nil) + tree.AddRoute(route, nil) }) if recv == nil { t.Fatalf("no panic while inserting duplicate route '%s", route) @@ -489,7 +489,7 @@ func TestEmptyWildcardName(t *testing.T) { } for _, route := range routes { recv := catchPanic(func() { - tree.addRoute(route, nil) + tree.AddRoute(route, nil) }) if recv == nil { t.Fatalf("no panic while inserting route with empty wildcard name '%s", route) @@ -519,7 +519,7 @@ func TestTreeCatchAllConflictRoot(t *testing.T) { func TestTreeCatchMaxParams(t *testing.T) { tree := &Node{} var route = "/cmd/*filepath" - tree.addRoute(route, fakeHandler(route)) + tree.AddRoute(route, fakeHandler(route)) } func TestTreeDoubleWildcard(t *testing.T) { @@ -534,7 +534,7 @@ func TestTreeDoubleWildcard(t *testing.T) { for _, route := range routes { tree := &Node{} recv := catchPanic(func() { - tree.addRoute(route, nil) + tree.AddRoute(route, nil) }) if rs, ok := recv.(string); !ok || !strings.HasPrefix(rs, panicMsg) { @@ -580,11 +580,18 @@ 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() { - tree.addRoute(route, fakeHandler(route)) + tree.AddRoute(route, fakeHandler(route)) }) if recv != nil { t.Fatalf("panic inserting route '%s': %v", route, recv) @@ -606,9 +613,20 @@ 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, false) + value := tree.GetValue(route, false) if value.Handler != nil { t.Fatalf("non-nil handler for TSR route '%s", route) } else if !value.Tsr { @@ -622,10 +640,14 @@ 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, false) + value := tree.GetValue(route, false) if value.Handler != nil { t.Fatalf("non-nil handler for No-TSR route '%s", route) } else if value.Tsr { @@ -638,13 +660,13 @@ func TestTreeRootTrailingSlashRedirect(t *testing.T) { tree := &Node{} recv := catchPanic(func() { - tree.addRoute("/:test", fakeHandler("/:test")) + tree.AddRoute("/:test", fakeHandler("/:test")) }) if recv != nil { t.Fatalf("panic inserting test route: %v", recv) } - value := tree.getValue("/", nil, false) + value := tree.GetValue("/", false) if value.Handler != nil { t.Fatalf("non-nil handler") } else if value.Tsr { @@ -696,7 +718,7 @@ func TestTreeFindCaseInsensitivePath(t *testing.T) { for _, route := range routes { recv := catchPanic(func() { - tree.addRoute(route, fakeHandler(route)) + tree.AddRoute(route, fakeHandler(route)) }) if recv != nil { t.Fatalf("panic inserting route '%s': %v", route, recv) @@ -816,15 +838,15 @@ func TestTreeInvalidNodeType(t *testing.T) { const panicMsg = "invalid Node type" tree := &Node{} - tree.addRoute("/", fakeHandler("/")) - tree.addRoute("/:page", fakeHandler("/:page")) + tree.AddRoute("/", fakeHandler("/")) + tree.AddRoute("/:page", fakeHandler("/:page")) // set invalid Node type tree.children[0].nType = 42 // normal lookup recv := catchPanic(func() { - tree.getValue("/test", nil, false) + tree.GetValue("/test", false) }) if rs, ok := recv.(string); !ok || rs != panicMsg { t.Fatalf("Expected panic '"+panicMsg+"', got '%v'", recv) @@ -845,11 +867,8 @@ func TestTreeInvalidParamsType(t *testing.T) { tree.children = append(tree.children, &Node{}) tree.children[0].nType = 2 - // set invalid Params type - params := make(Params, 0) - // try to trigger slice bounds out of range with capacity 0 - tree.getValue("/test", ¶ms, false) + tree.GetValue("/test", false) } func TestTreeWildcardConflictEx(t *testing.T) { @@ -868,7 +887,7 @@ func TestTreeWildcardConflictEx(t *testing.T) { for _, conflict := range conflicts { // I have to re-create a 'tree', because the 'tree' will be // in an inconsistent state when the loop recovers from the - // panic which threw by 'addRoute' function. + // panic which threw by 'AddRoute' function. tree := &Node{} routes := [...]string{ "/con:tact", @@ -877,11 +896,11 @@ func TestTreeWildcardConflictEx(t *testing.T) { } for _, route := range routes { - tree.addRoute(route, fakeHandler(route)) + tree.AddRoute(route, fakeHandler(route)) } recv := catchPanic(func() { - tree.addRoute(conflict.route, fakeHandler(conflict.route)) + tree.AddRoute(conflict.route, fakeHandler(conflict.route)) }) if !regexp.MustCompile(fmt.Sprintf("'%s' in new path .* conflicts with existing wildcard '%s' in existing prefix '%s'", conflict.segPath, conflict.existSegPath, conflict.existPath)).MatchString(fmt.Sprint(recv)) {