Code

Opened 5 years ago

Closed 5 years ago

Last modified 3 years ago

#10922 closed (fixed)

Model formsets - matching POST data to model instances is fragile

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

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 5 years ago.
formset.2.diff (3.5 KB) - added by Alex 5 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 kmtracey 5 years ago.
patch10784-r11027-regressiontests.diff (2.7 KB) - added by Will Hardy 5 years ago.
Regression test I wrote for #10784 which would be relevant here.

Download all attachments as: .zip

Change History (13)

Changed 5 years ago by Alex

comment:1 Changed 5 years ago by Alex

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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.

Changed 5 years ago by Alex

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 Changed 5 years ago by carljm

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.

Changed 5 years ago by kmtracey

comment:3 Changed 5 years ago by kmtracey

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 Changed 5 years ago by kmtracey

  • milestone set to 1.1

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

comment:5 Changed 5 years ago by ramiro

  • Has patch set
  • Needs tests set

comment:6 Changed 5 years ago by david

  • Cc david added

Changed 5 years ago by Will Hardy

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

comment:7 Changed 5 years ago by CollinAnderson

  • Cc cmawebsite@… added

comment:8 Changed 5 years ago by russellm

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

(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 Changed 3 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

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.