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)