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

[Margie Roginski]

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

[Ramiro Morales]

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

[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

[Alex Gaynor]

Marked as dupe because of above.

[Margie Roginski]

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.

[Russell Keith-Magee]

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.

[James Bennett]

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

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

[Julien Phalip]

See #11652 for a related issue.

[Jacob]

Milestone 1.3 deleted

[Julien Phalip]

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

[sailorfred@…]

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.

[Aymeric Augustin]

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!

[sailorfred@…]

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.

[sailorfred@…]

Documentation patch warning of bug

[anonymous]

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

[Aymeric Augustin]

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.

[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?

[Stefano Apostolico]

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

[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

[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!

[Collin Anderson]

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

[Benjamin Phillips]

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.

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

[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

[Tim Graham <timograham@…>]

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.