From f98dc970603c46573e6c6e6567956c1d5e962067 Mon Sep 17 00:00:00 2001 From: toimtoimtoim Date: Tue, 16 Mar 2021 21:00:16 +0200 Subject: [PATCH 1/2] when url ends with slash first param route is the match (fix #1804) --- router.go | 7 ++++++- router_test.go | 10 +++++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/router.go b/router.go index 2dd09fae2..226e4ce04 100644 --- a/router.go +++ b/router.go @@ -446,7 +446,12 @@ func (r *Router) Find(method, path string, c Context) { // Param node if child := currentNode.paramChild; search != "" && child != nil { currentNode = child - // FIXME: when param node does not have any children then param node should act similarly to any node - consider all remaining search as match + // when param node does not have any children then param node should act similarly to any node - consider all remaining search as match + if currentNode.staticChildren == nil && currentNode.paramChild == nil && currentNode.anyChild == nil { + paramValues[paramIndex] = search + break + } + i, l := 0, len(search) for ; i < l && search[i] != '/'; i++ { } diff --git a/router_test.go b/router_test.go index 47e499402..bcf9f011c 100644 --- a/router_test.go +++ b/router_test.go @@ -692,11 +692,11 @@ func TestRouterParam(t *testing.T) { expectRoute: "/users/:id", expectParam: map[string]string{"id": "1"}, }, - { // FIXME: this documents current implementation (slash at end is problematic) + { name: "route /users/1/ to /users/:id", whenURL: "/users/1/", - expectRoute: nil, // FIXME: should be "/users/:id", - expectParam: nil, // FIXME: should be map[string]string{"id": "1/"}, + expectRoute: "/users/:id", + expectParam: map[string]string{"id": "1/"}, }, } @@ -740,8 +740,8 @@ func TestRouterParamWithSlash(t *testing.T) { r.Find(http.MethodGet, "/a/1/c/d/2/3", c) // `2/3` should mapped to path `/a/:b/c/d/:e` and into `:e` err := c.handler(c) - assert.Equal(t, nil, c.Get("path")) // FIXME: should be "/a/:b/c/d/:e" - assert.EqualError(t, err, "code=404, message=Not Found") // FIXME: should be .NoError() + assert.Equal(t, "/a/:b/c/d/:e", c.Get("path")) + assert.NoError(t, err) } // Issue #1754 - router needs to backtrack multiple levels upwards in tree to find the matching route From 546a1e5995460e4d8d1268187776d6f91ccf300f Mon Sep 17 00:00:00 2001 From: toimtoimtoim Date: Tue, 16 Mar 2021 23:22:55 +0200 Subject: [PATCH 2/2] router should check if method is suitable for matching route and if not then continue search in tree (fix #1808) --- context_test.go | 6 ++- router.go | 116 ++++++++++++++++++++++++++++++++++++++---------- router_test.go | 69 ++++++++++++++++++++++++++-- 3 files changed, 163 insertions(+), 28 deletions(-) diff --git a/context_test.go b/context_test.go index 963c91e04..2c61ffb3a 100644 --- a/context_test.go +++ b/context_test.go @@ -464,7 +464,9 @@ func TestContextPath(t *testing.T) { e := New() r := e.Router() - r.Add(http.MethodGet, "/users/:id", nil) + handler := func(c Context) error { return c.String(http.StatusOK, "OK") } + + r.Add(http.MethodGet, "/users/:id", handler) c := e.NewContext(nil, nil) r.Find(http.MethodGet, "/users/1", c) @@ -472,7 +474,7 @@ func TestContextPath(t *testing.T) { assert.Equal("/users/:id", c.Path()) - r.Add(http.MethodGet, "/users/:uid/files/:fid", nil) + r.Add(http.MethodGet, "/users/:uid/files/:fid", handler) c = e.NewContext(nil, nil) r.Find(http.MethodGet, "/users/1/files/1", c) assert.Equal("/users/:uid/files/:fid", c.Path()) diff --git a/router.go b/router.go index 226e4ce04..5b2474b32 100644 --- a/router.go +++ b/router.go @@ -23,6 +23,10 @@ type ( methodHandler *methodHandler paramChild *node anyChild *node + // isLeaf indicates that node does not have child routes + isLeaf bool + // isHandler indicates that node has at least one handler registered to it + isHandler bool } kind uint8 children []*node @@ -50,6 +54,20 @@ const ( anyLabel = byte('*') ) +func (m *methodHandler) isHandler() bool { + return m.connect != nil || + m.delete != nil || + m.get != nil || + m.head != nil || + m.options != nil || + m.patch != nil || + m.post != nil || + m.propfind != nil || + m.put != nil || + m.trace != nil || + m.report != nil +} + // NewRouter returns a new Router instance. func NewRouter(e *Echo) *Router { return &Router{ @@ -73,6 +91,11 @@ func (r *Router) Add(method, path string, h HandlerFunc) { pnames := []string{} // Param names ppath := path // Pristine path + if h == nil && r.echo.Logger != nil { + // FIXME: in future we should return error + r.echo.Logger.Errorf("Adding route without handler function: %v:%v", method, path) + } + for i, lcpIndex := 0, len(path); i < lcpIndex; i++ { if path[i] == ':' { j := i + 1 @@ -86,6 +109,7 @@ func (r *Router) Add(method, path string, h HandlerFunc) { i, lcpIndex = j, len(path) if i == lcpIndex { + // path node is last fragment of route path. ie. `/users/:id` r.insert(method, path[:i], h, paramKind, ppath, pnames) } else { r.insert(method, path[:i], nil, paramKind, "", nil) @@ -136,6 +160,7 @@ func (r *Router) insert(method, path string, h HandlerFunc, t kind, ppath string currentNode.ppath = ppath currentNode.pnames = pnames } + currentNode.isLeaf = currentNode.staticChildren == nil && currentNode.paramChild == nil && currentNode.anyChild == nil } else if lcpLen < prefixLen { // Split node n := newNode( @@ -149,7 +174,6 @@ func (r *Router) insert(method, path string, h HandlerFunc, t kind, ppath string currentNode.paramChild, currentNode.anyChild, ) - // Update parent path for all children to new node for _, child := range currentNode.staticChildren { child.parent = n @@ -171,6 +195,8 @@ func (r *Router) insert(method, path string, h HandlerFunc, t kind, ppath string currentNode.pnames = nil currentNode.paramChild = nil currentNode.anyChild = nil + currentNode.isLeaf = false + currentNode.isHandler = false // Only Static children could reach here currentNode.addStaticChild(n) @@ -188,6 +214,7 @@ func (r *Router) insert(method, path string, h HandlerFunc, t kind, ppath string // Only Static children could reach here currentNode.addStaticChild(n) } + currentNode.isLeaf = currentNode.staticChildren == nil && currentNode.paramChild == nil && currentNode.anyChild == nil } else if lcpLen < searchLen { search = search[lcpLen:] c := currentNode.findChildWithLabel(search[0]) @@ -207,6 +234,7 @@ func (r *Router) insert(method, path string, h HandlerFunc, t kind, ppath string case anyKind: currentNode.anyChild = n } + currentNode.isLeaf = currentNode.staticChildren == nil && currentNode.paramChild == nil && currentNode.anyChild == nil } else { // Node already exists if h != nil { @@ -233,6 +261,8 @@ func newNode(t kind, pre string, p *node, sc children, mh *methodHandler, ppath methodHandler: mh, paramChild: paramChildren, anyChild: anyChildren, + isLeaf: sc == nil && paramChildren == nil && anyChildren == nil, + isHandler: mh.isHandler(), } } @@ -289,6 +319,12 @@ func (n *node) addHandler(method string, h HandlerFunc) { case REPORT: n.methodHandler.report = h } + + if h != nil { + n.isHandler = true + } else { + n.isHandler = n.methodHandler.isHandler() + } } func (n *node) findHandler(method string) HandlerFunc { @@ -343,6 +379,8 @@ func (r *Router) Find(method, path string, c Context) { currentNode := r.tree // Current node as root var ( + previousBestMatchNode *node + matchedHandler HandlerFunc // search stores the remaining path to check for match. By each iteration we move from start of path to end of the path // and search value gets shorter and shorter. search = path @@ -362,10 +400,11 @@ func (r *Router) Find(method, path string, c Context) { valid = currentNode != 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 previous.kind == anyKind { + nextNodeKind = staticKind + } else { + nextNodeKind = previous.kind + 1 + } if fromKind == staticKind { // when backtracking is done from static kind block we did not change search so nothing to restore @@ -380,6 +419,7 @@ func (r *Router) Find(method, path string, c Context) { // 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(paramValues[paramIndex]) + paramValues[paramIndex] = "" } search = path[searchIndex:] return @@ -421,7 +461,7 @@ func (r *Router) Find(method, path string, c Context) { // goto Any } else { // Not found (this should never be possible for static node we are looking currently) - return + break } } @@ -429,9 +469,17 @@ func (r *Router) Find(method, path string, c Context) { search = search[lcpLen:] searchIndex = searchIndex + lcpLen - // Finish routing if no remaining search and we are on an leaf node - if search == "" && currentNode.ppath != "" { - break + // Finish routing if no remaining search and we are on a node with handler and matching method type + if search == "" && currentNode.isHandler { + // check if current node has handler registered for http method we are looking for. we store currentNode as + // best matching in case we do no find no more routes matching this path+method + if previousBestMatchNode == nil { + previousBestMatchNode = currentNode + } + if h := currentNode.findHandler(method); h != nil { + matchedHandler = h + break + } } // Static node @@ -446,15 +494,16 @@ func (r *Router) Find(method, path string, c Context) { // Param node if child := currentNode.paramChild; search != "" && child != nil { currentNode = child - // when param node does not have any children then param node should act similarly to any node - consider all remaining search as match - if currentNode.staticChildren == nil && currentNode.paramChild == nil && currentNode.anyChild == nil { - paramValues[paramIndex] = search - break + i := 0 + l := len(search) + if currentNode.isLeaf { + // when param node does not have any children then param node should act similarly to any node - consider all remaining search as match + i = l + } else { + for ; i < l && search[i] != '/'; i++ { + } } - i, l := 0, len(search) - for ; i < l && search[i] != '/'; i++ { - } paramValues[paramIndex] = search[:i] paramIndex++ search = search[i:] @@ -468,29 +517,50 @@ func (r *Router) Find(method, path string, c Context) { // If any node is found, use remaining path for paramValues currentNode = child paramValues[len(currentNode.pnames)-1] = search - break + // update indexes/search in case we need to backtrack when no handler match is found + paramIndex++ + searchIndex += +len(search) + search = "" + + // check if current node has handler registered for http method we are looking for. we store currentNode as + // best matching in case we do no find no more routes matching this path+method + if previousBestMatchNode == nil { + previousBestMatchNode = currentNode + } + if h := currentNode.findHandler(method); h != nil { + matchedHandler = h + break + } } // Let's backtrack to the first possible alternative node of the decision path nk, ok := backtrackToNextNodeKind(anyKind) if !ok { - return // No other possibilities on the decision path + break // No other possibilities on the decision path } else if nk == paramKind { goto Param } else if nk == anyKind { goto Any } else { // Not found - return + break } } - ctx.handler = currentNode.findHandler(method) - ctx.path = currentNode.ppath - ctx.pnames = currentNode.pnames + if currentNode == nil && previousBestMatchNode == nil { + return // nothing matched at all + } - if ctx.handler == nil { + if matchedHandler != nil { + ctx.handler = matchedHandler + } else { + // use previous match as basis. although we have no matching handler we have path match. + // so we can send http.StatusMethodNotAllowed (405) instead of http.StatusNotFound (404) + currentNode = previousBestMatchNode ctx.handler = currentNode.checkMethodNotAllowed() } + ctx.path = currentNode.ppath + ctx.pnames = currentNode.pnames + return } diff --git a/router_test.go b/router_test.go index bcf9f011c..71cedf8b6 100644 --- a/router_test.go +++ b/router_test.go @@ -716,6 +716,69 @@ func TestRouterParam(t *testing.T) { } } +func TestMethodNotAllowedAndNotFound(t *testing.T) { + e := New() + r := e.router + + // Routes + r.Add(http.MethodGet, "/*", handlerFunc) + r.Add(http.MethodPost, "/users/:id", handlerFunc) + + var testCases = []struct { + name string + whenMethod string + whenURL string + expectRoute interface{} + expectParam map[string]string + expectError error + }{ + { + name: "exact match for route+method", + whenMethod: http.MethodPost, + whenURL: "/users/1", + expectRoute: "/users/:id", + expectParam: map[string]string{"id": "1"}, + }, + { + name: "matches node but not method. sends 405 from best match node", + whenMethod: http.MethodPut, + whenURL: "/users/1", + expectRoute: nil, + expectError: ErrMethodNotAllowed, + }, + { + name: "best match is any route up in tree", + whenMethod: http.MethodGet, + whenURL: "/users/1", + expectRoute: "/*", + expectParam: map[string]string{"*": "users/1"}, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + c := e.NewContext(nil, nil).(*context) + + method := http.MethodGet + if tc.whenMethod != "" { + method = tc.whenMethod + } + r.Find(method, tc.whenURL, c) + err := c.handler(c) + + if tc.expectError != nil { + assert.Equal(t, tc.expectError, err) + } else { + assert.NoError(t, err) + } + assert.Equal(t, tc.expectRoute, c.Get("path")) + for param, expectedValue := range tc.expectParam { + assert.Equal(t, expectedValue, c.Param(param)) + } + checkUnusedParamValues(t, c, tc.expectParam) + }) + } +} + func TestRouterTwoParam(t *testing.T) { e := New() r := e.router @@ -2004,10 +2067,10 @@ func TestRouterParam1466(t *testing.T) { expectRoute: "/users/:username", expectParam: map[string]string{"username": "sharewithme"}, }, - { + { // route `/users/signup` is registered for POST. so param route `/users/:username` (lesser priority) is matched as it has GET handler whenURL: "/users/signup", - expectRoute: nil, // method not found as this route is for POST but request is for GET - expectParam: map[string]string{"username": ""}, + expectRoute: "/users/:username", + expectParam: map[string]string{"username": "signup"}, }, // Additional assertions for #1479 {