fix(routing): raise on paths that only differ in their path parameters - #4989
fix(routing): raise on paths that only differ in their path parameters#4989NoiceHax wants to merge 1 commit into
Conversation
The routing trie collapses every path parameter at a given position into a
single node, so paths such as '/{id:int}' and '/{id:str}' resolve to the very
same node and the route registered last silently replaced the handlers
registered before it, making them unreachable. Registering such paths now
raises an ImproperlyConfiguredException instead.
Closes litestar-org#3622
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4989 +/- ##
==========================================
+ Coverage 67.41% 67.42% +0.01%
==========================================
Files 293 293
Lines 15308 15320 +12
Branches 1736 1739 +3
==========================================
+ Hits 10320 10330 +10
- Misses 4839 4841 +2
Partials 149 149 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thinking about this again, this does introduce some inconsistencies: You normally do not get an exception when you override a path, but in this special case you'll now get one. IMO we should either always raise / warn there or never. If we were to do this, my vote would be to emit a warning for every handler which overrides another, unless the handler explicitly states that it intends to override another handler (e.g. by setting @litestar-org/members thoughts on this? |
The routing trie collapses every path parameter at a given position into one node, so
/{id:int}and/{id:str}resolve to the same node. Whichever route got registered last silently overwrote the earlier handler and made it unreachable. The issue discussion settled on raising an error instead of routing by parameter type, so registering paths like that now raisesImproperlyConfiguredException.The check runs per method, so two paths that share a node but register different methods still work. OPTIONS is skipped. Litestar generates an OPTIONS handler for every path that does not declare one, so those handlers collide on a shared node even though no user handler is being shadowed. Re-registering the exact same route is still fine, which matters when the trie gets rebuilt after handlers are added to an app that already exists.
Tests are in tests/unit/test_asgi/test_routing_trie/test_mapping.py. I also added a note to the path parameter docs.
Closes #3622
📚 Documentation preview 📚: https://litestar-org.github.io/litestar-docs-preview/4989