fix(router): grow PathValues for contexts created before a route with more params (#3157)

Adding a route with more path params after Echo has served requests panicked with "index out of range" on the next request that matched it, with both DefaultRouter and ConcurrentRouter. Pooled Contexts keep the PathValues capacity they were created with. DefaultRouter.Route detected the short capacity but allocated a zero-length slice and then set values by index.

The Context's PathValues are now grown at full length in that branch and kept, so later requests do not grow them again. The Router contract documents that Route must handle such Contexts.

Refs #1705

Author: vishr

echo_test.go

@@ -1966,3 +1966,33 @@ func TestHasDotOrEmptySegment(t *testing.T) {
 		})
 	}
 }
+
+func TestEcho_AddParamRouteAfterServing(t *testing.T) {
+	var testCases = []struct {
+		name   string
+		router Router
+	}{
+		{name: "ok, default router", router: NewRouter(RouterConfig{})},
+		{name: "ok, concurrent router", router: NewConcurrentRouter(NewRouter(RouterConfig{}))},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			e := NewWithConfig(Config{Router: tc.router})
+			e.GET("/static", func(c *Context) error { return c.String(http.StatusOK, "static") })
+
+			rec := httptest.NewRecorder()
+			e.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/static", nil))
+			assert.Equal(t, "static", rec.Body.String())
+
+			// the pooled context from the first request has no PathValues capacity for this route. sync.Pool may hand
+			// out a new context instead (often under -race); TestDefaultRouter_RouteWithContextCreatedBeforeParamRouteAdded
+			// covers the stale context deterministically.
+			e.GET("/users/:id", func(c *Context) error { return c.String(http.StatusOK, c.Param("id")) })
+
+			rec = httptest.NewRecorder()
+			e.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/users/42", nil))
+			assert.Equal(t, http.StatusOK, rec.Code)
+			assert.Equal(t, "42", rec.Body.String())
+		})
+	}
+}

router.go

@@ -15,8 +15,11 @@ import (
 // Router is interface for routing request contexts to registered routes.
 //
 // Contract between Echo/Context instance and the router:
-//   - all routes must be added through methods on echo.Echo instance.
-//     Reason: Echo instance uses RouteInfo.Params() length to allocate slice for paths parameters (see `Echo.contextPathParamAllocSize`).
+//   - all routes should be added through methods on echo.Echo instance.
+//     Reason: Echo instance uses RouteInfo.Parameters length to allocate slice for paths parameters (see `Echo.contextPathParamAllocSize`),
+//     so Contexts are created with enough capacity.
+//   - Router.Route must handle a Context whose PathValues capacity is smaller than the maximum path parameter count
+//     of its routes (for example, when a route was added after the Context was created, or not through echo.Echo).
 //   - Router must populate Context during Router.Route call with:
 //   - Context.InitializeRoute (IMPORTANT! to reduce allocations use same slice that c.PathValues() returns)
 //   - Optionally can set additional information to Context with Context.Set
@@ -916,7 +919,11 @@ var optionsMethodHandler = func(c *Context) error {
 func (r *DefaultRouter) Route(c *Context) HandlerFunc {
 	pathValues := c.PathValues()
 	if cap(pathValues) < r.maxPathParamsLength {
-		pathValues = make(PathValues, 0, r.maxPathParamsLength)
+		// The Context has less capacity than this router's routes need, for example because it was created before a
+		// route with more params was added. Grow its PathValues at full length so values can be set by index below. The
+		// Context keeps the new slice, so later requests do not grow it again unless a route with more params is added.
+		*c.pathValues = make(PathValues, r.maxPathParamsLength)
+		pathValues = *c.pathValues
 	} else {
 		pathValues = pathValues[0:cap(pathValues)] // resize slice to maximum capacity so we can index set values
 	}

router_test.go

@@ -2617,6 +2617,56 @@ func TestRouterParam1466(t *testing.T) {
 	}
 }
 
+func TestDefaultRouter_RouteWithContextCreatedBeforeParamRouteAdded(t *testing.T) {
+	var testCases = []struct {
+		name             string
+		whenURL          string
+		expectRoute      string
+		expectPathValues PathValues
+	}{
+		{
+			name:             "ok, param and any route",
+			whenURL:          "/users/1/files/a.txt",
+			expectRoute:      "/users/:id/files/*",
+			expectPathValues: PathValues{{Name: "id", Value: "1"}, {Name: "*", Value: "a.txt"}},
+		},
+		{
+			name:             "ok, static route",
+			whenURL:          "/static",
+			expectRoute:      "/static",
+			expectPathValues: PathValues{},
+		},
+		{
+			name:             "ok, route not found",
+			whenURL:          "/missing",
+			expectRoute:      "",
+			expectPathValues: PathValues{},
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			e := New()
+			// the context is created while no route has path params, so its PathValues have no capacity
+			c := e.NewContext(httptest.NewRequest(http.MethodGet, tc.whenURL, nil), httptest.NewRecorder())
+
+			r := NewRouter(RouterConfig{})
+			_, err := r.Add(Route{Method: http.MethodGet, Path: "/users/:id/files/*", Handler: handlerFunc})
+			assert.NoError(t, err)
+			_, err = r.Add(Route{Method: http.MethodGet, Path: "/a/:b/:c/:d", Handler: handlerFunc})
+			assert.NoError(t, err)
+			_, err = r.Add(Route{Method: http.MethodGet, Path: "/static", Handler: handlerFunc})
+			assert.NoError(t, err)
+
+			r.Route(c)
+
+			assert.Equal(t, tc.expectRoute, c.Path())
+			assert.Equal(t, tc.expectPathValues, c.PathValues())
+			// the context keeps the grown capacity, so later requests do not grow it again
+			assert.Equal(t, 3, cap(c.PathValues()))
+		})
+	}
+}
+
 func TestPathValuesSizeOverMultipleRequests(t *testing.T) {
 	e := New()
 	e.GET("/test/:id/:action", handlerFunc) // max params is 2