Allow different param names in different methods with same path scheme (#2209)

* Change methodHandler element type to methodContext

Signed-off-by: ortyomka <iurin.art@gmail.com>

* Allow different param names in the smae path with different methods

Signed-off-by: ortyomka <iurin.art@gmail.com>

* Rename methodContext to routeMethod
Add paramsCount in each node for perfomance

Signed-off-by: ortyomka <iurin.art@gmail.com>

* Add backtracking to nearest path

Signed-off-by: ortyomka <iurin.art@gmail.com>

* Remove params from NotAllowed

Signed-off-by: ortyomka <iurin.art@gmail.com>

router.go

@@ -19,30 +19,35 @@ type (
 		prefix         string
 		parent         *node
 		staticChildren children
-		ppath          string
+		originalPath   string
-		pnames         []string
+		methods        *routeMethods
-		methodHandler  *methodHandler
 		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
+	kind        uint8
-	children      []*node
+	children    []*node
-	methodHandler struct {
+	routeMethod struct {
-		connect     HandlerFunc
+		ppath   string
-		delete      HandlerFunc
+		pnames  []string
-		get         HandlerFunc
+		handler HandlerFunc
-		head        HandlerFunc
+	}
-		options     HandlerFunc
+	routeMethods struct {
-		patch       HandlerFunc
+		connect     *routeMethod
-		post        HandlerFunc
+		delete      *routeMethod
-		propfind    HandlerFunc
+		get         *routeMethod
-		put         HandlerFunc
+		head        *routeMethod
-		trace       HandlerFunc
+		options     *routeMethod
-		report      HandlerFunc
+		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.addMethod(method, &rm)
-				currentNode.ppath = ppath
+				currentNode.paramsCount = len(rm.pnames)
-				currentNode.pnames = 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.originalPath,
-				currentNode.ppath,
+				currentNode.methods,
-				currentNode.pnames,
+				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.originalPath = ""
-			currentNode.ppath = ""
+			currentNode.methods = new(routeMethods)
-			currentNode.pnames = nil
+			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)
+				if rm.handler != nil {
-				currentNode.ppath = ppath
+					currentNode.addMethod(method, &rm)
-				currentNode.pnames = pnames
+					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 = newNode(t, search[lcpLen:], currentNode, nil, "", new(routeMethods), 0, nil, nil)
-				n.addHandler(method, h)
+				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 := newNode(t, search, currentNode, nil, rm.ppath, new(routeMethods), 0, nil, nil)
-			n.addHandler(method, h)
+			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 {
+			if rm.handler != nil {
-				currentNode.addHandler(method, h)
+				currentNode.addMethod(method, &rm)
-				currentNode.ppath = ppath
+				currentNode.paramsCount = len(rm.pnames)
-				if len(currentNode.pnames) == 0 { // Issue #729
+				currentNode.originalPath = rm.ppath
-					currentNode.pnames = pnames
-				}
 			}
 		}
 		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,
+		originalPath:   originalPath,
-		pnames:         pnames,
+		methods:        mh,
-		methodHandler:  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()
+	n.methods.updateAllowHeader()
-	if h != nil {
+	n.isHandler = true
-		n.isHandler = true
-	} else {
-		n.isHandler = n.methodHandler.isHandler()
-	}
 }
 
-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 {
+			if h := currentNode.findMethod(method); h != nil {
-				matchedHandler = h
+				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 {
+			if h := currentNode.findMethod(method); h != nil {
-				matchedHandler = h
+				matchedRouteMethod = h
 				break
 			}
 		}
@@ -604,22 +614,28 @@ func (r *Router) Find(method, path string, c Context) {
 		return // nothing matched at all
 	}
 
-	if matchedHandler != nil {
+	var rPath string
-		ctx.handler = matchedHandler
+	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.path = rPath
-	ctx.pnames = currentNode.pnames
+	ctx.pnames = rPNames
 }

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))
 		})
 	}
 }