Opened 7 years ago

Closed 6 months ago

#11313 closed Bug (fixed)

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

Reported by: margieroginski Owned by: nobody
Component: contrib.admin Version: 1.1-beta
Severity: Normal Keywords: model formsets
Cc: poswald, sailorfred@…, tomek@…, saxix.rome@…, patjlm, zachborboa, carsten.fuchs@…, cmawebsite@…, sggottlieb, 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


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 is not None:
                qs = qs.exclude(

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.


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

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

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

comment:2 Changed 7 years ago by margieroginski

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.


comment:3 Changed 7 years ago by Alex

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

Marked as dupe because of above.

comment:4 Changed 7 years ago by margieroginski

  • Resolution duplicate deleted
  • Status changed from closed to reopened

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 russellm

  • milestone set to 1.2
  • Triage Stage changed from Unreviewed to Accepted

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 6 years ago by ubernostrum

  • milestone changed from 1.2 to 1.3

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

comment:7 Changed 6 years ago by andybak

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

  • Severity set to Normal
  • Type set to New feature

comment:9 Changed 5 years ago by julien

See #11652 for a related issue.

comment:10 Changed 5 years ago by poswald

  • Cc poswald added
  • Easy pickings unset

comment:11 Changed 5 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:12 Changed 5 years ago by julien

  • 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 changed from Normal to Release blocker
  • Type changed from New feature to Bug
  • Version changed from 1.1-beta-1 to 1.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 aaugustin

  • Severity changed from Release blocker to Normal
  • Type changed from Bug to New feature
  • Version changed from 1.4 to 1.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 changed from Normal to Release blocker
  • Type changed from New feature to Bug

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@…

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 aaugustin

  • Severity changed from Release blocker to Normal

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 3 years ago by aaugustin

  • Status changed from reopened to new

comment:20 Changed 3 years ago by pombredanne@…

The approach would be imho to integrate something like 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 oinopion

  • Cc tomek@… added

comment:22 Changed 3 years ago by saxix

  • Cc saxix.rome@… added

comment:23 Changed 3 years ago by saxix

django-concurrency 0.6dev supports list_editable and admin's actions.
Info at

Last edited 3 years ago by saxix (previous) (diff)

comment:24 Changed 22 months 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.



Last edited 22 months ago by patjlm (previous) (diff)

comment:25 Changed 22 months ago by patjlm

  • Cc patjlm added

comment:26 Changed 22 months ago by patjlm

Looking very quickly into the code, I think that this can be solved by replacing the following in contrib/admin/ 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)


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?


comment:27 Changed 21 months ago by sailorfred

  • Severity changed from Normal to Release blocker

comment:28 Changed 21 months ago by timgraham

  • Severity changed from Release blocker to Normal

comment:29 Changed 21 months ago by zachborboa

  • Cc zachborboa added

comment:30 Changed 19 months ago by CarstenF

  • Cc carsten.fuchs@… added

comment:31 Changed 18 months ago by collinanderson

  • 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 13 months ago by sggottlieb

  • Cc sggottlieb added

comment:33 Changed 9 months ago by benred42

  • 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 9 months ago by timgraham

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 8 months ago by benred42

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

comment:36 Changed 6 months ago by timgraham

  • Needs documentation unset
  • Triage Stage changed from Accepted to Ready for checkin

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

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

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