launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:py3-sitesearch into launchpad:master.

Commit message:
Fix sitesearch on Python 3

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/396191

The test suite hung on Python 3, and JSON parsing wasn't being done quite right for < 3.6.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:py3-sitesearch into launchpad:master.

diff --git a/lib/lp/services/sitesearch/__init__.py b/lib/lp/services/sitesearch/__init__.py
index da896ee..f5a7e16 100644
--- a/lib/lp/services/sitesearch/__init__.py
+++ b/lib/lp/services/sitesearch/__init__.py
@@ -11,8 +11,6 @@ __all__ = [
     'PageMatches',
     ]
 
-import json
-
 from lazr.restful.utils import get_current_browser_request
 from lazr.uri import URI
 import requests
@@ -219,7 +217,12 @@ class BingSearchService:
                 "The response errored: %s" % str(error))
         finally:
             action.finish()
-        page_matches = self._parse_search_response(response.content, start)
+        try:
+            bing_doc = response.json()
+        except ValueError:
+            raise SiteSearchResponseError(
+                "The response was incomplete, no JSON.")
+        page_matches = self._parse_search_response(bing_doc, start)
         return page_matches
 
     def _checkParameter(self, name, value, is_int=False):
@@ -252,21 +255,16 @@ class BingSearchService:
             'Ocp-Apim-Subscription-Key': self.subscription_key,
             }
 
-    def _parse_search_response(self, bing_json, start=0):
+    def _parse_search_response(self, bing_doc, start=0):
         """Return a `PageMatches` object.
 
-        :param bing_json: A string containing Bing Custom Search API v7 JSON.
+        :param bing_doc: A JSON object containing Bing Custom Search API v7
+            data.
         :return: `ISearchResults` (PageMatches).
-        :raise: `SiteSearchResponseError` if the json response is incomplete or
-            cannot be parsed.
+        :raise: `SiteSearchResponseError` if the JSON object is incomplete
+            or cannot be parsed.
         """
         try:
-            bing_doc = json.loads(bing_json)
-        except (TypeError, ValueError):
-            raise SiteSearchResponseError(
-                "The response was incomplete, no JSON.")
-
-        try:
             response_type = bing_doc['_type']
         except (AttributeError, KeyError, ValueError):
             raise SiteSearchResponseError(
diff --git a/lib/lp/services/sitesearch/tests/test_bing.py b/lib/lp/services/sitesearch/tests/test_bing.py
index 61d623e..0108891 100644
--- a/lib/lp/services/sitesearch/tests/test_bing.py
+++ b/lib/lp/services/sitesearch/tests/test_bing.py
@@ -96,9 +96,8 @@ class TestBingSearchService(TestCase):
         file_name = os.path.join(
             self.base_path, 'bingsearchservice-incompatible-matches.json')
         with open(file_name, 'r') as response_file:
-            response = response_file.read()
-        self.assertEqual(
-            '~25', json.loads(response)['webPages']['totalEstimatedMatches'])
+            response = json.loads(response_file.read())
+        self.assertEqual('~25', response['webPages']['totalEstimatedMatches'])
 
         self.assertRaisesWithContent(
             SiteSearchResponseError,
@@ -112,9 +111,8 @@ class TestBingSearchService(TestCase):
         file_name = os.path.join(
             self.base_path, 'bingsearchservice-negative-total.json')
         with open(file_name, 'r') as response_file:
-            response = response_file.read()
-        self.assertEqual(
-            -25, json.loads(response)['webPages']['totalEstimatedMatches'])
+            response = json.loads(response_file.read())
+        self.assertEqual(-25, response['webPages']['totalEstimatedMatches'])
 
         matches = self.search_service._parse_search_response(response)
         self.assertEqual(0, matches.total)
@@ -129,9 +127,8 @@ class TestBingSearchService(TestCase):
         file_name = os.path.join(
             self.base_path, 'bingsearchservice-missing-title.json')
         with open(file_name, 'r') as response_file:
-            response = response_file.read()
-        self.assertThat(
-            json.loads(response)['webPages']['value'], HasLength(2))
+            response = json.loads(response_file.read())
+        self.assertThat(response['webPages']['value'], HasLength(2))
 
         matches = self.search_service._parse_search_response(response)
         self.assertThat(matches, MatchesListwise([
@@ -150,9 +147,8 @@ class TestBingSearchService(TestCase):
         file_name = os.path.join(
             self.base_path, 'bingsearchservice-missing-summary.json')
         with open(file_name, 'r') as response_file:
-            response = response_file.read()
-        self.assertThat(
-            json.loads(response)['webPages']['value'], HasLength(2))
+            response = json.loads(response_file.read())
+        self.assertThat(response['webPages']['value'], HasLength(2))
 
         matches = self.search_service._parse_search_response(response)
         self.assertThat(matches, MatchesListwise([
@@ -169,9 +165,8 @@ class TestBingSearchService(TestCase):
         file_name = os.path.join(
             self.base_path, 'bingsearchservice-missing-url.json')
         with open(file_name, 'r') as response_file:
-            response = response_file.read()
-        self.assertThat(
-            json.loads(response)['webPages']['value'], HasLength(2))
+            response = json.loads(response_file.read())
+        self.assertThat(response['webPages']['value'], HasLength(2))
 
         matches = self.search_service._parse_search_response(response)
         self.assertThat(matches, MatchesListwise([
@@ -192,9 +187,8 @@ class TestBingSearchService(TestCase):
         file_name = os.path.join(
             self.base_path, 'bingsearchservice-no-meaningful-results.json')
         with open(file_name, 'r') as response_file:
-            response = response_file.read()
-        self.assertThat(
-            json.loads(response)['webPages']['value'], HasLength(1))
+            response = json.loads(response_file.read())
+        self.assertThat(response['webPages']['value'], HasLength(1))
 
         matches = self.search_service._parse_search_response(response)
         self.assertThat(matches, HasLength(0))
@@ -221,23 +215,11 @@ class TestBingSearchService(TestCase):
         self.assertRaises(
             SiteSearchResponseError, self.search_service.search, 'fnord')
 
-    def test_parse_search_response_TypeError(self):
-        # The method converts TypeError to SiteSearchResponseError.
-        self.assertRaises(
-            SiteSearchResponseError,
-            self.search_service._parse_search_response, None)
-
-    def test_parse_search_response_ValueError(self):
-        # The method converts ValueError to SiteSearchResponseError.
-        self.assertRaises(
-            SiteSearchResponseError,
-            self.search_service._parse_search_response, '')
-
     def test_parse_search_response_KeyError(self):
         # The method converts KeyError to SiteSearchResponseError.
         self.assertRaises(
             SiteSearchResponseError,
-            self.search_service._parse_search_response, '{}')
+            self.search_service._parse_search_response, {})
 
     def test_search_uses_proxy(self):
         proxy = 'http://proxy.example:3128/'
diff --git a/lib/lp/services/sitesearch/testservice.py b/lib/lp/services/sitesearch/testservice.py
index 5cbc6f2..3e31609 100644
--- a/lib/lp/services/sitesearch/testservice.py
+++ b/lib/lp/services/sitesearch/testservice.py
@@ -50,7 +50,7 @@ class RequestHandler(BaseHTTPRequestHandler):
         self.end_headers()
 
         filepath = os.path.join(self.content_dir, filename)
-        with open(filepath) as f:
+        with open(filepath, 'rb') as f:
             content_body = f.read()
         self.wfile.write(content_body)