Code

Opened 2 years ago

Closed 8 months ago

#18508 closed Bug (fixed)

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

Reported by: olau Owned by: akaariai
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 olau 2 years ago.
Test case and simple fix

Download all attachments as: .zip

Change History (9)

Changed 2 years ago by olau

Test case and simple fix

comment:1 Changed 20 months ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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

comment:2 Changed 20 months ago by olau

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 Changed 20 months ago by olau

  • Has patch set

comment:4 Changed 19 months ago by akaariai

  • Owner changed from nobody to akaariai

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

I will try to work on this.

comment:5 Changed 9 months ago by olau

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 Changed 9 months ago by akaariai

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 Changed 8 months ago by olau

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 Changed 8 months ago by Anssi Kääriäinen <akaariai@…>

  • Resolution set to fixed
  • Status changed from new to closed

In f4f01fb03c2855d42b4d9589c0c090439e02c55b:

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

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.