Opened 3 years ago
Closed 3 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 , 3 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 , 3 years ago
comment:3 by , 3 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: | Unreviewed → Accepted |
Type: | New feature → Cleanup/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 , 3 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:5 by , 3 years ago
Cc: | added |
---|
comment:6 by , 3 years ago
Cc: | added |
---|
comment:7 by , 3 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
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.
comment:8 by , 3 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
I agree that this behavior should be edited, but I think the ticket type should be Cleanup/optimization.