Implicitly registered group routes should be allowed overwritten in default routes. fix issue #3047

Author: aldas

echo.go

@@ -177,11 +177,16 @@ const (
 	// QUERY Method is a safe, idempotent request method carrying request content (a query) in its body, see rfc 10008.
 	// It is not (yet) part of the `net/http` standard library, so Echo defines it here.
 	QUERY = "QUERY"
-	// RouteNotFound is special method type for routes handling "route not found" (404) cases
+	// RouteNotFound is a special method type for routes handling "route not found" (404) cases
 	RouteNotFound = "echo_route_not_found"
-	// RouteAny is special method type that matches any HTTP method in request. Any has lower
+	// RouteAny is a special method type that matches any HTTP method in request. Any has lower
 	// priority that other methods that have been registered with Router to that path.
 	RouteAny = "echo_route_any"
+
+	// GroupImplicitRouteName is a special route name for implicitly registered GROUP routes to execute group-level middleware
+	// when no route matches. This route is functionally the same as the RouteNotFound method, and a name exists to
+	// distinguish implicit group routes.
+	GroupImplicitRouteName = "echo_implicitly_registered_group_route_name"
 )
 
 // Headers

group.go

@@ -41,8 +41,12 @@ func (g *Group) Use(middleware ...MiddlewareFunc) {
 	// So we register catch all route (404 is a safe way to emulate route match) for this group and now during routing the
 	// Router would find route to match our request path and therefore guarantee the middleware(s) will get executed.
 	// Note: we use nil handler so Router would choose the default 404 handler. This may not work with custom routers.
-	g.RouteNotFound("", nil)
-	g.RouteNotFound("/*", nil)
+	if _, err := g.AddRoute(Route{Method: RouteNotFound, Name: GroupImplicitRouteName, Path: ""}); err != nil {
+		panic(err) // this is how `v4` handles errors. `v5` has methods to have panic-free usage
+	}
+	if _, err := g.AddRoute(Route{Method: RouteNotFound, Name: GroupImplicitRouteName, Path: "/*"}); err != nil {
+		panic(err) // this is how `v4` handles errors. `v5` has methods to have panic-free usage
+	}
 }
 
 // CONNECT implements `Echo#CONNECT()` for sub-routes within the Group. Panics on error.

group_test.go

@@ -866,3 +866,74 @@ func TestGroup_RouteNotFoundWithMiddleware(t *testing.T) {
 		})
 	}
 }
+
+func TestGroup_UseMultipleTimes(t *testing.T) {
+	t.Run("Group created without middleware can call Use multiple times", func(t *testing.T) {
+		e := NewWithConfig(Config{
+			Router: NewRouter(RouterConfig{AllowOverwritingRoute: false}),
+		})
+
+		g1 := e.Group("/api")
+		mw1Called := false
+		g1.Use(func(next HandlerFunc) HandlerFunc {
+			mw1Called = true
+			return func(c *Context) error { return next(c) }
+		})
+
+		mw2Called := false
+		g1.Use(func(next HandlerFunc) HandlerFunc {
+			mw2Called = true
+			return func(c *Context) error { return next(c) }
+		})
+
+		g1.GET("/test", func(c *Context) error {
+			return c.String(http.StatusTeapot, "OK")
+		})
+
+		req := httptest.NewRequest(http.MethodGet, "/api/test", nil)
+		rec := httptest.NewRecorder()
+		e.ServeHTTP(rec, req)
+
+		assert.True(t, mw1Called)
+		assert.True(t, mw2Called)
+		assert.Equal(t, http.StatusTeapot, rec.Code)
+	})
+
+	t.Run("Group created with middleware can call Use multiple times", func(t *testing.T) {
+		e := NewWithConfig(Config{
+			Router: NewRouter(RouterConfig{AllowOverwritingRoute: false}),
+		})
+
+		mw0Called := true
+		g1 := e.Group("/api", func(next HandlerFunc) HandlerFunc {
+			mw0Called = true
+			return func(c *Context) error { return next(c) }
+		})
+
+		mw1Called := false
+		g1.Use(func(next HandlerFunc) HandlerFunc {
+			mw1Called = true
+			return func(c *Context) error { return next(c) }
+		})
+
+		mw2Called := false
+		g1.Use(func(next HandlerFunc) HandlerFunc {
+			mw2Called = true
+			return func(c *Context) error { return next(c) }
+		})
+
+		g1.GET("/test", func(c *Context) error {
+			return c.String(http.StatusTeapot, "OK")
+		})
+
+		req := httptest.NewRequest(http.MethodGet, "/api/test", nil)
+		rec := httptest.NewRecorder()
+		e.ServeHTTP(rec, req)
+
+		assert.True(t, mw0Called)
+		assert.True(t, mw1Called)
+		assert.True(t, mw2Called)
+		assert.Equal(t, http.StatusTeapot, rec.Code)
+	})
+
+}

router.go

@@ -506,9 +506,14 @@ func newAddRouteError(route Route, err error) *AddRouteError {
 
 // Add registers a new route for method and path with matching handler.
 func (r *DefaultRouter) Add(route Route) (RouteInfo, error) {
+	allowOverwritingRoute := r.allowOverwritingRoute
+
 	if route.Handler == nil {
 		switch route.Method {
 		case RouteNotFound:
+			if route.Name == GroupImplicitRouteName {
+				allowOverwritingRoute = true
+			}
 			route.Handler = r.notFoundHandler
 		case http.MethodOptions:
 			route.Handler = r.optionsMethodHandler
@@ -521,7 +526,7 @@ func (r *DefaultRouter) Add(route Route) (RouteInfo, error) {
 	path := normalizePathSlash(route.Path)
 
 	h := applyMiddleware(route.Handler, route.Middlewares...)
-	if !r.allowOverwritingRoute {
+	if !allowOverwritingRoute {
 		for _, rr := range r.routes {
 			if route.Method == rr.Method && route.Path == rr.Path {
 				return RouteInfo{}, newAddRouteError(route, errors.New("adding duplicate route (same method+path) is not allowed"))