Opened 4 years ago
Closed 4 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 , 4 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 , 4 years ago
comment:3 by , 4 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 , 4 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
comment:5 by , 4 years ago
| Cc: | added |
|---|
comment:6 by , 4 years ago
| Cc: | added |
|---|
comment:7 by , 4 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 , 4 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.