Panic when a route is added with different path params

[pafuent]

From now, Echo panics if a route that was already added, is added again
but for a different HTTP method and the path params are different.
e.g.
GET /translation/:lang -> Added OK
PUT /translation/:lang -> Added OK
DELETE /translation/:id -> Panic

Partially Fixes #1726 #1744

[codecov]

Codecov Report

Merging #1762 (7ca4fd1) into master (b0f56ea) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1762      +/-   ##
==========================================
- Coverage   89.75%   89.74%   -0.02%     
==========================================
  Files          32       32              
  Lines        2675     2671       -4     
==========================================
- Hits         2401     2397       -4     
  Misses        175      175              
  Partials       99       99              

Impacted Files	Coverage Δ
router.go	97.10% <100.00%> (+0.04%)	⬆️
middleware/rewrite.go	73.33% <0.00%> (-3.14%)	⬇️
middleware/proxy.go	60.97% <0.00%> (-1.38%)	⬇️
middleware/slash.go	90.69% <0.00%> (-0.61%)	⬇️

---

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 b0f56ea...c39feca. Read the comment docs.

[aldas]

This is place where maybe it would be better if route adding would return an error. Panicing is not nice but we already do it for some other things and assuming that routes are defined only on application startup - we can turn a blind eye.

by returning an error err := e.Get("/myinvalid/:/", func(c echo.Context) error{ would help give better feedback for route definition errors - duplicates or invalid paths, path variables etc.

This is something to think about future.

[pafuent]

I'm not a big fan of panic also, but as @aldas mentioned, we are already doing it and I preferred to get something quickly to at least move forward with those two issues. Because this PR is tagged as v5, we have time to get it in shape. If we agree on it, I can try to change the Router to return errors (at least in this two panic cases, and later on we can add more errors)

[aldas]

mmm, if its v5 then I have additional ideas for router - make router implement 2 interfaces and reference router by those interfaces

• one with single method to find route - this is called after startup when echo "routes"/searches handler function for the request.
• one with two methods. One for adding a route (called multiple times during application initialization) and second method to build (return fully built, complete, immutable) router instance that implements previous (find) interface (called once during application startup method)

this would make possible to add/change router implementations in future easier. 2 interfaces to explicitly separate 2 phases of router lifecycle for implementor.

This is very far fetched idea but you could build/return different implementations of router depending on added routes characteristics - for example router for single static route vs. router handling thousands of routes efficiently. I admit that 99.9999% cases you do need this but by adding those interfaces does not increase complexity of code much.

might be good idea to start some thread in discussions to link all those ideas @lammel (where first post has list of things and verdic on those)

[pafuent]

I uploaded a proposal of the changes mentioned by @aldas. The builds are not passing due to linting errors (I need to add proper comments and rename some things), but the tests and benchmarks pass locally.
One thing for that I didn't find a nice solution is the necessity to make Echo#BuildRouters() public in order to call it in our tests and benchmarks.
Please let me know how this could be improved and I'll be glad to change it.

[aldas]

This definitely needs some planning and research. For example RouteBuilder methods Routes and Reverse - when are those called? during setup or after server has been started and Echo attached to the server/listener. Probably closed issues history will help with this somewhat.

Also there is use case how newly created route is used e.GET("/users/:id", h).Name = "foobar" @ https://echo.labstack.com/guide/routing so if we return error this is changed. I did not notice that before. Maybe even if _, err := e.GETWithError("/users/:id", h); err != nil { or something (ugly as that) should be used.

I think we should start by listing API changes and discuss pros and cons. Determine what will be backwards compatible and what not.

probably should separate this problem - 2 routes with same prefix but with different path variable names and Router API changes.

[aldas]

@pafuent maybe it would be better to follow #1306 route and PR #1430 implementation. I personally do not think having different param names for same prefix is conceptually correct (definitely in RESTful world) but seems that this is quite common problem for users.

#1430 seems to be one way to do it but I'm not really sure how well it handles adding routes in different order and adding routes with groups and without group with same prefix.

[stale]

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]

I'll leave it closed. This is revised in v5. See this discussion for details #2000