Fix group.RouteNotFound not working when group has attached middlewares

Author: aldas

group.go

@@ -23,10 +23,12 @@ func (g *Group) Use(middleware ...MiddlewareFunc) {
 	if len(g.middleware) == 0 {
 		return
 	}
-	// Allow all requests to reach the group as they might get dropped if router
-	// doesn't find a match, making none of the group middleware process.
-	g.Any("", NotFoundHandler)
-	g.Any("/*", NotFoundHandler)
+	// 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.
+	g.RouteNotFound("", NotFoundHandler)
+	g.RouteNotFound("/*", NotFoundHandler)
 }
 
 // CONNECT implements `Echo#CONNECT()` for sub-routes within the Group.

group_test.go

@@ -184,3 +184,73 @@ func TestGroup_RouteNotFound(t *testing.T) {
 		})
 	}
 }
+
+func TestGroup_RouteNotFoundWithMiddleware(t *testing.T) {
+	var testCases = []struct {
+		name           string
+		givenCustom404 bool
+		whenURL        string
+		expectBody     interface{}
+		expectCode     int
+	}{
+		{
+			name:           "ok, custom 404 handler is called with middleware",
+			givenCustom404: true,
+			whenURL:        "/group/test3",
+			expectBody:     "GET /group/*",
+			expectCode:     http.StatusNotFound,
+		},
+		{
+			name:           "ok, default group 404 handler is called with middleware",
+			givenCustom404: false,
+			whenURL:        "/group/test3",
+			expectBody:     "{\"message\":\"Not Found\"}\n",
+			expectCode:     http.StatusNotFound,
+		},
+		{
+			name:           "ok, (no slash) default group 404 handler is called with middleware",
+			givenCustom404: false,
+			whenURL:        "/group",
+			expectBody:     "{\"message\":\"Not Found\"}\n",
+			expectCode:     http.StatusNotFound,
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(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, c.Request().Method+" "+c.Path())
+			}
+
+			e := New()
+			e.GET("/test1", okHandler)
+			e.RouteNotFound("/*", notFoundHandler)
+
+			g := e.Group("/group")
+			g.GET("/test1", okHandler)
+
+			middlewareCalled := false
+			g.Use(func(next HandlerFunc) HandlerFunc {
+				return func(c Context) error {
+					middlewareCalled = true
+					return next(c)
+				}
+			})
+			if tc.givenCustom404 {
+				g.RouteNotFound("/*", notFoundHandler)
+			}
+
+			req := httptest.NewRequest(http.MethodGet, tc.whenURL, nil)
+			rec := httptest.NewRecorder()
+
+			e.ServeHTTP(rec, req)
+
+			assert.True(t, middlewareCalled)
+			assert.Equal(t, tc.expectCode, rec.Code)
+			assert.Equal(t, tc.expectBody, rec.Body.String())
+		})
+	}
+}