RouteNotFound handler does not falls back to root one

[endorama]

Issue Description

I think I found a bug in how RouteNotFound handler is propagated to route groups.

When using groups with a dedicated middleware there is no fallback to the root RouteNotFound handler, while that happens when no middleware is specified.

Checklist

• Dependencies installed (not sure what this means)
• No typos
• Searched existing issues and docs

Expected behaviour

The root RouteNotFound handler is used.

Actual behaviour

An internal(?) handler is used

Steps to reproduce

Put the code below in a test file, run go test ./...

Working code to debug

This is a test file that reproduce the issue.

package main_test

import (
	"io"
	"net/http"
	"net/http/httptest"
	"testing"

	"github.com/labstack/echo/v4"
	"github.com/stretchr/testify/require"
)

func server() http.Handler {
	e := echo.New()
	e.HideBanner = true
	e.HidePort = true

	// expect this handler is used as fallback unless a more specific is present
	e.RouteNotFound("/*", missingRouteHandler)

	// addig a middleware to the group overrides the RouteNotFound handler
	v0 := e.Group("/v0", func(next echo.HandlerFunc) echo.HandlerFunc {
		return func(c echo.Context) error {
			return next(c)
		}
	})
	// adding this would make it work
	// v0.RouteNotFound("/*", missingRouteHandler)
	v0.POST("/resource", handler)

	// the same does not happen when no middleware is set
	v1 := e.Group("/v1")
	v1.POST("/resource", handler)

	return e
}

func missingRouteHandler(c echo.Context) error {
	return c.NoContent(http.StatusNotFound)
}

func handler(c echo.Context) error {
	return nil
}

func TestMissing(t *testing.T) {
	srv := httptest.NewServer(server())

	routes := []string{
		// this is successful
		"/foo",
		// this fails
        //   Error Trace:    /home/endorama/code/REDACTED/main_test.go:68
        //   Error:          "{"message":"Not Found"}
        //                   " should have 0 item(s), but has 24
        //   Test:           TestMissing//v0/foo
		"/v0/foo",
		// this is successful
		"/v1/foo",
	}

	for _, path := range routes {
		t.Run(path, func(t *testing.T) {
			req, _ := http.NewRequest("POST", srv.URL+path, nil)

			resp, err := srv.Client().Do(req)
			require.NoError(t, err)
			defer resp.Body.Close()

			b, err := io.ReadAll(resp.Body)
			require.NoError(t, err)

			require.Equal(t, http.StatusNotFound, resp.StatusCode)
			require.Len(t, b, 0)
			require.Equal(t, "0", resp.Header.Get("Content-Length"))
		})
	}
}

Version/commit

github.com/labstack/echo/v4 v4.11.1

[aldas]

this is somewhat of a "feature" because of this block

echo/group.go

