Opened 15 years ago

Closed 15 years ago

Last modified 12 years ago

#10922 closed (fixed)

Model formsets - matching POST data to model instances is fragile

Reported by: Karen Tracey Owned by: nobody
Component: Forms Version: 1.0
Severity: Keywords:
Cc: David Larlet, cmawebsite@… Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

From the discussion in this thread:

http://groups.google.com/group/django-developers/browse_thread/thread/d25e756b373c7017

It's clear we could be a bit smarter about matching up POST data with model instances.

Attachments (4)

formset.diff (2.2 KB ) - added by Alex Gaynor 15 years ago.
formset.2.diff (3.5 KB ) - added by Alex Gaynor 15 years ago.
fixed the SQL abuse, and code duplication, however if you take a look at the test I added a comment to it seems to be testing the exact behavior we want to avoid, guessing at the object
patch10922-r10844.diff (2.1 KB ) - added by Karen Tracey 15 years ago.
patch10784-r11027-regressiontests.diff (2.7 KB ) - added by Will Hardy 15 years ago.
Regression test I wrote for #10784 which would be relevant here.

Download all attachments as: .zip

Change History (13)

by Alex Gaynor, 15 years ago

Attachment: formset.diff added

comment:1 by Alex Gaynor, 15 years ago

A few concerns here: a) this is may be SQL inefficient, I haven't checked yet. b) There's a fair amount of code duplications since add_fields needs to do something *very* similar to what we do here.

by Alex Gaynor, 15 years ago

Attachment: formset.2.diff added

fixed the SQL abuse, and code duplication, however if you take a look at the test I added a comment to it seems to be testing the exact behavior we want to avoid, guessing at the object

comment:2 by Carl Meyer, 15 years ago

Just noting the connection with #10784, which demonstrates the ease with which this bug can be triggered; all it takes is a non-unique list-editable field in the admin, if the user chooses to sort on that field.

by Karen Tracey, 15 years ago

Attachment: patch10922-r10844.diff added

comment:3 by Karen Tracey, 15 years ago

Attached updated patch.

First, it applies against current trunk.

Second, modified the alterations involving setting the pk_value in add_fields so that the old logic is used in the case where the form is not bound. Without that change, the hidden pk/id fields in admin pages had no values set when initially displayed, resulting in even bigger problems if you tried to edit anything. (It's possible I misunderstood how to adapt the code to current trunk, as this is the area that conflicted when attempting to apply the old patch.)

Third, removed the test changes as they are also currently conflicting and I've run out of time for today to look at this.

The patch I attached on a very brief initial test does seem to fix the problem noted in #10784.

comment:4 by Karen Tracey, 15 years ago

milestone: 1.1

The problem in #10784 (now closed as dup of this) makes this a 1.1 candidate, I think.

comment:5 by Ramiro Morales, 15 years ago

Has patch: set
Needs tests: set

comment:6 by David Larlet, 15 years ago

Cc: David Larlet added

by Will Hardy, 15 years ago

Regression test I wrote for #10784 which would be relevant here.

comment:7 by Collin Anderson, 15 years ago

Cc: cmawebsite@… added

comment:8 by Russell Keith-Magee, 15 years ago

Resolution: fixed
Status: newclosed

(In [11160]) Fixed #10922 -- Corrected handling of POST data to ensure that the right objects are updated on save when the ordering field is editable. Thanks to Alex Gaynor, Karen Tracy, and Will Hardy for their contributions to this patch.

comment:9 by Jacob, 12 years ago

milestone: 1.1

Milestone 1.1 deleted

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