Merge lp:~sinzui/launchpad/search-oopses-1 into lp:launchpad

[Curtis Hovey]

Suppress oopses from the remote Google search service.

    Launchpad bug: https://bugs.launchpad.net/bugs/267852
    Pre-implementation: no one
    Test command: ./bin/test -vv \
      -t launchpad-search-pages -m lp.services.search.tests.test_google

There are several kinds of oopses
    OOPS-1552A1017 (URLError: <urlopen error (110, 'Connection timed out')
    OOPS-979H877 (IndexError: list index out of range)
    OOPS-1539B1178 (HTTPError: HTTP Error 500: Internal Server Error)
that emanate from GoogleSearchService. It should catch these
errors and raise GoogleResponseError which is handled by the view.

LaunchpadSearchView should not report GoogleResponseError as an oopses
because we know there is nothing in Lp's code to fix.

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

RULES

    * Remove the oops reporting block from LaunchpadSearchView.
    * Convert the connection/response/incomplete-data errors to
      GoogleResponseError.

QA

    * None

LINT

    lib/lp/app/browser/root.py
    lib/lp/app/browser/tests/launchpad-search-pages.txt
    lib/lp/services/search/google.py
    lib/lp/services/search/tests/test_google.py

IMPLEMENTATION

Removed the oops reporting block from LaunchpadSearchView.
    lib/lp/app/browser/root.py
    lib/lp/app/browser/tests/launchpad-search-pages.txt

Converted the connection/response/data errors to GoogleResponseError.
    lib/lp/services/search/google.py
    lib/lp/services/search/tests/test_google.py

[Benji York (benji)]

This branch looks good.

review: Approve (code)

=== modified file 'buildout.cfg'
--- buildout.cfg	2011-03-04 17:05:09 +0000
+++ buildout.cfg	2011-03-07 16:35:45 +0000
@@ -55,7 +55,7 @@
     from lp_sitecustomize import main
     main('${configuration:instance_name}') # Initializes LP environment.
 entry-points = stxdocs=zope.configuration.stxdocs:main
-    googletestservice=lp.services.search.googletestservice:main
+    googletestservice=lp.services.googlesearch.googletestservice:main
     windmill=windmill.bin.windmill_bin:main
     lp-windmill=lp.scripts.utilities.lpwindmill:main
     jsbuild=lazr.js.build:main

=== modified file 'lib/canonical/config/schema-lazr.conf'
--- lib/canonical/config/schema-lazr.conf	2011-03-05 00:06:26 +0000
+++ lib/canonical/config/schema-lazr.conf	2011-03-07 16:35:45 +0000
@@ -903,10 +903,10 @@
 # Run a web service stub that simulates the Google search service.
 
 # Where are our canned XML responses stored?
-canned_response_directory: lib/lp/services/search/tests/data/
+canned_response_directory: lib/lp/services/googlesearch/tests/data/
 
 # Which file maps service URLs to the XML that the server returns?
-mapfile: lib/lp/services/search/tests/data/mapping.txt
+mapfile: lib/lp/services/googlesearch/tests/data/mapping.txt
 
 # Where should the service log files live?
 log: logs/google-stub.log

=== modified file 'lib/canonical/launchpad/scripts/runlaunchpad.py'
--- lib/canonical/launchpad/scripts/runlaunchpad.py	2011-03-04 17:05:09 +0000
+++ lib/canonical/launchpad/scripts/runlaunchpad.py	2011-03-07 16:35:45 +0000
@@ -24,7 +24,7 @@
     )
 from lp.services.mailman import runmailman
 from lp.services.osutils import ensure_directory_exists
-from lp.services.search import googletestservice
+from lp.services.googlesearch import googletestservice
 
 
 def make_abspath(path):

=== modified file 'lib/canonical/launchpad/testing/tests/test_googleservice.py'
--- lib/canonical/launchpad/testing/tests/test_googleservice.py	2011-03-04 17:05:09 +0000
+++ lib/canonical/launchpad/testing/tests/test_googleservice.py	2011-03-07 16:35:45 +0000
@@ -12,7 +12,7 @@
 import os
 import unittest
 
-from lp.services.search import googletestservice
+from lp.services.googlesearch import googletestservice
 from canonical.lazr.pidfile import pidfile_path
 
 

=== modified file 'lib/canonical/testing/layers.py'
--- lib/canonical/testing/layers.py	2011-03-04 17:12:24 +0000
+++ lib/canonical/testing/layers.py	2011-03-07 16:35:45 +0000
@@ -116,7 +116,7 @@
 import lp.services.mail.stub
 from lp.services.mail.mailbox import TestMailBox
 from canonical.launchpad.scripts import execute_zcml_for_scripts
