Echo: 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

• [X] Dependencies installed
• [X] No typos
• [X] 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:
https://github.com/labstack/echo/compare/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?

[lammel]

The assumption in your example is wrong which caused an error for /path.
A request to /path must not match /path/* because of the missing slash (they are different routes, so we have no valid route for that case actually).

But your test still pointed out an error in the router, so thanks for that.

So please retry with the latest commit in the PR. Should be all fixed now.

[zzwx]

Now it works!

Yes, my intention was to show erroneous route resolution, but obviously the test had to be written in a way that would show what to expect rather than to show the error!

Thank you for the fix!

I'll include my own fixed test here just in case someone reads the issue and questions it:

// 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, nil, c.Get("path"))
    assert.Equal(t, "", c.Param("*"))
}

Again thank you a lot! It now makes sense, and not anymore requires repetitive path definitions!