From 91b940d8bd379ed0f6397317de5635bc5ecafdbe Mon Sep 17 00:00:00 2001 From: stffabi Date: Wed, 10 Feb 2021 15:47:50 +0100 Subject: [PATCH 1/6] Router: PoC stack based backtracking --- router.go | 144 ++++++++++++++++++++++++++++--------------------- router_test.go | 33 ++++++++++++ 2 files changed, 115 insertions(+), 62 deletions(-) diff --git a/router.go b/router.go index 5010659a6..f6a1c51ad 100644 --- a/router.go +++ b/router.go @@ -2,7 +2,7 @@ package echo import ( "net/http" - "strings" + "sync" ) type ( @@ -320,6 +320,23 @@ func (n *node) checkMethodNotAllowed() HandlerFunc { return NotFoundHandler } +type findNextState struct { + nk kind + nn *node + ns string + np int +} + +type statePointer struct { + s []findNextState +} + +var syncPool = sync.Pool{ + New: func() interface{} { + return &statePointer{make([]findNextState, 0, 10)} + }, +} + // Find lookup a handler registered for method and path. It also parses URL for path // parameters and load them into context. // @@ -337,12 +354,48 @@ func (r *Router) Find(method, path string, c Context) { 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 ) + pState := syncPool.Get().(*statePointer) + state := pState.s[0:0] + defer func() { + for i := range state { + state[i].nn = nil + } + pState.s = state[0:0] + syncPool.Put(pState) + }() + + pushNext := func(nodeKind kind) { + i := len(state) + if i+1 > cap(state) { + state = append(state, make([]findNextState, 0, 10)...) + } + state = state[0 : i+1] + state[i].nk = nodeKind + state[i].nn = cn + state[i].ns = search + state[i].np = n + } + + popNext := func() (nodeKind kind, valid bool) { + if len(state) == 0 { + return + } + + i := len(state) - 1 + last := state[i] + state = state[0:i] + + nodeKind = last.nk + cn = last.nn + search = last.ns + n = last.np + valid = cn != nil + return + } + // Search order static > param > any for { if search == "" { @@ -369,7 +422,7 @@ func (r *Router) Find(method, path string, c Context) { // 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 != "") { + if search == "" && (len(state) == 0 || cn.parent == nil || cn.ppath != "") { break } // Handle special case of trailing slash route with existing any route (see #1526) @@ -380,15 +433,16 @@ func (r *Router) Find(method, path string, c Context) { // 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 { + nk, ok := popNext() + if !ok { + return // Issue #1348 => Not found + } else if nk == pkind { goto Param } else if nk == akind { goto Any + } else { + // Not found + return } } @@ -396,9 +450,9 @@ func (r *Router) Find(method, path string, c Context) { 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 + // Push a new entry into the decisition path, if we don't find anything downtree + // try the current node again searching for a param node + pushNext(pkind) } cn = child continue @@ -414,9 +468,9 @@ func (r *Router) Find(method, path string, c Context) { // Save next if cn.prefix[len(cn.prefix)-1] == '/' { // Issue #623 - nk = akind - nn = cn - ns = search + // Push a new entry into the decisition path, if we have nothing found downtree + // try the current node again searching for an any node + pushNext(akind) } cn = child @@ -437,52 +491,18 @@ func (r *Router) Find(method, path string, c Context) { 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 try the next possible node in the decision path + nk, ok := popNext() + if !ok { + return // Issue #1348 => Not found + } else if nk == pkind { + goto Param + } else if nk == akind { + goto Any + } else { + // Not found + return } - return // Not found - } ctx.handler = cn.findHandler(method) diff --git a/router_test.go b/router_test.go index a5e53c05b..f8bfe4bcf 100644 --- a/router_test.go +++ b/router_test.go @@ -791,6 +791,39 @@ 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")) +} + // TestRouterMatchAnySlash shall verify finding the best route // for any routes with trailing slash requests func TestRouterMatchAnySlash(t *testing.T) { From 7c948bc288ae0964be512858d8db4bc4efd43f50 Mon Sep 17 00:00:00 2001 From: stffabi Date: Wed, 10 Feb 2021 17:23:22 +0100 Subject: [PATCH 2/6] Router-PoC: Fix cleanup --- router.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/router.go b/router.go index f6a1c51ad..115c35d84 100644 --- a/router.go +++ b/router.go @@ -361,7 +361,10 @@ func (r *Router) Find(method, path string, c Context) { state := pState.s[0:0] defer func() { for i := range state { + state[i].nk = 0 state[i].nn = nil + state[i].ns = "" + state[i].np = 0 } pState.s = state[0:0] syncPool.Put(pState) @@ -393,6 +396,11 @@ func (r *Router) Find(method, path string, c Context) { search = last.ns n = last.np valid = cn != nil + + last.nk = 0 + last.nn = nil + last.ns = "" + last.np = 0 return } From 48facfff0686951885320dcd701ef10f2b23617c Mon Sep 17 00:00:00 2001 From: stffabi Date: Wed, 10 Feb 2021 18:01:41 +0100 Subject: [PATCH 3/6] Router PoC: Improve algorithm, removed special cases --- router.go | 86 +++++++++++++----------------------- router_test.go | 115 ++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 134 insertions(+), 67 deletions(-) diff --git a/router.go b/router.go index 115c35d84..338dab0ec 100644 --- a/router.go +++ b/router.go @@ -352,7 +352,6 @@ func (r *Router) Find(method, path string, c Context) { var ( search = path - child *node // Child node n int // Param counter pvalues = ctx.pvalues // Use the internal slice so the interface can keep the illusion of a dynamic slice ) @@ -373,7 +372,7 @@ func (r *Router) Find(method, path string, c Context) { pushNext := func(nodeKind kind) { i := len(state) if i+1 > cap(state) { - state = append(state, make([]findNextState, 0, 10)...) + state = append(state, make([]findNextState, 10)...) } state = state[0 : i+1] state[i].nk = nodeKind @@ -406,10 +405,6 @@ func (r *Router) Find(method, path string, c Context) { // Search order static > param > any for { - if search == "" { - break - } - pl := 0 // Prefix length l := 0 // LCP length @@ -426,21 +421,8 @@ 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 == "" && (len(state) == 0 || 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 - } - } - - // Attempt to go back up the tree on no matching prefix or no remaining search - if l != pl || search == "" { + if l != pl { + // No matching prefix, let's backtrack to the first possible alternative node of the decision path nk, ok := popNext() if !ok { return // Issue #1348 => Not found @@ -454,30 +436,35 @@ func (r *Router) Find(method, path string, c Context) { } } + // The full prefix has matched, remove the prefix from the remaining search + search = search[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 - // Push a new entry into the decisition path, if we don't find anything downtree - // try the current node again searching for a param node - pushNext(pkind) + if search != "" { + if child := cn.findStaticChild(search[0]); child != nil { + if cn.paramChildren != nil || cn.anyChildren != nil { + // Push a new entry into the decision path, if we don't find anything downtree + // try the current node again searching for a param or any node + // Optimization: The node is only pushed for backtracking if there's an praramChildren or an anyChildren + pushNext(pkind) + } + 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 - // Push a new entry into the decisition path, if we have nothing found downtree - // try the current node again searching for an any node + if child := cn.paramChildren; search != "" && child != nil { + if cn.anyChildren != nil { + // Push a new entry into the decision path, if we have nothing found downtree try the current node again + // searching for an any node. + // Optimization: The node is only pushed for backtracking if there's an anyChildren pushNext(akind) } @@ -493,13 +480,14 @@ func (r *Router) Find(method, path string, c Context) { 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 } - // Let's try the next possible node in the decision path + // Let's backtrack to the first possible alternative node of the decision path nk, ok := popNext() if !ok { return // Issue #1348 => Not found @@ -517,24 +505,8 @@ func (r *Router) Find(method, path string, c Context) { 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 f8bfe4bcf..df238d984 100644 --- a/router_test.go +++ b/router_test.go @@ -730,23 +730,27 @@ 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("*")) } @@ -818,10 +822,101 @@ func TestRouterBacktrackingFromParam(t *testing.T) { 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 From 3e817bcd279fe973725f82f147923b5981920457 Mon Sep 17 00:00:00 2001 From: stffabi Date: Thu, 11 Feb 2021 16:15:11 +0100 Subject: [PATCH 4/6] Router PoC: Optimization: put backtracking state on stack and not on the heap --- router.go | 72 ++++++++++++++++++------------------------------------- 1 file changed, 23 insertions(+), 49 deletions(-) diff --git a/router.go b/router.go index 338dab0ec..52b0dc347 100644 --- a/router.go +++ b/router.go @@ -2,7 +2,6 @@ package echo import ( "net/http" - "sync" ) type ( @@ -320,23 +319,6 @@ func (n *node) checkMethodNotAllowed() HandlerFunc { return NotFoundHandler } -type findNextState struct { - nk kind - nn *node - ns string - np int -} - -type statePointer struct { - s []findNextState -} - -var syncPool = sync.Pool{ - New: func() interface{} { - return &statePointer{make([]findNextState, 0, 10)} - }, -} - // Find lookup a handler registered for method and path. It also parses URL for path // parameters and load them into context. // @@ -346,6 +328,8 @@ var syncPool = sync.Pool{ // - Reset it `Context#Reset()` // - Return it `Echo#ReleaseContext()`. func (r *Router) Find(method, path string, c Context) { + const backTrackingDepth = 10 + ctx := c.(*context) ctx.path = path cn := r.tree // Current node as root @@ -354,52 +338,42 @@ func (r *Router) Find(method, path string, c Context) { search = path n int // Param counter pvalues = ctx.pvalues // Use the internal slice so the interface can keep the illusion of a dynamic slice - ) - pState := syncPool.Get().(*statePointer) - state := pState.s[0:0] - defer func() { - for i := range state { - state[i].nk = 0 - state[i].nn = nil - state[i].ns = "" - state[i].np = 0 + // Backtracking Information + state [backTrackingDepth]struct { + nk kind + nn *node + ns string + np int } - pState.s = state[0:0] - syncPool.Put(pState) - }() + stateIndex int = -1 + ) pushNext := func(nodeKind kind) { - i := len(state) - if i+1 > cap(state) { - state = append(state, make([]findNextState, 10)...) + stateIndex++ + if stateIndex >= backTrackingDepth { + panic("Max backtracking depth reached. TODO: this must be detected during registering the paths") } - state = state[0 : i+1] - state[i].nk = nodeKind - state[i].nn = cn - state[i].ns = search - state[i].np = n + + state[stateIndex].nk = nodeKind + state[stateIndex].nn = cn + state[stateIndex].ns = search + state[stateIndex].np = n } popNext := func() (nodeKind kind, valid bool) { - if len(state) == 0 { + if stateIndex < 0 { return } - i := len(state) - 1 - last := state[i] - state = state[0:i] + last := state[stateIndex] + stateIndex-- nodeKind = last.nk cn = last.nn search = last.ns n = last.np valid = cn != nil - - last.nk = 0 - last.nn = nil - last.ns = "" - last.np = 0 return } @@ -425,7 +399,7 @@ func (r *Router) Find(method, path string, c Context) { // No matching prefix, let's backtrack to the first possible alternative node of the decision path nk, ok := popNext() if !ok { - return // Issue #1348 => Not found + return // No other possibilities on the decision path } else if nk == pkind { goto Param } else if nk == akind { @@ -490,7 +464,7 @@ func (r *Router) Find(method, path string, c Context) { // Let's backtrack to the first possible alternative node of the decision path nk, ok := popNext() if !ok { - return // Issue #1348 => Not found + return // No other possibilities on the decision path } else if nk == pkind { goto Param } else if nk == akind { From 3093bab0d1dde227e7d4964d0ce10d9f308a25bc Mon Sep 17 00:00:00 2001 From: toimtoimtoim Date: Tue, 16 Feb 2021 09:52:16 +0200 Subject: [PATCH 5/6] backtrack by calculating next node instead of poping from stack --- router.go | 87 ++++++++++++++++++++------------------------------ router_test.go | 28 ++++++++++++++++ 2 files changed, 63 insertions(+), 52 deletions(-) diff --git a/router.go b/router.go index 52b0dc347..784fc7963 100644 --- a/router.go +++ b/router.go @@ -328,52 +328,45 @@ func (n *node) checkMethodNotAllowed() HandlerFunc { // - Reset it `Context#Reset()` // - Return it `Echo#ReleaseContext()`. func (r *Router) Find(method, path string, c Context) { - const backTrackingDepth = 10 - ctx := c.(*context) ctx.path = path cn := r.tree // Current node as root var ( - search = path - n int // Param counter - pvalues = ctx.pvalues // Use the internal slice so the interface can keep the illusion of a dynamic slice - - // Backtracking Information - state [backTrackingDepth]struct { - nk kind - nn *node - ns string - np int - } - stateIndex int = -1 + 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 ) - pushNext := func(nodeKind kind) { - stateIndex++ - if stateIndex >= backTrackingDepth { - panic("Max backtracking depth reached. TODO: this must be detected during registering the paths") - } + // backtracking happens when we reach dead end when matching nodes in the router tree. To backtrack we set + // current node to parent to current node (this was previous node we checked) and choose next node kind to check. + // Next node kind relies on routing priority (static->param->any). So 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 next sibling this + // is left up to matching logic + backtrackToNextNodeKind := func(fromKind kind) (nextNodeKind kind, valid bool) { + previous := cn + cn = previous.parent + valid = cn != nil - state[stateIndex].nk = nodeKind - state[stateIndex].nn = cn - state[stateIndex].ns = search - state[stateIndex].np = n - } + // next node type by priority + // NOTE: by current implementation we never backtrack from any route so `previous.kind` value is here always static or any + // if this requirement is to change then for any route next kind would be static kind and this statement should be changed + nextNodeKind = previous.kind + 1 - popNext := func() (nodeKind kind, valid bool) { - if stateIndex < 0 { + if fromKind == skind { + // when backtracking is done from static kind block we did not change search so nothing to restore return } - last := state[stateIndex] - stateIndex-- - - nodeKind = last.nk - cn = last.nn - search = last.ns - n = last.np - valid = cn != nil + if previous.kind == skind { + searchIndex -= len(previous.prefix) + search = path[searchIndex:] + } else { + n-- + searchIndex -= len(pvalues[n]) + search = path[searchIndex:] + } return } @@ -397,21 +390,23 @@ func (r *Router) Find(method, path string, c Context) { if l != pl { // No matching prefix, let's backtrack to the first possible alternative node of the decision path - nk, ok := popNext() + nk, ok := backtrackToNextNodeKind(skind) if !ok { return // No other possibilities on the decision path } else if nk == pkind { goto Param - } else if nk == akind { - goto Any + // 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 + // Not found (this should never be possible for static node we are looking currently) return } } // 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 != "" { @@ -421,12 +416,6 @@ func (r *Router) Find(method, path string, c Context) { // Static node if search != "" { if child := cn.findStaticChild(search[0]); child != nil { - if cn.paramChildren != nil || cn.anyChildren != nil { - // Push a new entry into the decision path, if we don't find anything downtree - // try the current node again searching for a param or any node - // Optimization: The node is only pushed for backtracking if there's an praramChildren or an anyChildren - pushNext(pkind) - } cn = child continue } @@ -435,13 +424,6 @@ func (r *Router) Find(method, path string, c Context) { Param: // Param node if child := cn.paramChildren; search != "" && child != nil { - if cn.anyChildren != nil { - // Push a new entry into the decision path, if we have nothing found downtree try the current node again - // searching for an any node. - // Optimization: The node is only pushed for backtracking if there's an anyChildren - pushNext(akind) - } - cn = child i, l := 0, len(search) for ; i < l && search[i] != '/'; i++ { @@ -449,6 +431,7 @@ func (r *Router) Find(method, path string, c Context) { pvalues[n] = search[:i] n++ search = search[i:] + searchIndex = searchIndex + i continue } @@ -462,7 +445,7 @@ func (r *Router) Find(method, path string, c Context) { } // Let's backtrack to the first possible alternative node of the decision path - nk, ok := popNext() + nk, ok := backtrackToNextNodeKind(akind) if !ok { return // No other possibilities on the decision path } else if nk == pkind { diff --git a/router_test.go b/router_test.go index df238d984..ba1890bd1 100644 --- a/router_test.go +++ b/router_test.go @@ -754,6 +754,34 @@ func TestRouterMatchAny(t *testing.T) { 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() From db75ddc67f06cb37d95b825e8527c107e8321beb Mon Sep 17 00:00:00 2001 From: toimtoimtoim Date: Tue, 2 Mar 2021 16:02:20 +0200 Subject: [PATCH 6/6] improve comments --- router.go | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/router.go b/router.go index 784fc7963..f0e9e51f4 100644 --- a/router.go +++ b/router.go @@ -339,19 +339,20 @@ func (r *Router) Find(method, path string, c Context) { pvalues = ctx.pvalues // Use the internal slice so the interface can keep the illusion of a dynamic slice ) - // backtracking happens when we reach dead end when matching nodes in the router tree. To backtrack we set - // current node to parent to current node (this was previous node we checked) and choose next node kind to check. - // Next node kind relies on routing priority (static->param->any). So 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 next sibling this - // is left up to matching logic + // 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: by current implementation we never backtrack from any route so `previous.kind` value is here always static or any - // if this requirement is to change then for any route next kind would be static kind and this statement should be changed + // 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 { @@ -359,14 +360,16 @@ func (r *Router) Find(method, path string, c Context) { 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) - search = path[searchIndex:] } 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:] } + search = path[searchIndex:] return }