Opened 3 years ago
Closed 3 years ago
#33830 closed Bug (fixed)
Variable lookup errors are logged rendering 'clearable_file_input.html'
| Reported by: | Horst Schneider | Owned by: | Neeraj Kumar |
|---|---|---|---|
| Component: | contrib.admin | Version: | 4.0 |
| Severity: | Normal | Keywords: | admin, template |
| Cc: | Neeraj Kumar | Triage Stage: | Ready for checkin |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
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'}
Change History (17)
comment:1 by , 3 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|
comment:2 by , 3 years ago
| Type: | Uncategorized → Bug |
|---|
comment:3 by , 3 years ago
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?
follow-up: 7 comment:4 by , 3 years ago
follow-up: 6 comment:5 by , 3 years ago
@Horst could you investigate setting a default (False) for disabled in ClearableFileInput for this case?
follow-up: 8 comment:6 by , 3 years ago
Replying to 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.
comment:7 by , 3 years ago
Replying to 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.
comment:8 by , 3 years ago
Replying to Horst Schneider:
Replying to Carlton Gibson:
@Horst could you investigate setting a default (
False) for disabled in ClearableFileInput for this case?
Setting it both to
TrueorFalseexplicitly in theattrsdoes not trigger theVariableDoesNotExistexception to be logged. Only the absence of thedisabledattribute on theattrsaltogether 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,
)
comment:9 by , 3 years ago
<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.
comment:10 by , 3 years ago
| Cc: | added |
|---|
comment:11 by , 3 years ago
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.
follow-up: 14 comment:13 by , 3 years ago
| Needs tests: | set |
|---|---|
| Owner: | changed from to |
| Status: | new → assigned |
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,
comment:14 by , 3 years ago
Replying to 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()
comment:15 by , 3 years ago
| Needs tests: | unset |
|---|
comment:16 by , 3 years ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
OK, yes — a fix to suppress that, similar to #32681 would be welcome. Thanks.