Merge lp:~sinzui/launchpad/librarian-merge-proposal into lp:launchpad

[Curtis Hovey]

Allow merge proposals to operate without the launchpad librarian.

When the librarian is not available, merge proposal pages will not
load at all. We expect the librarian to not be available for as much
as an hour when it is upgraded to precises, but we want Lp to be
operational.

--------------------------------------------------------------------

RULES

    Pre-implementation: wgrant
    * Diff.text opens the LFA, self.diff_text.open(), without specifying
      the timeout.
      * the default timeout is 5 seconds, which is the entire time Lp
        allocates to a request. The 5s timeout was added when Lp had a
        30s request time in 2009. It is impossible for the timeout to be
        raised from with the webapp. Maybe the default should be 4 seconds.
        when the the webapp is making the request.
        LIBRARIAN_SERVER_DEFAULT_TIMEOUT = 5
      * Diff.text can pass a timout that is 1s or 5s minus the remaing
        time to ensure that the request time does not run out. After
        some discussion, we believe 2s is more than amp for the
        the librarian...anything requiring more than cannot be shown
        in the page.
      * The LibrarianServerError needs to be wrapped for the web service.
    * DiffRenderingMixin.preview_diff_text wraps the call to Diff.text to
      sanitise and recover from exceptions.
      * The property could catch the LibrarianServerError raise when the
        open attempts exceed the timeout.
      * The diff_oversized property just counts lines to return a bool.
        It could return True if preview_diff_text handled the timeout.

    ADDENDUM:
    * I had a fight with with the import fascist. I won, by removing
      all th ancient cruft, but had to give some ground by acknowledging
      that there are dubious imports in some browser modules.
    * There was more lint then expected in the old modules.

QA

    * Visit https://code.qastaging.launchpad.net/~sinzui/impressive/lirc-0/+merge/29161
    * Verify the page loads and that it states that the diff could not
      be retrieved at this time.

LINT

    lib/lp/code/browser/branchmergeproposal.py
    lib/lp/code/browser/tests/test_branchmergeproposal.py
    lib/lp/code/model/diff.py
    lib/lp/code/model/tests/test_diff.py
    lib/lp/code/templates/branchmergeproposal-diff.pt
    lib/lp/scripts/utilities/importfascist.py
    lib/lp/services/librarian/interfaces/client.py
    lib/lp/services/librarian/tests/test_client.py
    lib/lp/services/webapp/adapter.py

LoC

    I have more than 16,000 line credit.

TEST

    ./bin/test -vvc lp.code.browser.tests.test_branchmergeproposal
    ./bin/test -vvc lp.code.model.tests.test_diff
    ./bin/test -vvc lp.services.librarian.tests.test_client

IMPLEMENTATION

I updated view to try and recover from a failure to get the diff from
the librarian. The page will explain that the diff wasn't available, but
the user can reload or download it.
    lib/lp/code/browser/branchmergeproposal.py
    lib/lp/code/browser/tests/test_branchmergeproposal.py
    lib/lp/code/templates/branchmergeproposal-diff.pt

I changed the model to pass the timeout time to the LibrarianFileAlias. The
rules are a but complicated. Scripts run without timeouts. The webapp
should get the diff within 2 seconds -- otherwise something is very wrong.
If there is less that 2 seconds left in the request, the code shaves a bit
of time to set the timeout.
    lib/lp/services/webapp/adapter.py
    lib/lp/code/model/diff.py
    lib/lp/code/model/tests/test_diff.py

I exported the LibrarianServerError as a httplib.TIMEOUT error.
    lib/lp/services/librarian/interfaces/client.py
    lib/lp/services/librarian/tests/test_client.py

I removed all the checks for non-existent paths. The changes illustrate
how much Lp has changes since 2009. I re-enabled the database checks,
but the rules are clearly difficult to enforce. I added a list of
dubious imports so that the code continues to build even though
we think the design is wrong.
    lib/lp/scripts/utilities/importfascist.py

[j.c.sackett (jcsackett)]

With the fix of the typo and comments we discussed in IRC, this looks good.

Thanks.

review: Approve

