Opened 7 years ago

Closed 10 months 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@… 4 years ago.
Documentation patch warning of bug

Download all attachments as: .zip

Change History (38)

comment:1 Changed 7 years ago by Ramiro Morales

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

comment:2 Changed 7 years ago by Margie Roginski

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 Changed 7 years ago by Alex Gaynor

Resolution: duplicate
Status: newclosed

Marked as dupe because of above.

comment:4 Changed 7 years ago by Margie Roginski

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 Changed 7 years ago by Russell Keith-Magee

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 Changed 7 years ago by James Bennett

milestone: 1.21.3

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

comment:7 Changed 6 years ago by Andy Baker

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 Changed 6 years ago by Julien Phalip

Severity: Normal
Type: New feature

comment:9 Changed 6 years ago by Julien Phalip

See #11652 for a related issue.

comment:10 Changed 6 years ago by Paul Oswald

Cc: Paul Oswald added
Easy pickings: unset

comment:11 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

comment:12 Changed 5 years ago by Julien Phalip

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 Changed 4 years ago by sailorfred@…

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 Changed 4 years ago by Aymeric Augustin

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 Changed 4 years ago by sailorfred@…

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.

Changed 4 years ago by sailorfred@…

Attachment: doc.patch added

Documentation patch warning of bug

comment:16 Changed 4 years ago by anonymous

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

comment:17 Changed 4 years ago by sailorfred@…

Has patch: set
Needs documentation: set

comment:18 Changed 4 years ago by Aymeric Augustin

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 Changed 4 years ago by Aymeric Augustin

Status: reopenednew

comment:20 Changed 3 years ago by pombredanne@…

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 Changed 3 years ago by Tomek Paczkowski

Cc: tomek@… added

comment:22 Changed 3 years ago by Stefano Apostolico

Cc: saxix.rome@… added

comment:23 Changed 3 years ago by Stefano Apostolico

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 3 years ago by Stefano Apostolico (previous) (diff)

comment:24 Changed 2 years ago by patjlm

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 2 years ago by patjlm (previous) (diff)

comment:25 Changed 2 years ago by patjlm

Cc: patjlm added

comment:26 Changed 2 years ago by patjlm

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 Changed 2 years ago by sailorfred

Severity: NormalRelease blocker

comment:28 Changed 2 years ago by Tim Graham

Severity: Release blockerNormal

comment:29 Changed 2 years ago by Zach Borboa

Cc: Zach Borboa added

comment:30 Changed 23 months ago by Carsten Fuchs

Cc: carsten.fuchs@… added

comment:31 Changed 23 months ago by Collin Anderson

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 Changed 17 months ago by Seth Gottlieb

Cc: Seth Gottlieb added

comment:33 Changed 13 months ago by Benjamin Phillips

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 Changed 13 months ago by Tim Graham

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 Changed 13 months ago by Benjamin Phillips

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 Changed 10 months ago by Tim Graham

Needs documentation: unset
Triage Stage: AcceptedReady for checkin

comment:37 Changed 10 months ago by Tim Graham <timograham@…>

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