launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02860
[Merge] lp:~sinzui/launchpad/search-oopses-1 into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/search-oopses-1 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#267852 error page when google search fails is unusable
https://bugs.launchpad.net/bugs/267852
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/search-oopses-1/+merge/52272
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
--
https://code.launchpad.net/~sinzui/launchpad/search-oopses-1/+merge/52272
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/search-oopses-1 into lp:launchpad.
=== 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-05 00:09:00 +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
@@ -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-05 00:09:00 +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
----------------------------------------
=== modified file 'lib/lp/services/search/google.py'
--- lib/lp/services/search/google.py 2011-03-04 17:05:09 +0000
+++ lib/lp/services/search/google.py 2011-03-05 00:09:00 +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,6 +23,7 @@
from zope.interface import implements
from canonical.config import config
+from canonical.lazr.timeout import TimeoutError
from lp.services.search.interfaces import (
GoogleResponseError,
GoogleWrongGSPVersion,
@@ -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):
=== added file 'lib/lp/services/search/tests/test_google.py'
--- lib/lp/services/search/tests/test_google.py 1970-01-01 00:00:00 +0000
+++ lib/lp/services/search/tests/test_google.py 2011-03-05 00:09:00 +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.search.google import GoogleSearchService
+from lp.services.search.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)
