launchpad-reviewers team mailing list archive

[Jonathan Lange <jml@xxxxxxxxx>]

Jonathan Lange has proposed merging lp:~julian-edwards/launchpad/move-write-transaction into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)


This moves the write_transaction and read_transaction methods to lp.services, where they belong.
-- 
https://code.launchpad.net/~julian-edwards/launchpad/move-write-transaction/+merge/36625
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/move-write-transaction into lp:launchpad/devel.

=== modified file 'lib/canonical/librarian/db.py'
--- lib/canonical/librarian/db.py	2010-09-06 09:48:41 +0000
+++ lib/canonical/librarian/db.py	2010-09-25 09:13:49 +0000
@@ -10,15 +10,10 @@
     'write_transaction',
     ]
 
-from psycopg2.extensions import TransactionRollbackError
 from sqlobject.sqlbuilder import AND
-from storm.exceptions import DisconnectionError, IntegrityError
 from storm.expr import SQL
-import transaction
-from twisted.python.util import mergeFunctionMetadata
 
 from canonical.database.sqlbase import (
-    reset_store,
     session_store,
     )
 from canonical.launchpad.database.librarian import (
@@ -28,65 +23,6 @@
     )
 
 
-RETRY_ATTEMPTS = 3
-
-
-def retry_transaction(func):
-    """Decorator used to retry database transaction failures.
-
-    The function being decorated should not have side effects outside
-    of the transaction.
-    """
-    def retry_transaction_decorator(*args, **kwargs):
-        attempt = 0
-        while True:
-            attempt += 1
-            try:
-                return func(*args, **kwargs)
-            except (DisconnectionError, IntegrityError,
-                    TransactionRollbackError), exc:
-                if attempt >= RETRY_ATTEMPTS:
-                    raise # tried too many times
-    return mergeFunctionMetadata(func, retry_transaction_decorator)
-
-
-def read_transaction(func):
-    """Decorator used to run the function inside a read only transaction.
-
-    The transaction will be aborted on successful completion of the
-    function.  The transaction will be retried if appropriate.
-    """
-    @reset_store
-    def read_transaction_decorator(*args, **kwargs):
-        transaction.begin()
-        try:
-            return func(*args, **kwargs)
-        finally:
-            transaction.abort()
-    return retry_transaction(mergeFunctionMetadata(
-        func, read_transaction_decorator))
-
-
-def write_transaction(func):
-    """Decorator used to run the function inside a write transaction.
-
-    The transaction will be committed on successful completion of the
-    function, and aborted on failure.  The transaction will be retried
-    if appropriate.
-    """
-    @reset_store
-    def write_transaction_decorator(*args, **kwargs):
-        transaction.begin()
-        try:
-            ret = func(*args, **kwargs)
-        except:
-            transaction.abort()
-            raise
-        transaction.commit()
-        return ret
-    return retry_transaction(mergeFunctionMetadata(
-        func, write_transaction_decorator))
-
 
 class Library:
     """Class that encapsulates the database interface for the librarian."""

=== modified file 'lib/canonical/librarian/ftests/test_db.py'
--- lib/canonical/librarian/ftests/test_db.py	2010-09-06 14:45:01 +0000
+++ lib/canonical/librarian/ftests/test_db.py	2010-09-25 09:13:49 +0000
@@ -44,8 +44,8 @@
         self.assertEqual('text/unknown', alias.mimetype)
 
 
-class TestTransactionDecorators(unittest.TestCase):
-    """Tests for the transaction decorators used by the librarian."""
+class TestLibrarianStuff(unittest.TestCase):
+    """Tests for the librarian."""
 
     layer = LaunchpadZopelessLayer
 
@@ -150,59 +150,6 @@
             ]
         self.assertEqual(expected_aliases, aliases)
 
