Return error if reverse() does not find a match

[akhayyat]

Issue Description

Currently, reverse() fails silently in case a matching route is not found, returning an empty string. Would it be possible to make it fail a little louder, returning an error? Ideally, a compile-time error would be perfect, but a run-time error is still better than failing silently.

Expected behaviour

Compile-time or run-time error if the provided route name does not exist.

Actual behaviour

Empty string URL.

Steps to reproduce

ctx.Echo().Reverse("undefined-route-name")

Version/commit

v4.11.3

[aldas]

We can not change Reverse signature to return errors. This would be API breaking change. And panic there would be also bad thing.

As every route gets their name when they are added. This is where every route gets its default name: https://github.com/labstack/echo/blob/d549290448fc65ae075f0b960b25489f6a38cb78/echo.go#L584-L585

and if you are not explicitly setting route name as empty

    r := e.GET("/", handler)
    r.Name = "" // you should not do that

the empty string returned from Reverse meaning is that no route was found.

so if you really need to panic you can do it like that.

    u := e.Reverse("xxx")
    if u == "" {
        panic("could not find route")
    }

[akhayyat]

How about a new function to avoid breaking backwards compatibility:

SafeReverse() or similar.

Would a compile-time error be possible?

[aldas]

Do not think at current situation it is that bad that we need to start adding new methods with strange names.

u := e.Reverse("xxx")
if u == "" {
    return errors.New("no route found)
}

is not that different from

u, err := e.ReverseWithErr("xxx")
if err != nil {
    return err
}

[aldas]

ok, after reading this another time I see that I got the problem wrong.

You are pointing that registering route with empty path and reversing that gives same result as reversing non existent path

func TestTest(t *testing.T) {
    e := echo.New()

    handler := func(c echo.Context) error {
        return c.String(http.StatusNotImplemented, "Nope")
    }
    r := e.GET("", handler) // path is registered as ""
    r.Name = "test"

    existingEmpty := e.Reverse("test")
    assert.Equal(t, "", existingEmpty)

    notExistingEmpty := e.Reverse("not-existing")
    assert.Equal(t, "", notExistingEmpty)
}

this reveals something else. We actually never register empty routes as empty. Echo sets them to / actually https://github.com/labstack/echo/blob/d549290448fc65ae075f0b960b25489f6a38cb78/router.go#L201-L209

but routes are stored little bit before that we switch themto /. https://github.com/labstack/echo/blob/d549290448fc65ae075f0b960b25489f6a38cb78/router.go#L188-L198

we probably could add changing from empty string to / to that Router.add thus removing possiblity that there could be valid empty string result from Reverse. Meaning if it returns empty string it means that route did not exist

[akhayyat]

I agree that a run-time error does not improve the situation.

A compile-time error, however, would not require any API changes, and would catch reverse-non-existing-routes errors at compile-time, which is a huge win.

[aldas]

I do not know that you mean by compile time error. English is not my native language so there is probably language barrier here.

Do you mean that if there is e.Reverse call in code and it reverses route that does not exists, the Go compiler should generate compile time error?

[akhayyat]

Do you mean that if there is e.Reverse call in code and it reverses route that does not exists, the Go compiler should generate compile time error?

Yes!

[pscheid92]

Hey @akhayyat 👋, perhaps I am overlooking something, but I'm unaware that Go Compiler supports compile-time assertions. How would something like that be implemented?

I found the following issue on Github, indicating that Go doesn't and won't support compile-time checks shortly.

https://github.com/golang/go/issues/34868

[aldas]

I am closing this issue as I do not think we can do anything more than https://github.com/labstack/echo/pull/2616 did.