[Django] #27416: ModelFormSet with queryset accepts invalid POST data for outer models and create unexpected empty data.

[Django]

#27416: ModelFormSet with queryset accepts invalid POST data for outer models and
create unexpected empty data.
-------------------------------------+-------------------------------------
Reporter: Hiroki | Owner: Hiroki Kiyohara
Kiyohara |
Type: Bug | Status: assigned
Component: Forms | Version: 1.10
Severity: Normal | Keywords: formset,modelform
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
ModelFormSet with queryset argument will accept invalid POST data and
create unexpected empty data.
Invalid data means that data won't be appear in `queryset=...`, like this.

{{{#!python
author = Author.objects.create(pk=1, name='Walt Whitman')
other_author = Author.objects.create(pk=2, name='Charles
Baudelaire')
AuthorFormSet = modelformset_factory(Author, fields="__all__",
extra=0)

data = {
'form-TOTAL_FORMS': '2', # the number of forms rendered
'form-INITIAL_FORMS': '2', # the number of forms with initial
data
'form-MAX_NUM_FORMS': '', # the max number of forms
'form-0-id': str(author.id),
'form-0-name': 'Walt Whitman',
'form-1-id': str(other_author.id), # Specifying outer model
of bellow queryset.
'form-1-name': 'Changed name',
}
# This formset is only for Walt Whitman
# and should not accept POST data for other_author
formset = AuthorFormSet(data=data,
queryset=Author.objects.filter(id__in=(author.id,)))
}}}

This formset should ignore unexpected `form-1-id': str(other_author.id)`
value, a value not in queryset.
The value in `other_author` won't be changed, but this formset will create
unexpected new **empty data** (which has `Author.name = ""`).

=== Why

Internally...

1. In Formsets, forms corresponding to invalid data will be instantiated
with `instance=None` (due to this line
https://github.com/django/django/blob/master/django/forms/models.py#L597)
2. ModelForm will set form.instance as empty instance (`self.instance =
opts.model()`)
3. Saving `formset.save()` will save this unexpected invalid data

=== Why it's problem

It will create unexpected empty data and may cause IntegrityError.
For instance, if Artist.name field has unique constraint, and invalid POST
data come sometimes,
it will create empty Artist data more than twice and cause UNIQUE
constraint error.

=== PS

Is this recognized as bug? or should I specify more extra args for
formset?
I searched similar tickets but not existed.
(Just ticket I found is #26142 to prevent to create new instance for
FormSet, It may prevent this problem too but not correct way to solve).

--
Ticket URL: <https://code.djangoproject.com/ticket/27416>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#27416: ModelFormSet with queryset accepts invalid POST data for outer models and
create unexpected empty data.
-------------------------------------+-------------------------------------

Reporter: Hiroki Kiyohara | Owner: Hiroki

| Kiyohara
Type: Bug | Status: assigned
Component: Forms | Version: 1.10

Severity: Normal | Resolution:
Keywords: formset,modelform | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Hiroki Kiyohara):

* has_patch: 0 => 1

Comment:

Opened a Pull Request https://github.com/django/django/pull/7462

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

[Django]

#27416: ModelFormSet with queryset accepts invalid POST data for outer models and
create unexpected empty data.
-------------------------------------+-------------------------------------

Reporter: Hiroki Kiyohara | Owner: Hiroki

| Kiyohara
Type: Bug | Status: assigned
Component: Forms | Version: 1.10

Severity: Normal | Resolution:
Keywords: formset,modelform | Triage Stage:
| Unreviewed

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Tim Graham):

My first instinct is that this is a duplicate of #26142 but the fact that
the extra instances are created without the supplied data leaves a
question as to whether or not the current behavior is actually useful. As
#27416 says, `extra=0` isn't really meant to prohibit the creation of new
instances. Without digging into the details, I'd say it might be
appropriate to fix the current behavior to create instances with the
provided data (as a separate ticket) and then in this ticket add a new
`modelformset_factory()` parameter to prohibit the creation of new
instances.

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

[Django]

#27416: ModelFormSet with queryset accepts invalid POST data for outer models and
create unexpected empty data.
-------------------------------------+-------------------------------------

Reporter: Hiroki Kiyohara | Owner: Hiroki

| Kiyohara
Type: Bug | Status: assigned
Component: Forms | Version: 1.10

Severity: Normal | Resolution:
Keywords: formset,modelform | Triage Stage:
| Unreviewed

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Hiroki Kiyohara):

Thanks Tim for your quick response.

And I agree with you, If we want to prohibit the creation of new
instances, the ticket #26142 is the best.
Actually #26142 will solve a latest problem on my working project
(Creation of unexpected instance by invalid POST param).
As Tim explained, now the second question is whether invalid POST data
should create new empty instance or not.

