Merge lp:~jtv/launchpad/fake-librarian into lp:launchpad

[Jeroen T. Vermeulen]

Fake Librarian.

= Fake Librarian =

This implements a fake librarian that we could use in tests.

The real librarian runs as a separate daemon that needs to be started and managed during test runs, taken up and down in some places (which takes ages), and sometimes manually killed in order to get things running again. It also writes files to the filesystem. In other words, it's lots of stuff we'd rather not have too much of in test runs.

In this branch I propose a simple fake librarian that could replace the real thing in many tests. It runs purely in-process and stores non-persistently, so it has none of that stuff that we don't like. Of course that also means it won't work for tests that fire off sub-processes.

To splice the fake librarian into a test's environment, e.g. in a cut-down LaunchpadLayer, we would ideally:
 * Create one global FakeLibrarian object.
 * Override the ILibrarianClient utility with the fake librarian.
 * Override the ILibraryFileAliasSet utility with the fake librarian.
 * Register the fake librarian as a transaction synchronizer.

The test would then use the fake librarian instead of the real one.

URLs and remote addition of files are not supported for now, so the fake librarian would be of little use in browser tests for now.

You'll also note that the fake librarian keeps track of whether a file has been committed to storage. This is an idiosyncrasy of the real librarian: any newly added files remain inaccessible until you commit. Stuart noted that it would be best to simulate the real librarian's behaviour here so that we don't hide nasty transaction-boundary bugs. Unlike the real librarian the fake one gives a reasonably helpful error message in this case. You'll also have the option of simulating a commit without actually forcing one on the test database.

The fake librarian uses the real database objects, just like the original. This means that it should still work when application code goes straight to the database, e.g. by joining or prejoining LibraryFileAlias and/or LibraryFileContent.

At the time of writing I'm sure we'll still want to play with this a bit before considering it for use in our test suite. I'm filing this as a Work-in-Progress MP.

Test with:
{{{
./bin/test -vvc -m lp.testing.tests.test_fakelibrarian
}}}

No lint.

Jeroen

[Curtis Hovey (sinzui)]

Thanks to doing this. I am very excited by this. I think it will help the test setup for project release files for the registry.

I have one concern. This will not work our of proc the way it is demonsdtrated. Like config and zcml registryations that happing in tests now, they do not affect an out-of-proc process. I think engineers should know via a comment in the FackLibrarian that it operates only in the current proc.

review: Approve (code)

[Gary Poster (gary)]

The changes since Curtis' approval look very good to me.

review: Approve

=== modified file 'lib/canonical/launchpad/browser/librarian.py'
--- lib/canonical/launchpad/browser/librarian.py	2010-08-04 20:13:18 +0000
+++ lib/canonical/launchpad/browser/librarian.py	2010-08-17 21:08:49 +0000
@@ -32,7 +32,7 @@
     IWebBrowserOriginatingRequest)
 from canonical.launchpad.webapp.url import urlappend
 from canonical.lazr.utils import get_current_browser_request
-from canonical.librarian.client import quote
+from canonical.librarian.client import url_path_quote
 from canonical.librarian.interfaces import LibrarianServerError
 from canonical.librarian.utils import filechunks, guess_librarian_encoding
 
@@ -216,5 +216,6 @@
         parent_url = canonical_url(self.parent, request=request)
         traversal_url = urlappend(parent_url, '+files')
         url = urlappend(
-            traversal_url, quote(self.context.filename.encode('utf-8')))
+            traversal_url,
+            url_path_quote(self.context.filename.encode('utf-8')))
         return url

=== modified file 'lib/canonical/librarian/client.py'
--- lib/canonical/librarian/client.py	2010-08-03 17:20:07 +0000
+++ lib/canonical/librarian/client.py	2010-08-17 21:08:49 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -6,9 +6,10 @@
 __all__ = [
     'FileDownloadClient',
     'FileUploadClient',
+    'get_libraryfilealias_download_path',
     'LibrarianClient',
     'RestrictedLibrarianClient',
-    'quote',
+    'url_path_quote',
     ]
 
 