=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py	2012-11-26 08:40:20 +0000
+++ lib/lp/code/browser/branchmergeproposal.py	2012-11-30 19:11:22 +0000
@@ -107,6 +107,7 @@
     Summary,
     Whiteboard,
     )
+from lp.services.librarian.interfaces.client import LibrarianServerError
 from lp.services.messages.interfaces.message import IMessageSet
 from lp.services.propertycache import cachedproperty
 from lp.services.webapp import (
@@ -517,9 +518,18 @@
 class DiffRenderingMixin:
     """A mixin class for handling diff text."""
 
+    @property
+    def diff_available(self):
+        """Is the preview diff available from the librarian?"""
+        if getattr(self, '_diff_available', None) is None:
+            # Load the cache so that the answer is known.
+            self.preview_diff_text
+        return self._diff_available
+
     @cachedproperty
     def preview_diff_text(self):
         """Return a (hopefully) intelligently encoded review diff."""
+        self._diff_available = True
         preview_diff = self.preview_diff
         if preview_diff is None:
             return None
@@ -527,6 +537,9 @@
             diff = preview_diff.text.decode('utf-8')
         except UnicodeDecodeError:
             diff = preview_diff.text.decode('windows-1252', 'replace')
+        except LibrarianServerError:
+            self._diff_available = False
+            diff = ''
         # Strip off the trailing carriage returns.
         return diff.rstrip('\n')
 

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2012-10-08 07:15:07 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2012-11-30 19:11:22 +0000
@@ -57,6 +57,7 @@
     PersonVisibility,
     TeamMembershipPolicy,
     )