I'm thinking it's not expected behavior. Because it will create
unvalidated empty instance. not so explicit.
But I'm not strongly sure. so I want some others opinion or I'll take time
to re-think about it.

I deleted `extra=0` argument on the PR (now it seems unrelated thing).
https://github.com/django/django/pull/7462/commits/20d4b8073c0f028ee23feb68f42cdd58549d58fa

--
Ticket URL: <https://code.djangoproject.com/ticket/27416#comment:3>

[Django]

#27416: ModelFormSet with queryset accepts invalid POST data for outer models and
create unexpected empty data.
-------------------------------------+-------------------------------------

Reporter: Hiroki Kiyohara | Owner: Hiroki

| Kiyohara
Type: Bug | Status: assigned
Component: Forms | Version: 1.10

Severity: Normal | Resolution:
Keywords: formset,modelform | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Tim Graham):

* stage: Unreviewed => Accepted

Comment:

I did some testing and the patch looks reasonable, although I'd like
another set of eyes.

Using the test in your patch, I didn't see the "empty data" object you
described. Without the fix, the new object has `name='Changed name'`,
doesn't it?

--
Ticket URL: <https://code.djangoproject.com/ticket/27416#comment:4>

[Django]

#27416: ModelFormSet with queryset accepts invalid POST data for outer models and
create unexpected empty data.
-------------------------------------+-------------------------------------

Reporter: Hiroki Kiyohara | Owner: Hiroki

| Kiyohara
Type: Bug | Status: assigned
Component: Forms | Version: 1.10

Severity: Normal | Resolution:
Keywords: formset,modelform | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Hiroki Kiyohara):

Thank you. I want to check not to exist the "empty data", so the test has
`self.assertEqual(Author.objects.count(), 2)`.
Without chanes of the code, It will be `3`, (`author`, `other_author`, and
unexpected empty data). The `other_author.name` won't be `"Changed name"`
even we don't chande codes.

I'll check something I said is correct again later.
If you feel the test or changes is hard to understand, I'll improve so
please give me any advices.

--
Ticket URL: <https://code.djangoproject.com/ticket/27416#comment:5>

[Django]

#27416: ModelFormSet with queryset accepts invalid POST data for outer models and
create unexpected empty data.
-------------------------------------+-------------------------------------

Reporter: Hiroki Kiyohara | Owner: Hiroki

| Kiyohara
Type: Bug | Status: assigned
Component: Forms | Version: 1.10

Severity: Normal | Resolution:
Keywords: formset,modelform | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Hiroki Kiyohara):

> Without changes of the code, It will be 3, (author, other_author, and

unexpected empty data). The other_author.name won't be "Changed name"

I checked that it's correct.

--
Ticket URL: <https://code.djangoproject.com/ticket/27416#comment:6>

[Django]

#27416: ModelFormSet with queryset accepts invalid POST data for outer models and
create unexpected empty data.
-------------------------------------+-------------------------------------

Reporter: Hiroki Kiyohara | Owner: Hiroki

| Kiyohara
Type: Bug | Status: assigned
Component: Forms | Version: 1.10

Severity: Normal | Resolution:
Keywords: formset,modelform | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Tim Graham):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/27416#comment:7>

[Django]

#27416: ModelFormSet with queryset accepts invalid POST data for outer models and
create unexpected empty data.
-------------------------------------+-------------------------------------

Reporter: Hiroki Kiyohara | Owner: Hiroki
| Kiyohara

Type: Bug | Status: closed
Component: Forms | Version: 1.10
Severity: Normal | Resolution: fixed

Keywords: formset,modelform | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Tim Graham <timograham@…>):

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

Comment:

In [changeset:"181f492ad021aeb43105aa9d38106ad7baf00211" 181f492a]:
{{{
#!CommitTicketReference repository=""
revision="181f492ad021aeb43105aa9d38106ad7baf00211"
Fixed #27416 -- Prevented ModelFormSet from creating objects for invalid
PKs in data.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/27416#comment:8>

[Django]

#27416: ModelFormSet with queryset accepts invalid POST data for outer models and
create unexpected empty data.
-------------------------------------+-------------------------------------

Reporter: Hiroki Kiyohara | Owner: Hiroki
| Kiyohara
Type: Bug | Status: closed

Component: Forms | Version: 1.10

Severity: Normal | Resolution: fixed
Keywords: formset,modelform | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Hiroki Kiyohara):

Thank you.

--
Ticket URL: <https://code.djangoproject.com/ticket/27416#comment:9>