From 6f9b71cd6fcbe51943e123298e1579c13e77b898 Mon Sep 17 00:00:00 2001 From: Martti T Date: Tue, 2 Mar 2021 20:56:40 +0200 Subject: [PATCH] Poc router stack backtracking (#1791) Router: PoC stack based backtracking Co-authored-by: stffabi --- router.go | 194 ++++++++++++++++++++----------------------------- router_test.go | 176 +++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 243 insertions(+), 127 deletions(-) diff --git a/router.go b/router.go index 5010659a..f0e9e51f 100644 --- a/router.go +++ b/router.go @@ -2,7 +2,6 @@ package echo import ( "net/http" - "strings" ) type ( @@ -334,21 +333,48 @@ func (r *Router) Find(method, path string, c Context) { cn := r.tree // Current node as root var ( - search = path - child *node // Child node - n int // Param counter - nk kind // Next kind - nn *node // Next node - ns string // Next search - pvalues = ctx.pvalues // Use the internal slice so the interface can keep the illusion of a dynamic slice + search = path + searchIndex = 0 + n int // Param counter + pvalues = ctx.pvalues // Use the internal slice so the interface can keep the illusion of a dynamic slice ) + // Backtracking is needed when a dead end (leaf node) is reached in the router tree. + // To backtrack the current node will be changed to the parent node and the next kind for the + // router logic will be returned based on fromKind or kind of the dead end node (static > param > any). + // For example if there is no static node match we should check parent next sibling by kind (param). + // Backtracking itself does not check if there is a next sibling, this is done by the router logic. + backtrackToNextNodeKind := func(fromKind kind) (nextNodeKind kind, valid bool) { + previous := cn + cn = previous.parent + valid = cn != nil + + // Next node type by priority + // NOTE: With the current implementation we never backtrack from an `any` route, so `previous.kind` is + // always `static` or `any` + // If this is changed then for any route next kind would be `static` and this statement should be changed + nextNodeKind = previous.kind + 1 + + if fromKind == skind { + // when backtracking is done from static kind block we did not change search so nothing to restore + return + } + + // restore search to value it was before we move to current node we are backtracking from. + if previous.kind == skind { + searchIndex -= len(previous.prefix) + } else { + n-- + // for param/any node.prefix value is always `:` so we can not deduce searchIndex from that and must use pValue + // for that index as it would also contain part of path we cut off before moving into node we are backtracking from + searchIndex -= len(pvalues[n]) + } + search = path[searchIndex:] + return + } + // Search order static > param > any for { - if search == "" { - break - } - pl := 0 // Prefix length l := 0 // LCP length @@ -365,60 +391,42 @@ func (r *Router) Find(method, path string, c Context) { } } - if l == pl { - // Continue search - search = search[l:] - // Finish routing if no remaining search and we are on an leaf node - if search == "" && (nn == nil || cn.parent == nil || cn.ppath != "") { - break - } - // Handle special case of trailing slash route with existing any route (see #1526) - if search == "" && path[len(path)-1] == '/' && cn.anyChildren != nil { - goto Any + if l != pl { + // No matching prefix, let's backtrack to the first possible alternative node of the decision path + nk, ok := backtrackToNextNodeKind(skind) + if !ok { + return // No other possibilities on the decision path + } else if nk == pkind { + goto Param + // NOTE: this case (backtracking from static node to previous any node) can not happen by current any matching logic. Any node is end of search currently + //} else if nk == akind { + // goto Any + } else { + // Not found (this should never be possible for static node we are looking currently) + return } } - // Attempt to go back up the tree on no matching prefix or no remaining search - if l != pl || search == "" { - if nn == nil { // Issue #1348 - return // Not found - } - cn = nn - search = ns - if nk == pkind { - goto Param - } else if nk == akind { - goto Any - } + // The full prefix has matched, remove the prefix from the remaining search + search = search[l:] + searchIndex = searchIndex + l + + // Finish routing if no remaining search and we are on an leaf node + if search == "" && cn.ppath != "" { + break } // Static node - if child = cn.findStaticChild(search[0]); child != nil { - // Save next - if cn.prefix[len(cn.prefix)-1] == '/' { // Issue #623 - nk = pkind - nn = cn - ns = search + if search != "" { + if child := cn.findStaticChild(search[0]); child != nil { + cn = child + continue } - cn = child - continue } Param: // Param node - if child = cn.paramChildren; child != nil { - // Issue #378 - if len(pvalues) == n { - continue - } - - // Save next - if cn.prefix[len(cn.prefix)-1] == '/' { // Issue #623 - nk = akind - nn = cn - ns = search - } - + if child := cn.paramChildren; search != "" && child != nil { cn = child i, l := 0, len(search) for ; i < l && search[i] != '/'; i++ { @@ -426,87 +434,39 @@ func (r *Router) Find(method, path string, c Context) { pvalues[n] = search[:i] n++ search = search[i:] + searchIndex = searchIndex + i continue } Any: // Any node - if cn = cn.anyChildren; cn != nil { + if child := cn.anyChildren; child != nil { // If any node is found, use remaining path for pvalues + cn = child pvalues[len(cn.pnames)-1] = search break } - // No node found, continue at stored next node - // or find nearest "any" route - if nn != nil { - // No next node to go down in routing (issue #954) - // Find nearest "any" route going up the routing tree - search = ns - np := nn.parent - // Consider param route one level up only - if cn = nn.paramChildren; cn != nil { - pos := strings.IndexByte(ns, '/') - if pos == -1 { - // If no slash is remaining in search string set param value - if len(cn.pnames) > 0 { - pvalues[len(cn.pnames)-1] = search - } - break - } else if pos > 0 { - // Otherwise continue route processing with restored next node - cn = nn - nn = nil - ns = "" - goto Param - } - } - // No param route found, try to resolve nearest any route - for { - np = nn.parent - if cn = nn.anyChildren; cn != nil { - break - } - if np == nil { - break // no further parent nodes in tree, abort - } - var str strings.Builder - str.WriteString(nn.prefix) - str.WriteString(search) - search = str.String() - nn = np - } - if cn != nil { // use the found "any" route and update path - pvalues[len(cn.pnames)-1] = search - break - } + // Let's backtrack to the first possible alternative node of the decision path + nk, ok := backtrackToNextNodeKind(akind) + if !ok { + return // No other possibilities on the decision path + } else if nk == pkind { + goto Param + } else if nk == akind { + goto Any + } else { + // Not found + return } - return // Not found - } ctx.handler = cn.findHandler(method) ctx.path = cn.ppath ctx.pnames = cn.pnames - // NOTE: Slow zone... if ctx.handler == nil { ctx.handler = cn.checkMethodNotAllowed() - - // Dig further for any, might have an empty value for *, e.g. - // serving a directory. Issue #207. - if cn = cn.anyChildren; cn == nil { - return - } - if h := cn.findHandler(method); h != nil { - ctx.handler = h - } else { - ctx.handler = cn.checkMethodNotAllowed() - } - ctx.path = cn.ppath - ctx.pnames = cn.pnames - pvalues[len(cn.pnames)-1] = "" } - return } diff --git a/router_test.go b/router_test.go index a5e53c05..ba1890bd 100644 --- a/router_test.go +++ b/router_test.go @@ -730,26 +730,58 @@ func TestRouterMatchAny(t *testing.T) { r := e.router // Routes - r.Add(http.MethodGet, "/", func(Context) error { - return nil - }) - r.Add(http.MethodGet, "/*", func(Context) error { - return nil - }) - r.Add(http.MethodGet, "/users/*", func(Context) error { - return nil - }) + r.Add(http.MethodGet, "/", handlerHelper("case", 1)) + r.Add(http.MethodGet, "/*", handlerHelper("case", 2)) + r.Add(http.MethodGet, "/users/*", handlerHelper("case", 3)) + c := e.NewContext(nil, nil).(*context) r.Find(http.MethodGet, "/", c) - assert.Equal(t, "", c.Param("*")) + c.handler(c) + + assert.Equal(t, 1, c.Get("case")) + assert.Equal(t, "/", c.Get("path")) r.Find(http.MethodGet, "/download", c) + c.handler(c) + assert.Equal(t, 2, c.Get("case")) + assert.Equal(t, "/*", c.Get("path")) assert.Equal(t, "download", c.Param("*")) r.Find(http.MethodGet, "/users/joe", c) + c.handler(c) + assert.Equal(t, 3, c.Get("case")) + assert.Equal(t, "/users/*", c.Get("path")) assert.Equal(t, "joe", c.Param("*")) } +// NOTE: this is to document current implementation. Last added route with `*` asterisk is always the match and no +// backtracking or more precise matching is done to find more suitable match. +// +// Current behaviour might not be correct or expected. +// But this is where we are without well defined requirements/rules how (multiple) asterisks work in route +func TestRouterAnyMatchesLastAddedAnyRoute(t *testing.T) { + e := New() + r := e.router + + r.Add(http.MethodGet, "/users/*", handlerHelper("case", 1)) + r.Add(http.MethodGet, "/users/*/action*", handlerHelper("case", 2)) + + c := e.NewContext(nil, nil).(*context) + + r.Find(http.MethodGet, "/users/xxx/action/sea", c) + c.handler(c) + assert.Equal(t, "/users/*/action*", c.Get("path")) + assert.Equal(t, "xxx/action/sea", c.Param("*")) + + // if we add another route then it is the last added and so it is matched + r.Add(http.MethodGet, "/users/*/action/search", handlerHelper("case", 3)) + + r.Find(http.MethodGet, "/users/xxx/action/sea", c) + c.handler(c) + assert.Equal(t, "/users/*/action/search", c.Get("path")) + assert.Equal(t, "xxx/action/sea", c.Param("*")) +} + // Issue #1739 func TestRouterMatchAnyPrefixIssue(t *testing.T) { e := New() @@ -791,6 +823,130 @@ func TestRouterMatchAnyPrefixIssue(t *testing.T) { assert.Equal(t, "users_prefix/", c.Param("*")) } +func TestRouteMultiLevelBacktracking(t *testing.T) { + e := New() + r := e.router + + r.Add(http.MethodGet, "/a/:b/c", handlerHelper("case", 1)) + r.Add(http.MethodGet, "/a/c/d", handlerHelper("case", 2)) + r.Add(http.MethodGet, "/:e/c/f", handlerHelper("case", 3)) + + c := e.NewContext(nil, nil).(*context) + r.Find(http.MethodGet, "/a/c/f", c) + + c.handler(c) + assert.Equal(t, 3, c.Get("case")) + assert.Equal(t, "/:e/c/f", c.Get("path")) +} + +// Issue # +func TestRouterBacktrackingFromParam(t *testing.T) { + e := New() + r := e.router + + r.Add(http.MethodGet, "/*", handlerHelper("case", 1)) + r.Add(http.MethodGet, "/users/:name/", handlerHelper("case", 2)) + + c := e.NewContext(nil, nil).(*context) + + r.Find(http.MethodGet, "/users/firstname/no-match", c) + c.handler(c) + assert.Equal(t, 1, c.Get("case")) + assert.Equal(t, "/*", c.Get("path")) + assert.Equal(t, "users/firstname/no-match", c.Param("*")) + + r.Find(http.MethodGet, "/users/firstname/", c) + c.handler(c) + assert.Equal(t, 2, c.Get("case")) + assert.Equal(t, "/users/:name/", c.Get("path")) + assert.Equal(t, "firstname", c.Param("name")) +} + +func TestRouterBacktrackingFromParamAny(t *testing.T) { + e := New() + r := e.router + + r.Add(http.MethodGet, "/*", handlerHelper("case", 1)) + r.Add(http.MethodGet, "/:name/lastname", handlerHelper("case", 2)) + + c := e.NewContext(nil, nil).(*context) + + r.Find(http.MethodGet, "/firstname/test", c) + c.handler(c) + assert.Equal(t, 1, c.Get("case")) + assert.Equal(t, "/*", c.Get("path")) + assert.Equal(t, "firstname/test", c.Param("*")) + + r.Find(http.MethodGet, "/firstname", c) + c.handler(c) + assert.Equal(t, 1, c.Get("case")) + assert.Equal(t, "/*", c.Get("path")) + assert.Equal(t, "firstname", c.Param("*")) + + r.Find(http.MethodGet, "/firstname/lastname", c) + c.handler(c) + assert.Equal(t, 2, c.Get("case")) + assert.Equal(t, "/:name/lastname", c.Get("path")) + assert.Equal(t, "firstname", c.Param("name")) +} + +func TestRouterBacktrackingFromParamAny2(t *testing.T) { + e := New() + r := e.router + + r.Add(http.MethodGet, "/*", handlerHelper("case", 1)) + r.Add(http.MethodGet, "/:name", handlerHelper("case", 2)) + r.Add(http.MethodGet, "/:name/lastname", handlerHelper("case", 3)) + + c := e.NewContext(nil, nil).(*context) + + r.Find(http.MethodGet, "/firstname/test", c) + c.handler(c) + assert.Equal(t, 1, c.Get("case")) + assert.Equal(t, "/*", c.Get("path")) + assert.Equal(t, "firstname/test", c.Param("*")) + + r.Find(http.MethodGet, "/firstname", c) + c.handler(c) + assert.Equal(t, 2, c.Get("case")) + assert.Equal(t, "/:name", c.Get("path")) + assert.Equal(t, "firstname", c.Param("name")) + + r.Find(http.MethodGet, "/firstname/lastname", c) + c.handler(c) + assert.Equal(t, 3, c.Get("case")) + assert.Equal(t, "/:name/lastname", c.Get("path")) + assert.Equal(t, "firstname", c.Param("name")) +} + +func TestRouterAnyCommonPath(t *testing.T) { + e := New() + r := e.router + + r.Add(http.MethodGet, "/ab*", handlerHelper("case", 1)) + r.Add(http.MethodGet, "/abcd", handlerHelper("case", 2)) + r.Add(http.MethodGet, "/abcd*", handlerHelper("case", 3)) + + c := e.NewContext(nil, nil).(*context) + + r.Find(http.MethodGet, "/abee", c) + c.handler(c) + assert.Equal(t, 1, c.Get("case")) + assert.Equal(t, "/ab*", c.Get("path")) + assert.Equal(t, "ee", c.Param("*")) + + r.Find(http.MethodGet, "/abcd", c) + c.handler(c) + assert.Equal(t, "/abcd", c.Get("path")) + assert.Equal(t, 2, c.Get("case")) + + r.Find(http.MethodGet, "/abcde", c) + c.handler(c) + assert.Equal(t, 3, c.Get("case")) + assert.Equal(t, "/abcd*", c.Get("path")) + assert.Equal(t, "e", c.Param("*")) +} + // TestRouterMatchAnySlash shall verify finding the best route // for any routes with trailing slash requests func TestRouterMatchAnySlash(t *testing.T) {