Opened 9 months ago

Closed 8 months ago

Last modified 8 months ago

#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 Adam Johnson)

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 Adam Johnson, 9 months ago

Description: modified (diff)

comment:2 by Adam Johnson, 9 months ago

Description: modified (diff)

comment:3 by Salvo Polizzi, 9 months ago

I've submitted a PR for this bug: https://github.com/django/django/pull/17703

comment:4 by Mariusz Felisiak, 9 months ago

Type: BugCleanup/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 Adam Johnson, 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 Adam Johnson, 9 months ago

Has patch: set
Patch needs improvement: set

comment:7 by Mariusz Felisiak, 9 months ago

Triage Stage: UnreviewedAccepted

OK, let's have it.

comment:8 by Natalia Bidart, 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 Salvo Polizzi, 9 months ago

Patch needs improvement: unset

comment:10 by Mariusz Felisiak, 9 months ago

Needs documentation: set
Owner: changed from nobody to Salvo Polizzi
Patch needs improvement: set
Status: newassigned

in reply to:  8 comment:11 by Salvo Polizzi, 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 Salvo Polizzi, 9 months ago

Needs documentation: unset

comment:13 by Salvo Polizzi, 9 months ago

Patch needs improvement: unset

comment:14 by Adam Johnson, 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 Mariusz Felisiak, 8 months ago

Patch needs improvement: set

comment:16 by Salvo Polizzi, 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 Mariusz Felisiak, 8 months ago

Patch needs improvement: set

comment:18 by Salvo Polizzi, 8 months ago

Patch needs improvement: unset

comment:19 by Mariusz Felisiak, 8 months ago

Triage Stage: AcceptedReady for checkin

comment:20 by Mariusz Felisiak <felisiak.mariusz@…>, 8 months ago

Resolution: fixed
Status: assignedclosed

In 6e1ece7:

Fixed #35090 -- Deprecated registering URL converters with the same name.

comment:21 by GitHub <noreply@…>, 8 months ago

In 0e84e70:

Refs #35090 -- Fixed urlpatterns.tests.SimplifiedURLTests when run in reverse.

Note: See TracTickets for help on using tickets.
Back to Top