[Django] #33134: Form with __repr__ crashes.
Django
--------------------------------------------+------------------------
Reporter: Mariusz Felisiak | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 4.0
Severity: Release blocker | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
--------------------------------------------+------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33134>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Mariusz Felisiak | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 4.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* Attachment "full_logs.txt" added.
Django
Reporter: Mariusz Felisiak | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 4.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* cc: tim-schilling, Matthias Kestenholz (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/33134#comment:1>
Django
----------------------------------+--------------------------------------
Reporter: Mariusz Felisiak | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 4.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak):
As far as I'm aware `debug_toolbar.panels.templates.TemplatesPanel`
recursively inspects contexts and call `pprint.saferepr()` on all values,
which causes an issue after 456466d932830b096d39806e291fe23ec5ed38d5.
`Form`'s `context` contains `fields`, `BoundField`'s renders `label_tag`
with `form` in the `context` and so on :|
We can consider this a bug in `django-debug-toolbar`, but IMO we should
try to avoid circular contexts and remove `form` from `context` passed for
the `label_tag`.
--
Ticket URL: <https://code.djangoproject.com/ticket/33134#comment:2>
Django
Reporter: Mariusz Felisiak | Owner: David Smith
Type: Bug | Status: assigned
Component: Forms | Version: 4.0
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by David Smith):
* owner: nobody => David Smith
* status: new => assigned
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33134#comment:3>
Django
----------------------------------+---------------------------------------
Reporter: Mariusz Felisiak | Owner: David Smith
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Keryn Knight):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/33134#comment:4>
Django
----------------------------------+---------------------------------------
Reporter: Mariusz Felisiak | Owner: David Smith
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/14903 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/33134#comment:5>
Django
----------------------------------+---------------------------------------
Reporter: Mariusz Felisiak | Owner: David Smith
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Matthias Kestenholz):
> 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!)
--
Ticket URL: <https://code.djangoproject.com/ticket/33134#comment:6>
Django
----------------------------------+---------------------------------------
Reporter: Mariusz Felisiak | Owner: David Smith
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak):
Replying to [comment:6 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33134#comment:7>
Django
----------------------------------+---------------------------------------
Reporter: Mariusz Felisiak | Owner: David Smith
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33134#comment:8>
Django
----------------------------------+---------------------------------------
Reporter: Mariusz Felisiak | Owner: David Smith
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak):
Replying to [comment:8 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33134#comment:9>
Django
----------------------------------+---------------------------------------
Reporter: Mariusz Felisiak | Owner: David Smith
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Keryn Knight):
Replying to [comment:8 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
--
Ticket URL: <https://code.djangoproject.com/ticket/33134#comment:10>
Django
----------------------------------+---------------------------------------
Reporter: Mariusz Felisiak | Owner: David Smith
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Matthias Kestenholz):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/33134#comment:11>
Django
----------------------------------+---------------------------------------
Reporter: Mariusz Felisiak | Owner: David Smith
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by David Smith):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/33134#comment:12>
Django
Reporter: Mariusz Felisiak | Owner: David
| Smith
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33134#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Mariusz Felisiak | Owner: David
| Smith
Component: Forms | Version: 4.0
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"4884a87e022056eda10534c13d74e49b8cdda632" 4884a87e]:
{{{
#!CommitTicketReference repository=""
revision="4884a87e022056eda10534c13d74e49b8cdda632"
Fixed #33134 -- Fixed recursion depth error when rendering Form with
BoundFields.
Regression in 456466d932830b096d39806e291fe23ec5ed38d5.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33134#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Mariusz Felisiak | Owner: David
| Smith
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"0b62518ff424b0fe442b41636871fd99832ebdcb" 0b62518f]:
{{{
#!CommitTicketReference repository=""
revision="0b62518ff424b0fe442b41636871fd99832ebdcb"
[4.0.x] Fixed #33134 -- Fixed recursion depth error when rendering Form
with BoundFields.
Regression in 456466d932830b096d39806e291fe23ec5ed38d5.
Backport of 4884a87e022056eda10534c13d74e49b8cdda632 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33134#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Mariusz Felisiak | Owner: David
| Smith
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson <carlton.gibson@…>):
In [changeset:"051d5944f86400b9b3476db60bc73de7e9964810" 051d594]:
{{{
#!CommitTicketReference repository=""
revision="051d5944f86400b9b3476db60bc73de7e9964810"
Refs #33134, Refs #34077 -- Adjusted form rendering recursion test.
Adjusted recursion depth test to use str() rather than the form or
field’s render() method.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33134#comment:16>