#35090 closed Cleanup/optimization (fixed)
Enforce uniqueness on custom path converters
Reported by: | Adam Johnson | Owned by: | Salvo Polizzi |
---|---|---|---|
Component: | Core (URLs) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
register_converter()
allows custom path converters to silently replace existing converters. This could lead to surprising behaviour, both when two places accidentally use the same custom name or when replacing a default converter. This could be particularly hard to debug if a third-party package uses a custom converter.
For example, say two modules register path converters named “year”. The first allows 1-4 digits:
class YearConverter: regex = r"[0-9]{1,4}" ... register_converter(YearConverter, "year")
Whilst the second requires exactly four digits:
class YearConverter: regex = r"[0-9]{4}" ... register_converter(YearConverter, "year")
Whichever module is loaded last will silently overwrite the other’s converter. URLs will then only be interpreted with that converter, leading to fewer URLs being matched than intended. This can be particularly difficult to spot as import order may change accidentally due to other code being rearranged.
See the full example project at https://github.com/adamchainz/django-ticket-35090
I propose that support for reusing path converter names be deprecated, eventually raising an exception. I think this should include the default names (int
etc.) as replacing them can break URLs from third-party packages.
Change History (21)
comment:1 by , 9 months ago
Description: | modified (diff) |
---|
comment:2 by , 9 months ago
Description: | modified (diff) |
---|
comment:3 by , 9 months ago
comment:4 by , 9 months ago
Type: | Bug → Cleanup/optimization |
---|
As for it's a cleanup not a bug. If someone registers converters with the same name as builtin converters, they will get what they deserve. What if they do it on purpose?
comment:5 by , 9 months ago
If someone registers converters with the same name as builtin converters, they will get what they deserve. What if they do it on purpose?
Even if done on purpose, users can break URLs in other parts of their project or packages unwittingly. If you need a modified int
converter, for example, there’s little harm in requiring it to have a different name, but potential harm in allowing silent replacement.
comment:6 by , 9 months ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
follow-up: 11 comment:8 by , 9 months ago
I'm not sure I understand the desired fix: is it to prevent any converter registered name overlap (ie a dynamic check at the time of project load)? or just a check against the Django/default's converters?
Would there be a way to override registered converters?
comment:9 by , 9 months ago
Patch needs improvement: | unset |
---|
comment:10 by , 9 months ago
Needs documentation: | set |
---|---|
Owner: | changed from | to
Patch needs improvement: | set |
Status: | new → assigned |
comment:11 by , 9 months ago
Replying to Natalia Bidart:
I'm not sure I understand the desired fix: is it to prevent any converter registered name overlap (ie a dynamic check at the time of project load)? or just a check against the Django/default's converters?
I interpreted it as an issue to avoid overlapping among different registered converters, but please correct me if I misunderstood.
Would there be a way to override registered converters?
I honestly don't know if there is a possibility to override
comment:12 by , 9 months ago
Needs documentation: | unset |
---|
comment:13 by , 9 months ago
Patch needs improvement: | unset |
---|
comment:14 by , 9 months ago
Natalia, I propose preventing *any* overlap, that is, blocking the overriding of both the default and custom converters. Replacing a converter opens the possibility of “action at a distance” and a hard-to-debug lack of correct URL resolution. Unique names prevent that.
comment:15 by , 8 months ago
Patch needs improvement: | set |
---|
comment:16 by , 8 months ago
Patch needs improvement: | unset |
---|
I don't know why javascript tests are failing. If anyone can review the PR, please give me an advice on how to fix that problem.
comment:17 by , 8 months ago
Patch needs improvement: | set |
---|
comment:18 by , 8 months ago
Patch needs improvement: | unset |
---|
comment:19 by , 8 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
I've submitted a PR for this bug: https://github.com/django/django/pull/17703