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

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