-    def test_read_transaction_reset_store(self):
-        """Make sure that the store is reset after the transaction."""
-        @db.read_transaction
-        def no_op():
-            pass
-        no_op()
-        self.failIf(
-            self.file_content is self._getTestFileContent(),
-            "Store wasn't reset properly.")
-
-    def test_write_transaction_reset_store(self):
-        """Make sure that the store is reset after the transaction."""
-        @db.write_transaction
-        def no_op():
-            pass
-        no_op()
-        self.failIf(
-            self.file_content is self._getTestFileContent(),
-            "Store wasn't reset properly.")
-
-    def test_write_transaction_reset_store_with_raise(self):
-        """Make sure that the store is reset after the transaction."""
-        @db.write_transaction
-        def no_op():
-            raise RuntimeError('an error occured')
-        self.assertRaises(RuntimeError, no_op)
-        self.failIf(
-            self.file_content is self._getTestFileContent(),
-            "Store wasn't reset properly.")
-
-    def test_writing_transaction_reset_store_on_commit_failure(self):
-        """The store should be reset even if committing the transaction fails.
-        """
-        class TransactionAborter:
-            """Make the next commit() fails."""
-            def newTransaction(self, txn):
-                pass
-
-            def beforeCompletion(self, txn):
-                raise RuntimeError('the commit will fail')
-        aborter = TransactionAborter()
-        transaction.manager.registerSynch(aborter)
-        try:
-            @db.write_transaction
-            def no_op():
-                pass
-            self.assertRaises(RuntimeError, no_op)
-            self.failIf(
-                self.file_content is self._getTestFileContent(),
-                "Store wasn't reset properly.")
-        finally:
-            transaction.manager.unregisterSynch(aborter)
-
 
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/canonical/librarian/storage.py'
--- lib/canonical/librarian/storage.py	2010-09-05 21:53:21 +0000
+++ lib/canonical/librarian/storage.py	2010-09-25 09:13:49 +0000
@@ -13,7 +13,7 @@
 
 from canonical.launchpad.webapp.interfaces import (
         IStoreSelector, MAIN_STORE, DEFAULT_FLAVOR)
