get_or_create can cause a transaction to fail silently

[samuel@…]

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.

[nullie]

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):
         """

[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):
         """

[Julian Bez]

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.

[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.

[Julian Bez]

Patch from nullie as proper .diff

[Malcolm Tredinnick]

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.

[Malcolm Tredinnick]

(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.

[Jacob]

Milestone 1.0 beta deleted