@@ -34,6 +35,18 @@
     LIBRARIAN_SERVER_DEFAULT_TIMEOUT, LibrarianServerError, UploadFailed)
 
 
+def url_path_quote(filename):
+    """Quote `filename` for use in a URL."""
+    # XXX RobertCollins 2004-09-21: Perhaps filenames with / in them
+    # should be disallowed?
+    return urllib.quote(filename).replace('/', '%2F')
+
+
+def get_libraryfilealias_download_path(aliasID, filename):
+    """Download path for a given `LibraryFileAlias` id and filename."""
+    return '/%d/%s' % (int(aliasID), url_path_quote(filename))
+
+
 class FileUploadClient:
     """Simple blocking client for uploading to the librarian."""
 
@@ -226,18 +239,12 @@
             status, ids = response.split()
             contentID, aliasID = ids.split('/', 1)
 
-            path = '/%d/%s' % (int(aliasID), quote(name))
+            path = get_libraryfilealias_download_path(aliasID, name)
             return urljoin(self.download_url, path)
         finally:
             self._close()
 
 
-def quote(s):
-    # XXX: Robert Collins 2004-09-21: Perhaps filenames with / in them
-    # should be disallowed?
-    return urllib.quote(s).replace('/', '%2F')
-
-
 class _File:
     """A wrapper around a file like object that has security assertions"""
 
@@ -300,7 +307,8 @@
                 'Alias %d cannot be downloaded from this client.' % aliasID)
         if lfa.deleted:
             return None
-        return '/%d/%s' % (aliasID, quote(lfa.filename.encode('utf-8')))
+        return get_libraryfilealias_download_path(
+            aliasID, lfa.filename.encode('utf-8'))
 
     def getURLForAlias(self, aliasID):
         """Returns the url for talking to the librarian about the given

=== modified file 'lib/canonical/librarian/interfaces.py'
--- lib/canonical/librarian/interfaces.py	2010-04-20 14:27:26 +0000
+++ lib/canonical/librarian/interfaces.py	2010-08-17 21:08:49 +0000
@@ -1,9 +1,18 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # PyLint doesn't grok Zope interfaces.
 # pylint: disable-msg=E0213
 __metaclass__ = type
+__all__ = [
+    'DownloadFailed',
+    'IFileUploadClient',
+    'ILibrarianClient',
+    'IRestrictedLibrarianClient',
+    'LibrarianServerError',
+    'LIBRARIAN_SERVER_DEFAULT_TIMEOUT',
+    'UploadFailed',
+    ]
 
 import signal
 
@@ -35,14 +44,18 @@
 # LibrarianServerError.
 LIBRARIAN_SERVER_DEFAULT_TIMEOUT = 5
 
+
 class IFileUploadClient(Interface):
+    """Upload API for the Librarian client."""
+
     def addFile(name, size, file, contentType, expires=None):
         """Add a file to the librarian.
 
-        :param name: Name to store the file as
-        :param size: Size of the file
-        :param file: File-like object with the content in it
-        :param expires: Expiry time of file, or None to keep until unreferenced
+        :param name: Name to store the file as.
+        :param size: Size of the file.
+        :param file: File-like object with the content in it.
+        :param expires: Expiry time of file, or None to keep until
+            unreferenced.
 
         :raises UploadFailed: If the server rejects the upload for some reason
 
@@ -74,6 +87,8 @@
 
 
 class IFileDownloadClient(Interface):
+    """Download API for the Librarian client."""
+
     def getURLForAlias(aliasID):
         """Returns the URL to the given file"""
 
@@ -87,7 +102,7 @@
             LibrarianServerError is raised.
         :return: A file-like object to read the file contents from.
         :raises DownloadFailed: If the alias is not found.
