Opened 7 years ago

Closed 7 years ago

Last modified 3 years ago

#7402 closed (fixed)

get_or_create can cause a transaction to fail silently

Reported by: samuel@… Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

In [7289] a change was made to get_or_create to work around a race condition; however, the change means that if the race condition occurs then the current transaction will fail.

When an IntegrityError is raised it cannot be ignored. The transaction must be rolled back. I believe the proper action in this case is to simply let the application itself deal with the IntegrityError as it is better equipped to handle it. That is, the exception should not be caught in get_or_create.

Attachments (1)

get_or_create.diff (963 bytes) - added by julianb 7 years ago.
Patch from nullie as proper .diff

Download all attachments as: .zip

Change History (8)

comment:1 Changed 7 years ago by nullie

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Following patch is a hack, but not more that concurrency problem solution is. It still can get messy with managed transactions (but we shouldn't have concurrency problems there, should we?)
Anyway, it still better to have somewhat working solution.

Index: django/db/models/query.py
===================================================================
--- django/db/models/query.py	(revision 7932)
+++ django/db/models/query.py	(working copy)
@@ -319,6 +319,7 @@
         Returns a tuple of (object, created), where created is a boolean
         specifying whether an object was created.
         """
+        
         assert kwargs, \
                 'get_or_create() must be passed at least one keyword argument'
         defaults = kwargs.pop('defaults', {})
@@ -332,7 +333,16 @@
                 obj.save()
                 return obj, True
             except IntegrityError, e:
-                return self.get(**kwargs), False
+                # Try to get object, maybe this was just concurrency error.
+                try:
+                    transaction.rollback_unless_managed()
+                    obj = self.get(**kwargs)
+                except self.model.DoesNotExist:
+                     # No object found, raise error
+                     raise e
+                 
+                return obj, False
+                
 
     def latest(self, field_name=None):
         """

comment:2 Changed 7 years ago by nullie

Here is better patch, it fail correctly in manual transaction mode.

Index: django/db/models/query.py
===================================================================
--- django/db/models/query.py	(revision 7932)
+++ django/db/models/query.py	(working copy)
@@ -319,6 +319,7 @@
         Returns a tuple of (object, created), where created is a boolean
         specifying whether an object was created.
         """
+        
         assert kwargs, \
                 'get_or_create() must be passed at least one keyword argument'
         defaults = kwargs.pop('defaults', {})
@@ -332,7 +333,20 @@
                 obj.save()
                 return obj, True
             except IntegrityError, e:
-                return self.get(**kwargs), False
+                # If transactions are managed manually, we must fail.
+                if transaction.is_managed():
+                    raise e
+
+                # Try to get object, maybe this was just concurrency error.
+                try:
+                    transaction.rollback_unless_managed()
+                    obj = self.get(**kwargs)
+                except self.model.DoesNotExist:
+                     # No object found, re-raise error.
+                     raise e
+                 
+                return obj, False
+                
 
     def latest(self, field_name=None):
         """

comment:3 Changed 7 years ago by julianb

  • milestone set to 1.0 beta
  • Triage Stage changed from Unreviewed to Accepted

I have the same problem, seems valid to work on that.
Is also related to #7789 where you get DoesNotExist, but IntegrityError is really the fault.

comment:4 Changed 7 years ago by nullie

[7289] opened a can of worms. Exact matches, unique constraints and transactions don't play well together.

It would be nice if we could interpret IntegrityError messages and decide what to do. Also, unique constraints should be case sensitive too.

Changed 7 years ago by julianb

Patch from nullie as proper .diff

comment:5 Changed 7 years ago by mtredinnick

  • Patch needs improvement set

This patch isn't correct. Rolling back a transaction and then trying to carry on as if nothing has happened is a bad idea. There could be any number of intended changes to the database that are now no longer going to be applied, so the caller's code has to know about that, since the whole situation has changed. We'll probably have to just back out the race condition handling here and let the caller catch the integrity error.

comment:6 Changed 7 years ago by mtredinnick

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

(In [8315]) Added savepoint protection to get_or_create() to avoid problems on PostgreSQL.
Fixed #7402.

Also made savepoint handling easier to use when wrapped around calls that might
commit a transaction. This is tested by the get_or_create tests.

comment:7 Changed 3 years ago by jacob

  • milestone 1.0 beta deleted

Milestone 1.0 beta deleted

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