From b3259fd9ec9ed43f8e2c1e023b249cc545ba1060 Mon Sep 17 00:00:00 2001 From: Ashald Date: Fri, 4 Oct 2013 11:31:28 +0300 Subject: [PATCH 1/3] Fixed bug that caused KeyError in johnny.transaction.TransactionManager._commit_savepoint - issue #30 on Github (#69 on Bitbucket) This patch is fixing cause of the problem, not hiding it as pull requests #26 and #30. To reproduce cause of the problem you should: 1) Enable QueryCacheMiddleware 2) Work in unmanaged transaction state (it means autocommit mode, i.e. without 'django.middleware.transaction.TransactionMiddleware' and not within 'transaction.enter_transaction_management()' block) 3) Create a savepoint and then try to save some object (it is done by 'django.db.models.query.QuerySet#get_or_create' or by 'django.contrib.sessions.backends.db.SessionStore#save') The essence of the problem is the fact that Django ORM widely uses a 'django/db/transaction.py#commit_unless_managed' (which is unpatched by Johny Cache) function instead of 'django/db/transaction.py#commit_unless_managed' (which is patched by Johny Cache). So, when we working in unmanaged transaction state Johny Cache will still register savepoints upon creation but will be unable to handle commits and rollbacks that wast performed by 'unless_managed' functions. Since Django is using dumb algorithm for savepoint names generation (they reuse same names within thread once they were commited) it will lead to a situation when Johny Cache will register several savepoints with same name within one transaction and fail when it will try to commit or rollback them. Proposed solution adds patching to 'django/db/transaction.py#commit_unless_managed' and 'django/db/transaction.py#rollback_unless_managed' functions in the same way as it was done for 'django/db/transaction.py#commit' and 'django/db/transaction.py#rollback' functions with one difference - additional check whether current transaction is unmanaged. --- johnny/transaction.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/johnny/transaction.py b/johnny/transaction.py index 146276d..b5969a5 100755 --- a/johnny/transaction.py +++ b/johnny/transaction.py @@ -119,12 +119,15 @@ def _flush(self, commit=True, using=None): self._clear(using) self._clear_sid_stack(using) - def _patched(self, original, commit=True): + def _patched(self, original, commit=True, unless_managed=False): @wraps(original, assigned=available_attrs(original)) def newfun(using=None): #1.2 version original(using=using) - self._flush(commit=commit, using=using) + # copying behavior of original func + # if it is an 'unless_managed' version we should do nothing is transaction is managed + if not unless_managed or not django_transaction.is_managed(): + self._flush(commit=commit, using=using) return newfun @@ -269,12 +272,18 @@ def patch(self): """ if not self._patched_var: self._originals['rollback'] = self._getreal('rollback') + self._originals['rollback_unless_managed'] = self._getreal('rollback_unless_managed') self._originals['commit'] = self._getreal('commit') + self._originals['commit_unless_managed'] = self._getreal('commit_unless_managed') self._originals['savepoint'] = self._getreal('savepoint') self._originals['savepoint_rollback'] = self._getreal('savepoint_rollback') self._originals['savepoint_commit'] = self._getreal('savepoint_commit') django_transaction.rollback = self._patched(django_transaction.rollback, False) + django_transaction.rollback_unless_managed = self._patched(django_transaction.rollback_unless_managed, + False, unless_managed=True) django_transaction.commit = self._patched(django_transaction.commit, True) + django_transaction.commit_unless_managed = self._patched(django_transaction.commit_unless_managed, + True, unless_managed=True) django_transaction.savepoint = self._savepoint(django_transaction.savepoint) django_transaction.savepoint_rollback = self._savepoint_rollback(django_transaction.savepoint_rollback) django_transaction.savepoint_commit = self._savepoint_commit(django_transaction.savepoint_commit) From 5fb24098e9c0afef4ab08bc8abfa5811b0fc2862 Mon Sep 17 00:00:00 2001 From: Ashald Date: Fri, 4 Oct 2013 12:22:33 +0300 Subject: [PATCH 2/3] Little update in 'is transaction managed' checking logic --- johnny/transaction.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/johnny/transaction.py b/johnny/transaction.py index b5969a5..08ceffd 100755 --- a/johnny/transaction.py +++ b/johnny/transaction.py @@ -126,7 +126,7 @@ def newfun(using=None): original(using=using) # copying behavior of original func # if it is an 'unless_managed' version we should do nothing is transaction is managed - if not unless_managed or not django_transaction.is_managed(): + if not unless_managed or not self.is_managed(using=using): self._flush(commit=commit, using=using) return newfun From e046ac3e917c4eeaf602cda3ca1fe89f176412bf Mon Sep 17 00:00:00 2001 From: Ashald Date: Fri, 4 Oct 2013 12:23:48 +0300 Subject: [PATCH 3/3] Comment typo fix --- johnny/transaction.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/johnny/transaction.py b/johnny/transaction.py index 08ceffd..4c37674 100755 --- a/johnny/transaction.py +++ b/johnny/transaction.py @@ -125,7 +125,7 @@ def newfun(using=None): #1.2 version original(using=using) # copying behavior of original func - # if it is an 'unless_managed' version we should do nothing is transaction is managed + # if it is an 'unless_managed' version we should do nothing if transaction is managed if not unless_managed or not self.is_managed(using=using): self._flush(commit=commit, using=using)