From 44c655dcfc2feebb91801ec2144c72a1f7c1fbe3 Mon Sep 17 00:00:00 2001 From: toim Date: Mon, 8 Jun 2026 22:49:21 +0300 Subject: [PATCH] Revert back to v4 behavior for group registering implicit 404 handlers. This will fix: CORS middleware doesnt automatically handle OPTIONS routes for groups anymore since upgrade to v5 --- echo.go | 22 +++++++++++++-- echo_test.go | 4 +-- group.go | 31 ++++++++++++++++++--- group_test.go | 74 ++++++++++++++++++++++++++++++++++++++------------- route.go | 11 +++++--- router.go | 36 ++++++++++++++++++++----- 6 files changed, 141 insertions(+), 37 deletions(-) diff --git a/echo.go b/echo.go index 0ad07615a..297706ffc 100644 --- a/echo.go +++ b/echo.go @@ -110,6 +110,10 @@ type Echo struct { // formParseMaxMemory is passed to Context for multipart form parsing (See http.Request.ParseMultipartForm) formParseMaxMemory int64 + // noGroupAutoRegisterRoutes is a flag that indicates whether echo.Group should NOT register 404 routes automatically + // when there are middlewares registered with the group. + noGroupAutoRegisterRoutes bool + enablePathUnescapingStaticFiles bool } @@ -327,6 +331,12 @@ type Config struct { // // Applies to methods: Echo.Static, Echo.StaticFS, Group.Static, Group.StaticFS. EnablePathUnescapingStaticFiles bool + + // NoGroupAutoRegister404Routes bool is a flag that indicates whether echo.Group should NOT register 404 routes automatically + // when there are middlewares registered with the group. + // Note: if you decide not to register 404 routes automatically, make sure to check if all your middlewares are executed + // as expected. For example - CORS middleware. + NoGroupAutoRegister404Routes bool } // NewWithConfig creates an instance of Echo with given configuration. @@ -367,6 +377,8 @@ func NewWithConfig(config Config) *Echo { } e.enablePathUnescapingStaticFiles = config.EnablePathUnescapingStaticFiles + e.noGroupAutoRegisterRoutes = config.NoGroupAutoRegister404Routes + return e } @@ -383,7 +395,9 @@ func New() *Echo { } e.serveHTTPFunc = e.serveHTTP - e.router = NewRouter(RouterConfig{}) + e.router = NewRouter(RouterConfig{ + AllowOverwritingRoute: true, + }) e.HTTPErrorHandler = DefaultHTTPErrorHandler(false) e.contextPool.New = func() any { return newContext(nil, nil, e) @@ -737,7 +751,11 @@ func (e *Echo) Add(method, path string, handler HandlerFunc, middleware ...Middl // Group creates a new router group with prefix and optional group-level middleware. func (e *Echo) Group(prefix string, m ...MiddlewareFunc) (g *Group) { - g = &Group{prefix: prefix, echo: e} + g = &Group{ + prefix: prefix, + echo: e, + noAutoRegisterRoutes: e.noGroupAutoRegisterRoutes, + } g.Use(m...) return } diff --git a/echo_test.go b/echo_test.go index 3f45e9552..46a175eaf 100644 --- a/echo_test.go +++ b/echo_test.go @@ -1014,12 +1014,12 @@ func TestEchoServeHTTPPathEncoding(t *testing.T) { func TestEchoGroup(t *testing.T) { e := New() buf := new(bytes.Buffer) - e.Use(MiddlewareFunc(func(next HandlerFunc) HandlerFunc { + e.Use(func(next HandlerFunc) HandlerFunc { return func(c *Context) error { buf.WriteString("0") return next(c) } - })) + }) h := func(c *Context) error { return c.NoContent(http.StatusOK) } diff --git a/group.go b/group.go index 1ab39ccfe..76382627d 100644 --- a/group.go +++ b/group.go @@ -15,12 +15,34 @@ type Group struct { echo *Echo prefix string middleware []MiddlewareFunc + + // noAutoRegisterRoutes is a flag that indicates whether Group should NOT register 404 routes automatically + // when there are middlewares registered with the group. + // Note: if you decide not to register 404 routes automatically, make sure to check if all your middlewares are executed + // as expected. For example - CORS middleware. + noAutoRegisterRoutes bool } // Use implements `Echo#Use()` for sub-routes within the Group. -// Group middlewares are not executed on request when there is no matching route found. +// +// Important! Group middlewares are executed in case there was no exact route match as by default Group registers +// `/*` NotFound routes for itself. If this kind of behavior is not needed, then create an Echo instance with the ` noAutoRegisterRoutes ` +// flag set to true. Example `echo.NewWithConfig(echo.Config{NoGroupAutoRegister404Routes: true})`. func (g *Group) Use(middleware ...MiddlewareFunc) { g.middleware = append(g.middleware, middleware...) + if len(g.middleware) == 0 { + return + } + if g.noAutoRegisterRoutes { + return + } + // group level middlewares are different from Echo `Pre` and `Use` middlewares (those are global). Group level middlewares + // are only executed if they are added to the Router with route. + // 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) } // CONNECT implements `Echo#CONNECT()` for sub-routes within the Group. Panics on error. @@ -102,9 +124,10 @@ func (g *Group) Match(methods []string, path string, handler HandlerFunc, middle } // Group creates a new sub-group with prefix and optional sub-group-level middleware. -// Important! Group middlewares are only executed in case there was exact route match and not -// for 404 (not found) or 405 (method not allowed) cases. If this kind of behaviour is needed then add -// a catch-all route `/*` for the group which handler returns always 404 +// +// Important! Group middlewares are executed in case there was no exact route match as by default Group registers +// `/*` NotFound routes for itself. If this kind of behavior is not needed, then create an Echo instance with the ` noAutoRegisterRoutes ` +// flag set to true. Example `echo.NewWithConfig(echo.Config{NoGroupAutoRegister404Routes: true})`. func (g *Group) Group(prefix string, middleware ...MiddlewareFunc) (sg *Group) { m := make([]MiddlewareFunc, 0, len(g.middleware)+len(middleware)) m = append(m, g.middleware...) diff --git a/group_test.go b/group_test.go index 1ce1f76c5..14535ea54 100644 --- a/group_test.go +++ b/group_test.go @@ -14,7 +14,7 @@ import ( "github.com/stretchr/testify/assert" ) -func TestGroup_withoutRouteWillNotExecuteMiddleware(t *testing.T) { +func TestGroup_withoutRouteWillExecuteMiddleware(t *testing.T) { e := New() called := false @@ -24,7 +24,29 @@ func TestGroup_withoutRouteWillNotExecuteMiddleware(t *testing.T) { return c.NoContent(http.StatusTeapot) } } - // even though group has middleware it will not be executed when there are no routes under that group + // even though group has middleware it will be executed when there are no routes under that group + // because implicit routes ("" and "/*") are created for the group + _ = e.Group("/group", mw) + + status, body := request(http.MethodGet, "/group/nope", e) + assert.Equal(t, http.StatusTeapot, status) + assert.Equal(t, "", body) + + assert.True(t, called) +} + +func TestGroup_withoutRouteWillNotExecuteMiddleware(t *testing.T) { + e := NewWithConfig(Config{NoGroupAutoRegister404Routes: true}) + + called := false + mw := func(next HandlerFunc) HandlerFunc { + return func(c *Context) error { + called = true + return c.NoContent(http.StatusTeapot) + } + } + // even though group has middleware it will be executed when there are no routes under that group + // because implicit routes ("" and "/*") are created for the group _ = e.Group("/group", mw) status, body := request(http.MethodGet, "/group/nope", e) @@ -34,7 +56,7 @@ func TestGroup_withoutRouteWillNotExecuteMiddleware(t *testing.T) { assert.False(t, called) } -func TestGroup_withRoutesWillNotExecuteMiddlewareFor404(t *testing.T) { +func TestGroup_withRoutesWillExecuteMiddlewareFor404(t *testing.T) { e := New() called := false @@ -45,15 +67,17 @@ func TestGroup_withRoutesWillNotExecuteMiddlewareFor404(t *testing.T) { } } // even though group has middleware and routes when we have no match on some route the middlewares for that - // group will not be executed + // group will be executed g := e.Group("/group", mw) g.GET("/yes", handlerFunc) + // route was `/group/yes` but we are requesting `/group/nope` which will result 404 by Router, but middleware will be + // not reach the handler and return 418 status, body := request(http.MethodGet, "/group/nope", e) - assert.Equal(t, http.StatusNotFound, status) - assert.Equal(t, `{"message":"Not Found"}`+"\n", body) + assert.Equal(t, http.StatusTeapot, status) + assert.Equal(t, "", body) - assert.False(t, called) + assert.True(t, called) } func TestGroup_multiLevelGroup(t *testing.T) { @@ -425,7 +449,9 @@ func TestGroup_Match(t *testing.T) { } func TestGroup_MatchWithErrors(t *testing.T) { - e := New() + e := NewWithConfig(Config{ + Router: NewRouter(RouterConfig{AllowOverwritingRoute: false}), // to trigger "duplicate route" error + }) users := e.Group("/users") users.GET("/activate", func(c *Context) error { @@ -770,25 +796,25 @@ func TestGroup_RouteNotFoundWithMiddleware(t *testing.T) { name: "ok, custom 404 handler is called with middleware", givenCustom404: true, whenURL: "/group/test3", - expectBody: "404 GET /group/*", + expectBody: "404 (local) GET /group/*", expectCode: http.StatusNotFound, expectMiddlewareCalled: true, // because RouteNotFound is added after middleware is added }, { - name: "ok, default group 404 handler is not called with middleware", + name: "ok, default group 404 handler is called with middleware", givenCustom404: false, whenURL: "/group/test3", - expectBody: "404 GET /*", + expectBody: "404 (global) GET /group/*", expectCode: http.StatusNotFound, - expectMiddlewareCalled: false, // because RouteNotFound is added before middleware is added + expectMiddlewareCalled: true, // because RouteNotFound is added before middleware is added }, { name: "ok, (no slash) default group 404 handler is called with middleware", givenCustom404: false, whenURL: "/group", - expectBody: "404 GET /*", + expectBody: "404 (global) GET /group", expectCode: http.StatusNotFound, - expectMiddlewareCalled: false, // because RouteNotFound is added before middleware is added + expectMiddlewareCalled: true, // because RouteNotFound is added before middleware is added }, } for _, tc := range testCases { @@ -797,13 +823,23 @@ func TestGroup_RouteNotFoundWithMiddleware(t *testing.T) { okHandler := func(c *Context) error { return c.String(http.StatusOK, c.Request().Method+" "+c.Path()) } - notFoundHandler := func(c *Context) error { - return c.String(http.StatusNotFound, "404 "+c.Request().Method+" "+c.Path()) + old404 := notFoundHandler + defer func() { notFoundHandler = old404 }() + + localNotFoundHandler := func(c *Context) error { + return c.String(http.StatusNotFound, "404 (local) "+c.Request().Method+" "+c.Path()) } - e := New() + e := NewWithConfig(Config{ + Router: NewRouter(RouterConfig{ + AllowOverwritingRoute: true, + NotFoundHandler: func(c *Context) error { + return c.String(http.StatusNotFound, "404 (global) "+c.Request().Method+" "+c.Path()) + }, + }), + }) e.GET("/test1", okHandler) - e.RouteNotFound("/*", notFoundHandler) + e.RouteNotFound("/*", localNotFoundHandler) g := e.Group("/group") g.GET("/test1", okHandler) @@ -816,7 +852,7 @@ func TestGroup_RouteNotFoundWithMiddleware(t *testing.T) { } }) if tc.givenCustom404 { - g.RouteNotFound("/*", notFoundHandler) + g.RouteNotFound("/*", localNotFoundHandler) } req := httptest.NewRequest(http.MethodGet, tc.whenURL, nil) diff --git a/route.go b/route.go index 970520435..2f484e6da 100644 --- a/route.go +++ b/route.go @@ -12,11 +12,14 @@ import ( ) // Route contains information to adding/registering new route with the router. -// Method+Path pair uniquely identifies the Route. It is mandatory to provide Method+Path+Handler fields. +// Method+Path pair uniquely identifies the Route. It is mandatory to provide Method+Path fields. type Route struct { - Method string - Path string - Name string + Method string + Path string + Name string + + // HandlerFunc is a function that handles HTTP requests. This could be left nil when the Router implementation allows + // fallback to default/global handlers in certain situations. Handler HandlerFunc Middlewares []MiddlewareFunc } diff --git a/router.go b/router.go index 36ea71795..e315b75a9 100644 --- a/router.go +++ b/router.go @@ -74,11 +74,27 @@ type DefaultRouter struct { // RouterConfig is configuration options for (default) router type RouterConfig struct { - NotFoundHandler HandlerFunc - MethodNotAllowedHandler HandlerFunc - OptionsMethodHandler HandlerFunc - AllowOverwritingRoute bool - UnescapePathParamValues bool + // NotFoundHandler is a handler that is executed when no route matches the request. + NotFoundHandler HandlerFunc + + // MethodNotAllowedHandler is a handler that is executed when no route with exact METHOD matches the request but + // there is a route with same path but different method. + MethodNotAllowedHandler HandlerFunc + + // OptionsMethodHandler is a handler that is executed when an OPTIONS request is made. + OptionsMethodHandler HandlerFunc + + // AllowOverwritingRoute allows overwriting existing routes. If false, then adding a route with the same method + // and path will return an error. + AllowOverwritingRoute bool + + // UnescapePathParamValues forces router to unescape path parameter values before setting them in context. + UnescapePathParamValues bool + + // UseEscapedPathForMatching forces router to use an escaped path (req.URL.RawPath instead of req.URL.Path) for matching. + // Difference between URL.RawPath and URL.Path is: + // * URL.Path is where request path is stored. Value is stored in decoded form: /%47%6f%2f becomes /Go/. + // * URL.RawPath is an optional field which only gets set if the default encoding is different from Path. UseEscapedPathForMatching bool // AutoHandleHEAD enables automatic handling of HTTP HEAD requests by @@ -491,8 +507,16 @@ 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) { if route.Handler == nil { - return RouteInfo{}, newAddRouteError(route, errors.New("adding route without handler function")) + switch route.Method { + case RouteNotFound: + route.Handler = r.notFoundHandler + case http.MethodOptions: + route.Handler = r.optionsMethodHandler + default: + return RouteInfo{}, newAddRouteError(route, errors.New("adding route without handler function")) + } } + method := route.Method path := normalizePathSlash(route.Path)