Django

Code

Ticket #10922 (closed: fixed)

Opened 1 year ago

Last modified 9 months ago

Model formsets - matching POST data to model instances is fragile

Reported by: kmtracey Assigned to: nobody
Milestone: 1.1 Component: Forms
Version: 1.0 Keywords:
Cc: david, cmawebsite@gmail.com Triage Stage: Unreviewed
Has patch: 1 Needs documentation: 0
Needs tests: 1 Patch needs improvement: 0

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

formset.diff (2.2 kB) - added by Alex on 04/25/09 13:30:37.
formset.2.diff (3.5 kB) - added by Alex on 04/25/09 14:07:45.
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 on 05/26/09 13:10:00.
patch10784-r11027-regressiontests.diff (2.7 kB) - added by Will Hardy on 06/17/09 11:51:24.
Regression test I wrote for #10784 which would be relevant here.

Change History

04/25/09 13:30:37 changed by Alex

  • attachment formset.diff added.

04/25/09 13:31:25 changed by Alex

  • needs_better_patch changed.
  • needs_tests changed.
  • needs_docs changed.

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.

04/25/09 14:07:45 changed by Alex

  • 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

05/26/09 10:28:43 changed 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.

05/26/09 13:10:00 changed by kmtracey

  • attachment patch10922-r10844.diff added.

05/26/09 13:20:01 changed 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.

05/26/09 13:27:23 changed 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.

06/08/09 20:32:22 changed by ramiro

  • has_patch set to 1.
  • needs_tests set to 1.

06/09/09 12:29:31 changed by david

  • cc set to david.

06/17/09 11:51:24 changed by Will Hardy

  • attachment patch10784-r11027-regressiontests.diff added.

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

06/25/09 16:32:56 changed by CollinAnderson

  • cc changed from david to david, cmawebsite@gmail.com.

07/02/09 22:05:18 changed by russellm

  • status changed from new to closed.
  • resolution set to fixed.

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

Add/Change #10922 (Model formsets - matching POST data to model instances is fragile)




Change Properties
Action