Opened 12 years ago

Closed 11 years ago

#18508 closed Bug (fixed)

Bug in handling out-of-date POST data with inlineformset_factory

Reported by: Ole Laursen Owned by: Anssi Kääriäinen
Component: Forms Version: 1.4
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hi, there's a bug in BaseModelFormSet._construct_form. There's a guard supposed to take care of bound forms, but if one of the entries has been deleted, self._existing_object(pk) will return None so you can end up in

        if i < self.initial_form_count() and not kwargs.get('instance'):
            kwargs['instance'] = self.get_queryset()[i]

which is dangerous for a bound form where you must use ids to identify elements since element "i" can have changed from when the form was generated. In my case, I got an IndexError because there weren't enough elements left from a recent deletion.

I think the correct fix is to change the "if" to an "elif".

I've attached a patch with this fix and a test case for reproducing the problem (there doesn't seem to be any tests of model formsets?).

Note that the test as entered doesn't pass even with the fix because of issue #14877 - if you delete formset.save() and below in the test, it does pass.

Attachments (1)

django-forms-bug.patch (3.9 KB ) - added by Ole Laursen 12 years ago.
Test case and simple fix

Download all attachments as: .zip

Change History (9)

by Ole Laursen, 12 years ago

Attachment: django-forms-bug.patch added

Test case and simple fix

comment:1 by Aymeric Augustin, 12 years ago

Triage Stage: UnreviewedAccepted

Accepting because there's a patch with a test case. I'm not very familiar with formsets, though.

comment:2 by Ole Laursen, 12 years ago

Thanks! I can see now that my description is a bit cryptic. Basically, the problem here is a concurrency problem.

If you post a formset, Django is supposed to use another branch of the code to match each individual form with its object because you can't assume that a POSTed formset[i] corresponds to get_queryset()[i] - an element could have been added/deleted or the ordering changed.

Unfortunately, there's a trivial bug in this logic so it fails if one of the elements has been deleted.

I only noticed this because there was a Javascript bug in a site I've been doing maintenance for, so people would sometimes end up double-POSTing a delete, causing a mysterious crash. This patch and the patch in #14877 fixed it. You can still get concurrency issues but can at least deal with it yourself (e.g. through form validation) since Django passes the data on to you rather than crashing. :)

comment:3 by Ole Laursen, 12 years ago

Has patch: set

comment:4 by Anssi Kääriäinen, 12 years ago

Owner: changed from nobody to Anssi Kääriäinen

For what it is worth I just got hit by this issue.

I will try to work on this.

comment:5 by Ole Laursen, 11 years ago

I just noticed I had still the code for this lying around. Is there any chance you'll have time to look at it again? The bug itself is a trivial oversight so it's a one-line fix.

comment:6 by Anssi Kääriäinen, 11 years ago

I think #14877's patch (https://github.com/django/django/pull/1829) will take care of this ticket, too. Can you confirm? If you are going to confirm this, a tests-only patch would be very welcome...

comment:7 by Ole Laursen, 11 years ago

Thanks for fixing it! It was one of Django's dark corners I guess. I've updated the test case to fit in with the other test cases that now exist for model formsets (that's nice to see), and sent a pull request (https://github.com/django/django/pull/1833).

I can confirm that without the patch from #14877, the test case fails, and with the patch (with latest trunk) everything works. So I think you fixed it for good.

comment:8 by Anssi Kääriäinen <akaariai@…>, 11 years ago

Resolution: fixed
Status: newclosed

In f4f01fb03c2855d42b4d9589c0c090439e02c55b:

Fixed #18508 -- tests for repeated deletion bug in ModelFormSet

The ticket's issue was already fixed by patch for #14877.

Note: See TracTickets for help on using tickets.
Back to Top