list_editable will update wrong rows on a multiuser system if pagination is enabled

[emilianodelvalle@…]

If there is a model with fields which are "list editable" and the number of rows for that model enables pagination, an update on those fields could not work as expected (other rows are updated).

1. Lets say that the user updates fields on the first page on the change list view.
2. While saving those changes, the information on the database changes (another user or another process added rows that should be shown before the rows updated by the user, shifting the rows modified by the user to a second page).
3. Django will update the editable fields on the newest rows with the data entered for other rows.

I debugged the code and I think that the bug is on the BaseModelFormSet. For some reason, if it can't retrieve a instance model using the pk, it will use the index to retrieve it. However, the queryset that it's using has been limited by pagination. Therefore if the row updated has been moved to another page, it won't be found.

This belongs to BaseModelFormSet, take a look at the final if on the _construct_form method.

    def _existing_object(self, pk):
        if not hasattr(self, '_object_dict'):
            self._object_dict = dict([(o.pk, o) for o in self.get_queryset()])
        return self._object_dict.get(pk)

    def _construct_form(self, i, **kwargs):
        if self.is_bound and i < self.initial_form_count():
            # Import goes here instead of module-level because importing
            # django.db has side effects.
            from django.db import connections
            pk_key = "%s-%s" % (self.add_prefix(i), self.model._meta.pk.name)
            pk = self.data[pk_key]
            pk_field = self.model._meta.pk
            pk = pk_field.get_db_prep_lookup('exact', pk,
                connection=connections[self.get_queryset().db])
            if isinstance(pk, list):
                pk = pk[0]
            kwargs['instance'] = self._existing_object(pk)
        if i < self.initial_form_count() and not kwargs.get('instance'):
            kwargs['instance'] = self.get_queryset()[i]
        return super(BaseModelFormSet, self)._construct_form(i, **kwargs)

[Julien Phalip]

Note that currently list_editable isn't well suited for multiuser environment (see #11313).

However, I think the particular problem you're running into is that there is no consistent default ordering set on the changelist. Please take a look at #16819 and see whether this ticket is a duplicate of that one.

[anonymous]

No, it's still happening even with ordering.

Let me explain how I'm testing it:

1. I built a simple model. It has an id (primary key) and a name (char field).
2. I made an admin for that model with name as list_editable and a page size = 3.
3. I run the server on debug mode with a break point at the begining the ModelAdmin.changelist_view method.
4. I opened the app and I edited the three elements on the first page.
5. The debug stopped at the breakpoint and I added three new elements to the model table using sql. The three elements are meant to be shown before the three elements edited.
6. I resume my submit.

The result is that the three new elements are shown in the first page with its name changed. The other three elements have not been edited at all.

I don't understand why if it has the primary key on the list editable form, django is not using it to recover the element to edit. I'm trying the following solution (so far it's working):

class FixedBaseModelFormSet(BaseModelFormSet):
    def _existing_object(self, pk):
        if not hasattr(self, '_object_dict'):
            self._object_dict = dict([(o.pk, o) for o in self.get_queryset()])
        object = self._object_dict.get(pk)
        if object is None:
            object = self.model._default_manager.get_query_set().get(pk = pk)
        return object

By the way, if list_editable isn't multithread safe, it would be good to specify it on the help: ​https://docs.djangoproject.com/en/dev/ref/contrib/admin/#django.contrib.admin.ModelAdmin.list_editable

[Julien Phalip]

Thanks a lot for taking the time to detail out all the steps to reproduce this problem. Here the _object_dict cache is used for performance reasons but obviously it isn't thread-safe, at least in the context of list_editable.

However, I think this is just one particular issue among the several that would need to be addressed as part of #11313. If we can fix this then we should fix it as a whole rather than piecemeal. For this reason I'll close this ticket as a duplicate of #11313. Let's continue the conversation over there.

By the way, you've made a good point about the documentation. A warning should be added indicating that list_editable isn't thread safe. I'll add a comment about that in the other ticket. Thank you!

[chad.lyon@…]

That work around doesn't work but is really close. I think you want to avoid using the query set altogether when list editing because it could possibly return different results than it did when the list to be edited was POSTed. This actually works as a work around:

def _existing_object(self, pk):
        return self.model._default_manager.get_query_set().get(pk = pk)

...but has side effects. For example, what if another user deletes one of the items in the list to be edited? This workaround will raise DoesNotExist. Django committers need to get busy on the solution talked about in #11313 because this bug is really ugly.