#33134 closed Bug (fixed)
Circular contexts when rendering Form with BoundFields.
Reported by: | Mariusz Felisiak | Owned by: | David Smith |
---|---|---|---|
Component: | Forms | Version: | 4.0 |
Severity: | Release blocker | Keywords: | |
Cc: | David Smith, Nick Pope, tim-schilling, Matthias Kestenholz | 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
I noticed really strange error when trying to test django-debug-toolbar
with Django 4.0a1. The following form:
class TemplateReprForm(forms.Form): user = forms.ModelChoiceField(queryset=User.objects.all()) def __repr__(self): return str(self)
causes Fatal Python error: Cannot recover from stack overflow
:
$ export TOXENV=py38-dj40-sqlite $ tox tests.panels.test_template.TemplatesPanelTestCase.test_template_repr ... test_template_repr (tests.panels.test_template.TemplatesPanelTestCase) ... Fatal Python error: Cannot recover from stack overflow. Python runtime state: initialized Current thread 0x00007f28e059d740 (most recent call first): File "/django-debug-toolbar/tests/forms.py", line 9 in __repr__ File "/usr/lib/python3.8/pprint.py", line 569 in _safe_repr File "/usr/lib/python3.8/pprint.py", line 67 in saferepr File "/django-debug-toolbar/debug_toolbar/panels/templates/panel.py", line 123 in _store_template_info File "/django-debug-toolbar/.tox/py38-dj40-sqlite/lib/python3.8/site-packages/django/dispatch/dispatcher.py", line 171 in <listcomp> File "/django-debug-toolbar/.tox/py38-dj40-sqlite/lib/python3.8/site-packages/django/dispatch/dispatcher.py", line 170 in send File "/django-debug-toolbar/.tox/py38-dj40-sqlite/lib/python3.8/site-packages/django/test/utils.py", line 100 in instrumented_test_render File "/django-debug-toolbar/.tox/py38-dj40-sqlite/lib/python3.8/site-packages/django/template/base.py", line 176 in render File "/django-debug-toolbar/.tox/py38-dj40-sqlite/lib/python3.8/site-packages/django/template/backends/django.py", line 61 in render File "/django-debug-toolbar/.tox/py38-dj40-sqlite/lib/python3.8/site-packages/django/forms/renderers.py", line 23 in render File "/django-debug-toolbar/.tox/py38-dj40-sqlite/lib/python3.8/site-packages/django/forms/utils.py", line 53 in render File "/django-debug-toolbar/.tox/py38-dj40-sqlite/lib/python3.8/site-packages/django/forms/boundfield.py", line 186 in label_tag File "/django-debug-toolbar/.tox/py38-dj40-sqlite/lib/python3.8/site-packages/django/template/base.py", line 891 in _resolve_lookup ... Aborted (core dumped) make: *** [Makefile:41: coverage] Error 134
Removing form
from context
in BoundField.label_tag()
fixes this issue for me:
diff --git a/django/forms/boundfield.py b/django/forms/boundfield.py index d1e98719d2..5bbfcbe41c 100644 --- a/django/forms/boundfield.py +++ b/django/forms/boundfield.py @@ -177,7 +177,6 @@ class BoundField: else: attrs['class'] = self.form.required_css_class context = { - 'form': self.form, 'field': self, 'label': contents, 'attrs': attrs,
I will try to debug it later.
Attachments (1)
Change History (17)
by , 3 years ago
Attachment: | full_logs.txt added |
---|
comment:1 by , 3 years ago
Cc: | added |
---|
comment:2 by , 3 years ago
Summary: | Form with __repr__ crashes. → Circular contexts when rendering Form with BoundFields. |
---|
comment:3 by , 3 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Thank you for the investigation.
I'll have a look at fixing this over the weekend. I wonder if should / could remove field as well.
comment:4 by , 3 years ago
FWIW, the likely 'fix' in DjDT may be to add the new template path's common prefix to SKIP_TEMPLATE_PREFIXES
which was specifically introduced when template based rendering was added for widgets, because it made walking the contexts super slow. I've not seen what DjDT's context output looks like with the new templates being used, but if it's massive/slow someone will almost certainly add the prefix.
saferepr
was used over repr
as a middle ground in case of recursive objects being encountered, which obviously this sort of is, but also isn't, alas. Using repr
was also specifically to avoid __str__
getting called, because that forces a render which may cause side effects like queries being run (ie: in an ideal world your __repr__
should never call __str__
).
From Django's end, we could reduce the available context to that which is currently necessary to render the label_tag (and any others that might be an issue?) and let "new" context keys/values be requested as cases might be made for them?
follow-up: 7 comment:6 by , 3 years ago
From Django's end, we could reduce the available context to that which is currently necessary to render the label_tag (and any others that might be an issue?) and let "new" context keys/values be requested as cases might be made for them?
I think it's a bit sad that the field isn't available in the label's context anymore. Sure, it isn't used right now. It would still be nice if there was an easy way to fix this from django-debug-toolbar's side and allow the additional data to stay in the label's template.
I'll have a look at it later today and see if I can reproduce the problem and also see whether there's an easy fix or workaround starting with SKIP_TEMPLATE_PREFIXES
(thanks for the reminder!)
comment:7 by , 3 years ago
Replying to Matthias Kestenholz:
I think it's a bit sad that the field isn't available in the label's context anymore. Sure, it isn't used right now. It would still be nice if there was an easy way to fix this from django-debug-toolbar's side and allow the additional data to stay in the label's template.
As far as I'm aware it's enough to remove form
from the context, so we can limit the patch to just that.
follow-ups: 9 10 comment:8 by , 3 years ago
I've looked into it and I'm unsure why django-debug-toolbar's test form calls str()
inside __repr__()
. I don't think this makes sense
This may fix it: https://github.com/jazzband/django-debug-toolbar/pull/1507/
The tests pass with Django 3.2 and main; a pull request for Django 4.0a1 follows soon.
comment:9 by , 3 years ago
Replying to Matthias Kestenholz:
I've looked into it and I'm unsure why django-debug-toolbar's test form calls
str()
inside__repr__()
. I don't think this makes sense
This may fix it: https://github.com/jazzband/django-debug-toolbar/pull/1507/
The tests pass with Django 3.2 and main; a pull request for Django 4.0a1 follows soon.
Thanks for fixing this in django-debug-toolbar
. Nonetheless I still believe that we should avoid circular contexts in Django itself.
comment:10 by , 3 years ago
Replying to Matthias Kestenholz:
I've looked into it and I'm unsure why django-debug-toolbar's test form calls
str()
inside__repr__()
. I don't think this makes sense [...]
Not to sidetrack the ticket too much, but I think the calling str
within __repr__
is precisely the point of the test, and indeed it's working as intended insofar as Mariusz has opened this ticket. Admitted, it's not ideal that the failure is just a stack overflow :) If any repr causes additional templates to run, and those templates themselves have a context which includes reprs which need templates (ad infinitum), it'll eventually just unavoidably explode.
See https://github.com/jazzband/django-debug-toolbar/pull/1156 and https://github.com/jazzband/django-debug-toolbar/issues/1155 by way of https://github.com/wagtail/wagtail/issues/5243 for the history, such as it is. It happens that rendering a form's str in the repr is an easy way to encounter the problem (accidentally?), because str
will render the form, with side-effects (SQL queries, additional template signals fired...) The problem being described in the latter, I think can be traced back to this ... https://github.com/jazzband/django-debug-toolbar/pull/933
comment:11 by , 3 years ago
You are right! I noticed this a bit later; the fix doesn't trigger the bug anymore because it doesn't test what it was supposed to anymore.
I have already opened an issue in the django-debug-toolbar issue tracker here https://github.com/jazzband/django-debug-toolbar/issues/1509
comment:12 by , 3 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:13 by , 3 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
As far as I'm aware
debug_toolbar.panels.templates.TemplatesPanel
recursively inspects contexts and callpprint.saferepr()
on all values, which causes an issue after 456466d932830b096d39806e291fe23ec5ed38d5.Form
'scontext
containsfields
,BoundField
's renderslabel_tag
withform
in thecontext
and so on :|We can consider this a bug in
django-debug-toolbar
, but IMO we should try to avoid circular contexts and removeform
fromcontext
passed for thelabel_tag
.