Circular contexts when rendering Form with BoundFields.

[Mariusz Felisiak]

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.

[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.

[David Smith]

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.

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

[Mariusz Felisiak]

​PR

[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!)

[Mariusz Felisiak]

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.

[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.

[Mariusz Felisiak]

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.

[Keryn Knight]

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

[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

[Mariusz Felisiak <felisiak.mariusz@…>]

In 4884a87e:

Fixed #33134 -- Fixed recursion depth error when rendering Form with BoundFields.

Regression in 456466d932830b096d39806e291fe23ec5ed38d5.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 0b62518f:

[4.0.x] Fixed #33134 -- Fixed recursion depth error when rendering Form with BoundFields.

Regression in 456466d932830b096d39806e291fe23ec5ed38d5.

Backport of 4884a87e022056eda10534c13d74e49b8cdda632 from main

[Carlton Gibson <carlton.gibson@…>]

In 051d594:

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.