+from lp.services.librarian.interfaces.client import LibrarianServerError
 from lp.services.messages.model.message import MessageSet
 from lp.services.webapp import canonical_url
 from lp.services.webapp.interfaces import (
@@ -69,6 +70,7 @@
     BrowserTestCase,
     feature_flags,
     login_person,
+    monkey_patch,
     person_logged_in,
     set_feature_flag,
     TestCaseWithFactory,
@@ -865,6 +867,7 @@
         view = create_initialized_view(self.bmp, '+index')
         self.assertEqual(diff_bytes.decode('utf-8'),
                          view.preview_diff_text)
+        self.assertTrue(view.diff_available)
 
     def test_preview_diff_all_chars(self):
         """preview_diff should work on diffs containing all possible bytes."""
@@ -875,6 +878,26 @@
         view = create_initialized_view(self.bmp, '+index')
         self.assertEqual(diff_bytes.decode('windows-1252', 'replace'),
                          view.preview_diff_text)
+        self.assertTrue(view.diff_available)
+
+    def test_preview_diff_timeout(self):
+        # The preview_diff will recover from a timeout set to get the
+        # librarian content.
+        text = ''.join(chr(x) for x in range(255))
+        diff_bytes = ''.join(unified_diff('', text))
+        self.setPreviewDiff(diff_bytes)
+        transaction.commit()
+
+        def fake_open(*args):
+            raise LibrarianServerError
+
+        lfa = removeSecurityProxy(self.bmp.preview_diff).diff.diff_text
+        with monkey_patch(lfa, open=fake_open):
+            view = create_initialized_view(self.bmp.preview_diff, '+diff')
+            self.assertEqual('', view.preview_diff_text)
+            self.assertFalse(view.diff_available)
+            markup = view()
+            self.assertIn('The diff is not available at this time.', markup)
 
     def setPreviewDiff(self, preview_diff_bytes):
         preview_diff = PreviewDiff.create(

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2012-09-19 01:19:35 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2012-11-30 19:11:22 +0000
@@ -415,7 +415,7 @@
         elif status == BranchMergeProposalStatus.MERGE_FAILED:
             self._transitionToState(status, user=user)
         else:
-            raise AssertionError('Unexpected queue status: ' % status)
+            raise AssertionError('Unexpected queue status: %s' % status)
 
     def setAsWorkInProgress(self):
         """See `IBranchMergeProposal`."""

=== modified file 'lib/lp/code/model/diff.py'
--- lib/lp/code/model/diff.py	2012-10-02 01:25:48 +0000
+++ lib/lp/code/model/diff.py	2012-11-30 19:11:22 +0000
@@ -51,10 +51,14 @@
 from lp.services.database.bulk import load_referencing
 from lp.services.database.sqlbase import SQLBase
 from lp.services.librarian.interfaces import ILibraryFileAliasSet
+from lp.services.librarian.interfaces.client import (
+    LIBRARIAN_SERVER_DEFAULT_TIMEOUT,
+    )
 from lp.services.propertycache import (
     cachedproperty,
     get_property_cache,
     )
+from lp.services.webapp.adapter import get_request_remaining_seconds
 
 
 class Diff(SQLBase):
@@ -94,12 +98,32 @@
         if self.diff_text is None:
             return ''
         else:
-            self.diff_text.open()
+            self.diff_text.open(self._getDiffTimeout())
             try:
                 return self.diff_text.read(config.diff.max_read_size)
             finally:
                 self.diff_text.close()
 
+    def _getDiffTimeout(self):
+        """Return the seconds allocated to get the diff from the librarian.
+
+         the value will be Non for scripts, 2 for the webapp, or if thre is
+         little request time left, the number will be smaller or equal to
+         the remaining request time.
+        """
+        remaining = get_request_remaining_seconds()
+        if remaining is None:
+            return LIBRARIAN_SERVER_DEFAULT_TIMEOUT
+        elif remaining > 2.0:
+            # The maximum permitted time for webapp requests.
+            return 2.0
+        elif remaining > 0.01:
+            # Shave off 1 hundreth of a second off so that the call site
+            # has a chance to recover.
+            return remaining - 0.01
+        else:
+            return remaining
+
     @property
     def oversized(self):
         # If the size of the content of the librarian file is over the

=== modified file 'lib/lp/code/model/tests/test_diff.py'
--- lib/lp/code/model/tests/test_diff.py	2012-01-01 02:58:52 +0000
+++ lib/lp/code/model/tests/test_diff.py	2012-11-30 19:11:22 +0000
@@ -29,11 +29,15 @@
     PreviewDiff,
     )
 from lp.code.model.directbranchcommit import DirectBranchCommit
+from lp.services.librarian.interfaces.client import (
+    LIBRARIAN_SERVER_DEFAULT_TIMEOUT,
+    )
 from lp.services.webapp import canonical_url
 from lp.services.webapp.testing import verifyObject
 from lp.testing import (
     login,
     login_person,
+    monkey_patch,
     TestCaseWithFactory,
     )
 from lp.testing.layers import (
@@ -172,6 +176,30 @@
         diff = self._create_diff(content)
         self.assertTrue(diff.oversized)
 
+    def test_getDiffTimeout(self):
+        # The time permitted to get the diff from the librarian may be None,
+        # or 2. If there is not 2 seconds left in the request, the number will
+        # will 0.01 smaller or the actual remaining time.
+        content = ''.join(unified_diff('', "1234567890" * 10))
+        diff = self._create_diff(content)
+        value = None
+
+        def fake():
+            return value
+
+        from lp.code.model import diff as diff_module
+        with monkey_patch(diff_module, get_request_remaining_seconds=fake):
+            self.assertIs(
+                LIBRARIAN_SERVER_DEFAULT_TIMEOUT, diff._getDiffTimeout())
+            value = 3.1
+            self.assertEqual(2.0, diff._getDiffTimeout())
+            value = 1.11
+            self.assertEqual(1.1, diff._getDiffTimeout())
+            value = 0.11
+            self.assertEqual(0.1, diff._getDiffTimeout())
+            value = 0.01
+            self.assertEqual(0.01, diff._getDiffTimeout())
+
 
 class TestDiffInScripts(DiffTestCase):
 

=== modified file 'lib/lp/code/templates/branchmergeproposal-diff.pt'
--- lib/lp/code/templates/branchmergeproposal-diff.pt	2010-02-17 19:10:51 +0000
+++ lib/lp/code/templates/branchmergeproposal-diff.pt	2012-11-30 19:11:22 +0000
@@ -17,6 +17,10 @@
       </div>
       <div class="boardCommentBody attachmentBody">
         <tal:diff replace="structure view/preview_diff_text/fmt:diff" />
+        <tal:diff-not-available condition="not: view/diff_available">
+            The diff is not available at this time. You can reload the
+            page or download it.
+        </tal:diff-not-available>
       </div>
       <div class="boardCommentFooter"
            tal:condition="view/diff_oversized">

=== modified file 'lib/lp/scripts/utilities/importfascist.py'
--- lib/lp/scripts/utilities/importfascist.py	2012-01-19 17:23:59 +0000
+++ lib/lp/scripts/utilities/importfascist.py	2012-11-30 19:11:22 +0000
@@ -9,7 +9,6 @@
 
 
 original_import = __builtin__.__import__
-database_root = 'canonical.launchpad.database'
 naughty_imports = set()
 
 # Silence bogus warnings from Hardy's python-pkg-resources package.
@@ -22,39 +21,6 @@
     return set(line.strip() for line in text.splitlines() if line.strip())
 
 
-permitted_database_imports = text_lines_to_set("""
-    canonical.archivepublisher.deathrow
-    canonical.archivepublisher.domination
-    canonical.archivepublisher.ftparchive
-    canonical.archivepublisher.publishing
-    lp.codehosting.inmemory
-    canonical.launchpad.browser.branchlisting
-    lp.code.browser.branchlisting
-    lp.services.librarian.browser
-    canonical.launchpad.feed.branch
-    lp.code.feed.branch
-    lp.scripts.garbo
-    lp.bugs.vocabularies
-    lp.registry.interfaces.person
-    lp.registry.vocabularies
-    lp.services.worlddata.vocabularies
-    lp.soyuz.vocabularies
-    lp.translations.vocabularies
-    lp.services.librarian.client
-    lp.services.librarianserver.db
-    doctest
-    """)
-
-
-warned_database_imports = text_lines_to_set("""
-    lp.soyuz.scripts.obsolete_distroseries
-    lp.soyuz.scripts.gina.handlers
-    lp.registry.browser.distroseries
-    lp.translations.scripts.po_import
-    lp.systemhomes
-    """)
-
-
 # Sometimes, third-party modules don't export all of their public APIs through
 # __all__. The following dict maps from such modules to a list of attributes
 # that are allowed to be imported, whether or not they are in __all__.
@@ -76,27 +42,51 @@
     }
 
 
+unsafe_parts = set(['browser', 'feed', 'xmlrpc', 'widgets'])
+
+dubious = [
+    'lp.answers.browser.question',
+    'lp.app.browser.vocabulary',
+    'lp.blueprints.browser.sprint',
+    'lp.bugs.browser.bug',
+    'lp.bugs.browser.bugalsoaffects',
+    'lp.bugs.browser.bugsubscription',
+    'lp.bugs.browser.bugtarget',
+    'lp.bugs.browser.bugtask',
+    'lp.bugs.browser.person',
+    'lp.code.browser.branchlisting',
+    'lp.code.browser.sourcepackagerecipe',
+    'lp.registry.browser.distroseries',
+    'lp.registry.browser.distroseriesdifference',
+    'lp.registry.browser.milestone',
+    'lp.registry.browser.pillar',
+    'lp.registry.browser.person',
+    'lp.registry.browser.project',
+    'lp.registry.browser.sourcepackage',
+    'lp.soyuz.browser.archive',
+    'lp.soyuz.browser.builder',
+    'lp.soyuz.browser.queue',
+    'lp.translations.browser.potemplate',
+    'lp.translations.browser.serieslanguage',
+    'lp.translations.browser.sourcepackage',
+    'lp.translations.browser.translationlinksaggregator',
+    'lp.translations.browser.translationtemplatesbuild',
+    ]
+
+
 def database_import_allowed_into(module_path):
-    """Return True if database code is allowed to be imported into the given
-    module path.  Otherwise, returns False.
+    """Return True if model code can be imported into the module path.
 
     It is allowed if:
         - The import was made with the __import__ hook.
-        - The importer is from within canonical.launchpad.database.
         - The importer is a 'test' module.
-        - The importer is in the set of permitted_database_imports.
-        - The importer is within a model module or package.
-
-    Note that being in the set of warned_database_imports does not make
-    the import allowed.
-
+        - The importer is in a nodule that does not face users.
+        - The import is recognised to be dubious, but not a priority to fix.
     """
-    if (module_path == '__import__ hook' or
-        module_path.startswith('canonical.launchpad.database') or
-        '.model' in module_path or
-        is_test_module(module_path)):
-        return True
-    return module_path in permitted_database_imports
+    module_parts = set(module_path.split('.'))
+    return (unsafe_parts.isdisjoint(module_parts)
+        or is_test_module(module_path)
+        or module_path in dubious)
 
 
 def is_test_module(module_path):
@@ -105,9 +95,7 @@
     Otherwise returns False.
     """
     name_splitted = module_path.split('.')
-    return ('tests' in name_splitted or
-            'ftests' in name_splitted or
-            'testing' in name_splitted)
+    return ('tests' in name_splitted or 'testing' in name_splitted)
 
 
 class attrsgetter:
@@ -166,9 +154,7 @@
 
 
 class NotFoundPolicyViolation(JackbootError):
-    """import of zope.exceptions.NotFoundError into
-    canonical.launchpad.database.
-    """
+    """import of zope.exceptions.NotFoundError into lp models modules."""
 
     def __init__(self, import_into):
         JackbootError.__init__(self, import_into, '')
@@ -182,7 +168,6 @@
 # The names of the arguments form part of the interface of __import__(...),
 # and must not be changed, as code may choose to invoke __import__ using
 # keyword arguments - e.g. the encodings module in Python 2.6.
-# pylint: disable-msg=W0102,W0602
 def import_fascist(name, globals={}, locals={}, fromlist=[], level=-1):
     global naughty_imports
 
@@ -223,25 +208,17 @@
         import_into = '__import__ hook'
 
     # Check the "NotFoundError" policy.
-    if (import_into.startswith('canonical.launchpad.database') and
-        name == 'zope.exceptions'):
+    if ('.model.' in import_into and name == 'zope.exceptions'):
         if fromlist and 'NotFoundError' in fromlist:
             raise NotFoundPolicyViolation(import_into)
 
     # Check the database import policy.
-    if (name.startswith(database_root) and
-        not database_import_allowed_into(import_into)):
+    if '.model.' in name and not database_import_allowed_into(import_into):
         error = DatabaseImportPolicyViolation(import_into, name)
         naughty_imports.add(error)
-        # Raise an error except in the case of browser.traversers.
-        # This exception to raising an error is only temporary, until
-        # browser.traversers is cleaned up.
-        if import_into not in warned_database_imports:
-            raise error
 
     # Check the import from __all__ policy.
-    if fromlist is not None and (
-        import_into.startswith('canonical') or import_into.startswith('lp')):
+    if fromlist is not None and import_into.startswith('lp'):
         # We only want to warn about "from foo import bar" violations in our
         # own code.
         fromlist = list(fromlist)

=== modified file 'lib/lp/services/librarian/interfaces/client.py'
--- lib/lp/services/librarian/interfaces/client.py	2011-12-30 01:14:33 +0000
+++ lib/lp/services/librarian/interfaces/client.py	2012-11-30 19:11:22 +0000
@@ -14,8 +14,10 @@
     'UploadFailed',
     ]
 
