Opened 15 years ago

Closed 9 years ago

#11313 closed Bug (fixed)

list_editable fields don't support 'save' in multiuser environment

Reported by: Margie Roginski Owned by: nobody
Component: contrib.admin Version: 1.1-beta
Severity: Normal Keywords: model formsets
Cc: Paul Oswald, sailorfred@…, tomek@…, saxix.rome@…, patjlm, Zach Borboa, carsten.fuchs@…, cmawebsite@…, Seth Gottlieb, benred42@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I don't think that the list_editable fields concept in the admin, as implmemented in 1.1, works. Imagine this:

  1. User 1 brings up a changelist for an object and orders it by a 'deadline' field, most recent first, resulting in object 'foo' being shown as the most recent object
  2. User 2 edits object 'foo' and changes its deadline field so that it is no longer the most recent object
  3. User 1 still has the changelist form on his screen, and modifies the 'name' field of object foo. Then he hits 'save'.

When the 'save' request from user 1 is processed, a queryset for the objects is created that is ordered by deadline. However, the ordering of the queryset in the POST request is different than the ordering of the queryset in the GET request made by user 1, due to the edit made by user 2. In the example I describe above, object foo is no longer the object that has the most recent deadline when the code processes user 1's POST request. Instead, object 'bar' is. In effect, it's as if the code thinks the user wants to change the name of object 'bar' instead of object 'foo'. However, the id sent with the post data is for object 'foo'. Eventually we get into _perform_unique_checks() and this sort of identifies that there is a problem. There is the following code:

            # Exclude the current object from the query if we are editing an
            # instance (as opposed to creating a new one)
            if self.instance.pk is not None:
                qs = qs.exclude(pk=self.instance.pk)

In a non-multiuser case I think this would exclude the instance being edited, but in this case it doesn't. The result is that we drop into the next lines of code which generate an error, but the error looks like this: "Task with this None already exists." It contains the word "None" because the id field does not have a laberl attribute (ie self.fieldsid.label is None). I see this error when I print the form but it actually doesn't even show up in the admin ui. I just see the message "Please correct the errors below", but no errors are shown below.

In general it seems like a different tact needs to be taken for the POST request related to modification of editable fields. It seems to me that the code needs to identify the object being modified based on the id, and modify that object, rather than creating a queryset based on the filters and ordering and simply indexing in by form number.

Margie

Attachments (1)

doc.patch (799 bytes ) - added by sailorfred@… 12 years ago.
Documentation patch warning of bug

Download all attachments as: .zip

Change History (38)

comment:1 by Ramiro Morales, 15 years ago

