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

Return error if reverse() does not find a match #2615

Closed
akhayyat opened this issue Mar 23, 2024 · 9 comments
Closed

Return error if reverse() does not find a match #2615

akhayyat opened this issue Mar 23, 2024 · 9 comments

Comments

@akhayyat
Copy link

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

This comment was marked as outdated.

@akhayyat
Copy link
Author

akhayyat commented Mar 23, 2024

How about a new function to avoid breaking backwards compatibility:

SafeReverse() or similar.

Would a compile-time error be possible?

@aldas

This comment was marked as outdated.

@aldas
Copy link
Contributor

aldas commented Mar 23, 2024

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

echo/router.go

Lines 201 to 209 in d549290

func (r *Router) Add(method, path string, h HandlerFunc) {
// Validate path
if path == "" {
path = "/"
}
if path[0] != '/' {
path = "/" + path
}
pnames := []string{} // Param names

but routes are stored little bit before that we switch themto /.

echo/router.go

Lines 188 to 198 in d549290

func (r *Router) add(method, path, name string, h HandlerFunc) *Route {
r.Add(method, path, h)
route := &Route{
Method: method,
Path: path,
Name: name,
}
r.routes[method+path] = route
return route
}

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
Copy link
Author

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
Copy link
Contributor

aldas commented Mar 27, 2024

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
Copy link
Author

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
Copy link

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.

golang/go#34868

@aldas
Copy link
Contributor

aldas commented Apr 6, 2024

I am closing this issue as I do not think we can do anything more than #2616 did.

@aldas aldas closed this as completed Apr 6, 2024
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