[Django] #35909: Mutation of FormMixin.initial can cause a FormView with a formset to crash

[Django]

#35909: Mutation of FormMixin.initial can cause a FormView with a formset to crash
-------------------------------------+-------------------------------------
Reporter: David Sanders | Type: Bug
Status: new | Component: Generic
| views
Version: 5.1 | Severity: Normal
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
-------------------------------------+-------------------------------------
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35909>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

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

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

--
Ticket URL: <https://code.djangoproject.com/ticket/35909#comment:1>

[Django]

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,

Ticket URL: <https://code.djangoproject.com/ticket/35909#comment:2>

[Django]

#35909: Mutation of FormMixin.initial can cause a FormView with a formset to crash
----------------------------------+--------------------------------------
Reporter: David Sanders | Owner: (none)

Type: Bug | Status: closed

Component: Generic views | Version: 5.1

Severity: Normal | Resolution: invalid

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
----------------------------------+--------------------------------------

Changes (by Sarah Boyce):

* resolution: => invalid
* status: new => closed

Comment:

Hi David, long time no see!

So it looks like we don't have tests covering setting the initial data but
I would expect folks to either set the `initial` attribute or override
`get_initial()`, I also feel like that's in the docs although there isn't
an explicit example: https://docs.djangoproject.com/en/5.1/ref/class-
based-views/mixins-editing/#formmixin

So in short, I think what they have done is not how it should be used and
so I wouldn't call this a bug and would close as invalid

I would be open to adding more test coverage (and maybe doc updates to
make the above clearer)
Something like (with better naming)...
{{{#!diff
--- a/tests/generic_views/test_edit.py
+++ b/tests/generic_views/test_edit.py
@@ -14,13 +14,26 @@ from .models import Artist, Author
class FormMixinTests(SimpleTestCase):
request_factory = RequestFactory()

- def test_initial_data(self):
+ def test_initial_data_independent(self):
"""Test instance independence of initial data dict (see
#16138)"""
initial_1 = FormMixin().get_initial()
initial_1["foo"] = "bar"
initial_2 = FormMixin().get_initial()
self.assertNotEqual(initial_1, initial_2)

+ def test_set_initial_data(self):
+ class InitialAttribute(FormMixin):
+ initial = {"foo": "bar"}
+
+ class GetInitial(FormMixin):
+ def get_initial(self):
+ initial = super().get_initial()
+ initial["foo"] = "bar"
+ return initial
+
+ self.assertEqual(InitialAttribute().get_initial(), {"foo":
"bar"})
+ self.assertEqual(GetInitial().get_initial(), {"foo": "bar"})
+
def test_get_prefix(self):
"""Test prefix can be set (see #18872)"""
test_string = "test"
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35909#comment:3>