[Django] #33351: URLPattern should have a guard/check for arguments.
Django
-------------------------------------+-------------------------------------
Reporter: Keryn | Owner: nobody
Knight |
Type: New | Status: new
feature |
Component: Core | Version: dev
(URLs) | Keywords: path _path
Severity: Normal | URLPattern check
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33351>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Keryn Knight | Owner: nobody
Type: New feature | Status: new
Component: Core (URLs) | Version: dev
Severity: Normal | Resolution:
Keywords: path _path | Triage Stage:
URLPattern check | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/33351#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Keryn Knight | Owner: nobody
Type: New feature | Status: new
Component: Core (URLs) | Version: dev
Severity: Normal | Resolution:
Keywords: path _path | Triage Stage:
URLPattern check | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mohamed Nabil Rady):
I agree that this behavior should be edited, but I think the ticket type
should be Cleanup/optimization.
--
Ticket URL: <https://code.djangoproject.com/ticket/33351#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Keryn Knight | Owner: nobody
Cleanup/optimization |
Component: Core (URLs) | Version: dev
Severity: Normal | Resolution:
URLPattern check |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* type: New feature => Cleanup/optimization
* easy: 0 => 1
* stage: Unreviewed => Accepted
Comment:
> 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33351#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Nabil Rady
Cleanup/optimization | Status: assigned
Component: Core (URLs) | Version: dev
Severity: Normal | Resolution:
Keywords: path _path | Triage Stage: Accepted
URLPattern check |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* owner: nobody => Mohamed Nabil Rady
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33351#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Keryn Knight | Owner: Mohamed
Type: | Nabil Rady
Cleanup/optimization | Status: assigned
Component: Core (URLs) | Version: dev
Severity: Normal | Resolution:
Keywords: path _path | Triage Stage: Accepted
URLPattern check |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* cc: Egor R (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/33351#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Keryn Knight | Owner: Mohamed
Type: | Nabil Rady
Cleanup/optimization | Status: assigned
Component: Core (URLs) | Version: dev
Severity: Normal | Resolution:
Keywords: path _path | Triage Stage: Accepted
URLPattern check |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* cc: Pedro Schlickmann Mendes (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/33351#comment:6>
Django
-------------------------------------+-------------------------------------
Type: | Schlickmann Mendes
Cleanup/optimization | Status: assigned
Component: Core (URLs) | Version: dev
Severity: Normal | Resolution:
Keywords: path _path | Triage Stage: Accepted
URLPattern check |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Pedro Schlickmann Mendes):
* owner: Mohamed Nabil Rady => Pedro Schlickmann Mendes
* has_patch: 0 => 1
Comment:
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
--
Ticket URL: <https://code.djangoproject.com/ticket/33351#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Keryn Knight | Owner: Pedro
Type: | Schlickmann Mendes
Cleanup/optimization | Status: assigned
Component: Core (URLs) | Version: dev
Severity: Normal | Resolution:
URLPattern check | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33351#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Keryn Knight | Owner: Pedro
Type: | Schlickmann Mendes
Component: Core (URLs) | Version: dev
Keywords: path _path | Triage Stage: Ready for
URLPattern check | checkin
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"75485d16a289c1fc4eb39702c14d2f08f0d8ccce" 75485d16]:
{{{
#!CommitTicketReference repository=""
revision="75485d16a289c1fc4eb39702c14d2f08f0d8ccce"
Fixed #33351 -- Made path()/re_path() raise TypeError when kwargs argument
is not a dict.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33351#comment:9>