Opened 5 months ago

Closed 3 months 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 Changed 5 months ago by Carlton Gibson

Triage Stage: UnreviewedAccepted

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

comment:2 Changed 5 months ago by Mariusz Felisiak

Type: UncategorizedBug

comment:3 Changed 5 months ago 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?

Last edited 5 months ago by Horst Schneider (previous) (diff)

comment:4 Changed 5 months ago 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? 🤔

comment:5 Changed 5 months ago by Carlton Gibson

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

comment:6 in reply to:  5 ; Changed 5 months ago by 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.

comment:7 in reply to:  4 Changed 5 months ago by Horst Schneider

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 in reply to:  6 Changed 3 months ago by Neeraj Kumar

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 Changed 3 months ago 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.

comment:10 Changed 3 months ago by Neeraj Kumar

Cc: Neeraj Kumar added

comment:11 Changed 3 months ago 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.

comment:12 Changed 3 months ago by Neeraj Kumar

Has patch: set

comment:13 Changed 3 months ago by Carlton Gibson

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,

comment:14 in reply to:  13 Changed 3 months ago by Neeraj Kumar

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 Changed 3 months ago by Carlton Gibson

Needs tests: unset

comment:16 Changed 3 months ago by Mariusz Felisiak

Triage Stage: AcceptedReady for checkin

comment:17 Changed 3 months ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In 9942f3fb:

Fixed #33830 -- Fixed VariableDoesNotExist when rendering ClearableFileInput.

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