-from canonical.librarian.db import write_transaction
+from lp.services.database import write_transaction
 
 __all__ = [
     'DigestMismatchError',

=== modified file 'lib/canonical/librarian/web.py'
--- lib/canonical/librarian/web.py	2010-09-06 08:33:15 +0000
+++ lib/canonical/librarian/web.py	2010-09-25 09:13:49 +0000
@@ -12,7 +12,10 @@
 
 from canonical.config import config
 from canonical.librarian.client import url_path_quote
-from canonical.librarian.db import read_transaction, write_transaction
+from lp.services.database import (
+    read_transaction,
+    write_transaction,
+    )
 from canonical.librarian.utils import guess_librarian_encoding
 
 

=== modified file 'lib/lp/buildmaster/manager.py'
--- lib/lp/buildmaster/manager.py	2010-09-20 10:21:32 +0000
+++ lib/lp/buildmaster/manager.py	2010-09-25 09:13:49 +0000
@@ -32,7 +32,7 @@
 
 from canonical.config import config
 from canonical.launchpad.webapp import urlappend
-from canonical.librarian.db import write_transaction
+from lp.services.database import write_transaction
 from lp.buildmaster.enums import BuildStatus
 from lp.services.twistedsupport.processmonitor import ProcessWithTimeout
 

=== modified file 'lib/lp/services/database/__init__.py'
--- lib/lp/services/database/__init__.py	2009-07-17 02:25:09 +0000
+++ lib/lp/services/database/__init__.py	2010-09-25 09:13:49 +0000
@@ -4,5 +4,77 @@
 """The lp.services.database package."""
 
 __metaclass__ = type
-__all__ = []
+__all__ = [
+    'read_transaction',
+    'write_transaction',
+    ]
+
+from psycopg2.extensions import TransactionRollbackError
+from storm.exceptions import DisconnectionError, IntegrityError
+import transaction
+from twisted.python.util import mergeFunctionMetadata
+
+from canonical.database.sqlbase import (
+    reset_store,
+    )
+
+
+RETRY_ATTEMPTS = 3
+
+
+def retry_transaction(func):
+    """Decorator used to retry database transaction failures.
+
+    The function being decorated should not have side effects outside
+    of the transaction.
+    """
+    def retry_transaction_decorator(*args, **kwargs):
+        attempt = 0
+        while True:
+            attempt += 1
+            try:
+                return func(*args, **kwargs)
+            except (DisconnectionError, IntegrityError,
+                    TransactionRollbackError), exc:
+                if attempt >= RETRY_ATTEMPTS:
+                    raise # tried too many times
+    return mergeFunctionMetadata(func, retry_transaction_decorator)
+
+
+def read_transaction(func):
+    """Decorator used to run the function inside a read only transaction.
+
+    The transaction will be aborted on successful completion of the
+    function.  The transaction will be retried if appropriate.
+    """
+    @reset_store
+    def read_transaction_decorator(*args, **kwargs):
+        transaction.begin()
+        try:
+            return func(*args, **kwargs)
+        finally:
+            transaction.abort()
+    return retry_transaction(mergeFunctionMetadata(
+        func, read_transaction_decorator))
+
+
+def write_transaction(func):
+    """Decorator used to run the function inside a write transaction.
+
+    The transaction will be committed on successful completion of the
+    function, and aborted on failure.  The transaction will be retried
+    if appropriate.
+    """
+    @reset_store
+    def write_transaction_decorator(*args, **kwargs):
+        transaction.begin()
+        try:
+            ret = func(*args, **kwargs)
+        except:
+            transaction.abort()
+            raise
+        transaction.commit()
+        return ret
+    return retry_transaction(mergeFunctionMetadata(
+        func, write_transaction_decorator))
 

=== added file 'lib/lp/services/database/tests/test_transaction_decorators.py'
--- lib/lp/services/database/tests/test_transaction_decorators.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/database/tests/test_transaction_decorators.py	2010-09-25 09:13:49 +0000
@@ -0,0 +1,92 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+import unittest
+
+import transaction
+from zope.component import getUtility
+
+from lp.services.database import (
+    read_transaction,
+    write_transaction,
+    )
+from canonical.launchpad.database.librarian import LibraryFileContent
+from canonical.launchpad.webapp.interfaces import (
+        IStoreSelector, MAIN_STORE, DEFAULT_FLAVOR)
+from canonical.librarian import db
+from canonical.testing import LaunchpadZopelessLayer
+
+
+class TestTransactionDecorators(unittest.TestCase):
+    """Tests for the transaction decorators used by the librarian."""
+
+    layer = LaunchpadZopelessLayer
+
+    def setUp(self):
+        self.layer.switchDbUser('librarian')
+        self.store = getUtility(IStoreSelector).get(
+                MAIN_STORE, DEFAULT_FLAVOR)
+        self.content_id = db.Library().add('deadbeef', 1234, 'abababab')
+        self.file_content = self._getTestFileContent()
+        transaction.commit()
+
+    def _getTestFileContent(self):
+        """Return the file content object that created."""
+        return self.store.find(LibraryFileContent, id=self.content_id).one()
+
+    def test_read_transaction_reset_store(self):
+        """Make sure that the store is reset after the transaction."""
+        @read_transaction
+        def no_op():
+            pass
+        no_op()
+        self.failIf(
+            self.file_content is self._getTestFileContent(),
+            "Store wasn't reset properly.")
+
+    def test_write_transaction_reset_store(self):
+        """Make sure that the store is reset after the transaction."""
+        @write_transaction
+        def no_op():
+            pass
+        no_op()
+        self.failIf(
+            self.file_content is self._getTestFileContent(),
+            "Store wasn't reset properly.")
+
+    def test_write_transaction_reset_store_with_raise(self):
+        """Make sure that the store is reset after the transaction."""
+        @write_transaction
+        def no_op():
+            raise RuntimeError('an error occured')
+        self.assertRaises(RuntimeError, no_op)
+        self.failIf(
+            self.file_content is self._getTestFileContent(),
+            "Store wasn't reset properly.")
+
+    def test_writing_transaction_reset_store_on_commit_failure(self):
+        """The store should be reset even if committing the transaction fails.
+        """
+        class TransactionAborter:
+            """Make the next commit() fails."""
+            def newTransaction(self, txn):
+                pass
+
+            def beforeCompletion(self, txn):
+                raise RuntimeError('the commit will fail')
+        aborter = TransactionAborter()
+        transaction.manager.registerSynch(aborter)
+        try:
+            @write_transaction
+            def no_op():
+                pass
+            self.assertRaises(RuntimeError, no_op)
+            self.failIf(
+                self.file_content is self._getTestFileContent(),
+                "Store wasn't reset properly.")
+        finally:
+            transaction.manager.unregisterSynch(aborter)
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)