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

[Ole Laursen]

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.

[Ole Laursen]

Test case and simple fix

[Aymeric Augustin]

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

[Ole Laursen]

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. :)

[Anssi Kääriäinen]

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

I will try to work on this.

[Ole Laursen]

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.

[Anssi Kääriäinen]

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

[Ole Laursen]

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.

[Anssi Kääriäinen <akaariai@…>]

In f4f01fb03c2855d42b4d9589c0c090439e02c55b:

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

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