#8419 closed (fixed)
Fix race in get_or_create() which can lead to update
Reported by: | 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)
Change History (11)
by , 16 years ago
Attachment: | get_or_create.race_patch added |
---|
by , 16 years ago
Attachment: | fix_get_or_create_race.diff added |
---|
comment:1 by , 16 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
by , 16 years ago
Attachment: | fix_get_or_create_race.v2.diff added |
---|
Same patch as before, updated for the docs refactoring of r8506
by , 16 years ago
Attachment: | option1-create-patch.diff added |
---|
Moving to create(), this enforces a similar force_insert=True - my preferred solution
by , 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 , 16 years ago
Attachment: | doc-improvement.diff added |
---|
Minor improvement to documentation of force_insert and force_update for save()
comment:2 by , 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 , 16 years ago
Resolution: | → fixed |
---|---|
Status: | new → 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 by , 16 years ago
how is this not controversial? get_or_create is now completely broken for manually specified primary keys.
Same patch as before, now in correct Django patch style