-        :raises LibrarianServerError: If the librarain server is
+        :raises LibrarianServerError: If the librarian server is
             unreachable or returns an 5xx HTTPError.
         """
 
@@ -98,4 +113,3 @@
 
 class IRestrictedLibrarianClient(ILibrarianClient):
     """A version of the client that connects to a restricted librarian."""
-

=== modified file 'lib/canonical/librarian/storage.py'
--- lib/canonical/librarian/storage.py	2010-02-09 00:17:40 +0000
+++ lib/canonical/librarian/storage.py	2010-08-17 21:08:49 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -15,11 +15,18 @@
         IStoreSelector, MAIN_STORE, DEFAULT_FLAVOR)
 from canonical.librarian.db import write_transaction
 
-__all__ = ['DigestMismatchError', 'LibrarianStorage', 'LibraryFileUpload',
-           'DuplicateFileIDError', 'WrongDatabaseError',
-           # _relFileLocation needed by other modules in this package.
-           # Listed here to keep the import facist happy
-           '_relFileLocation', '_sameFile']
+__all__ = [
+    'DigestMismatchError',
+    'LibrarianStorage',
+    'LibraryFileUpload',
+    'DuplicateFileIDError',
+    'WrongDatabaseError',
+    # _relFileLocation needed by other modules in this package.
+    # Listed here to keep the import fascist happy
+    '_relFileLocation',
+    '_sameFile',
+    ]
+
 
 class DigestMismatchError(Exception):
     """The given digest doesn't match the SHA-1 digest of the file."""
@@ -31,6 +38,7 @@
 
 class WrongDatabaseError(Exception):
     """The client's database name doesn't match our database."""
+
     def __init__(self, clientDatabaseName, serverDatabaseName):
         Exception.__init__(self, clientDatabaseName, serverDatabaseName)
         self.clientDatabaseName = clientDatabaseName
@@ -40,8 +48,8 @@
 class LibrarianStorage:
     """Blob storage.
 
-    This manages the actual storage of files on disk and the record of those in
-    the database; it has nothing to do with the network interface to those
+    This manages the actual storage of files on disk and the record of those
+    in the database; it has nothing to do with the network interface to those
     files.
     """
 
@@ -109,11 +117,11 @@
             # the file really is removed or renamed, and can't possibly be
             # left in limbo
             os.remove(self.tmpfilepath)
-            raise DigestMismatchError, (self.srcDigest, dstDigest)
+            raise DigestMismatchError(self.srcDigest, dstDigest)
 
         try:
-            # If the client told us the name database of the database
-            # its using, check that it matches
+            # If the client told us the name of the database it's using,
+            # check that it matches.
             if self.databaseName is not None:
                 store = getUtility(IStoreSelector).get(
                         MAIN_STORE, DEFAULT_FLAVOR)
@@ -122,7 +130,8 @@
                 if self.databaseName != databaseName:
                     raise WrongDatabaseError(self.databaseName, databaseName)
 
-            self.debugLog.append('database name %r ok' % (self.databaseName,))
+            self.debugLog.append(
+                'database name %r ok' % (self.databaseName, ))
             # If we haven't got a contentID, we need to create one and return
             # it to the client.
             if self.contentID is None:
@@ -135,7 +144,7 @@
             else:
                 contentID = self.contentID
                 aliasID = None
-                self.debugLog.append('received contentID: %r' % (contentID,))
+                self.debugLog.append('received contentID: %r' % (contentID, ))
 
         except:
             # Abort transaction and re-raise

=== modified file 'lib/canonical/librarian/web.py'
--- lib/canonical/librarian/web.py	2009-11-22 22:27:12 +0000
+++ lib/canonical/librarian/web.py	2010-08-17 21:08:49 +0000
@@ -9,7 +9,7 @@
 from twisted.web import resource, static, util, server, proxy
 from twisted.internet.threads import deferToThread
 
-from canonical.librarian.client import quote
+from canonical.librarian.client import url_path_quote
 from canonical.librarian.db import read_transaction, write_transaction
 from canonical.librarian.utils import guess_librarian_encoding
 
@@ -163,7 +163,7 @@
     @read_transaction
     def _matchingAliases(self, digest):
         library = self.storage.library
