Opened 16 years ago

Closed 16 years ago

Last modified 13 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: dev
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: no UI/UX: no

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@…> 16 years ago.
fix_get_or_create_race.diff (1.5 KB ) - added by Richard Davies <richard.davies@…> 16 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@…> 16 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@…> 16 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@…> 16 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@…> 16 years ago.
Minor improvement to documentation of force_insert and force_update for save()

Download all attachments as: .zip

Change History (11)

by Richard Davies <richard.davies@…>, 16 years ago

Attachment: get_or_create.race_patch added

by Richard Davies <richard.davies@…>, 16 years ago

Attachment: fix_get_or_create_race.diff added

Same patch as before, now in correct Django patch style

comment:1 by Jacob, 16 years ago

Triage Stage: UnreviewedDesign decision needed

by Richard Davies <richard.davies@…>, 16 years ago

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

by Richard Davies <richard.davies@…>, 16 years ago

Attachment: option1-create-patch.diff added

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

by Richard Davies <richard.davies@…>, 16 years ago

Attachment: option2-create-patch.diff added

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

by Richard Davies <richard.davies@…>, 16 years ago

Attachment: doc-improvement.diff added

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

comment:2 by Richard Davies <richard.davies@…>, 16 years ago

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 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: newclosed

(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 by anonymous, 16 years ago

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

comment:5 by Jacob, 13 years ago

milestone: 1.0

Milestone 1.0 deleted

Note: See TracTickets for help on using tickets.
Back to Top