Opened 20 months ago

Last modified 12 months 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: master
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 Changed 20 months ago by Dave Hall

Cc: dave@… added

comment:2 Changed 20 months ago by Tim Graham

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 Changed 20 months ago by Dave Hall

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

comment:4 Changed 19 months ago by Tim Graham

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

comment:5 Changed 19 months ago by Dave Hall

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

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

Triage Stage: UnreviewedSomeday/Maybe

comment:8 Changed 12 months ago by Jeppe Vesterbæk

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