-        matches = ['%s/%s' % (aID, quote(aName))
+        matches = ['%s/%s' % (aID, url_path_quote(aName))
                    for fID in library.lookupBySHA1(digest)
                    for aID, aName, aType in library.getAliases(fID)]
         return matches

=== added file 'lib/lp/testing/fakelibrarian.py'
--- lib/lp/testing/fakelibrarian.py	1970-01-01 00:00:00 +0000
+++ lib/lp/testing/fakelibrarian.py	2010-08-17 21:08:49 +0000
@@ -0,0 +1,201 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Fake, in-process implementation of the Librarian API.
+
+This works in-process only.  It does not support exchange of files
+between processes, or URL access.  Nor will it completely support all
+details of the Librarian interface.  But where it's enough, this
+provides a simple and fast alternative to the full Librarian in unit
+tests.
+"""
+
+__metaclass__ = type
+__all__ = [
+    'FakeLibrarian',
+    ]
+
+import hashlib
+from StringIO import StringIO
+from urlparse import urljoin
+
+import zope.component
+from zope.interface import implements
+import transaction
+from transaction.interfaces import ISynchronizer
+
+from canonical.config import config
+
+from canonical.launchpad.database.librarian import (
+    LibraryFileContent, LibraryFileAlias)
+from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
+from canonical.librarian.client import get_libraryfilealias_download_path
+from canonical.librarian.interfaces import (
+    ILibrarianClient,
+    LIBRARIAN_SERVER_DEFAULT_TIMEOUT)
+
+
+class InstrumentedLibraryFileAlias(LibraryFileAlias):
+    """A `ILibraryFileAlias` implementation that fakes library access."""
+
+    file_committed = False
+
+    def checkCommitted(self):
+        """Raise an error if this file has not been committed yet."""
+        if not self.file_committed:
+            raise LookupError(
+                "Attempting to retrieve file '%s' from the fake "
+                "librarian, but the file has not yet been committed to "
+                "storage." % self.filename)
+
+    def open(self, timeout=LIBRARIAN_SERVER_DEFAULT_TIMEOUT):
+        self.checkCommitted()
+        self._datafile = StringIO(self.content_string)
+
+    def read(self, chunksize=None, timeout=LIBRARIAN_SERVER_DEFAULT_TIMEOUT):
+        return self._datafile.read(chunksize)
+
+
+class FakeLibrarian(object):
+    """A fake, in-process Librarian.
+
+    This takes the role of both the librarian client and the LibraryFileAlias
+    utility.
+    """
+    provided_utilities = [ILibrarianClient, ILibraryFileAliasSet]
+    implements(ISynchronizer, *provided_utilities)
+
+    installed_as_librarian = False
+
+    def installAsLibrarian(self):
+        """Install this `FakeLibrarian` as the default Librarian."""
+        if self.installed_as_librarian:
+            return
+
+        transaction.manager.registerSynch(self)
+
+        # Original utilities that need to be restored.
+        self.original_utilities = {}
+
+        site_manager = zope.component.getGlobalSiteManager()
+        for utility in self.provided_utilities:
+            original = zope.component.getUtility(utility)
+            if site_manager.unregisterUtility(original, utility):
+                # We really disabled a utility, so remember to restore
+                # it later.  (Alternatively, the utility object might
+                # implement an interface that extends the utility one,
+                # in which case we should not restore it.)
+                self.original_utilities[utility] = original
+            zope.component.provideUtility(self, utility)
+
+        self.installed_as_librarian = True
+
+    def uninstall(self):
+        """Un-install this `FakeLibrarian` as the default Librarian."""
+        if not self.installed_as_librarian:
+            return
+
+        transaction.manager.unregisterSynch(self)
+
+        site_manager = zope.component.getGlobalSiteManager()
+        for utility in reversed(self.provided_utilities):
+            site_manager.unregisterUtility(self, utility)
+            original_utility = self.original_utilities.get(utility)
+            if original_utility is not None:
+                # We disabled a utility to get here; restore the
+                # original.  We do not do this for utilities that were
+                # implemented through interface inheritance, because in
+                # that case we would never have unregistered anything in
+                # the first place.  Re-registering would register the
+                # same object twice, for related but different
+                # interfaces.
+                zope.component.provideUtility(original_utility, utility)
+
+        self.installed_as_librarian = False
+
+    def __init__(self):
+        self.aliases = {}
+        self.download_url = config.librarian.download_url
+
+    def addFile(self, name, size, file, contentType, expires=None):
+        """See `IFileUploadClient`."""
+        content = file.read()
+        real_size = len(content)
+        if real_size != size:
+            raise AssertionError(
+                "Uploading '%s' to the fake librarian with incorrect "
+                "size %d; actual size is %d." % (name, size, real_size))
+
+        file_ref = self._makeLibraryFileContent(content)
+        alias = self._makeAlias(file_ref.id, name, content, contentType)
+        self.aliases[alias.id] = alias
+
+        return alias.id
+
+    def remoteAddFile(self, name, size, file, contentType, expires=None):
+        """See `IFileUploadClient`."""
+        return NotImplementedError()
+
+    def getURLForAlias(self, aliasID):
+        """See `IFileDownloadClient`."""
+        alias = self.aliases.get(aliasID)
+        path = get_libraryfilealias_download_path(aliasID, alias.filename)
+        return urljoin(self.download_url, path)
+
+    def getFileByAlias(self, aliasID,
+                       timeout=LIBRARIAN_SERVER_DEFAULT_TIMEOUT):
+        """See `IFileDownloadClient`."""
+        alias = self[aliasID]
+        alias.checkCommitted()
+        return StringIO(alias.content_string)
+
+    def _makeAlias(self, file_id, name, content, content_type):
+        """Create a `LibraryFileAlias`."""
+        alias = InstrumentedLibraryFileAlias(
+            contentID=file_id, filename=name, mimetype=content_type)
+        alias.content_string = content
+        return alias
+
+    def _makeLibraryFileContent(self, content):
+        """Create a `LibraryFileContent`."""
+        size = len(content)
+        sha1 = hashlib.sha1(content).hexdigest()
+        md5 = hashlib.md5(content).hexdigest()
+
+        content_object = LibraryFileContent(filesize=size, sha1=sha1, md5=md5)
+        return content_object
+
+    def create(self, name, size, file, contentType, expires=None,
+               debugID=None, restricted=False):
+        "See `ILibraryFileAliasSet`."""
+        return self.addFile(
+            name, size, file, contentType, expires=expires, debugID=debugID)
+
+    def __getitem__(self, key):
+        "See `ILibraryFileAliasSet`."""
+        alias = self.aliases.get(key)
+        if alias is None:
+            raise LookupError(
+                "Attempting to retrieve file alias %d from the fake "
+                "librarian, who has never heard of it." % key)
+        return alias
+
+    def findBySHA1(self, sha1):
+        "See `ILibraryFileAliasSet`."""
+        for alias in self.aliases.itervalues():
+            if alias.content.sha1 == sha1:
+                return alias
+
+        return None
+
+    def beforeCompletion(self, txn):
+        """See `ISynchronizer`."""
+
+    def afterCompletion(self, txn):
+        """See `ISynchronizer`."""
+        # Note that all files have been committed to storage.
+        for alias in self.aliases.itervalues():
+            alias.file_committed = True
+
+    def newTransaction(self, txn):
+        """See `ISynchronizer`."""

=== added file 'lib/lp/testing/tests/test_fakelibrarian.py'
--- lib/lp/testing/tests/test_fakelibrarian.py	1970-01-01 00:00:00 +0000
+++ lib/lp/testing/tests/test_fakelibrarian.py	2010-08-17 21:08:49 +0000
@@ -0,0 +1,119 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test the fake librarian."""
+
+__metaclass__ = type
+
+from StringIO import StringIO
+
+import transaction
+from transaction.interfaces import ISynchronizer
+from zope.component import getUtility
+
+from canonical.launchpad.database.librarian import LibraryFileAliasSet
+from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
+from canonical.librarian.client import LibrarianClient
+from canonical.librarian.interfaces import ILibrarianClient
+from canonical.launchpad.webapp.testing import verifyObject
+from canonical.testing import (
+    DatabaseFunctionalLayer, LaunchpadFunctionalLayer)
+from lp.testing import TestCaseWithFactory
+from lp.testing.fakelibrarian import FakeLibrarian
+
+
+class LibraryAccessScenarioMixin:
+    """Simple Librarian uses that can be serviced by the FakeLibrarian.
+
+    This tests the subset of the Librarian interface that is also
+    implemented by the FakeLibrarian.  If your test needs anything more
+    than this, then you want the real Librarian.
+    """
+
+    def _storeFile(self):
+        """Store a file in the `FakeLibrarian`.
+
+        :return: Tuple of filename, file contents, alias id.
+        """
+        name = self.factory.getUniqueString() + '.txt'
+        text = self.factory.getUniqueString()
+        alias_id = getUtility(ILibrarianClient).addFile(
+            name, len(text), StringIO(text), 'text/plain')
+        return name, text, alias_id
+
+    def test_baseline(self):
+        self.assertTrue(
+            verifyObject(
+                ILibrarianClient, getUtility(ILibrarianClient)))
+        self.assertTrue(
+            verifyObject(
+                ILibraryFileAliasSet, getUtility(ILibraryFileAliasSet)))
+
+    def test_insert_retrieve(self):
+        name, text, alias_id = self._storeFile()
+        self.assertIsInstance(alias_id, (int, long))
+
+        transaction.commit()
+
+        library_file = getUtility(ILibrarianClient).getFileByAlias(alias_id)
+        self.assertEqual(text, library_file.read())
+
+    def test_alias_set(self):
+        name, text, alias_id = self._storeFile()
+
+        retrieved_alias = getUtility(ILibraryFileAliasSet)[alias_id]
+
+        self.assertEqual(alias_id, retrieved_alias.id)
+        self.assertEqual(name, retrieved_alias.filename)
+
+    def test_read(self):
+        name, text, alias_id = self._storeFile()
+        transaction.commit()
+
+        retrieved_alias = getUtility(ILibraryFileAliasSet)[alias_id]
+        retrieved_alias.open()
+        self.assertEqual(text, retrieved_alias.read())
+
+    def test_uncommitted_file(self):
+        name, text, alias_id = self._storeFile()
+        retrieved_alias = getUtility(ILibraryFileAliasSet)[alias_id]
+        self.assertRaises(LookupError, retrieved_alias.open)
+
+    def test_incorrect_upload_size(self):
+        name = self.factory.getUniqueString()
+        text = self.factory.getUniqueString()
+        wrong_length = len(text) + 1
+        self.assertRaises(
+            AssertionError,
+            getUtility(ILibrarianClient).addFile,
+            name, wrong_length, StringIO(text), 'text/plain')
+
+
+class TestFakeLibrarian(LibraryAccessScenarioMixin, TestCaseWithFactory):
+    """Test the supported interface subset on the fake librarian."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestFakeLibrarian, self).setUp()
+        self.fake_librarian = FakeLibrarian()
+        self.fake_librarian.installAsLibrarian()
+
+    def tearDown(self):
+        super(TestFakeLibrarian, self).tearDown()
+        self.fake_librarian.uninstall()
+
+    def test_fake(self):
+        self.assertTrue(verifyObject(ISynchronizer, self.fake_librarian))
+        self.assertIsInstance(self.fake_librarian, FakeLibrarian)
+
+
+class TestRealLibrarian(LibraryAccessScenarioMixin, TestCaseWithFactory):
+    """Test the supported interface subset on the real librarian."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def test_real(self):
+        self.assertIsInstance(getUtility(ILibrarianClient), LibrarianClient)
+        self.assertIsInstance(
+            getUtility(ILibraryFileAliasSet), LibraryFileAliasSet)