Making Echo#Reverse() deterministic

[pafuent]

When the same handler/name is assigned to more than one route, Echo now returns the first route every time.

Fixes #1237

[codecov[bot]]

Codecov Report

Merging #1893 (7a492e3) into master (fdacff0) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1893   +/-   ##
=======================================
  Coverage   90.22%   90.22%           
=======================================
  Files          31       31           
  Lines        2773     2773           
=======================================
  Hits         2502     2502           
  Misses        173      173           
  Partials       98       98           

Impacted Files	Coverage Δ
echo.go	91.64% <100.00%> (ø)
router.go	95.03% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update fdacff0...7a492e3. Read the comment docs.

[aldas]

@pafuent I do not know if making it slice helps. Router Node can have only single handler per method type. So if you add route with same path+method multiple times last one is set and previous is forgotten but if you keep adding it to slice you would have routes in that slice that are no longer in actually router.

https://github.com/labstack/echo/blob/fdacff0d93d54e8065ac7f5072041146f054a201/router.go#L297

[aldas]

p.s. I think echo.Reverse is flawed in a way that it does not check all routers that exist.

		router           *Router
		routers          map[string]*Router  // <--- those are not checked

[pafuent]

@pafuent I do not know if making it slice helps. Router Node can have only single handler per method type. So if you add route with same path+method multiple times last one is set and previous is forgotten but if you keep adding it to slice you would have routes in that slice that are no longer in actually router.

https://github.com/labstack/echo/blob/fdacff0d93d54e8065ac7f5072041146f054a201/router.go#L297

You are right, and as you mention, that behavior is a property of the current implementation, but it shouldn't be something that Echo users should/need to do. If I got correctly your point, the app should set the same route and method more than one time, and that doesn't seem to be a valid use case. Besides of that I agree that a slice is not suited to handle this use case, so I'll try to came up with something else.

p.s. I think echo.Reverse is flawed in a way that it does not check all routers that exist.

Well, when I read your comment, I though the same, but after writing a test that in my mind should fail (but passed), I discovered that because Echo#Host is implemented through Groups, and that Groups at the end calls Echo#add, the routes of all the routers in Echo#routers are on routes of Echo#routes. This of course sounds pretty awkward to me. I'll try to see if I can also change this to make more sense. Besides of that, I also noticed that Echo#Host is not tested. I think I'll write some tests for it in a new PR, probably before updating this one, to give me some time to think about how should I approach what we are discussing.

[aldas]

Some historical info why Reverse and URI/URL exists and what are some of the usecases for them:

• https://github.com/labstack/echo/pull/77
• https://github.com/labstack/echo/issues/12
• https://echo.labstack.com/guide/routing/#route-naming
• https://github.com/labstack/echo/pull/960

One thing worth investigating is node.ppath has original route value and why Reverse tries to build similar value.

p.s. fields Route.Method and Route.Path probably should not be public. It is little bit wierd in sense that Route.Name is something that is meant to be changed but changing Method/Path will lead to problems as things are not sync with route you added or that e.router.routes[method+path] = r being unique per method+path. Making them public and adding json tags if probably for case you want to list all routes from API. I think func (e *Echo) Routes() []*Route { should not return maybe pointer and should return value instead and making fields unchangeable after returning from that method. At least that part would guards against change.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed within a month if no further activity occurs. Thank you for your contributions.

[aldas]

This is addressed in v5 proposal. See this discussion for details https://github.com/labstack/echo/discussions/2000