This seems to be a duplicate of (#10784 and) #10922.

comment:2 by Margie Roginski, 15 years ago

Yes, I think you are right that this is a dup of 10922. I looked at the patch and based on a quick review it looks like it will solve this problem. So feel free to close this.

Margie

comment:3 by Alex Gaynor, 15 years ago

Resolution: duplicate
Status: newclosed

Marked as dupe because of above.

comment:4 by Margie Roginski, 15 years ago

Resolution: duplicate
Status: closedreopened

You know, I thought about this some more and I actually am now thinking that the patch for 10922 is not sufficient. In the admin, the creation of the ChangeList() object is what identifies the objects that will be changed. It is this changelist that contains the incorrect queryset, and the patch does not address this at all. In the case described in my ticket, the queryset that is returned by the ChangeList() for user1's GET request is actually different from the queryset returned by the ChangeList for user1's POST request. It is different due to changes that were applied by a POST by a different user.

I think the fixes in the patch are addressing the case where the queryset contains the objects specified in the POST data, but in a different order. In my ticket, this is not the case. Basically, in the admin application, I don't think that the processing of the POST should be creating a ChangeList through the same mechanism that it uses as in the GET. There is no reason to do all of the filtering and ordering that is done when creating a ChangeList. Instead, the POST processing should simply identify all of the objects specified in the POST and save the changes accordingly. If there is an error, it should re-render those exact items with the appropriate errors (not a newly filtered set of items). If there is no error, it should then create a ChangeList, doing the standard filtering/ordering and render that.

So I'm going to reopen this since I am fairly certain that it is not fixed by 10922.

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

milestone: 1.2
Triage Stage: UnreviewedAccepted

Strictly, #10922 is the same problem - i.e., there is a problem ensuring that the objects in the queryset used to construct an initial form correspond to the objects in the queryset used to save the form. The patch on #10922 fixes the problems with the single-user use case; the multi-user use case described here is another manifestation of the larger problem.

Both problems have existed for a long time (at least since newforms-admin was introduced); the specific problem described in #10922 is a little more prominent now due to the list_editable changes introduced in v1.1. In light of the need to get v1.1 out the door, I'll keep this ticket open as a placeholder for work required in the v1.2 timeframe to fix the multiuser case.

comment:6 by James Bennett, 15 years ago

milestone: 1.21.3

With no further action -- not even a patch -- there's no way this makes 1.2.

comment:7 by Andy Baker, 14 years ago

Is there not a much bigger problem with list_editable?

Admin1 has the changelist open
Admin2 makes a load of changes
Admin1 makes a single change and wipes all of Admin2's changes

This will affect least for the number of items specified in ModelAdmin.list_per_page which is 100 by default.

comment:8 by Julien Phalip, 14 years ago

Severity: Normal
Type: New feature

comment:9 by Julien Phalip, 14 years ago

See #11652 for a related issue.

comment:10 by Paul Oswald, 13 years ago

Cc: Paul Oswald added
Easy pickings: unset

comment:11 by Jacob, 13 years ago

milestone: 1.3

Milestone 1.3 deleted

comment:12 by Julien Phalip, 13 years ago

UI/UX: unset

#17118 reported one of possibly several issues making list_editable non thread safe, ie with the BaseModelFormSet._object_dict cache.

By the way, as suggested by the reporter in #17118, a warning should be added to the documentation indicating that list_editable isn't thread safe. That could be committed right away if a patch is provided.

comment:13 by sailorfred@…, 12 years ago

Severity: NormalRelease blocker
Type: New featureBug
Version: 1.1-beta-11.4

This is still biting people in 1.4.2. :( Also, this is a critical bug, not a new feature.

The case we're seeing is when rows get inserted into the database while an admin index page is up.

At least modify the docs to warn in huge red letters that that this is unsafe for all cases where data may enter the system other than from a single admin user.

comment:14 by Aymeric Augustin, 12 years ago

Severity: Release blockerNormal
Type: BugNew feature
Version: 1.41.1-beta-1

Django 1.1 to 1.4 have been released while this was a know bug. I don't see a compelling reason why it should delay 1.5.

If you want to help, write a patch, don't attempt to block releases!

comment:15 by sailorfred@…, 12 years ago

Cc: sailorfred@… added
Severity: NormalRelease blocker
Type: New featureBug

Point me to the right place in the code, and I'll write you a patch.

From someone who is a user, but hasn't had the time to dig into your code base, it seems that the fields that are changing need to be identified by the PK, not the position in a query set, which will change between the GET and the POST.

This is not a feature, it's a long standing bug that has caused data destruction for multiple organizations.

At the very least, update the documentation to point out the quicksand awaiting active sites.

by sailorfred@…, 12 years ago

Attachment: doc.patch added

Documentation patch warning of bug

comment:16 by anonymous, 12 years ago

I attached a documentation patch. Feel free to change the wording, or fix syntax problems. I worked from example in the document.

comment:17 by sailorfred@…, 12 years ago

Has patch: set
Needs documentation: set

comment:18 by Aymeric Augustin, 12 years ago

Severity: Release blockerNormal

1.5 beta has been released. At this stage, only tickets that must be fixed for RC1 can be release blockers.

Please don't abuse Trac's flags to push your personal agenda at the expense of everyone else who's waiting for 1.5 final.

If you want to drive more attention to this issue, please write to the django-developers mailing list.

comment:19 by Aymeric Augustin, 12 years ago

Status: reopenednew

comment:20 by pombredanne@…, 11 years ago

The approach would be imho to integrate something like django-concurrency https://github.com/saxix/django-concurrency though for now it does not support list_editable yet ....
What would be the best approach to build that ins django?

comment:21 by Tomek Paczkowski, 11 years ago

Cc: tomek@… added

comment:22 by Stefano Apostolico, 11 years ago

Cc: saxix.rome@… added

comment:23 by Stefano Apostolico, 11 years ago

django-concurrency 0.6dev supports list_editable and admin's actions.
Info at https://django-concurrency.readthedocs.org/en/latest/admin.html#list-editable

Last edited 11 years ago by Stefano Apostolico (previous) (diff)

comment:24 by patjlm, 10 years ago

This issue persists un Django 1.6.7. If an admin opens a page with list_editable items while data can be inserted (from admin or other flow), field values may end up being updated with values from other instances of the model.
This created a big mess for us recently: around 50 records got updated with wrong values.
Is there any timeline for a fix? Does anyone know what should be patched?

Note: I reproduced in Django 1.7 as well.

Thanks,

Patrick

Last edited 10 years ago by patjlm (previous) (diff)

comment:25 by patjlm, 10 years ago

Cc: patjlm added

comment:26 by patjlm, 10 years ago

Looking very quickly into the code, I think that this can be solved by replacing the following in contrib/admin/options.py changelist_view

replace (line 1526 in 1.7 / line 1382 in 1.6.7)

formset = cl.formset = FormSet(request.POST, request.FILES, queryset=cl.result_list)

by

formset = cl.formset = FormSet(request.POST, request.FILES)

in this test section:

# Handle POSTed bulk-edit data.
if (request.method == "POST" and cl.list_editable and '_save' in request.POST and not action_failed):

I am not used to generate patches nor am I knowledgeable enough in Django to assess if this change has a more global impact.
Could anyone have a look?

Thanks!

comment:27 by sailorfred, 10 years ago

Severity: NormalRelease blocker

comment:28 by Tim Graham, 10 years ago

Severity: Release blockerNormal

comment:29 by Zach Borboa, 10 years ago

Cc: Zach Borboa added

comment:30 by Carsten Fuchs, 10 years ago

Cc: carsten.fuchs@… added

comment:31 by Collin Anderson, 10 years ago

Cc: cmawebsite@… added
Keywords: model formsets added

As others have said, ModelFormsets in general should rely on a pk identifier rather than where the object falls in queryset ordering.

comment:32 by Seth Gottlieb, 9 years ago

Cc: Seth Gottlieb added

comment:33 by Benjamin Phillips, 9 years ago

Cc: benred42@… added

Is this issue still present in 1.8? I was attempting to write a test for the issue and could not reproduce the issue as stated in the ticket.

comment:34 by Tim Graham, 9 years ago

I suspect so, but you could try your test on Django 1.7 to see if it fails there as patjlm indicated he could reproduce the issue there.

comment:35 by Benjamin Phillips, 9 years ago

While I still have not been able to reproduce the issue exactly as stated on the ticket, I have found a potentially related issue:

1) User1 opens an admin changelist for a model with list-editable fields
2) User2 edits object "Foo" of that same model such that it moves to another page in the pagination order and saves
3) User1 edits object "Foo" and saves
4) The edit made by User1 does not get applied to object "Foo" but instead are used to create a new object

I have attempted to address this issue in PR 5686 https://github.com/django/django/pull/5686

comment:36 by Tim Graham, 9 years ago

Needs documentation: unset
Triage Stage: AcceptedReady for checkin

comment:37 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: newclosed

In 917cc288:

Fixed #11313 -- Made ModelAdmin.list_editable more resilient to concurrent edits.

Allowed admin POSTed bulk-edit data to use modeladmin.get_queryset()
so that the ids in the POST data have a chance to match up even if
the objects on the current page changed based on the ordering.

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