Ensure routes are unique #1538
Ensure routes are unique #1538
Conversation
paulcsmith
force-pushed
the
pcs/enforce-unique
branch
from
9d22089 to
7fe8cfa
Compare
| # So "/users/:user_id" is changed to "/users/:normalized" | ||
| {% normalized_path = original_path.gsub(/(\:\w*)/, ":normalized") %} |
There was a problem hiding this comment.
This way /users/:id and /users/:user_id are counted as the same route, since that's how the router tries to match them. I tested this locally to make sure it works
|
There are some edge cases with optional route parts, but overall this should catch most route collissions |
| @@ -0,0 +1,6 @@ | |||
| # Include this in an action to skip route uniqueness checks. | |||
| module Lucky::SkipUniqueRouteCheck | |||
There was a problem hiding this comment.
Not sure that it's necessary to provide this. If you skip the compile time check, you're still going to get a runtime error from the duplicate checks in lucky_router https://github.com/luckyframework/lucky_router/blob/d6020f0c404e033abb920cf29e34ec8d752525c2/src/lucky_router/matcher.cr#L50
There was a problem hiding this comment.
Good point. We discussed this in chat and I think we can leave this here pre-1.0 (or at least for one version) since this uses different logic for checking duplicates. If it seems to not have issues then we can remove it!
Does that sound good?
There was a problem hiding this comment.
Created an issue to track this here: #1541
| # So "/users/:user_id" is changed to "/users/:normalized" | ||
| {% normalized_path = original_path.gsub(/(\:\w*)/, ":normalized") %} | ||
|
|
||
| {% if already_used_route = NORMALIZED_ROUTES.find { |route| route[:normalized_path] == normalized_path && route[:method] == method } %} |
There was a problem hiding this comment.
Not sure if this matter or not, but would it make sense (from a lookup perspective) to make NORMALIZED_ROUTES a Hash instead? The normalized path would be the key in this case.
There was a problem hiding this comment.
@kevinsjoberg I tried, but there are some issues with Hash/NameTuples in macro constants that I couldn't seem to get around :(
Also since it needs to match both method and normalized path I think this may actually work better. Since if we used the normalized path as key that would mean there is no way to lookup based on method. I suppose we could combine the method and path into one string key though 🤔 Maybe someone can do a cleanup pass later and can figure out how to use the Hash in a macro
Tested by changing one of our routes in the specs to collide and it worked
Closes #1539