Repository: labstack/echo
Pull request #1201 (2018-10-04 02:40:10)

Fix node's parameter names assignment problem
This commit fixed the issues(#1052, #1139) on which the parameters cannot be obtained on some paths defined as having the same prefix but with different parameter names.
This issue is caused because the `node.pnames` of shorter paths is overwritten by the node of longer paths inserted.
With this change, I managed to apply the changes to the `pnames` of a specific node only when the appending node corresponds to the end of URL's path.

I added the following test cases (#1052 and #1139). We have confirmed that the behavior is as expected.
However, this commit does not fix the case (#1139) found below because it needs to store `ppath` and `pnames` for each HTTP method like a `methodHandler`.

```go
func main() {
    e := echo.New()
    e.GET("/order/:shopid", func(c echo.Context) error {
        return c.NoContent(http.StatusOK)
    })
    e.DELETE("/order/:ref", func(c echo.Context) error {
        log.Printf("params %v", c.ParamNames())
        foo := c.Param("ref")
        return c.String(http.StatusOK, foo)
    })
    e.Logger.Fatal(e.Start("127.0.0.1:8080"))
}
```


codecov[bot]: # [Codecov](https://codecov.io/gh/labstack/echo/pull/1201?src=pr&el=h1) Report
> Merging [#1201](https://codecov.io/gh/labstack/echo/pull/1201?src=pr&el=desc) into [master](https://codecov.io/gh/labstack/echo/commit/036e6961aebec9e95c29c16bb7b1df64ac5ac601?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/labstack/echo/pull/1201/graphs/tree.svg?width=650&token=vqnT76l79i&height=150&src=pr)](https://codecov.io/gh/labstack/echo/pull/1201?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master    #1201   +/-   ##
=======================================
  Coverage   81.51%   81.51%           
=======================================
  Files          25       25           
  Lines        1920     1920           
=======================================
  Hits         1565     1565           
  Misses        248      248           
  Partials      107      107
```


| [Impacted Files](https://codecov.io/gh/labstack/echo/pull/1201?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [router.go](https://codecov.io/gh/labstack/echo/pull/1201/diff?src=pr&el=tree#diff-cm91dGVyLmdv) | `93.3% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/labstack/echo/pull/1201?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/labstack/echo/pull/1201?src=pr&el=footer). Last update [036e696...7b4be99](https://codecov.io/gh/labstack/echo/pull/1201?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/echo.go b/echo.go
index 996bc4b..2b3b8eb 100644
--- a/echo.go
+++ b/echo.go
@@ -256,6 +256,10 @@ var (
 	ErrMethodNotAllowed            = NewHTTPError(http.StatusMethodNotAllowed)
 	ErrStatusRequestEntityTooLarge = NewHTTPError(http.StatusRequestEntityTooLarge)
 	ErrTooManyRequests             = NewHTTPError(http.StatusTooManyRequests)
+	ErrBadRequest                  = NewHTTPError(http.StatusBadRequest)
+	ErrBadGateway                  = NewHTTPError(http.StatusBadGateway)
+	ErrInternalServerError         = NewHTTPError(http.StatusInternalServerError)
+	ErrRequestTimeout              = NewHTTPError(http.StatusRequestTimeout)
 	ErrValidatorNotRegistered      = errors.New("validator not registered")
 	ErrRendererNotRegistered       = errors.New("renderer not registered")
 	ErrInvalidRedirectCode         = errors.New("invalid redirect status code")
diff --git a/router.go b/router.go
index ff53da8..3badddb 100644
--- a/router.go
+++ b/router.go
@@ -79,7 +79,7 @@ func (r *Router) Add(method, path string, h HandlerFunc) {
 				r.insert(method, path[:i], h, pkind, ppath, pnames)
 				return
 			}
-			r.insert(method, path[:i], nil, pkind, ppath, pnames)
+			r.insert(method, path[:i], nil, pkind, "", nil)
 		} else if path[i] == '*' {
 			r.insert(method, path[:i], nil, skind, "", nil)
 			pnames = append(pnames, "*")


Test patch:
diff --git a/router_test.go b/router_test.go
index 3a8b259..d637017 100644
--- a/router_test.go
+++ b/router_test.go
@@ -874,6 +874,42 @@ func TestRouterParamAlias(t *testing.T) {
 	testRouterAPI(t, api)
 }
 
+// Issue #1052
+func TestRouterParamOrdering(t *testing.T) {
+	api := []*Route{
+		{GET, "/:a/:b/:c/:id", ""},
+		{GET, "/:a/:id", ""},
+		{GET, "/:a/:e/:id", ""},
+	}
+	testRouterAPI(t, api)
+	api2 := []*Route{
+		{GET, "/:a/:id", ""},
+		{GET, "/:a/:e/:id", ""},
+		{GET, "/:a/:b/:c/:id", ""},
+	}
+	testRouterAPI(t, api2)
+	api3 := []*Route{
+		{GET, "/:a/:b/:c/:id", ""},
+		{GET, "/:a/:e/:id", ""},
+		{GET, "/:a/:id", ""},
+	}
+	testRouterAPI(t, api3)
+}
+
+// Issue #1139
+func TestRouterMixedParams(t *testing.T) {
+	api := []*Route{
+		{GET, "/teacher/:tid/room/suggestions", ""},
+		{GET, "/teacher/:id", ""},
+	}
+	testRouterAPI(t, api)
+	api2 := []*Route{
+		{GET, "/teacher/:id", ""},
+		{GET, "/teacher/:tid/room/suggestions", ""},
+	}
+	testRouterAPI(t, api2)
+}
+
 func benchmarkRouterRoutes(b *testing.B, routes []*Route) {
 	e := New()
 	r := e.router
@@ -914,7 +950,7 @@ func BenchmarkRouterGooglePlusAPI(b *testing.B) {
 
 func (n *node) printTree(pfx string, tail bool) {
 	p := prefix(tail, pfx, "└── ", "├── ")
-	fmt.Printf("%s%s, %p: type=%d, parent=%p, handler=%v\n", p, n.prefix, n, n.kind, n.parent, n.methodHandler)
+	fmt.Printf("%s%s, %p: type=%d, parent=%p, handler=%v, pnames=%v\n", p, n.prefix, n, n.kind, n.parent, n.methodHandler, n.pnames)
 
 	children := n.children
 	l := len(children)