launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-707741-addFile-master into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #707741 Translations export script failing.
  https://bugs.launchpad.net/bugs/707741

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-707741-addFile-master/+merge/47606

LibrarianClient.addFile does not like being wrapped in SlaveDatabasePolicy, causing rosetta-export-queue.py to crash like https://pastebin.canonical.com/42396 (bug #707741). This branch migrates addFile from the SQLObject compatibility layer to explicit use of the Storm master store, allowing it to work even when the slave store is the default.

Direct tests for the crash are impractical; they require a replicated environment, which tests are not. Instead I opted to close the slave store, so any code attempting to use it will crash.

I've tested this on both staging and a local replicated environment.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-707741-addFile-master/+merge/47606
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-707741-addFile-master into lp:launchpad.

=== modified file 'lib/canonical/librarian/client.py'
--- lib/canonical/librarian/client.py	2011-01-19 02:05:39 +0000
+++ lib/canonical/librarian/client.py	2011-01-26 23:41:44 +0000
@@ -33,7 +33,7 @@
 from zope.interface import implements
 
 from canonical.config import config, dbconfig
-from canonical.database.sqlbase import cursor
+from canonical.launchpad.interfaces.lpstorm import IMasterStore
 from canonical.librarian.interfaces import (
     DownloadFailed, ILibrarianClient, IRestrictedLibrarianClient,
     LIBRARIAN_SERVER_DEFAULT_TIMEOUT, LibrarianServerError, UploadFailed)
@@ -139,15 +139,15 @@
             # Get the name of the database the client is using, so that
             # the server can check that the client is using the same
             # database as the server.
-            cur = cursor()
-            databaseName = self._getDatabaseName(cur)
+            store = IMasterStore(LibraryFileAlias)
+            databaseName = self._getDatabaseName(store)
 
             # Generate new content and alias IDs.
             # (we'll create rows with these IDs later, but not yet)
-            cur.execute("SELECT nextval('libraryfilecontent_id_seq')")
-            contentID = cur.fetchone()[0]
-            cur.execute("SELECT nextval('libraryfilealias_id_seq')")
-            aliasID = cur.fetchone()[0]
+            contentID = store.execute(
+                "SELECT nextval('libraryfilecontent_id_seq')").get_one()[0]
+            aliasID = store.execute(
+                "SELECT nextval('libraryfilealias_id_seq')").get_one()[0]
 
             # Send command
             self._sendLine('STORE %d %s' % (size, name))
@@ -205,10 +205,8 @@
         finally:
             self._close()
 
-    def _getDatabaseName(self, cur):
-        cur.execute("SELECT current_database();")
-        databaseName = cur.fetchone()[0]
-        return databaseName
+    def _getDatabaseName(self, store):
+        return store.execute("SELECT current_database();").get_one()[0]
 
     def remoteAddFile(self, name, size, file, contentType, expires=None):
         """See `IFileUploadClient`."""

=== modified file 'lib/canonical/librarian/ftests/test_client.py'
--- lib/canonical/librarian/ftests/test_client.py	2010-11-06 12:50:22 +0000
+++ lib/canonical/librarian/ftests/test_client.py	2011-01-26 23:41:44 +0000
@@ -11,6 +11,8 @@
 from canonical.testing.layers import DatabaseLayer, LaunchpadFunctionalLayer
 from canonical.config import config
 from canonical.database.sqlbase import block_implicit_flushes
+from canonical.launchpad.interfaces.lpstorm import ISlaveStore
+from canonical.launchpad.webapp.dbpolicy import SlaveDatabasePolicy
 from canonical.librarian import client as client_module
 from canonical.librarian.client import (
     LibrarianClient, LibrarianServerError, RestrictedLibrarianClient)
@@ -92,6 +94,16 @@
         else:
             self.fail("UploadFailed not raised")
 
+    def test_addFile_uses_master(self):
+        # addFile is a write operation, so it should always use the
+        # master store, even if the slave is the default. Close the
+        # slave store and try to add a file, verifying that the master
+        # is used.
+        client = LibrarianClient()
+        ISlaveStore(LibraryFileAlias).close()
+        with SlaveDatabasePolicy():
+            client.addFile('sample.txt', 6, StringIO('sample'), 'text/plain')
+
     def test__getURLForDownload(self):
         # This protected method is used by getFileByAlias. It is supposed to
         # use the internal host and port rather than the external, proxied