#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)
Change History (13)
by , 16 years ago
| Attachment: | formset.diff added |
|---|
comment:1 by , 16 years ago
by , 16 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 , 16 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 , 16 years ago
| Attachment: | patch10922-r10844.diff added |
|---|
comment:3 by , 16 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 , 16 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 , 16 years ago
| Has patch: | set |
|---|---|
| Needs tests: | set |
comment:6 by , 16 years ago
| Cc: | added |
|---|
by , 16 years ago
| Attachment: | patch10784-r11027-regressiontests.diff added |
|---|
Regression test I wrote for #10784 which would be relevant here.
comment:7 by , 16 years ago
| Cc: | added |
|---|
comment:8 by , 15 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
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.