launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/explicit-proxy-sitesearch into lp:launchpad.

Commit message:
Use the configured proxy for site search rather than relying on the environment.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/explicit-proxy-sitesearch/+merge/348437
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/explicit-proxy-sitesearch into lp:launchpad.

=== modified file 'lib/lp/services/sitesearch/__init__.py'
--- lib/lp/services/sitesearch/__init__.py	2018-05-21 20:30:16 +0000
+++ lib/lp/services/sitesearch/__init__.py	2018-06-23 12:51:57 +0000
@@ -212,7 +212,9 @@
         timeline = get_request_timeline(request)
         action = timeline.start("bing-search-api", search_url)
         try:
-            response = urlfetch(search_url, headers=search_headers)
+            response = urlfetch(
+                search_url, headers=search_headers,
+                trust_env=False, use_proxy=True)
         except (TimeoutError, requests.RequestException) as error:
             raise SiteSearchResponseError(
                 "The response errored: %s" % str(error))

=== modified file 'lib/lp/services/sitesearch/tests/test_bing.py'
--- lib/lp/services/sitesearch/tests/test_bing.py	2018-04-25 15:46:14 +0000
+++ lib/lp/services/sitesearch/tests/test_bing.py	2018-06-23 12:51:57 +0000
@@ -9,13 +9,18 @@
 
 import json
 import os.path
+import re
 
 from fixtures import MockPatch
+from requests import Response
 from requests.exceptions import (
     ConnectionError,
     HTTPError,
     )
+import responses
 from testtools.matchers import (
+    ContainsDict,
+    Equals,
     HasLength,
     MatchesListwise,
     MatchesStructure,
@@ -26,6 +31,7 @@
 from lp.services.sitesearch.interfaces import SiteSearchResponseError
 from lp.services.timeout import TimeoutError
 from lp.testing import TestCase
+from lp.testing.fakemethod import FakeMethod
 from lp.testing.layers import BingLaunchpadFunctionalLayer
 
 
@@ -39,6 +45,7 @@
         self.search_service = BingSearchService()
         self.base_path = os.path.normpath(
             os.path.join(os.path.dirname(__file__), 'data'))
+        self.pushConfig('launchpad', http_proxy='none')
 
     def test_configuration(self):
         self.assertEqual(config.bing.site, self.search_service.site)
@@ -192,27 +199,25 @@
         matches = self.search_service._parse_search_response(response)
         self.assertThat(matches, HasLength(0))
 
+    @responses.activate
     def test_search_converts_HTTPError(self):
         # The method converts HTTPError to SiteSearchResponseError.
         args = ('url', 500, 'oops', {}, None)
-        self.useFixture(MockPatch(
-            'lp.services.sitesearch.urlfetch', side_effect=HTTPError(*args)))
+        responses.add('GET', re.compile(r'.*'), body=HTTPError(*args))
         self.assertRaises(
             SiteSearchResponseError, self.search_service.search, 'fnord')
 
+    @responses.activate
     def test_search_converts_ConnectionError(self):
         # The method converts ConnectionError to SiteSearchResponseError.
-        self.useFixture(MockPatch(
-            'lp.services.sitesearch.urlfetch',
-            side_effect=ConnectionError('oops')))
+        responses.add('GET', re.compile(r'.*'), body=ConnectionError('oops'))
         self.assertRaises(
             SiteSearchResponseError, self.search_service.search, 'fnord')
 
+    @responses.activate
     def test_search_converts_TimeoutError(self):
         # The method converts TimeoutError to SiteSearchResponseError.
-        self.useFixture(MockPatch(
-            'lp.services.sitesearch.urlfetch',
-            side_effect=TimeoutError('oops')))
+        responses.add('GET', re.compile(r'.*'), body=TimeoutError('oops'))
         self.assertRaises(
             SiteSearchResponseError, self.search_service.search, 'fnord')
 
@@ -234,6 +239,22 @@
             SiteSearchResponseError,
             self.search_service._parse_search_response, '{}')
 
+    def test_search_uses_proxy(self):
+        proxy = 'http://proxy.example:3128/'
+        self.pushConfig('launchpad', http_proxy=proxy)
+        fake_send = FakeMethod(result=Response())
+        self.useFixture(
+            MockPatch('requests.adapters.HTTPAdapter.send', fake_send))
+        # Our mock doesn't return a valid response, but we don't care; we
+        # only care about how the adapter is called.
+        self.assertRaises(
+            SiteSearchResponseError, self.search_service.search, 'fnord')
+        self.assertThat(
+            fake_send.calls[0][1]['proxies'],
+            ContainsDict({
+                scheme: Equals(proxy) for scheme in ('http', 'https')
+                }))
+
     def test_search_with_results(self):
         matches = self.search_service.search('bug')
         self.assertEqual(0, matches.start)