+import httplib
 import signal
 
+from lazr.restful.declarations import error_status
 from zope.interface import Interface
 
 
@@ -35,6 +37,7 @@
     pass
 
 
+@error_status(httplib.REQUEST_TIMEOUT)
 class LibrarianServerError(Exception):
     """An error indicating that the Librarian server is not responding."""
 
@@ -91,7 +94,7 @@
 
     def getURLForAlias(aliasID, secure=False):
         """Returns the URL to the given file.
-        
+
         :param aliasID: The LibraryFileAlias for the file to get. A DB lookup
             will be done for this - if many are to be calculated, eagar loading
             is recommended.

=== modified file 'lib/lp/services/librarian/tests/test_client.py'
--- lib/lp/services/librarian/tests/test_client.py	2012-09-28 06:01:02 +0000
+++ lib/lp/services/librarian/tests/test_client.py	2012-11-30 19:11:22 +0000
@@ -2,6 +2,7 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from cStringIO import StringIO
+import httplib
 import textwrap
 import unittest
 from urllib2 import (
@@ -25,8 +26,10 @@
 from lp.services.librarian.model import LibraryFileAlias
 from lp.testing.layers import (
     DatabaseLayer,
+    FunctionalLayer,
     LaunchpadFunctionalLayer,
     )
+from lp.testing.views import create_webservice_error_view
 
 
 class InstrumentedLibrarianClient(LibrarianClient):
@@ -36,12 +39,14 @@
         self.check_error_calls = 0
 
     sentDatabaseName = False
+
     def _sendHeader(self, name, value):
         if name == 'Database-Name':
             self.sentDatabaseName = True
         return LibrarianClient._sendHeader(self, name, value)
 
     called_getURLForDownload = False
+
     def _getURLForDownload(self, aliasID):
         self.called_getURLForDownload = True
         return LibrarianClient._getURLForDownload(self, aliasID)
@@ -79,7 +84,7 @@
     def test_addFileSendsDatabaseName(self):
         # addFile should send the Database-Name header.
         client = InstrumentedLibrarianClient()
-        id1 = client.addFile(
+        client.addFile(
             'sample.txt', 6, StringIO('sample'), 'text/plain')
         self.failUnless(client.sentDatabaseName,
             "Database-Name header not sent by addFile")
@@ -91,7 +96,7 @@
         # different process, we need to explicitly tell the DatabaseLayer to
         # fully tear down and set up the database.
         DatabaseLayer.force_dirty_database()
-        id1 = client.remoteAddFile('sample.txt', 6, StringIO('sample'),
+        client.remoteAddFile('sample.txt', 6, StringIO('sample'),
                                    'text/plain')
         self.failUnless(client.sentDatabaseName,
             "Database-Name header not sent by remoteAddFile")
@@ -180,7 +185,7 @@
             # If the alias has been deleted, _getURLForDownload returns None.
             lfa = LibraryFileAlias.get(alias_id)
             lfa.content = None
-            call = block_implicit_flushes( # Prevent a ProgrammingError
+            call = block_implicit_flushes(  # Prevent a ProgrammingError
                 LibrarianClient._getURLForDownload)
             self.assertEqual(call(client, alias_id), None)
         finally:
@@ -216,7 +221,7 @@
             # If the alias has been deleted, _getURLForDownload returns None.
             lfa = LibraryFileAlias.get(alias_id)
             lfa.content = None
-            call = block_implicit_flushes( # Prevent a ProgrammingError
+            call = block_implicit_flushes(  # Prevent a ProgrammingError
                 RestrictedLibrarianClient._getURLForDownload)
             self.assertEqual(call(client, alias_id), None)
         finally:
@@ -231,7 +236,7 @@
         client = InstrumentedLibrarianClient()
         alias_id = client.addFile(
             'sample.txt', 6, StringIO('sample'), 'text/plain')
-        transaction.commit() # Make sure the file is in the "remote" database
+        transaction.commit()  # Make sure the file is in the "remote" database.
         self.failIf(client.called_getURLForDownload)
         # (Test:)
         f = client.getFileByAlias(alias_id)
@@ -306,3 +311,13 @@
             client.getFileByAlias(alias_id), 'This is a fake file object', 3)
 
         client_module._File = _File
+
+
+class TestWebServiceErrors(unittest.TestCase):
+    """ Test that errors are correctly mapped to HTTP status codes."""
+
+    layer = FunctionalLayer
+
+    def test_LibrarianServerError_timeout(self):
+        error_view = create_webservice_error_view(LibrarianServerError())
+        self.assertEqual(httplib.REQUEST_TIMEOUT, error_view.status)

=== modified file 'lib/lp/services/webapp/adapter.py'
--- lib/lp/services/webapp/adapter.py	2012-11-14 09:27:39 +0000
+++ lib/lp/services/webapp/adapter.py	2012-11-30 19:11:22 +0000
@@ -89,6 +89,7 @@
     'RequestExpired',
     'set_request_started',
     'clear_request_started',
+    'get_request_remaining_seconds',
     'get_request_statements',
     'get_request_start_time',
     'get_request_duration',