From 9bf1e3c8ce6de029186945e4ee82f89f2e2cc661 Mon Sep 17 00:00:00 2001 From: Artem Iurin Date: Mon, 11 Jul 2022 20:25:41 +0300 Subject: [PATCH] Allow different param names in different methods with same path scheme (#2209) * Change methodHandler element type to methodContext Signed-off-by: ortyomka * Allow different param names in the smae path with different methods Signed-off-by: ortyomka * Rename methodContext to routeMethod Add paramsCount in each node for perfomance Signed-off-by: ortyomka * Add backtracking to nearest path Signed-off-by: ortyomka * Remove params from NotAllowed Signed-off-by: ortyomka --- router.go | 210 ++++++++++++++++++++++++++----------------------- router_test.go | 29 ++++++- 2 files changed, 141 insertions(+), 98 deletions(-) diff --git a/router.go b/router.go index b5e50d94f..6a8615d83 100644 --- a/router.go +++ b/router.go @@ -19,30 +19,35 @@ type ( prefix string parent *node staticChildren children - ppath string - pnames []string - methodHandler *methodHandler + originalPath string + methods *routeMethods paramChild *node anyChild *node + paramsCount int // 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 - methodHandler struct { - connect HandlerFunc - delete HandlerFunc - get HandlerFunc - head HandlerFunc - options HandlerFunc - patch HandlerFunc - post HandlerFunc - propfind HandlerFunc - put HandlerFunc - trace HandlerFunc - report HandlerFunc + kind uint8 + children []*node + routeMethod struct { + ppath string + pnames []string + handler HandlerFunc + } + routeMethods struct { + connect *routeMethod + delete *routeMethod + get *routeMethod + head *routeMethod + options *routeMethod + patch *routeMethod + post *routeMethod + propfind *routeMethod + put *routeMethod + trace *routeMethod + report *routeMethod allowHeader string } ) @@ -56,7 +61,7 @@ const ( anyLabel = byte('*') ) -func (m *methodHandler) isHandler() bool { +func (m *routeMethods) isHandler() bool { return m.connect != nil || m.delete != nil || m.get != nil || @@ -70,7 +75,7 @@ func (m *methodHandler) isHandler() bool { m.report != nil } -func (m *methodHandler) updateAllowHeader() { +func (m *routeMethods) updateAllowHeader() { buf := new(bytes.Buffer) buf.WriteString(http.MethodOptions) @@ -119,7 +124,7 @@ func (m *methodHandler) updateAllowHeader() { func NewRouter(e *Echo) *Router { return &Router{ tree: &node{ - methodHandler: new(methodHandler), + methods: new(routeMethods), }, routes: map[string]*Route{}, echo: e, @@ -153,7 +158,7 @@ func (r *Router) Add(method, path string, h HandlerFunc) { } j := i + 1 - r.insert(method, path[:i], nil, staticKind, "", nil) + r.insert(method, path[:i], staticKind, routeMethod{}) for ; i < lcpIndex && path[i] != '/'; i++ { } @@ -163,23 +168,23 @@ func (r *Router) Add(method, path string, h HandlerFunc) { if i == lcpIndex { // path node is last fragment of route path. ie. `/users/:id` - r.insert(method, path[:i], h, paramKind, ppath, pnames) + r.insert(method, path[:i], paramKind, routeMethod{ppath, pnames, h}) } else { - r.insert(method, path[:i], nil, paramKind, "", nil) + r.insert(method, path[:i], paramKind, routeMethod{}) } } else if path[i] == '*' { - r.insert(method, path[:i], nil, staticKind, "", nil) + r.insert(method, path[:i], staticKind, routeMethod{}) pnames = append(pnames, "*") - r.insert(method, path[:i+1], h, anyKind, ppath, pnames) + r.insert(method, path[:i+1], anyKind, routeMethod{ppath, pnames, h}) } } - r.insert(method, path, h, staticKind, ppath, pnames) + r.insert(method, path, staticKind, routeMethod{ppath, pnames, h}) } -func (r *Router) insert(method, path string, h HandlerFunc, t kind, ppath string, pnames []string) { +func (r *Router) insert(method, path string, t kind, rm routeMethod) { // Adjust max param - paramLen := len(pnames) + paramLen := len(rm.pnames) if *r.echo.maxParam < paramLen { *r.echo.maxParam = paramLen } @@ -207,11 +212,11 @@ func (r *Router) insert(method, path string, h HandlerFunc, t kind, ppath string // At root node currentNode.label = search[0] currentNode.prefix = search - if h != nil { + if rm.handler != nil { currentNode.kind = t - currentNode.addHandler(method, h) - currentNode.ppath = ppath - currentNode.pnames = pnames + currentNode.addMethod(method, &rm) + currentNode.paramsCount = len(rm.pnames) + currentNode.originalPath = rm.ppath } currentNode.isLeaf = currentNode.staticChildren == nil && currentNode.paramChild == nil && currentNode.anyChild == nil } else if lcpLen < prefixLen { @@ -221,9 +226,9 @@ func (r *Router) insert(method, path string, h HandlerFunc, t kind, ppath string currentNode.prefix[lcpLen:], currentNode, currentNode.staticChildren, - currentNode.methodHandler, - currentNode.ppath, - currentNode.pnames, + currentNode.originalPath, + currentNode.methods, + currentNode.paramsCount, currentNode.paramChild, currentNode.anyChild, ) @@ -243,9 +248,9 @@ func (r *Router) insert(method, path string, h HandlerFunc, t kind, ppath string currentNode.label = currentNode.prefix[0] currentNode.prefix = currentNode.prefix[:lcpLen] currentNode.staticChildren = nil - currentNode.methodHandler = new(methodHandler) - currentNode.ppath = "" - currentNode.pnames = nil + currentNode.originalPath = "" + currentNode.methods = new(routeMethods) + currentNode.paramsCount = 0 currentNode.paramChild = nil currentNode.anyChild = nil currentNode.isLeaf = false @@ -257,13 +262,19 @@ func (r *Router) insert(method, path string, h HandlerFunc, t kind, ppath string if lcpLen == searchLen { // At parent node currentNode.kind = t - currentNode.addHandler(method, h) - currentNode.ppath = ppath - currentNode.pnames = pnames + if rm.handler != nil { + currentNode.addMethod(method, &rm) + currentNode.paramsCount = len(rm.pnames) + currentNode.originalPath = rm.ppath + } } else { // Create child node - n = newNode(t, search[lcpLen:], currentNode, nil, new(methodHandler), ppath, pnames, nil, nil) - n.addHandler(method, h) + n = newNode(t, search[lcpLen:], currentNode, nil, "", new(routeMethods), 0, nil, nil) + if rm.handler != nil { + n.addMethod(method, &rm) + n.paramsCount = len(rm.pnames) + n.originalPath = rm.ppath + } // Only Static children could reach here currentNode.addStaticChild(n) } @@ -277,8 +288,12 @@ func (r *Router) insert(method, path string, h HandlerFunc, t kind, ppath string continue } // Create child node - n := newNode(t, search, currentNode, nil, new(methodHandler), ppath, pnames, nil, nil) - n.addHandler(method, h) + n := newNode(t, search, currentNode, nil, rm.ppath, new(routeMethods), 0, nil, nil) + if rm.handler != nil { + n.addMethod(method, &rm) + n.paramsCount = len(rm.pnames) + } + switch t { case staticKind: currentNode.addStaticChild(n) @@ -290,28 +305,26 @@ func (r *Router) insert(method, path string, h HandlerFunc, t kind, ppath string currentNode.isLeaf = currentNode.staticChildren == nil && currentNode.paramChild == nil && currentNode.anyChild == nil } else { // Node already exists - if h != nil { - currentNode.addHandler(method, h) - currentNode.ppath = ppath - if len(currentNode.pnames) == 0 { // Issue #729 - currentNode.pnames = pnames - } + if rm.handler != nil { + currentNode.addMethod(method, &rm) + currentNode.paramsCount = len(rm.pnames) + currentNode.originalPath = rm.ppath } } return } } -func newNode(t kind, pre string, p *node, sc children, mh *methodHandler, ppath string, pnames []string, paramChildren, anyChildren *node) *node { +func newNode(t kind, pre string, p *node, sc children, originalPath string, mh *routeMethods, paramsCount int, paramChildren, anyChildren *node) *node { return &node{ kind: t, label: pre[0], prefix: pre, parent: p, staticChildren: sc, - ppath: ppath, - pnames: pnames, - methodHandler: mh, + originalPath: originalPath, + methods: mh, + paramsCount: paramsCount, paramChild: paramChildren, anyChild: anyChildren, isLeaf: sc == nil && paramChildren == nil && anyChildren == nil, @@ -345,64 +358,60 @@ func (n *node) findChildWithLabel(l byte) *node { return nil } -func (n *node) addHandler(method string, h HandlerFunc) { +func (n *node) addMethod(method string, h *routeMethod) { switch method { case http.MethodConnect: - n.methodHandler.connect = h + n.methods.connect = h case http.MethodDelete: - n.methodHandler.delete = h + n.methods.delete = h case http.MethodGet: - n.methodHandler.get = h + n.methods.get = h case http.MethodHead: - n.methodHandler.head = h + n.methods.head = h case http.MethodOptions: - n.methodHandler.options = h + n.methods.options = h case http.MethodPatch: - n.methodHandler.patch = h + n.methods.patch = h case http.MethodPost: - n.methodHandler.post = h + n.methods.post = h case PROPFIND: - n.methodHandler.propfind = h + n.methods.propfind = h case http.MethodPut: - n.methodHandler.put = h + n.methods.put = h case http.MethodTrace: - n.methodHandler.trace = h + n.methods.trace = h case REPORT: - n.methodHandler.report = h + n.methods.report = h } - n.methodHandler.updateAllowHeader() - if h != nil { - n.isHandler = true - } else { - n.isHandler = n.methodHandler.isHandler() - } + n.methods.updateAllowHeader() + n.isHandler = true } -func (n *node) findHandler(method string) HandlerFunc { +func (n *node) findMethod(method string) *routeMethod { switch method { case http.MethodConnect: - return n.methodHandler.connect + return n.methods.connect case http.MethodDelete: - return n.methodHandler.delete + return n.methods.delete case http.MethodGet: - return n.methodHandler.get + return n.methods.get case http.MethodHead: - return n.methodHandler.head + return n.methods.head case http.MethodOptions: - return n.methodHandler.options + return n.methods.options case http.MethodPatch: - return n.methodHandler.patch + return n.methods.patch case http.MethodPost: - return n.methodHandler.post + return n.methods.post case PROPFIND: - return n.methodHandler.propfind + return n.methods.propfind case http.MethodPut: - return n.methodHandler.put + return n.methods.put case http.MethodTrace: - return n.methodHandler.trace + return n.methods.trace case REPORT: - return n.methodHandler.report + return n.methods.report default: return nil } @@ -433,7 +442,7 @@ func (r *Router) Find(method, path string, c Context) { var ( previousBestMatchNode *node - matchedHandler HandlerFunc + matchedRouteMethod *routeMethod // 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 @@ -529,8 +538,8 @@ func (r *Router) Find(method, path string, c Context) { if previousBestMatchNode == nil { previousBestMatchNode = currentNode } - if h := currentNode.findHandler(method); h != nil { - matchedHandler = h + if h := currentNode.findMethod(method); h != nil { + matchedRouteMethod = h break } } @@ -569,7 +578,8 @@ func (r *Router) Find(method, path string, c Context) { if child := currentNode.anyChild; child != nil { // If any node is found, use remaining path for paramValues currentNode = child - paramValues[len(currentNode.pnames)-1] = search + paramValues[currentNode.paramsCount-1] = search + // update indexes/search in case we need to backtrack when no handler match is found paramIndex++ searchIndex += +len(search) @@ -580,8 +590,8 @@ func (r *Router) Find(method, path string, c Context) { if previousBestMatchNode == nil { previousBestMatchNode = currentNode } - if h := currentNode.findHandler(method); h != nil { - matchedHandler = h + if h := currentNode.findMethod(method); h != nil { + matchedRouteMethod = h break } } @@ -604,22 +614,28 @@ func (r *Router) Find(method, path string, c Context) { return // nothing matched at all } - if matchedHandler != nil { - ctx.handler = matchedHandler + var rPath string + var rPNames []string + if matchedRouteMethod != nil { + ctx.handler = matchedRouteMethod.handler + rPath = matchedRouteMethod.ppath + rPNames = matchedRouteMethod.pnames } 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 + rPath = currentNode.originalPath + rPNames = nil // no params here ctx.handler = NotFoundHandler if currentNode.isHandler { - ctx.Set(ContextKeyHeaderAllow, currentNode.methodHandler.allowHeader) + ctx.Set(ContextKeyHeaderAllow, currentNode.methods.allowHeader) ctx.handler = MethodNotAllowedHandler if method == http.MethodOptions { - ctx.handler = optionsMethodHandler(currentNode.methodHandler.allowHeader) + ctx.handler = optionsMethodHandler(currentNode.methods.allowHeader) } } } - ctx.path = currentNode.ppath - ctx.pnames = currentNode.pnames + ctx.path = rPath + ctx.pnames = rPNames } diff --git a/router_test.go b/router_test.go index 457566b90..8645a26c1 100644 --- a/router_test.go +++ b/router_test.go @@ -2318,6 +2318,33 @@ func TestRouterPanicWhenParamNoRootOnlyChildsFailsFind(t *testing.T) { } } +// Issue #1726 +func TestRouterDifferentParamsInPath(t *testing.T) { + e := New() + r := e.router + r.Add(http.MethodPut, "/*", func(Context) error { + return nil + }) + r.Add(http.MethodPut, "/users/:vid/files/:gid", func(Context) error { + return nil + }) + r.Add(http.MethodGet, "/users/:uid/files/:fid", func(Context) error { + return nil + }) + c := e.NewContext(nil, nil).(*context) + + r.Find(http.MethodGet, "/users/1/files/2", c) + assert.Equal(t, "1", c.Param("uid")) + assert.Equal(t, "2", c.Param("fid")) + + r.Find(http.MethodGet, "/users/1/shouldBacktrackToFirstAnyRouteAnd405", c) + assert.Equal(t, "/*", c.Path()) + + r.Find(http.MethodPut, "/users/3/files/4", c) + assert.Equal(t, "3", c.Param("vid")) + assert.Equal(t, "4", c.Param("gid")) +} + func TestRouterHandleMethodOptions(t *testing.T) { e := New() r := e.router @@ -2380,7 +2407,7 @@ func TestRouterHandleMethodOptions(t *testing.T) { assert.NoError(t, err) assert.Equal(t, tc.expectStatus, rec.Code) } - assert.Equal(t, tc.expectAllowHeader, c.Response().Header().Get("Allow")) + assert.Equal(t, tc.expectAllowHeader, c.Response().Header().Get(HeaderAllow)) }) } }