-from lp.services.search.tests.googleserviceharness import (
+from lp.services.googlesearch.tests.googleserviceharness import (
     GoogleServiceTestSetup)
 from canonical.launchpad.webapp.interfaces import (
         DEFAULT_FLAVOR, IStoreSelector, MAIN_STORE)

=== modified file 'lib/lp/app/browser/root.py'
--- lib/lp/app/browser/root.py	2011-03-04 17:05:09 +0000
+++ lib/lp/app/browser/root.py	2011-03-07 16:35:45 +0000
@@ -10,13 +10,11 @@
 
 
 import re
-import sys
 import time
 
 import feedparser
 from lazr.batchnavigator.z3batching import batch
 from zope.component import getUtility
-from zope.error.interfaces import IErrorReportingUtility
 from zope.schema.interfaces import TooLong
 from zope.schema.vocabulary import getVocabularyRegistry
 
@@ -28,7 +26,7 @@
 from canonical.launchpad.interfaces.launchpadstatistic import (
     ILaunchpadStatisticSet,
     )
-from lp.services.search.interfaces import (
+from lp.services.googlesearch.interfaces import (
     GoogleResponseError,
     ISearchService,
     )
@@ -489,8 +487,8 @@
             page_matches = google_search.search(
                 terms=query_terms, start=start)
         except GoogleResponseError:
-            error_utility = getUtility(IErrorReportingUtility)
-            error_utility.raising(sys.exc_info(), self.request)
+            # There was a connectivity or Google service issue that means
+            # there is no data available at this moment.
             self.has_page_service = False
             return None
         if len(page_matches) == 0:

=== modified file 'lib/lp/app/browser/tests/launchpad-search-pages.txt'
--- lib/lp/app/browser/tests/launchpad-search-pages.txt	2011-03-04 17:05:09 +0000
+++ lib/lp/app/browser/tests/launchpad-search-pages.txt	2011-03-07 16:35:45 +0000
@@ -535,15 +535,6 @@
     >>> print search_view.url
     http://launchpad.dev/+search?field.text=gnomebaker
 
-An oops was reported by the view so that the GoogleResponseError can be
-tracked.
-
-    >>> oops = search_view.request.oops
-    >>> oops.type
-    'GoogleResponseError'
-    >>> oops.value
-    'The response was incomplete, no xml.'
-
 
 SearchFormView and SearchFormPrimaryView
 ----------------------------------------
@@ -615,8 +606,7 @@
 the first 20 items are None. Only the last 5 items are PageMatches.
 
     >>> from lp.app.browser.root import WindowedList
-    >>> from lp.services.search.google import (
-    ...     GoogleSearchService)
+    >>> from lp.services.googlesearch import GoogleSearchService
 
     >>> google_search = GoogleSearchService()
     >>> page_matches = google_search.search(terms='bug', start=20)

=== modified file 'lib/lp/services/configure.zcml'
--- lib/lp/services/configure.zcml	2011-03-04 17:05:09 +0000
+++ lib/lp/services/configure.zcml	2011-03-07 16:35:45 +0000
@@ -8,6 +8,7 @@
   <include package=".features" />
   <include package=".fields" />
   <include package=".geoip" />
+  <include package=".googlesearch" />
   <include package=".inlinehelp" file="meta.zcml" />
   <include package=".job" />
   <include package=".memcache" />
@@ -15,6 +16,5 @@
   <include package=".profile" />
   <include package=".salesforce" />
   <include package=".scripts" />
-  <include package=".search" />
   <include package=".worlddata" />
 </configure>

=== renamed directory 'lib/lp/services/search' => 'lib/lp/services/googlesearch'
=== renamed file 'lib/lp/services/search/google.py' => 'lib/lp/services/googlesearch/__init__.py'
--- lib/lp/services/search/google.py	2011-03-04 17:05:09 +0000
+++ lib/lp/services/googlesearch/__init__.py	2011-03-07 16:35:45 +0000
@@ -15,6 +15,7 @@
 
 import xml.etree.cElementTree as ET
 import urllib
+import urllib2
 from urlparse import urlunparse
 
 from lazr.restful.utils import get_current_browser_request
@@ -22,7 +23,8 @@
 from zope.interface import implements
 
 from canonical.config import config
-from lp.services.search.interfaces import (
+from canonical.lazr.timeout import TimeoutError
+from lp.services.googlesearch.interfaces import (
     GoogleResponseError,
     GoogleWrongGSPVersion,
     ISearchResult,
@@ -196,6 +198,11 @@
         action = timeline.start("google-search-api", search_url)
         try:
             gsp_xml = urlfetch(search_url)
+        except (TimeoutError, urllib2.HTTPError, urllib2.URLError), error:
+            # Google search service errors are not code errors. Let the
+            # call site choose to handle the unavailable service.
+            raise GoogleResponseError(
+                "The response errored: %s" % str(error))
         finally:
             action.finish()
         page_matches = self._parse_google_search_protocol(gsp_xml)
@@ -266,10 +273,10 @@
         """
         try:
             gsp_doc = ET.fromstring(gsp_xml)
-        except SyntaxError:
+            start_param = self._getElementByAttributeValue(
+                gsp_doc, './PARAM', 'name', 'start')
+        except (SyntaxError, IndexError):
             raise GoogleResponseError("The response was incomplete, no xml.")
-        start_param = self._getElementByAttributeValue(
-            gsp_doc, './PARAM', 'name', 'start')
         try:
             start = int(start_param.get('value'))
         except (AttributeError, ValueError):

=== modified file 'lib/lp/services/googlesearch/configure.zcml'
--- lib/lp/services/search/configure.zcml	2011-03-04 17:57:32 +0000
+++ lib/lp/services/googlesearch/configure.zcml	2011-03-07 16:35:45 +0000
@@ -6,18 +6,18 @@
   xmlns="http://namespaces.zope.org/zope">
 
   <class
-    class="lp.services.search.google.PageMatch">
-    <allow interface="lp.services.search.interfaces.ISearchResult" />
+    class="lp.services.googlesearch.PageMatch">
+    <allow interface="lp.services.googlesearch.interfaces.ISearchResult" />
   </class>
 
   <class
-    class="lp.services.search.google.PageMatches">
-    <allow interface="lp.services.search.interfaces.ISearchResults" />
+    class="lp.services.googlesearch.PageMatches">
+    <allow interface="lp.services.googlesearch.interfaces.ISearchResults" />
   </class>
 
   <securedutility
-    class="lp.services.search.google.GoogleSearchService"
-    provides="lp.services.search.interfaces.ISearchService">
-    <allow interface="lp.services.search.interfaces.ISearchService" />
+    class="lp.services.googlesearch.GoogleSearchService"
+    provides="lp.services.googlesearch.interfaces.ISearchService">
+    <allow interface="lp.services.googlesearch.interfaces.ISearchService" />
   </securedutility>
 </configure>

=== modified file 'lib/lp/services/googlesearch/doc/google-searchservice.txt'
--- lib/lp/services/search/doc/google-searchservice.txt	2011-03-04 17:05:09 +0000
+++ lib/lp/services/googlesearch/doc/google-searchservice.txt	2011-03-07 16:35:45 +0000
@@ -14,7 +14,7 @@
 
     >>> from zope.component import getUtility
     >>> from zope.interface.verify import verifyObject
-    >>> from lp.services.search.interfaces import (
+    >>> from lp.services.googlesearch.interfaces import (
     ...     ISearchService)
 
     >>> google_search = getUtility(ISearchService)
@@ -32,7 +32,7 @@
 argument of start. The terms are the same as the text that would be
 entered in Google search form; the terms should not be escaped.
 
-    >>> from lp.services.search.interfaces import (
+    >>> from lp.services.googlesearch.interfaces import (
     ...     ISearchResults)
 
     >>> first_page_matches = google_search.search(terms='bug')
@@ -121,8 +121,8 @@
 set. It is created by passing a title, url, and a summary. It is
 an implementation of ISearchResult.
 
-    >>> from lp.services.search.interfaces import ISearchResult
-    >>> from lp.services.search.google import PageMatch
+    >>> from lp.services.googlesearch.interfaces import ISearchResult
+    >>> from lp.services.googlesearch import PageMatch
 
     >>> page_match = PageMatch(
     ...     u'Unicode Titles in Launchpad',
@@ -155,8 +155,7 @@
 configuration may set a testing site.
 
     >>> from canonical.config import config
-    >>> from lp.services.search.google import (
-    ...     GoogleSearchService)
+    >>> from lp.services.googlesearch import GoogleSearchService
 
     >>> google_search = GoogleSearchService()
     >>> config.google.site == google_search.site
@@ -612,7 +611,7 @@
     >>> google_search.search(terms='bug')
     Traceback (most recent call last):
      ...
-    TimeoutError: ...
+    GoogleResponseError: ... timeout exceeded.
 
     # Restore the configuration and the timeout state.
     >>> timeout_data = config.pop('timeout_data')

=== modified file 'lib/lp/services/googlesearch/doc/google-service-stub.txt.disabled'
--- lib/lp/services/search/doc/google-service-stub.txt.disabled	2011-03-04 17:05:09 +0000
+++ lib/lp/services/googlesearch/doc/google-service-stub.txt.disabled	2011-03-07 16:35:45 +0000
@@ -14,7 +14,7 @@
 determines which XML files will be returned when a specific URL is
 accessed.
 
-    >>> from lp.services.search.googletestservice import (
+    >>> from lp.services.googlesearch.googletestservice import (
     ...     url_to_xml_map)
     >>> routes = url_to_xml_map()
 

=== modified file 'lib/lp/services/googlesearch/tests/googleserviceharness.py'
--- lib/lp/services/search/tests/googleserviceharness.py	2011-03-04 17:05:09 +0000
+++ lib/lp/services/googlesearch/tests/googleserviceharness.py	2011-03-07 16:35:45 +0000
@@ -14,7 +14,7 @@
 import os
 import signal
 
-from lp.services.search import googletestservice
+from lp.services.googlesearch import googletestservice
 
 
 class GoogleServiceTestSetup:
@@ -29,7 +29,7 @@
     # google-service-stub.txt, is also disabled.  See
     # canonical/launchpad/ftests/test_system_documentation.py.
     """
-    >>> from lp.services.search.googletestservice import (
+    >>> from lp.services.googlesearch.googletestservice import (
     ...     service_is_available)
     >>> from canonical.config import config
 

=== added file 'lib/lp/services/googlesearch/tests/test_google.py'
--- lib/lp/services/googlesearch/tests/test_google.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/googlesearch/tests/test_google.py	2011-03-07 16:35:45 +0000
@@ -0,0 +1,84 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test the google search service."""
+
+__metaclass__ = type
+
+from contextlib import contextmanager
+from urllib2 import (
+    HTTPError,
+    URLError,
+    )
+
+from canonical.lazr.timeout import TimeoutError
+from canonical.testing.layers import FunctionalLayer
+from lp.services.googlesearch import GoogleSearchService
+from lp.services.googlesearch.interfaces import GoogleResponseError
+from lp.testing import TestCase
+
+
+@contextmanager
+def urlfetch_exception(test_error, *args):
+    """Raise an error during the execution of urlfetch.
+
+    This function replaces urlfetch() with a function that
+    raises an error.
+    """
+
+    def raise_exception(url):
+        raise test_error(*args)
+
+    from canonical.lazr import timeout
+    original_urlfetch = timeout.urlfetch
+    timeout.urlfetch = raise_exception
+    try:
+        yield
+    finally:
+        timeout.urlfetch = original_urlfetch
+
+
+class TestGoogleSearchService(TestCase):
+    """Test GoogleSearchService."""
+
+    layer = FunctionalLayer
+
+    def setUp(self):
+        super(TestGoogleSearchService, self).setUp()
+        self.search_service = GoogleSearchService()
+
+    def test_search_converts_HTTPError(self):
+        # The method converts HTTPError to GoogleResponseError.
+        args = ('url', 500, 'oops', {}, None)
+        with urlfetch_exception(HTTPError, *args):
+            self.assertRaises(
+                GoogleResponseError, self.search_service.search, 'fnord')
+
+    def test_search_converts_URLError(self):
+        # The method converts URLError to GoogleResponseError.
+        with urlfetch_exception(URLError, 'oops'):
+            self.assertRaises(
+                GoogleResponseError, self.search_service.search, 'fnord')
+
+    def test_search_converts_TimeoutError(self):
+        # The method converts TimeoutError to GoogleResponseError.
+        with urlfetch_exception(TimeoutError, 'oops'):
+            self.assertRaises(
+                GoogleResponseError, self.search_service.search, 'fnord')
+
+    def test___parse_google_search_protocol_SyntaxError(self):
+        # The method converts SyntaxError to GoogleResponseError.
+        with urlfetch_exception(SyntaxError, 'oops'):
+            self.assertRaises(
+                GoogleResponseError,
+                self.search_service._parse_google_search_protocol, '')
+
+    def test___parse_google_search_protocol_IndexError(self):
+        # The method converts IndexError to GoogleResponseError.
+        with urlfetch_exception(IndexError, 'oops'):
+            data = (
+                '<?xml version="1.0" encoding="UTF-8" standalone="no"?>'
+                '<GSP VER="3.2"></GSP>')
+            self.assertRaises(
+                GoogleResponseError,
+                self.search_service._parse_google_search_protocol, data)

=== modified file 'lib/lp/services/googlesearch/tests/test_googleharness.py'
--- lib/lp/services/search/tests/test_googleharness.py	2011-03-04 17:12:24 +0000
+++ lib/lp/services/googlesearch/tests/test_googleharness.py	2011-03-07 16:35:45 +0000
@@ -6,5 +6,5 @@
 
 def test_suite():
     return doctest.DocTestSuite(
-            'lp.services.search.tests.googleserviceharness',
+            'lp.services.googlesearch.tests.googleserviceharness',
             optionflags=doctest.NORMALIZE_WHITESPACE | doctest.ELLIPSIS)

=== removed file 'lib/lp/services/search/__init__.py'