Lines 21 to 32 in a2e7085

	func (g *Group) Use(middleware ...MiddlewareFunc) {
	g.middleware = append(g.middleware, middleware...)
	if len(g.middleware) == 0 {
	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.
	g.RouteNotFound("", NotFoundHandler)
	g.RouteNotFound("/*", NotFoundHandler)
	}

[endorama]

@aldas thanks for your reply!

From a framework user perspective this behaviour is surprising. The fact that it happens only when a middleware is added to the group make it feel more like an unintended side effect than a desired behaviour.

The reason why this behaviour happens is also a bit obscure, in particular the wording are only executed if they are added to the Router with route. Are middlewares for groups executed only when a matching route is found? Does that include global and group middlewares or only group middlewares?

Would it be possible to consider a change in behaviour to make it less surprising? From the framework user perspective the ideal behaviour would be to use Echo global route not found handlers for paths "" and /* instead of the default ones when they are defined. Could this be achieved with the current framework design?

In case this isn't be possible, would it be ok to add/update the documentation to signal this particular behaviour?

[escb005]

I think I have found a bug, that is either related to this issue or happens because of the same "somewhat feature".

Due to the added NotFoundHandlers when using Middlewares on Groups, echo does not return a method not allowed status when using the wrong method. It always returns a not found status, even if the route exists with a different method.

package main_test

import (
	"net/http"
	"net/http/httptest"
	"testing"

	"github.com/labstack/echo/v4"
	"github.com/stretchr/testify/require"
)

func server() http.Handler {
	e := echo.New()

	// group without middle ware
	g1 := e.Group("/api/v1")
	g1.GET("/hello", func(c echo.Context) error {
		return c.String(200, "Hello, World!")
	})

	// group with middle ware
	g2 := e.Group("/api/v2")
	g2.Use(func(next echo.HandlerFunc) echo.HandlerFunc {
		return func(c echo.Context) error {
			return next(c)
		}
	})
	g2.GET("/hello", func(c echo.Context) error {
		return c.String(200, "Hello, World!")
	})

	return e
}

func TestMethodNotAllowed(t *testing.T) {
	srv := httptest.NewServer(server())

	req, _ := http.NewRequest(http.MethodPost, srv.URL+"/api/v1/hello", nil)
	resp, err := srv.Client().Do(req)
	require.NoError(t, err)
	require.Equal(t, http.StatusMethodNotAllowed, resp.StatusCode, "/api/v1/hello should not allow POST and return 405")

	req, _ = http.NewRequest(http.MethodPost, srv.URL+"/api/v2/hello", nil)
	resp, err = srv.Client().Do(req)
	require.NoError(t, err)
	require.Equal(t, http.StatusMethodNotAllowed, resp.StatusCode, "/api/v2/hello should not allow POST and return 405")
}

[aldas]

@escb005, yep, it does not seems to work properly. I'll take a look into it. It is probably time to investigate how to remove these 2 hidden routes that are added and replace them with something else that helps to execute middlewares.

[Spotnag]

do i really need to do the below? this is the only way i could make it work. Would it not be better to default to all routes using the root level RouteNotFound but you can override if you want as i would think this would be the wanted behavour 99% of the time. Or am i missing another way of doing this?

e.RouteNotFound("/*", shared.MissingRouteHandler)

// Authed routes
userGroup := e.Group("/user", handler.RequireLoginMiddleware)
userGroup.Any("/*", shared.MissingRouteHandler)
userGroup.Any("", shared.MissingRouteHandler)

// Admin routes
adminGroup := userGroup.Group("/admin", handler.RequireAdminMiddleware)
adminGroup.Any("/*", shared.MissingRouteHandler)
adminGroup.Any("", shared.MissingRouteHandler)

[pcfreak30]

Hmm I think this is related to a gotcha I just hit.. i have some middleware helpers for serving a SPA. Then I use .Group() with apis but I use a empty prefix to store middleware 1 time.

func setupDirectoryFallback(echoRouter *echo.Echo, handler http.Handler) error {
	echoRouter.GET("/*", func(c echo.Context) error {
		if !strings.HasPrefix(c.Request().URL.Path, "/api/") {
			c.Request().URL.Path = "/"
			handler.ServeHTTP(c.Response(), c.Request())
		}
		return nil
	})
	return nil
}

func RegisterRoutes(
	gRouter Router,
	accessSvc AccessService,
	subdomain string,
	routes []RouteDefinition,
	commonMiddleware ...echo.MiddlewareFunc,
) error {
	// Create a group with common middleware if any exist
	var group Router = gRouter
	if len(commonMiddleware) > 0 {
		var err error
		group, err = gRouter.Group("")
		if err != nil {
			return fmt.Errorf("failed to create route group: %w", err)
		}
		group.Use(commonMiddleware...)
	}

The 404 routes from the group clobber the spa fallback. It seems it will force me to apply the middleware functions to every individual router in my higher layer API's to bypass this....

[pcfreak30]

@aldas