Merge lp:~wgrant/launchpad/poppy-transaction-unbreakage into lp:launchpad

[William Grant]

[r=stevenk,thumper][bug=733033] Ensure that poppy-sftp aborts the transaction after validating GPG signatures.

poppy-sftp's FTP handler checks the signature of uploaded changes files, but then fails to end the transaction. This branch wraps the check method in @read_transaction, ensuring that the transaction is aborted afterwards.

I also moved lp.bugs.externalbugtracker.isolation to lp.services.database.isolation, since it's generic and useful for the new test.

[Steve Kowalik (stevenk)]

This looks great to me.

review: Approve (code*)

[Tim Penhey (thumper)]

review: Approve (code)

=== modified file 'lib/lp/bugs/externalbugtracker/base.py'
--- lib/lp/bugs/externalbugtracker/base.py	2011-03-02 00:04:45 +0000
+++ lib/lp/bugs/externalbugtracker/base.py	2011-03-11 03:36:15 +0000
@@ -32,7 +32,6 @@
 
 from canonical.config import config
 from lp.bugs.adapters import treelookup
-from lp.bugs.externalbugtracker.isolation import ensure_no_transaction
 from lp.bugs.interfaces.bugtask import BugTaskStatus
 from lp.bugs.interfaces.externalbugtracker import (
     IExternalBugTracker,
@@ -40,6 +39,7 @@
     ISupportsCommentImport,
     ISupportsCommentPushing,
     )
+from lp.services.database.isolation import ensure_no_transaction
 
 # The user agent we send in our requests
 LP_USER_AGENT = "Launchpad Bugscraper/0.2 (https://bugs.launchpad.net/)"

=== modified file 'lib/lp/bugs/externalbugtracker/bugzilla.py'
--- lib/lp/bugs/externalbugtracker/bugzilla.py	2011-02-23 06:44:23 +0000
+++ lib/lp/bugs/externalbugtracker/bugzilla.py	2011-03-11 03:36:15 +0000
@@ -39,7 +39,6 @@
     UnparsableBugData,
     UnparsableBugTrackerVersion,
     )
-from lp.bugs.externalbugtracker.isolation import ensure_no_transaction
 from lp.bugs.externalbugtracker.xmlrpc import UrlLib2Transport
 from lp.bugs.interfaces.bugtask import (
     BugTaskImportance,
@@ -52,6 +51,7 @@
     UNKNOWN_REMOTE_IMPORTANCE,
     )
 from lp.services import encoding
+from lp.services.database.isolation import ensure_no_transaction
 
 
 class Bugzilla(ExternalBugTracker):

=== modified file 'lib/lp/bugs/externalbugtracker/debbugs.py'
--- lib/lp/bugs/externalbugtracker/debbugs.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/externalbugtracker/debbugs.py	2011-03-11 03:36:15 +0000
@@ -35,7 +35,6 @@
     InvalidBugId,
     UnknownRemoteStatusError,
     )
-from lp.bugs.externalbugtracker.isolation import ensure_no_transaction
 from lp.bugs.interfaces.bugtask import (
     BugTaskImportance,
     BugTaskStatus,
@@ -47,6 +46,7 @@
     UNKNOWN_REMOTE_IMPORTANCE,
     )
 from lp.bugs.scripts import debbugs
+from lp.services.database.isolation import ensure_no_transaction
 
 
 debbugsstatusmap = {'open':      BugTaskStatus.NEW,

=== modified file 'lib/lp/bugs/externalbugtracker/mantis.py'
--- lib/lp/bugs/externalbugtracker/mantis.py	2011-03-02 00:04:45 +0000
+++ lib/lp/bugs/externalbugtracker/mantis.py	2011-03-11 03:36:15 +0000
@@ -30,12 +30,12 @@
     UnknownRemoteStatusError,
     UnparsableBugData,
     )
-from lp.bugs.externalbugtracker.isolation import ensure_no_transaction
 from lp.bugs.interfaces.bugtask import (
     BugTaskImportance,
     BugTaskStatus,
     )
 from lp.bugs.interfaces.externalbugtracker import UNKNOWN_REMOTE_IMPORTANCE
+from lp.services.database.isolation import ensure_no_transaction
 from lp.services.propertycache import cachedproperty
 
 

=== modified file 'lib/lp/bugs/externalbugtracker/rt.py'
--- lib/lp/bugs/externalbugtracker/rt.py	2010-08-24 10:45:57 +0000
+++ lib/lp/bugs/externalbugtracker/rt.py	2011-03-11 03:36:15 +0000
@@ -20,9 +20,9 @@
     LookupTree,
     UnknownRemoteStatusError,
     )
-from lp.bugs.externalbugtracker.isolation import ensure_no_transaction
 from lp.bugs.interfaces.bugtask import BugTaskStatus
 from lp.bugs.interfaces.externalbugtracker import UNKNOWN_REMOTE_IMPORTANCE
+from lp.services.database.isolation import ensure_no_transaction
 from lp.services.propertycache import cachedproperty
 
 

=== modified file 'lib/lp/bugs/externalbugtracker/trac.py'
--- lib/lp/bugs/externalbugtracker/trac.py	2011-03-09 05:50:48 +0000
+++ lib/lp/bugs/externalbugtracker/trac.py	2011-03-11 03:36:15 +0000
@@ -33,7 +33,6 @@
     UnknownRemoteStatusError,
     UnparsableBugData,
     )
