Opened 2 years ago

Closed 2 years ago

#33351 closed Cleanup/optimization (fixed)

path()/re_path() should raise a TypeError when kwargs is not a dict.

Reported by: Keryn Knight Owned by: Pedro Schlickmann Mendes
Component: Core (URLs) Version: dev
Severity: Normal Keywords: path _path URLPattern check
Cc: Egor R, Pedro Schlickmann Mendes Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Apparently, however many years into using Django, I'm still capable of making a "newbie" mistake and getting confused. So perhaps other actual new users encounter similar, especially given the lack of typing specifiers.

I defined a URL like so:

urlpatterns = [
    path("path/to/thing", MyView.as_view(), "my_view"),
]

which ... well, you either spot the issue immediately or you don't, and end up with the following. If you try and resolve() the path (eg: by making a request in your browser), you'll get something like:

In [3]: resolve("/path/to/thing")
~/Code/django/django/urls/base.py in resolve(path, urlconf)
     22     if urlconf is None:
     23         urlconf = get_urlconf()
---> 24     return get_resolver(urlconf).resolve(path)
     25
     26

~/Code/django/django/urls/resolvers.py in resolve(self, path)
    586             for pattern in self.url_patterns:
    587                 try:
--> 588                     sub_match = pattern.resolve(new_path)
    589                 except Resolver404 as e:
    590                     self._extend_tried(tried, pattern, e.args[0].get('tried'))

~/Code/django/django/urls/resolvers.py in resolve(self, path)
    388             new_path, args, kwargs = match
    389             # Pass any extra_kwargs as **kwargs.
--> 390             kwargs.update(self.default_args)
    391             return ResolverMatch(self.callback, args, kwargs, self.pattern.name, route=str(self.pattern))
    392

ValueError: dictionary update sequence element #0 has length 1; 2 is required

The crux of the issue being that I meant to give the URL a name, and it's a super unfortunate history that kwargs comes before the name argument (because nearly everyone gives a URL a name, but passing static kwargs is comparatively infrequent). So what's actually happened is that kwargs = "my_view" and eventually self.default_args = "my_view".

If I update to path("path/to/thing", MyView.as_view(), "my_view", name="my_view"), leaving the type incorrect, I can get the following error via reverse, too:

In [4]: reverse("my_view")
~/Code/django/django/urls/base.py in reverse(viewname, urlconf, args, kwargs, current_app)
     84             resolver = get_ns_resolver(ns_pattern, resolver, tuple(ns_converters.items()))
     85
---> 86     return resolver._reverse_with_prefix(view, prefix, *args, **kwargs)
     87
     88

~/Code/django/django/urls/resolvers.py in _reverse_with_prefix(self, lookup_view, _prefix, *args, **kwargs)
    669                     if set(kwargs).symmetric_difference(params).difference(defaults):
    670                         continue
--> 671                     if any(kwargs.get(k, v) != v for k, v in defaults.items()):
    672                         continue
    673                     candidate_subs = kwargs

AttributeError: 'str' object has no attribute 'items'

Both of these suggest that either there should be a type-guard in _path to assert it's dict-ish (if not None), or a system check on URLPattern to raise a friendly message. Well, they actually continue to suggest to me that everything after the view argument should be keyword-only, or that kwargs should come later, but I suspect those to be a harder sell ;)

This is specifically around the kwargs, but it doesn't look like there's any guarding on the name either, and I feel like a name of {'test': 'test'} (i.e. accidentally swapped both positionals) is likely to bite & cause an issue somewhere.

Change History (9)

comment:1 by Keryn Knight, 2 years ago

Summary: URLPattern should have a guard/check for arguments.URLPattern (or _path) should have a guard/check for provided arguments.

comment:2 by Mohamed Nabil Rady, 2 years ago

I agree that this behavior should be edited, but I think the ticket type should be Cleanup/optimization.

comment:3 by Mariusz Felisiak, 2 years ago

Easy pickings: set
Summary: URLPattern (or _path) should have a guard/check for provided arguments.path()/re_path() should raise a TypeError when kwargs is not a dict.
Triage Stage: UnreviewedAccepted
Type: New featureCleanup/optimization

Well, they actually continue to suggest to me that everything after the view argument should be keyword-only, or that kwargs should come later, but I suspect those to be a harder sell ;)

Keyword-only arguments would be great, but it will affect too many users. We reject such tickets in most of cases, however here it's justified because kwargs as a positional argument can be confusing, so let's raise a TypeError when kwargs is not a dict.

comment:4 by Mohamed Nabil Rady, 2 years ago

Owner: changed from nobody to Mohamed Nabil Rady
Status: newassigned

comment:5 by Egor R, 2 years ago

Cc: Egor R added

comment:6 by Pedro Schlickmann Mendes, 2 years ago

Cc: Pedro Schlickmann Mendes added

comment:7 by Pedro Schlickmann Mendes, 2 years ago

Has patch: set
Owner: changed from Mohamed Nabil Rady to Pedro Schlickmann Mendes

Based on the internals documentation, 'It is always acceptable, regardless whether someone has claimed it or not, to submit patches to a ticket if you happen to have a patch ready.' I am now assuming this ticket as I have a patch ready. Everyone should feel free to add sugestions to the provided solution! Thank you.

Ref: https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/submitting-patches/#ticket-claimers-responsibility

comment:8 by Mariusz Felisiak, 2 years ago

Triage Stage: AcceptedReady for checkin

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In 75485d16:

Fixed #33351 -- Made path()/re_path() raise TypeError when kwargs argument is not a dict.

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