#35909: Mutation of FormMixin.initial can cause a FormView with a formset to
crash
----------------------------------+--------------------------------------
Reporter: David Sanders | Owner: (none)
Type: Bug | Status: new
Component: Generic views | Version: 5.1
Severity: Normal | Resolution:
Keywords: FormView FormSet | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------------------------
Description changed by David Sanders:
Old description:
> Hi Djangonauts, long time no see… have been super busy, hope you're all
> well!
>
> Here's an interesting problem with some backstory for which I never knew
> Django behaved like this.
>
> So some of the old timers may know that `FormMixin.initial` is a mutable
> class attribute, and there have been past bug reports concerning this,
> the main one from 13 years ago is #16138
>
> The fix for this was to call `.copy()` during `get_initial()`.
>
> However, I just spent a couple of days tracking down and fixing a 500
> which was caused by this 😅 The direct cause for this was a junior dev
> mutating `initial` directly, not realising that because it is a **class
> attribute** then it's a long-living mutation.
>
> == Example
>
> Given this simple example model setup:
>
> {{{
> class Foo(Model):
> foo = IntegerField()
>
> class Bar(Model):
> foo = IntegerField()
> }}}
>
> and these 2 views:
>
> {{{
> class FooCreateView(CreateView):
> model = Foo
> fields = ["foo"]
>
> def get_context_data(self, **kwargs):
> self.initial["foo"] = 2
> return super().get_context_data(**kwargs)
>
> class BarCreateView(CreateView):
> model = Bar
> fields = ["foo"]
> }}}
>
> If you first visit `FooCreateView`, then visit `BarCreateView`; then this
> will have the **unintended side-effect** of initialising `BarCreateView`
> with `2`.
>
> This alone doesn't cause a 500 but it is a source of potential bug that
> may go unnoticed!
>
> Furthermore if you create a `FormView` with a model formset, this will
> cause it to 500!
>
> {{{
> BarFormSet = modelformset_factory(Bar, fields=["foo"])
>
> class LotsOfBarCreateView(FormView):
> template_name = "bars.html"
> form_class = BarFormSet
> }}}
>
> The unintended initialisation is incompatible with the way initialisation
> works with formsets - formsets expect a list, not a dictionary - see the
> traceback below
>
> == Potential Solution?
>
> If we simply move the initialisation of `initial` to an `__init__()` the
> problem goes away. Furthermore I ran the full test suite with this
> change and nothing seems to break.
>
> Claude seems to the only active contributor that was part of the original
> ticket - Would you (or anyone else) know why the solution wasn't to
> initialise from `__init__()` to begin with? I did see mention of it in a
> duplicate ticket #16701. The original ticket appears to be from prior
> GitHub times so there's no PR discussion to review.
>
> {{{
> index ebd071cf00..bff9ac68ce 100644
> --- a/django/views/generic/edit.py
> +++ b/django/views/generic/edit.py
> @@ -13,11 +13,15 @@ from django.views.generic.detail import (
> class FormMixin(ContextMixin):
> """Provide a way to show and handle a form in a request."""
>
> - initial = {}
> + initial = None
> form_class = None
> success_url = None
> prefix = None
>
> + def __init__(self, *args, **kwargs):
> + super().__init__(*args, **kwargs)
> + self.initial = {}
> +
> def get_initial(self):
> """Return the initial data to use for forms on this view."""
> return self.initial.copy()
> }}}
>
> == Traceback
>
> Here's the traceback:
>
> {{{
> Traceback (most recent call last):
> File "/path/to/test-project/django/core/handlers/exception.py", line
> 55, in inner
> response = get_response(request)
> ^^^^^^^^^^^^^^^^^^^^^
> File "/path/to/test-project/django/core/handlers/base.py", line 220, in
> _get_response
> response = response.render()
> ^^^^^^^^^^^^^^^^^
> File "/path/to/test-project/django/template/response.py", line 114, in
> render
> self.content = self.rendered_content
> ^^^^^^^^^^^^^^^^^^^^^
> File "/path/to/test-project/django/template/response.py", line 92, in
> rendered_content
> return template.render(context, self._request)
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> File "/path/to/test-project/django/template/backends/django.py", line
> 107, in render
> return self.template.render(context)
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> File "/path/to/test-project/django/template/base.py", line 171, in
> render
> return self._render(context)
> ^^^^^^^^^^^^^^^^^^^^^
> File "/path/to/test-project/django/template/base.py", line 163, in
> _render
> return self.nodelist.render(context)
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> File "/path/to/test-project/django/template/base.py", line 1016, in
> render
> return SafeString("".join([node.render_annotated(context) for node in
> self]))
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> File "/path/to/test-project/django/template/base.py", line 1016, in
> <listcomp>
> return SafeString("".join([node.render_annotated(context) for node in
> self]))
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> File "/path/to/test-project/django/template/base.py", line 977, in
> render_annotated
> return self.render(context)
> ^^^^^^^^^^^^^^^^^^^^
> File "/path/to/test-project/django/template/base.py", line 1081, in
> render
> return render_value_in_context(output, context)
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> File "/path/to/test-project/django/template/base.py", line 1058, in
> render_value_in_context
> value = str(value)
> ^^^^^^^^^^
> File "/path/to/test-project/django/forms/utils.py", line 55, in render
> return mark_safe(renderer.render(template, context))
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> File "/path/to/test-project/django/forms/renderers.py", line 29, in
> render
> return template.render(context, request=request).strip()
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> File "/path/to/test-project/django/template/backends/django.py", line
> 107, in render
> return self.template.render(context)
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> File "/path/to/test-project/django/template/base.py", line 171, in
> render
> return self._render(context)
> ^^^^^^^^^^^^^^^^^^^^^
> File "/path/to/test-project/django/template/base.py", line 163, in
> _render
> return self.nodelist.render(context)
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> File "/path/to/test-project/django/template/base.py", line 1016, in
> render
> return SafeString("".join([node.render_annotated(context) for node in
> self]))
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> File "/path/to/test-project/django/template/base.py", line 1016, in
> <listcomp>
> return SafeString("".join([node.render_annotated(context) for node in
> self]))
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> File "/path/to/test-project/django/template/base.py", line 977, in
> render_annotated
> return self.render(context)
> ^^^^^^^^^^^^^^^^^^^^
> File "/path/to/test-project/django/template/defaulttags.py", line 199,
> in render
> len_values = len(values)
> ^^^^^^^^^^^
> File "/path/to/test-project/django/forms/formsets.py", line 121, in
> __len__
> return len(self.forms)
> ^^^^^^^^^^
> File "/path/to/test-project/django/utils/functional.py", line 47, in
> __get__
> res = instance.__dict__[self.name] = self.func(instance)
> ^^^^^^^^^^^^^^^^^^^
> File "/path/to/test-project/django/forms/formsets.py", line 205, in
> forms
> return [
> ^
> File "/path/to/test-project/django/forms/formsets.py", line 206, in
> <listcomp>
> self._construct_form(i, **self.get_form_kwargs(i))
> File "/path/to/test-project/django/forms/models.py", line 740, in
> _construct_form
> kwargs["initial"] = self.initial_extra[i - self.initial_form_count()]
> ~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> KeyError: 0
> }}}
New description:
Hi Djangonauts, long time no see… have been super busy, hope you're all
well!
Here's an interesting problem with some backstory for which I never knew
Django behaved like this.
So some of the old timers may know that `FormMixin.initial` is a mutable
class attribute, and there have been past bug reports concerning this, the
main one from 13 years ago is #16138
The fix for this was to call `.copy()` during `get_initial()`.
However, I just spent a couple of days tracking down and fixing a 500
which was caused by this 😅 The direct cause for this was a junior dev
mutating `initial` directly, not realising that because it is a **class
attribute** then it's a long-living mutation.
== Example
Given this simple example model setup:
{{{
class Foo(Model):
foo = IntegerField()
class Bar(Model):
foo = IntegerField()
}}}
and these 2 views:
{{{
class FooCreateView(CreateView):
model = Foo
fields = ["foo"]
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.initial["foo"] = 2
class BarCreateView(CreateView):
model = Bar
fields = ["foo"]
}}}
If you first visit `FooCreateView`, then visit `BarCreateView`; then this
will have the **unintended side-effect** of initialising `BarCreateView`
with `2`.
This alone doesn't cause a 500 but it is a source of potential bug that
may go unnoticed!
Furthermore if you create a `FormView` with a model formset, this will
cause it to 500!
{{{
BarFormSet = modelformset_factory(Bar, fields=["foo"])
class LotsOfBarCreateView(FormView):
template_name = "bars.html"
form_class = BarFormSet
}}}
The unintended initialisation is incompatible with the way initialisation
works with formsets - formsets expect a list, not a dictionary - see the
traceback below
== Potential Solution?
If we simply move the initialisation of `initial` to an `__init__()` the
problem goes away. Furthermore I ran the full test suite with this change
and nothing seems to break.
Claude seems to the only active contributor that was part of the original
ticket - Would you (or anyone else) know why the solution wasn't to
initialise from `__init__()` to begin with? I did see mention of it in a
duplicate ticket #16701. The original ticket appears to be from prior
GitHub times so there's no PR discussion to review.
{{{
index ebd071cf00..bff9ac68ce 100644
--- a/django/views/generic/edit.py
+++ b/django/views/generic/edit.py
@@ -13,11 +13,15 @@ from django.views.generic.detail import (
class FormMixin(ContextMixin):
"""Provide a way to show and handle a form in a request."""
- initial = {}
+ initial = None
form_class = None
success_url = None
prefix = None
+ def __init__(self, *args, **kwargs):
+ super().__init__(*args, **kwargs)
+ self.initial = {}
+
def get_initial(self):
"""Return the initial data to use for forms on this view."""
return self.initial.copy()
}}}
== Traceback
Here's the traceback:
{{{
Traceback (most recent call last):
File "/path/to/test-project/django/core/handlers/exception.py", line 55,
in inner
response = get_response(request)
^^^^^^^^^^^^^^^^^^^^^
File "/path/to/test-project/django/core/handlers/base.py", line 220, in
_get_response
response = response.render()
^^^^^^^^^^^^^^^^^
File "/path/to/test-project/django/template/response.py", line 114, in
render
self.content = self.rendered_content
^^^^^^^^^^^^^^^^^^^^^
File "/path/to/test-project/django/template/response.py", line 92, in
rendered_content
return template.render(context, self._request)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path/to/test-project/django/template/backends/django.py", line
107, in render
return self.template.render(context)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path/to/test-project/django/template/base.py", line 171, in
render
return self._render(context)
^^^^^^^^^^^^^^^^^^^^^
File "/path/to/test-project/django/template/base.py", line 163, in
_render
return self.nodelist.render(context)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path/to/test-project/django/template/base.py", line 1016, in
render
return SafeString("".join([node.render_annotated(context) for node in
self]))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path/to/test-project/django/template/base.py", line 1016, in
<listcomp>
return SafeString("".join([node.render_annotated(context) for node in
self]))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path/to/test-project/django/template/base.py", line 977, in
render_annotated
return self.render(context)
^^^^^^^^^^^^^^^^^^^^
File "/path/to/test-project/django/template/base.py", line 1081, in
render
return render_value_in_context(output, context)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path/to/test-project/django/template/base.py", line 1058, in
render_value_in_context
value = str(value)
^^^^^^^^^^
File "/path/to/test-project/django/forms/utils.py", line 55, in render
return mark_safe(renderer.render(template, context))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path/to/test-project/django/forms/renderers.py", line 29, in
render
return template.render(context, request=request).strip()
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path/to/test-project/django/template/backends/django.py", line
107, in render
return self.template.render(context)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path/to/test-project/django/template/base.py", line 171, in
render
return self._render(context)
^^^^^^^^^^^^^^^^^^^^^
File "/path/to/test-project/django/template/base.py", line 163, in
_render
return self.nodelist.render(context)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path/to/test-project/django/template/base.py", line 1016, in
render
return SafeString("".join([node.render_annotated(context) for node in
self]))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path/to/test-project/django/template/base.py", line 1016, in
<listcomp>
return SafeString("".join([node.render_annotated(context) for node in
self]))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path/to/test-project/django/template/base.py", line 977, in
render_annotated
return self.render(context)
^^^^^^^^^^^^^^^^^^^^
File "/path/to/test-project/django/template/defaulttags.py", line 199,
in render
len_values = len(values)
^^^^^^^^^^^
File "/path/to/test-project/django/forms/formsets.py", line 121, in
__len__
return len(self.forms)
^^^^^^^^^^
File "/path/to/test-project/django/utils/functional.py", line 47, in
__get__
res = instance.__dict__[self.name] = self.func(instance)
^^^^^^^^^^^^^^^^^^^
File "/path/to/test-project/django/forms/formsets.py", line 205, in
forms
return [
^
File "/path/to/test-project/django/forms/formsets.py", line 206, in
<listcomp>
self._construct_form(i, **self.get_form_kwargs(i))
File "/path/to/test-project/django/forms/models.py", line 740, in
_construct_form
kwargs["initial"] = self.initial_extra[i - self.initial_form_count()]
~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
KeyError: 0
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/35909#comment:1>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion visit
https://groups.google.com/d/msgid/django-updates/0107019326111df2-23f96f72-ce64-46ae-a73c-6077ec61cd90-000000%40eu-central-1.amazonses.com.
