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

Wrong route chosen for match any routes on multiple levels #1412

Closed
3 tasks done
lammel opened this issue Oct 3, 2019 · 10 comments
Closed
3 tasks done

Wrong route chosen for match any routes on multiple levels #1412

lammel opened this issue Oct 3, 2019 · 10 comments

Comments

@lammel
Copy link
Contributor

lammel commented Oct 3, 2019

Issue Description

For match any routes configured on multiple levels the wrong route seems to be chosen.

It is expected that if a route is not found, that the nearest (longest matching) match any route is chosen.

For the following defined routes

/api/user
/api/*
/*

it would be expected for any request under /api that can not be resolved to return the match any /api/* route, even those under /api/user/something.
This is currently handled for static only routes that way.

A PR will be opened to resolve this issue (probably alongside with #1406).

Checklist

  • Dependencies installed
  • No typos
  • Searched existing issues and docs

Expected behaviour

The route /api/* will be chosen

Actual behaviour

A not found is returned for /api/user/notexists

Steps to reproduce

Run example below.

Working code to debug

package main

import (
	"net/http"
	"github.com/labstack/echo/v4"
)

func main() {
	// Echo instance
	e := echo.New()

	// Routes
	e.GET("/admin", func(c echo.Context) error { return c.String(http.StatusOK, "admin") })
	e.GET("/api/help", func(c echo.Context) error { return c.String(http.StatusOK, "api help") })
	e.GET("/api/*", func(c echo.Context) error { return c.String(http.StatusOK, "api any") })
	e.GET("/*", func(c echo.Context) error { return c.String(http.StatusOK, "root any") })
	
	// Start server
	e.Logger.Fatal(e.Start(":1323"))
}

Version/commit

Tested with v4.1.10

@lammel
Copy link
Contributor Author

lammel commented Oct 3, 2019

Here are some the benchmarks observed on my system.

Before (current master b129098):

BenchmarkRouterStaticRoutes-4    	  100000	     21359 ns/op	       0 B/op	       0 allocs/op
BenchmarkRouterGitHubAPI-4       	   50000	     38609 ns/op	       1 B/op	       0 allocs/op
BenchmarkRouterParseAPI-4        	  200000	      6683 ns/op	       0 B/op	       0 allocs/op
BenchmarkRouterGooglePlusAPI-4   	  100000	     11278 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/labstack/echo/v4	7.348s
Success: Benchmarks passed.

With this PR:

BenchmarkRouterStaticRoutes-4    	  100000	     17465 ns/op	       0 B/op	       0 allocs/op
BenchmarkRouterGitHubAPI-4       	   50000	     33817 ns/op	       1 B/op	       0 allocs/op
BenchmarkRouterParseAPI-4        	  200000	      6578 ns/op	       0 B/op	       0 allocs/op
BenchmarkRouterGooglePlusAPI-4   	  200000	     10046 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/labstack/echo/v4	7.470s
Success: Benchmarks passed.

lammel added a commit to neotel-at/echo that referenced this issue Oct 16, 2019
lammel added a commit to neotel-at/echo that referenced this issue Oct 16, 2019
lammel added a commit to neotel-at/echo that referenced this issue Oct 16, 2019
lammel added a commit to neotel-at/echo that referenced this issue Oct 16, 2019
@stale
Copy link

stale bot commented Dec 2, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Dec 2, 2019
@lammel
Copy link
Contributor Author

lammel commented Dec 3, 2019

There seem to be no interest so far in fixing the router for the wrong any matches...
This is probably considered a corner case, so I guess we have to keep using our fork for now.

Feedback would be welcome though.

@stale stale bot removed the wontfix label Dec 3, 2019
@ghost
Copy link

ghost commented Dec 11, 2019

Looks like this project has been abandoned

@lammel
Copy link
Contributor Author

lammel commented Dec 26, 2019

The PR #1413 for this issue went stale and has been auto-closed by the bot without any comments or feedback.
I really hope this project is not abandoned as it is a really nice web framework and we'd love to switch back from our fork with the router fixes to the main project.

Comments for this issue or PR #1413 would be greatly appreciated.

@asahasrabuddhe
Copy link
Contributor

@vishr Please merge this. We need this ASAP

@vishr vishr closed this as completed Dec 30, 2019
@vishr
Copy link
Member

vishr commented Dec 30, 2019

Released in https://github.com/labstack/echo/releases/tag/v4.1.12

@asahasrabuddhe
Copy link
Contributor

@vishr Life Saver! Thanks :)

Consider getting more people to help you out. I for one would love to be a maintainer :)

@vishr
Copy link
Member

vishr commented Dec 30, 2019

@asahasrabuddhe I am planning to form a core team to maintain the project, will keep you in mind.

@lammel
Copy link
Contributor Author

lammel commented Jan 2, 2020

@vishr Thanks for merging!

We are using echo in most of our internal web service projects, so we might step up to as contributer/maintainer if needed (with focus on router and proxy middleware for now).

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

3 participants