Opened 3 years ago

Closed 3 years ago

Last modified 20 months ago

#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)

full_logs.txt (15.2 KB ) - added by Mariusz Felisiak 3 years ago.

Download all attachments as: .zip

Change History (17)

by Mariusz Felisiak, 3 years ago

Attachment: full_logs.txt added

comment:1 by Mariusz Felisiak, 3 years ago

Cc: tim-schilling Matthias Kestenholz added

comment:2 by Mariusz Felisiak, 3 years ago

Summary: Form with __repr__ crashes.Circular contexts when rendering Form with BoundFields.

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.

comment:3 by David Smith, 3 years ago

Owner: changed from nobody to David Smith
Status: newassigned

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 Keryn Knight, 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?

comment:5 by Mariusz Felisiak, 3 years ago

Has patch: set
Patch needs improvement: set

comment:6 by Matthias Kestenholz, 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!)

in reply to:  6 comment:7 by Mariusz Felisiak, 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.

comment:8 by Matthias Kestenholz, 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.

in reply to:  8 comment:9 by Mariusz Felisiak, 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.

in reply to:  8 comment:10 by Keryn Knight, 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 Matthias Kestenholz, 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 David Smith, 3 years ago

Triage Stage: UnreviewedAccepted

comment:13 by Mariusz Felisiak, 3 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:14 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 4884a87e:

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

Regression in 456466d932830b096d39806e291fe23ec5ed38d5.

comment:15 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 0b62518f:

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

Regression in 456466d932830b096d39806e291fe23ec5ed38d5.

Backport of 4884a87e022056eda10534c13d74e49b8cdda632 from main

comment:16 by Carlton Gibson <carlton.gibson@…>, 20 months ago

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.

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