#27416 closed Bug (fixed)
ModelFormSet with queryset accepts invalid POST data for outer models and create unexpected empty data.
Reported by: | Hiroki Kiyohara | Owned by: | Hiroki Kiyohara |
---|---|---|---|
Component: | Forms | Version: | 1.10 |
Severity: | Normal | Keywords: | formset, modelform |
Cc: | 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
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.
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...
- 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) - ModelForm will set form.instance as empty instance (
self.instance = opts.model()
) - 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).
Change History (9)
comment:1 by , 8 years ago
Has patch: | set |
---|
comment:2 by , 8 years ago
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.
comment:3 by , 8 years ago
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
comment:4 by , 8 years ago
Triage Stage: | Unreviewed → Accepted |
---|
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?
comment:5 by , 8 years ago
Thank you. I want to check that the "empty data" won't be created and other_author.name
won't be "Changed name"
.
Without changes of the code, The number of models will be 3
, (author
, other_author
, and unexpected empty data).
The other_author.name
won't be "Changed name"
even we don't change 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.
*edited*
I improved tests to be more readable and fixed this response to be correct.
comment:6 by , 8 years ago
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.
comment:7 by , 8 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Opened a Pull Request https://github.com/django/django/pull/7462