Opened 2 years ago

Closed 2 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 Carlton Gibson, 2 years ago

Triage Stage: UnreviewedAccepted

OK, yes — a fix to suppress that, similar to #32681 would be welcome. Thanks.

comment:2 by Mariusz Felisiak, 2 years ago

Type: UncategorizedBug

comment:3 by Horst Schneider, 2 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?

Version 0, edited 2 years ago by Horst Schneider (next)

comment:4 by Carlton Gibson, 2 years ago

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? 🤔

comment:5 by Carlton Gibson, 2 years ago

@Horst could you investigate setting a default (False) for disabled in ClearableFileInput for this case?

in reply to:  5 ; comment:6 by Horst Schneider, 2 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.

in reply to:  4 comment:7 by Horst Schneider, 2 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.

in reply to:  6 comment:8 by Neeraj Kumar, 2 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 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,
            )

comment:9 by Neeraj Kumar, 2 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 Neeraj Kumar, 2 years ago

Cc: Neeraj Kumar added

comment:11 by Carlton Gibson, 2 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.

comment:12 by Neeraj Kumar, 2 years ago

Has patch: set

comment:13 by Carlton Gibson, 2 years ago

Needs tests: set
Owner: changed from nobody to Neeraj Kumar
Status: newassigned

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,

in reply to:  13 comment:14 by Neeraj Kumar, 2 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 Carlton Gibson, 2 years ago

Needs tests: unset

comment:16 by Mariusz Felisiak, 2 years ago

Triage Stage: AcceptedReady for checkin

comment:17 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In 9942f3fb:

Fixed #33830 -- Fixed VariableDoesNotExist when rendering ClearableFileInput.

Note: See TracTickets for help on using tickets.
Back to Top