Opened 7 years ago

Last modified 7 years ago

#27477 new New feature

Use QuerySet.select_for_update() in admin change form to fix race condition

Reported by: Dave Hall Owned by: nobody
Component: contrib.admin Version: dev
Severity: Normal Keywords: admin
Cc: dave@…, Jeppe Vesterbæk Triage Stage: Someday/Maybe
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

There is a race condition in the admin change form where a user can edit a model instance at the same time as another user. Consider the following two operations occurring in parallel:

  1. ModelAdmin.change_view()
  2. User.objects.update(is_superuser=True)

If (1) load the model instance, then (2) runs, then (1) saves the model instance, the update in (2) will be lost.

The solution is to call select_for_update on the queryset in the changeform_view() method of ModelAdmin.

Change History (8)

comment:1 by Dave Hall, 7 years ago

Cc: dave@… added

comment:2 by Tim Graham, 7 years ago

I don't know if this is correct or if it might have some unintended side effects. Is the same issue present in the generic class-based editing views, for example?

comment:3 by Dave Hall, 7 years ago

Yes, the same issue is also present in the class-based generic edit views.

comment:4 by Tim Graham, 7 years ago

Can you provide a patch so we can get feedback on it?

comment:5 by Dave Hall, 7 years ago

Sure, will do. Probably this week.

It's going to be hard to do this in a way that doesn't hurt backwards compatibility. There are a few options we can do:

  1. Make the get_object() method take a for_update boolean keyword arg. This will have backwards-compatibility problems.
  2. Make the get_object() method call select_for_update() on the queryset if the HTTP method is POST, PUT, PATCH or DELETE. This may result in spurious select_for_update() calls on the queryset, but is backwards compatible.
  3. Make the get_queryset() method call select_for_update() on the queryset if the HTTP method is POST, PUT, PATCH or DELETE. This may result in spurious select_for_update() calls on the queryset, but is backwards compatible.

I think that (2) or (3) is the correct approach. I'm erring on the side of (3), since it'll also help with admin bulk actions.

All approaches can be applied to the admin admin generic views. Do people have a preference as to the approach?

comment:6 by Tim Graham, 7 years ago

If you don't get feedback here, I'd post to the DevelopersMailingList as that reaches a wider audience for design decisions like this.

comment:7 by Tim Graham, 7 years ago

Triage Stage: UnreviewedSomeday/Maybe

comment:8 by Jeppe Vesterbæk, 7 years ago

Cc: Jeppe Vesterbæk added
Note: See TracTickets for help on using tickets.
Back to Top