Routing Bug or Feature?

[zzwx]

Issue Description

Before v4.1.15 these routes used to satisfy both "/articles", "/articles/" and "/articles/whatever" URLs.

		"/articles",
		"/articles/*",

Checklist

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

Expected behaviour

		"/articles",
		"/articles/*",

satisfy both "/articles", "/articles/" and "/articles/whatever" URLs.

Actual behaviour

Since v4.1.15 now the list of routes to satisfy required behavior is this:

		"/articles",
		"/articles/",
		"/articles/*",

Version/commit

v4.1.15

Please confirm whether this is a new behavior or bug or there's a better way of handling this scenario.

[Shareed2k]

+1

[lammel]

This is rather interessting. I don't think this was intended.
I wonder why no tests want red, so it might be a test for that is missing

[lammel]

I'm going to look into that and prepare a PR with added tests.

@vishr Thumbsup to keep the old behaviour?

[lammel]

The issue is not as simple as that and could not be reproduced in the tests that way.

The described scenario works as expected:

    r.Add(http.MethodGet, "/users", handler)
    r.Add(http.MethodGet, "/users/*", handler)

• GET /users => OK (route /users)
• GET /users/ => OK (route /users/*)
• GET /users/joe => OK (route /users/*)

Whereas nested any routes do not work as expected:

    r.Add(http.MethodGet, "/img/*", handler)
    r.Add(http.MethodGet, "/img/upload", handler)
    r.Add(http.MethodGet, "/img/upload/*", handler)

• GET /img/dummy => OK (route /img/*)
• GET /img/upload => OK (route /img/upload)
• GET /img/upload/ => FAIL (route /img/* instead of /img/upload/*)
• GET /img/upload/dummy => OK (route /img/upload/*)

So it seems that only nested any routes are causing the issue, because the outer route is chosen first, not the longest match.

PR is being prepared, but the router needs to be fixed first.

@zzwx Can you confirm this or provide a working code snipppet?

[zzwx]

I have a feeling it's showing only when more and more paths are introduced. So if you just begin with something like

e.GET("/video", h)
e.GET("/video/*", h)

it satisfies all /video, /video/ and video/anything URLs.

But as soon as you add more like:

e.GET("/articles", hh)
e.GET("/articles/", hh)
e.GET("/articles/*", hh)

... immediately previous empty /video/ is not working anymore.

I'm actually not sure about how they are supposed to be handled, I would assume that e.GET("/video/*", h) should satisfy both empty /video/ and non-empty /video/anything cases, but definitely not the /video. Or it is actually awesome to have a full control over all three scenarios and therefore be very specific in the way the path looks.

But in any case, the way it work should definitely be consistent.

Preparing a test in a meanwhile....

[lammel]

PR #1563 created with a fix for the reproducable issue with non-root any routes and requests toward that routes with a traling slash.

I strongöy believe that should resolve this issue.

[lammel]

@zzwx Can you try to check if the PR #1563 fixes your issue?

[zzwx]

On it, give me a few hours please

[zzwx]

OK, I pulled the solution, with the tests provided along with the solution, it seems to work.

However, my originally described test is failing.

Here's what I'm adding to router_test.go:

// Issue #1526
func TestRouterMatchAnyAgain(t *testing.T) {
	e := New()
	r := e.router
	handler := func(c Context) error {
		c.Set("path", c.Path())
		return nil
	}

	// A minimum of routes
	r.Add(http.MethodGet, "/video", handler)
	r.Add(http.MethodGet, "/video/*", handler)


	c := e.NewContext(nil, nil).(*context)

	// "/video/" > "/video/*"
	testVideoSlash := func() {
		r.Find(http.MethodGet, "/video/", c)
		c.handler(c)
		assert.Equal(t, "/video/*", c.Get("path"))
		assert.Equal(t, "", c.Param("*"))
	}
	testVideoSlash()

	// Adding more routes (comment out any, it doesn't matter)
	r.Add(http.MethodGet, "/art", handler)
	r.Add(http.MethodGet, "/art/", handler)
	r.Add(http.MethodGet, "/art/*", handler)

	c = e.NewContext(nil, nil).(*context)

	// Same exact testVideoSlash() not working anymore
	testVideoSlash()
}

See the last testVideoSlash() (and I'm insisting on creating a new context just in case to avoid leaking from the previous test, although in this case it doesn't matter), the /video/ is not satisfied anymore, it seems not finding any provided solutions so the c.Get("path") actually returns <nil>(<nil>).

If helpful:
master...zzwx:master

[lammel]

@zzwx I added the test to the pull request branch, but can not reproduce it there using your provided test. The tests I added for fixing the issue look quite similar to the one you provided.

Did you use the PR branch for your tests?

[zzwx]

So weird! I guess my HEAD was one commit behind, where the test has already existed, but not the fix itself yet.

So my apologies, it seems fixing my use case and apparently not break the rest of the tests!

However....

The fix doesn't address at least one case, which is, in case there's only "/path/*" registered and a "/path" is requested, it actually returns a weird result. I'll demonstrate it in a minute with the test.

[zzwx]

// Issue #1526
func TestRouterMatchAnyAgain(t *testing.T) {
	e := New()
	r := e.router
	handler := func(c Context) error {
		c.Set("path", c.Path())
		return nil
	}

	// A minimum of routes
	r.Add(http.MethodGet, "/video", handler)
	r.Add(http.MethodGet, "/video/*", handler)

	c := e.NewContext(nil, nil).(*context)

	// "/video/" > "/video/*"
	testVideoSlash := func() {
		r.Find(http.MethodGet, "/video/", c)
		c.handler(c)
		assert.Equal(t, "/video/*", c.Get("path"))
		assert.Equal(t, "", c.Param("*"))
	}
	testVideoSlash()

	// Adding more routes
	r.Add(http.MethodGet, "/art", handler)
	r.Add(http.MethodGet, "/art/", handler)
	r.Add(http.MethodGet, "/art/*", handler)

	c = e.NewContext(nil, nil).(*context)

	// Same exact testVideoSlash()
	testVideoSlash()

	c = e.NewContext(nil, nil).(*context)
	r.Find(http.MethodGet, "/art/", c)
	c.handler(c)
	assert.Equal(t, "/art/", c.Get("path"))
	assert.Equal(t, "", c.Param("*"))

	c = e.NewContext(nil, nil).(*context)
	r.Find(http.MethodGet, "/art", c)
	c.handler(c)
	assert.Equal(t, "/art", c.Get("path"))
	assert.Equal(t, "", c.Param("*"))

	c = e.NewContext(nil, nil).(*context)
	r.Find(http.MethodGet, "/art/available", c)
	c.handler(c)
	assert.Equal(t, "/art/*", c.Get("path"))
	assert.Equal(t, "available", c.Param("*"))

	// Adding one more rout for just /path/*
	r.Add(http.MethodGet, "/path/*", handler)

	c = e.NewContext(nil, nil).(*context)
	r.Find(http.MethodGet, "/path", c)
	c.handler(c)
	assert.Equal(t, "/path/*", c.Get("path"))
	assert.Equal(t, "", c.Param("*"))
}

Please try this and you'll notice that the very last test erroneously matches the "/path/* and returns "path" for c.Param("*") which is questionable.
I think in this particular case, when just the /path/* is provided as a handler, /path shouldn't resolve at all. Or if it resolves to /path/*, then at least return "" for c.Param("*").

What do you think?