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

regexp route nested wrongly -> no routing #411

Closed
mvrhov opened this issue Mar 13, 2019 · 7 comments · Fixed by #506
Closed

regexp route nested wrongly -> no routing #411

mvrhov opened this issue Mar 13, 2019 · 7 comments · Fixed by #506

Comments

@mvrhov
Copy link

mvrhov commented Mar 13, 2019

I've been scratching my head for the past day where the chi wouldn't route the request properly. Here are the registered routes.

r.Route("/accounts/{id:(me|[a-zA-Z0-9]{10,})})", func(r chi.Router) {
//works
})
r.Route("/accounts/{id:(me|[a-zA-Z0-9]{10,})})/new_link", func(r chi.Router) {
//works
})
r.Route("/accounts/{id:(me|[0-9]+)}/Backward_compat_link, func(r chi.Router) {
//no request won't be routed here debugger shows that this route has been added under /accounts/{id:(me|[a-zA-Z0-9]{10,})})
})
@FelipeRenault
Copy link

Apparently, if you make a different regex, but it matches the other, it returns a 404. I had this example:

r.Get("/one/{firstId:[a-z0-9-]+}/{secondId:[a-z0-9-]+}/type.json", handler)
r.Get("/one/{firstId:[a-z0-9-_]+}/{secondId:[a-z0-9-_]+}.json", secondHandler}

I could access the first path, but not the second.

@rodrigo-brito
Copy link

Same problem here! 😞

@AlisonOSouza
Copy link

AlisonOSouza commented Nov 18, 2019

The same happen in here.

In this example, get routes works but delete returns 405 (not allowed):

r.Route("/one", func(r chi.Router) {
	r.Get("/{dns:[a-z-0-9_]+}", getHandler)
	r.Get("/{dns:[a-z-0-9_]+}/info", infoHandle)
	...
	r.Delete("/{id:[0-9]+}", deleteHandler)
})

if I remove the first get route, then delete returns 404 (route not found), like Felipe said:

r.Route("/one", func(r chi.Router) {
//	r.Get("/{dns:[a-z-0-9_]+}", getHandler)
	r.Get("/{dns:[a-z-0-9_]+}/info", infoHandle)
	...
	r.Delete("/{id:[0-9]+}", deleteHandler)
})

And in this, all routes works:

r.Route("/one", func(r chi.Router) {
	r.Get("/{dns:[a-z-0-9_]+}", getHandler)
	r.Get("/{dns:[a-z-0-9_]+}/info", infoHandle)
	...
})
r.Delete("/one/{id:[0-9]+}", deleteHandler)

🤷‍♂️

@mohamedfar97
Copy link

mohamedfar97 commented Nov 19, 2019

I think that’s because the Route struct is composed of one pattern string and a map of handlers.
Better option is to create a new router and mount it over the desired path
Note: try use the reset context method in the documentation to reset the route context after each request

@mohamedfar97
Copy link

Try putting /*/ before each request pattern

@pkieltyka
Copy link
Member

hey all, I've found the bug for this issue. The issue is in the findRoute method where for a regexp search, it fails to recursively try all paths until it finds the terminal/final handler.

func TestMuxRegexp4(t *testing.T) {
	r := NewRouter()
	r.Get("/one/{firstId:[a-z0-9-]+}/{secondId:[a-z]+}/first", func(w http.ResponseWriter, r *http.Request) {
		w.Write([]byte("first"))
	})
	r.Get("/one/{firstId:[a-z0-9-_]+}/{secondId:[0-9]+}/second", func(w http.ResponseWriter, r *http.Request) {
		w.Write([]byte("second"))
	})

	ts := httptest.NewServer(r)
	defer ts.Close()

	if _, body := testRequest(t, ts, "GET", "/one/hello/peter/first", nil); body != "first" {
		t.Fatalf(body)
	}
	if _, body := testRequest(t, ts, "GET", "/one/hithere/123/second", nil); body != "second" {
		t.Fatalf(body)
	}
}

here is a test case that should pass but in fact is not. The reason for it is that the GET /one/hithere/123/second request will match the first routing pattern as "hithere" segment matches the {firstId:[a-z0-9-]+} param, then as it tests the second param {secondId:[a-z]+}, this will of course not match the 123 value in the request, as a result returning a 404. What this should be doing though is a recursive search to the second route, attempting each permutation. This is how the rest of the chi tree walking algorithm works, but I've noticed here https://github.com/go-chi/chi/blob/master/tree.go#L414-L454 needs to be revised to be a recursive.

just FYI in case someone wants to take a stab at it

@pkieltyka pkieltyka changed the title route nested wrongly -> no routing regexp route nested wrongly -> no routing Jan 10, 2020
Jahaja added a commit to Jahaja/chi that referenced this issue Apr 15, 2020
@pkieltyka
Copy link
Member

thanks to @Jahaja 's PR, this is now solved in master

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

Successfully merging a pull request may close this issue.

6 participants