[Django] #35531: BaseModelFormSet.clean() recursively triggers is_valid() which may cause problems when is_valid() was overriden

[Django]

#35531: BaseModelFormSet.clean() recursively triggers is_valid() which may cause
problems when is_valid() was overriden
--------------------------------------------+------------------------
Reporter: Christophe Henry | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: dev
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
--------------------------------------------+------------------------
The current implementation of
[https://github.com/django/django/blob/main/django/forms/models.py#L800-L801
`BaseModelFormSet.clean()`] calls
[https://github.com/django/django/blob/main/django/forms/models.py#L800-L801
`BaseModelFormSet.validate_unique()`] which itself calls
[https://github.com/django/django/blob/main/django/forms/formsets.py#L286
`self.is_valid()`] though
[https://github.com/django/django/blob/main/django/forms/models.py#L807
`self.deleted_forms`].

This recursive call of `is_valid()` may mess up the validation if
`is_valid()` was overriden and `self.data` is altered during the process.
For instance:

```
{{{#!python
from django.forms import BaseModelFormSet
from django.forms.formsets import TOTAL_FORM_COUNT


class ExampleFormSet(BaseModelFormSet):
def __init__(self, extra_data_provider, **kwargs):
self.extra_data_provider = extra_data_provider
super().__init__(**kwargs)

def is_valid(self):
if not self.is_bound:
return False

next_idx =
self.data[self.management_form.add_prefix(TOTAL_FORM_COUNT)]

old_data = self.data
self.data = self.data.copy()

self.data[self.management_form.add_prefix(TOTAL_FORM_COUNT)] =
f"{next_idx+1}"
self.management_form.full_clean()

# Retreiving form data from somewhere else (this is an example)
next_form_data = self.extra_data_provider.retreive()
self.data.update(next_form_data)
self.forms.append(
self._construct_form(next_idx,
**self.get_form_kwargs(next_idx))
)

result = super().is_valid()

self._validate_empty_form = False

if not result:
# Reverting our changes
self.data = old_data
self.management_form.full_clean()
self.forms.pop()

return result
}}}

Let's assume `super().is_valid()` is `False` because `self.clean()`
failed. Then `self.forms.append()` will be called twice, but the cleaning
in the `if not result` block ` will perform only once.

In my opinion, this is a bug that produces unexpected behavior and is
difficult to debug.

This could be solved
[https://github.com/django/django/blob/main/django/forms/formsets.py#L286
by not calling `self.is_valid()` in `BaseFormSet.deleted_forms`] and
instead perform only a partial validation here. I enclosed a patch to this
ticket to show my solution.
--
Ticket URL: <https://code.djangoproject.com/ticket/35531>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35531: BaseModelFormSet.clean() recursively triggers is_valid() which may cause
problems when is_valid() was overriden

----------------------------------+--------------------------------------

Reporter: Christophe Henry | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: dev

Severity: Normal | Resolution:

Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0

----------------------------------+--------------------------------------
Changes (by Christophe Henry):

* Attachment "form.patch" added.

[Django]

#35531: BaseModelFormSet.clean() recursively triggers is_valid() which may cause
problems when is_valid() was overriden
----------------------------------+--------------------------------------

Reporter: Christophe Henry | Owner: Rish
Type: Bug | Status: assigned

Component: Forms | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
----------------------------------+--------------------------------------

Changes (by Rish):

* owner: nobody => Rish
* status: new => assigned

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

[Django]

#35531: BaseModelFormSet.clean() recursively triggers is_valid() which may cause
problems when is_valid() was overriden
----------------------------------+--------------------------------------
Reporter: Christophe Henry | Owner: Rish

Type: Bug | Status: closed
Component: Forms | Version: dev
Severity: Normal | Resolution: invalid

Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
----------------------------------+--------------------------------------

Changes (by Natalia Bidart):

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

Comment:

Hello Christophe Henry! Thank you for taking the time to create this
ticket.

I think I understand your explanation but, from my point of view, the
method `is_valid()` should not be changed and instead, custom validation
logic should be done in `clean()` or similar methods. If your app
absolutely needs to override `is_valid()`, it should do so in a way that
the method remains idempotent. Django provides no guarantees that
`is_valid()` is going to be called only once, so your app should not
depend on that.

Given the above, I'll close the ticket accordingly, but if you disagree,
you can consider starting a new conversation on the
[https://forum.djangoproject.com/c/internals/5 Django Forum], where you'll
reach a wider audience and likely get extra feedback. If you do so, please
make sure to provide a complete and correct example, because the code
provided in the description is incomplete and can not be used to reproduce
the issue you are describing.
--
Ticket URL: <https://code.djangoproject.com/ticket/35531#comment:2>