-from lp.bugs.externalbugtracker.isolation import ensure_no_transaction
 from lp.bugs.externalbugtracker.xmlrpc import UrlLib2Transport
 from lp.bugs.interfaces.bugtask import (
     BugTaskImportance,
@@ -45,6 +44,7 @@
     ISupportsCommentPushing,
     UNKNOWN_REMOTE_IMPORTANCE,
     )
+from lp.services.database.isolation import ensure_no_transaction
 
 # Symbolic constants used for the Trac LP plugin.
 LP_PLUGIN_BUG_IDS_ONLY = 0

=== modified file 'lib/lp/bugs/scripts/checkwatches/base.py'
--- lib/lp/bugs/scripts/checkwatches/base.py	2010-11-08 18:33:15 +0000
+++ lib/lp/bugs/scripts/checkwatches/base.py	2011-03-11 03:36:15 +0000
@@ -32,7 +32,7 @@
 from canonical.launchpad.webapp.interaction import setupInteraction
 from canonical.launchpad.webapp.interfaces import IPlacelessAuthUtility
 from lp.bugs.externalbugtracker import BugWatchUpdateWarning
-from lp.bugs.externalbugtracker.isolation import check_no_transaction
+from lp.services.database.isolation import check_no_transaction
 from lp.services.limitedlist import LimitedList
 
 # For OOPS reporting keep up to this number of SQL statements.

=== modified file 'lib/lp/bugs/scripts/checkwatches/tests/test_base.py'
--- lib/lp/bugs/scripts/checkwatches/tests/test_base.py	2010-12-20 03:21:03 +0000
+++ lib/lp/bugs/scripts/checkwatches/tests/test_base.py	2011-03-11 03:36:15 +0000
@@ -16,11 +16,11 @@
     queryInteraction,
     )
 from canonical.testing.layers import LaunchpadZopelessLayer
-from lp.bugs.externalbugtracker.isolation import (
+from lp.bugs.scripts.checkwatches.base import WorkingBase
+from lp.services.database.isolation import (
     is_transaction_in_progress,
     TransactionInProgress,
     )
-from lp.bugs.scripts.checkwatches.base import WorkingBase
 from lp.services.log.logger import BufferLogger
 from lp.testing import TestCaseWithFactory
 

=== modified file 'lib/lp/poppy/tests/test_twistedftp.py'
--- lib/lp/poppy/tests/test_twistedftp.py	2011-02-24 17:42:26 +0000
+++ lib/lp/poppy/tests/test_twistedftp.py	2011-03-11 03:36:15 +0000
@@ -10,6 +10,7 @@
 from testtools.deferredruntest import (
     AsynchronousDeferredRunTest,
     )
+import transaction
 from twisted.protocols import ftp
 from zope.component import getUtility
 
@@ -22,6 +23,7 @@
 from lp.registry.interfaces.gpg import (
     GPGKeyAlgorithm,
     IGPGKeySet)
+from lp.services.database.isolation import check_no_transaction
 from lp.testing import TestCaseWithFactory
 from lp.testing.keyserver import KeyServerTac
 
@@ -56,6 +58,8 @@
             algorithm=GPGKeyAlgorithm.items[key.algorithm],
             keysize=key.keysize, can_encrypt=key.can_encrypt,
             active=True)
+        # validateGPG runs in its own transaction.
+        transaction.commit()
 
         # Locate the directory with test files.
         self.test_files_dir = os.path.join(
@@ -92,3 +96,16 @@
             d = file_writer.close()
             d.addCallbacks(success_callback, error_callback)
             return d
+
+    def test_aborts_transaction(self):
+        valid_changes_file = os.path.join(
+            self.test_files_dir, "etherwake_1.08-1_source.changes")
+
+        def callback(result):
+            check_no_transaction()
+
+        with open(valid_changes_file) as opened_file:
+            file_writer = PoppyFileWriter(opened_file)
+            d = file_writer.close()
+            d.addBoth(callback)
+            return d

=== modified file 'lib/lp/poppy/twistedftp.py'
--- lib/lp/poppy/twistedftp.py	2011-02-25 16:36:38 +0000
+++ lib/lp/poppy/twistedftp.py	2011-03-11 03:36:15 +0000
@@ -33,6 +33,7 @@
 from lp.poppy.filesystem import UploadFileSystem
 from lp.poppy.hooks import Hooks
 from lp.registry.interfaces.gpg import IGPGKeySet
+from lp.services.database import read_transaction
 
 
 class PoppyAccessCheck:
@@ -156,6 +157,7 @@
                 return defer.fail(ftp.PermissionDeniedError(error))
         return defer.succeed(None)
 
+    @read_transaction
     def validateGPG(self, signed_file):
         """Check the GPG signature in the file referenced by signed_file.
 

=== renamed file 'lib/lp/bugs/externalbugtracker/isolation.py' => 'lib/lp/services/database/isolation.py'
=== renamed file 'lib/lp/bugs/externalbugtracker/tests/test_isolation.py' => 'lib/lp/services/database/tests/test_isolation.py'
--- lib/lp/bugs/externalbugtracker/tests/test_isolation.py	2011-01-22 17:57:02 +0000
+++ lib/lp/services/database/tests/test_isolation.py	2011-03-11 03:36:15 +0000
@@ -11,7 +11,7 @@
 from zope.component import getUtility
 
 from canonical.testing.layers import LaunchpadZopelessLayer
-from lp.bugs.externalbugtracker import isolation
+from lp.services.database import isolation
 from lp.testing import TestCase