[Django] #33830: Variable lookup errors are logged rendering 'clearable_file_input.html'
Django
-------------------------------------+-------------------------------------
Reporter: Horst | Owner: nobody
Schneider |
Type: | Status: new
Uncategorized |
Component: | Version: 4.0
contrib.admin |
Severity: Normal | Keywords: admin, template
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
It seems like the fix to #31536 raised a problem similar to #32681: Not
checking whether the `disabled` attribute actually exists on the `attrs`
of the 'clear' checkbox widget causes a `VariableDoesNotExist` exception
to be logged every time one of the patched `clearable_file_input.html`
templates is rendered with a checkbox that has no `disabled` atrribute
(i.e. is enabled):
{{{
[2022-07-06 10:06:03,452] DEBUG django.template base: Exception while
resolving variable 'disabled' in template
'admin/widgets/clearable_file_input.html'.
Traceback (most recent call last):
File "/home/horst/some_project/venv/lib/python3.10/site-
packages/django/template/base.py", line 875, in _resolve_lookup
current = current[bit]
KeyError: 'disabled'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/horst/some_project/venv/lib/python3.10/site-
packages/django/template/base.py", line 885, in _resolve_lookup
current = getattr(current, bit)
AttributeError: 'dict' object has no attribute 'disabled'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/horst/some_project/venv/lib/python3.10/site-
packages/django/template/base.py", line 891, in _resolve_lookup
current = current[int(bit)]
ValueError: invalid literal for int() with base 10: 'disabled'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/horst/some_project/venv/lib/python3.10/site-
packages/django/template/base.py", line 898, in _resolve_lookup
raise VariableDoesNotExist(
django.template.base.VariableDoesNotExist: Failed lookup for key
[disabled] in {'id': 'id_document'}
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33830>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Horst Schneider | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.admin | Version: 4.0
Severity: Normal | Resolution:
Keywords: admin, template | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Unreviewed => Accepted
Comment:
OK, yes — a fix to suppress that, similar to #32681 would be welcome.
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/33830#comment:1>
Django
---------------------------------+------------------------------------
Reporter: Horst Schneider | Owner: nobody
Component: contrib.admin | Version: 4.0
Severity: Normal | Resolution:
Keywords: admin, template | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
* type: Uncategorized => Bug
--
Ticket URL: <https://code.djangoproject.com/ticket/33830#comment:2>
Django
---------------------------------+------------------------------------
Reporter: Horst Schneider | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 4.0
Severity: Normal | Resolution:
Keywords: admin, template | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by Horst Schneider):
Although the problem is similar, the fix must be different here. We can
not easily supply some default context from the outside here since the
missing variable is in the `attrs` attribute of the checkbox widget.
It is guaranteed by the base `Widget` that `attrs` will always be a `dict
` instance. But it is **not** guaranteed that the key `disabled` will
always be present on the `attrs`, so we will mostly find it to be either
missing or set to `True` (could as well be set to `False`, if the default
is explicitly stated).
I guess a proper fix would be something along the lines of
{{{
<input
type="checkbox" name="{{ widget.checkbox_name }}"
id="{{ widget.checkbox_id }}"
{% if 'disabled' in widget.attrs and widget.attrs.disabled %}
disabled{% endif %}
>
}}}
(First checking if key is present, then checking its truthiness).
Or is there a more idiomatic way to perform those checks?
--
Ticket URL: <https://code.djangoproject.com/ticket/33830#comment:3>
Django
---------------------------------+------------------------------------
Reporter: Horst Schneider | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 4.0
Severity: Normal | Resolution:
Keywords: admin, template | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by Carlton Gibson):
Either that or we'd need a fix to #28172 maybe? See also #28526 — I wonder
if there isn't a cluster of duplicates here? 🤔
--
Ticket URL: <https://code.djangoproject.com/ticket/33830#comment:4>
Django
---------------------------------+------------------------------------
Reporter: Horst Schneider | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 4.0
Severity: Normal | Resolution:
Keywords: admin, template | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by Carlton Gibson):
@Horst could you investigate setting a default (`False`) for disabled in
ClearableFileInput for this case?
--
Ticket URL: <https://code.djangoproject.com/ticket/33830#comment:5>
Django
---------------------------------+------------------------------------
Reporter: Horst Schneider | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 4.0
Severity: Normal | Resolution:
Keywords: admin, template | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by Horst Schneider):
Replying to [comment:5 Carlton Gibson]:
> @Horst could you investigate setting a default (`False`) for disabled in
ClearableFileInput for this case?
Setting it both to `True` or `False` explicitly in the `attrs` does not
trigger the `VariableDoesNotExist` exception to be logged. Only the
absence of the `disabled` attribute on the `attrs` altogether triggers it.
--
Ticket URL: <https://code.djangoproject.com/ticket/33830#comment:6>
Django
---------------------------------+------------------------------------
Reporter: Horst Schneider | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 4.0
Severity: Normal | Resolution:
Keywords: admin, template | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by Horst Schneider):
Replying to [comment:4 Carlton Gibson]:
> Either that or we'd need a fix to #28172 maybe? See also #28526 — I
wonder if there isn't a cluster of duplicates here? 🤔
Not really duplicates, right? #28172 seems to be a very specific different
issue with filters and #28526 addresses the whole concept of logging
missing template variables in such a verbose fashion.
--
Ticket URL: <https://code.djangoproject.com/ticket/33830#comment:7>
Django
---------------------------------+------------------------------------
Reporter: Horst Schneider | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 4.0
Severity: Normal | Resolution:
Keywords: admin, template | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by Neeraj Kumar):
Replying to [comment:6 Horst Schneider]:
> Replying to [comment:5 Carlton Gibson]:
> > @Horst could you investigate setting a default (`False`) for disabled
in ClearableFileInput for this case?
>
> Setting it both to `True` or `False` explicitly in the `attrs` does not
trigger the `VariableDoesNotExist` exception to be logged. Only the
absence of the `disabled` attribute on the `attrs` altogether triggers it.
This error already handled in Exception when we do not provide "disabled"
in attrs.
{{{
raise VariableDoesNotExist("Failed lookup for key "
"[%s] in %r",
(bit, current)) # missing attribute
}}}
{{{
except Exception as e:
template_name = getattr(context, 'template_name', None) or
'unknown'
logger.debug(
"Exception while resolving variable '%s' in template
'%s'.",
bit,
template_name,
exc_info=True,
)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33830#comment:8>
Django
---------------------------------+------------------------------------
Reporter: Horst Schneider | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 4.0
Severity: Normal | Resolution:
Keywords: admin, template | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by Neeraj Kumar):
{{{
<input
type="checkbox" name="{{ widget.checkbox_name }}"
id="{{ widget.checkbox_id }}"
{% if 'disabled' in widget.attrs and widget.attrs.disabled %}
disabled{% endif %}
>
}}}
This solution can work to stop the trigger for {{{disabled}}} attribute in
attrs.
--
Ticket URL: <https://code.djangoproject.com/ticket/33830#comment:9>
Django
---------------------------------+------------------------------------
Reporter: Horst Schneider | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 4.0
Severity: Normal | Resolution:
Keywords: admin, template | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
* cc: Neeraj Kumar (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/33830#comment:10>
Django
---------------------------------+------------------------------------
Reporter: Horst Schneider | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 4.0
Severity: Normal | Resolution:
Keywords: admin, template | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by Carlton Gibson):
The issue with the `{% if 'disabled' in widget.attrs and
widget.attrs.disabled %} disabled{% endif %}` approach is that it makes
the template somewhat verbose.
The thought is that was can avoid that by setting a default `False` for
the disabled attribute in `ClearableFileInput` — If that works (i.e.
doesn't trigger the `VariableDoesNotExist`, shown by a new test similar to
those added in e.g. 0a4a5e5bacc354df3132d0fcf706839c21afb89d, and doesn't
break any existing tests) then that would be the preferred candidate for a
fix.
--
Ticket URL: <https://code.djangoproject.com/ticket/33830#comment:11>
Django
---------------------------------+------------------------------------
Reporter: Horst Schneider | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 4.0
Severity: Normal | Resolution:
Keywords: admin, template | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
* has_patch: 0 => 1
Comment:
PR for this
https://github.com/django/django/pull/15991
--
Ticket URL: <https://code.djangoproject.com/ticket/33830#comment:12>
Django
Reporter: Horst Schneider | Owner: Neeraj Kumar
Type: Bug | Status: assigned
Component: contrib.admin | Version: 4.0
Severity: Normal | Resolution:
Keywords: admin, template | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* owner: nobody => Neeraj Kumar
* status: new => assigned
* needs_tests: 0 => 1
Comment:
Thanks for the patch Neeraj.
Can you add a test case using `assertNoLogs()` ?
> If that works (i.e. doesn't trigger the VariableDoesNotExist, shown by a
new test similar to those added in e.g.
0a4a5e5bacc354df3132d0fcf706839c21afb89d,
--
Ticket URL: <https://code.djangoproject.com/ticket/33830#comment:13>
Django
---------------------------------+----------------------------------------
Reporter: Horst Schneider | Owner: Neeraj Kumar
Type: Bug | Status: assigned
Component: contrib.admin | Version: 4.0
Severity: Normal | Resolution:
Keywords: admin, template | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (by Neeraj Kumar):
Replying to [comment:13 Carlton Gibson]:
> Thanks for the patch Neeraj.
>
> Can you add a test case using `assertNoLogs()` ?
>
> > If that works (i.e. doesn't trigger the VariableDoesNotExist, shown by
a new test similar to those added in e.g.
0a4a5e5bacc354df3132d0fcf706839c21afb89d,
Added testcase using `assertNoLogs()`
--
Ticket URL: <https://code.djangoproject.com/ticket/33830#comment:14>
Django
---------------------------------+----------------------------------------
Reporter: Horst Schneider | Owner: Neeraj Kumar
Type: Bug | Status: assigned
Component: contrib.admin | Version: 4.0
Severity: Normal | Resolution:
Keywords: admin, template | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/33830#comment:15>
Django
Reporter: Horst Schneider | Owner: Neeraj
| Kumar
Type: Bug | Status: assigned
Component: contrib.admin | Version: 4.0
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33830#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Horst Schneider | Owner: Neeraj
| Kumar
Component: contrib.admin | Version: 4.0
Severity: Normal | Resolution: fixed
Keywords: admin, template | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"9942f3fb490f56bf28ee69f0a07f3eb62e7d3ab3" 9942f3fb]:
{{{
#!CommitTicketReference repository=""
revision="9942f3fb490f56bf28ee69f0a07f3eb62e7d3ab3"
Fixed #33830 -- Fixed VariableDoesNotExist when rendering
ClearableFileInput.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33830#comment:17>