Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

question: group middleware adds NotFoundHandler to prefix and prefix*, but it works without it anyway #2256

Closed
3 tasks done
yudintsevegor opened this issue Aug 31, 2022 · 3 comments

Comments

@yudintsevegor
Copy link

yudintsevegor commented Aug 31, 2022

Issue Description

Hello team!

I have a question about the implementation detail of the echo. When I try to use an echo.Group with middleware, I found that there were several extra paths with NotFoundHandler handlers that had been registered.

g.Any("", NotFoundHandler)
g.Any("/*", NotFoundHandler)

Why is it implemented like this? I have the assumption that it could be removed. I find really confusing to have these prefix and prefix* when I call Echo.Routes

If we look at the ServeHTTP function, then in the case of an unknown path NotFoundHandler will be executed anyway due to a value in the handler field.

I rectified a little a file group.go locally and unknown paths still return the correct 404.

Checklist

Expected behavior

routes in the logs if we remove

g.Any("", NotFoundHandler)
g.Any("/*", NotFoundHandler)
2022/08/31 16:07:48 path /path/one
2022/08/31 16:07:48 path /path/two
2022/08/31 16:07:48 path /group/three
⇨ http server started on [::]:9999

Actual behavior

routes in the logs:

2022/08/31 16:06:30 path /group
2022/08/31 16:06:30 path /group
2022/08/31 16:06:30 path /group
2022/08/31 16:06:30 path /group
2022/08/31 16:06:30 path /group/*
2022/08/31 16:06:30 path /group
2022/08/31 16:06:30 path /group
2022/08/31 16:06:30 path /group/*
2022/08/31 16:06:30 path /group/*
2022/08/31 16:06:30 path /group/*
2022/08/31 16:06:30 path /group
2022/08/31 16:06:30 path /group
2022/08/31 16:06:30 path /group/*
2022/08/31 16:06:30 path /group/*
2022/08/31 16:06:30 path /group/*
2022/08/31 16:06:30 path /group/*
2022/08/31 16:06:30 path /group/*
2022/08/31 16:06:30 path /group/three
2022/08/31 16:06:30 path /group
2022/08/31 16:06:30 path /path/two
2022/08/31 16:06:30 path /group
2022/08/31 16:06:30 path /group
2022/08/31 16:06:30 path /group/*
2022/08/31 16:06:30 path /group/*
2022/08/31 16:06:30 path /path/one
⇨ http server started on [::]:9999

Steps to reproduce

With help of the command go mod vendor I changed the group.go file https://github.com/labstack/echo/blob/master/group.go#L28

to

// g.Any("", NotFoundHandler)
// g.Any("/*", NotFoundHandler)

then
in the first terminal execute the code below

go run main.go

In the second terminal send the curl requests

# curl localhost:9999/group
{"message":"Not Found"}

curl localhost:9999/group/
{"message":"Not Found"}

curl  localhost:9999/path       
{"message":"Not Found"}

and curl localhost:9999/group/three returns the correct response with 200 OK.

Working code to debug

package main

import (
	"errors"
	"fmt"
	"log"
	"net/http"

	"github.com/labstack/echo/v4"
)

func main() {
	e := echo.New()
	e.HideBanner = true

	mw := func(handlerFunc echo.HandlerFunc) echo.HandlerFunc {
		return handlerFunc
	}

	h := func(c echo.Context) error { return nil }

	e.GET("/path/one", h)
	e.GET("/path/two", h)

	gr := e.Group("/group", mw)
	{
		gr.GET("/three", h)
	}

	for _, r := range e.Routes() {
		log.Println("path", r.Path)
	}

	e.Logger.Fatal(e.Start(":9999"))
}

Version/commit

I tried to play with the version github.com/labstack/echo/v4 v4.8.0

@aldas
Copy link
Contributor

aldas commented Aug 31, 2022

You might want to read #1728 (comment)

TLDR: these are there because group middlewares are executed only when there is a route match and these 2 guarantee a match

@aldas
Copy link
Contributor

aldas commented Aug 31, 2022

NB: these notfound routes have been removed from group in v5

@aldas
Copy link
Contributor

aldas commented May 23, 2023

closing, see #2411 and v5. Current behavior is not going to change in v4.

@aldas aldas closed this as completed May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants