Code

Opened 6 years ago

Closed 6 years ago

Last modified 3 years ago

#8419 closed (fixed)

Fix race in get_or_create() which can lead to update

Reported by: Richard Davies <richard.davies@…> Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Keywords:
Cc: richard.davies@… Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

There is a race condition in get_or_create - if the object is not present for the initial get(), but is swiftly created elsewhere, then the save() will update the object created elsewhere and return it with created=True.

This patch uses save(force_insert=True) to stop this from happening, and to drop through into the existing code that re-attempts the get(). I also update some of the documentation to make the behavior completely clear.

Note: Patch follows discussion with Malcolm at http://groups.google.com/group/django-developers/browse_thread/thread/179ed55b3bf44af0/067be6c02aba2bb3?hl=en#067be6c02aba2bb3
I am only submitting the patch to get_or_create(), which seemed uncontroversial. I personally believe that a similar force_insert=True should be used in create(), or if not then at least that force_insert and force_update flags should be available for create() as well as for save().

Attachments (6)

get_or_create.race_patch (1.5 KB) - added by Richard Davies <richard.davies@…> 6 years ago.
fix_get_or_create_race.diff (1.5 KB) - added by Richard Davies <richard.davies@…> 6 years ago.
Same patch as before, now in correct Django patch style
fix_get_or_create_race.v2.diff (1.5 KB) - added by Richard Davies <richard.davies@…> 6 years ago.
Same patch as before, updated for the docs refactoring of r8506
option1-create-patch.diff (783 bytes) - added by Richard Davies <richard.davies@…> 6 years ago.
Moving to create(), this enforces a similar force_insert=True - my preferred solution
option2-create-patch.diff (1.7 KB) - added by Richard Davies <richard.davies@…> 6 years ago.
Moving to create(), this makes the force_insert and force_update flags available - an alternative
doc-improvement.diff (995 bytes) - added by Richard Davies <richard.davies@…> 6 years ago.
Minor improvement to documentation of force_insert and force_update for save()

Download all attachments as: .zip

Change History (11)

Changed 6 years ago by Richard Davies <richard.davies@…>

Changed 6 years ago by Richard Davies <richard.davies@…>

Same patch as before, now in correct Django patch style

comment:1 Changed 6 years ago by jacob

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

Changed 6 years ago by Richard Davies <richard.davies@…>

Same patch as before, updated for the docs refactoring of r8506

Changed 6 years ago by Richard Davies <richard.davies@…>

Moving to create(), this enforces a similar force_insert=True - my preferred solution

Changed 6 years ago by Richard Davies <richard.davies@…>

Moving to create(), this makes the force_insert and force_update flags available - an alternative

Changed 6 years ago by Richard Davies <richard.davies@…>

Minor improvement to documentation of force_insert and force_update for save()

comment:2 Changed 6 years ago by Richard Davies <richard.davies@…>

I've just posted several patches, so I'll explain what they are!


The original fix_get_or_create_race.v2.diff is uncontroversial, fixes a race in get_or_create(), and should be applied regardless of everything that follows.


doc-improvement.diff is a minor suggestion to improve the documentation of the force_insert and force_update flags to save().


option1-create-patch.diff and option2-create-patch.diff are two alternative patches to create(). We should get the semantics of create() clear before 1.0, so a design decision is needed here.

  • Option 1 is my preferred solution, in which create() always performs an SQL insert and is documented as doing so. This seems the obvious meaning of the word "create"! I have run with this patch, which causes no problems for me.
  • Option 2 is an alternative, prompted by Malcolm's comments on Django developers. In this version create() continues to behave as at present - i.e. it usually performs an SQL insert, but may perform an SQL update in some cases. I add force_insert and force_update flags so that an advanced user can guarantee one or the other behavior and ensure that they will not lose data.

comment:3 Changed 6 years ago by mtredinnick

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

(In [8670]) Changed create() and get_or_create() to force an insert (not update an existing value).

Backwards incompatible if you are using manually-specific primary key values
and relying on the previously documented behaviour that the new values would
always exist in the database (i.e. it would update the existing entry).

Fixed #8419.

comment:4 Changed 6 years ago by anonymous

how is this not controversial? get_or_create is now completely broken for manually specified primary keys.

comment:5 Changed 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.