Revert "gerrit_util: Refactor ReadHttpResponse and add more tests."

This reverts commit 5bfa3ae88d.

Reason for revert:
Fails when fetching status for cl 1708084 

Original change's description:
> gerrit_util: Refactor ReadHttpResponse and add more tests.
> 
> Bug: 1016601
> Change-Id: Ie6afc5b1ea29888b0bf40bdb39b2b492d2d0494c
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1880014
> Reviewed-by: Anthony Polito <apolito@google.com>
> Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

TBR=ehmaldonado@chromium.org,apolito@google.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1016601
Change-Id: I84bc378ed5f58e911e0900b1a5dbea70dc06ade1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1883677
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

.vpython3

@@ -1,23 +1 @@
 python_version: "3.8"
-
-# Used by:
-#   auth.py
-#   gerrit_util.py
-#   git_cl.py
-#   my_activity.py
-#   TODO(crbug.com/1002153): Add ninjalog_uploader.py
-wheel: <
-  name: "infra/python/wheels/httplib2-py3"
-  version: "version:0.13.1"
->
-
-# Used by:
-#   my_activity.py
-wheel: <
-  name: "infra/python/wheels/python-dateutil-py2_py3"
-  version: "version:2.7.3"
->
-wheel: <
-  name: "infra/python/wheels/six-py2_py3"
-  version: "version:1.10.0"
->

gerrit_util.py

@@ -9,10 +9,11 @@ https://gerrit-review.googlesource.com/Documentation/rest-api.html
 """
 
 from __future__ import print_function
-from __future__ import unicode_literals
 
 import base64
 import contextlib
+import cookielib
+import httplib  # Still used for its constants.
 import httplib2
 import json
 import logging
@@ -25,6 +26,9 @@ import stat
 import sys
 import tempfile
 import time
+import urllib
+import urlparse
+from cStringIO import StringIO
 from multiprocessing.pool import ThreadPool
 
 import auth
@@ -33,16 +37,6 @@ import metrics
 import metrics_utils
 import subprocess2
 
-from third_party import six
-from six.moves import urllib
-
-if sys.version_info.major == 2:
-  import cookielib
-  from cStringIO import StringIO
-else:
-  import http.cookiejar as cookielib
-  from io import StringIO
-
 LOGGER = logging.getLogger()
 # With a starting sleep time of 1.5 seconds, 2^n exponential backoff, and seven
 # total tries, the sleep time between the first and last tries will be 94.5 sec.
@@ -54,18 +48,16 @@ TRY_LIMIT = 3
 GERRIT_PROTOCOL = 'https'
 
 
-def time_sleep(seconds):
-  # Use this so that it can be mocked in tests without interfering with python
-  # system machinery.
-  return time.sleep(seconds)
-
-
 class GerritError(Exception):
   """Exception class for errors commuicating with the gerrit-on-borg service."""
-  def __init__(self, http_status, message, *args, **kwargs):
+  def __init__(self, http_status, *args, **kwargs):
     super(GerritError, self).__init__(*args, **kwargs)
     self.http_status = http_status
-    self.message = '(%d) %s' % (self.http_status, message)
+    self.message = '(%d) %s' % (self.http_status, self.message)
+
+
+class GerritAuthenticationError(GerritError):
+  """Exception class for authentication errors during Gerrit communication."""
 
 
 def _QueryString(params, first_param=None):
@@ -73,11 +65,21 @@ def _QueryString(params, first_param=None):
 
   https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#list-changes
   """
-  q = [urllib.parse.quote(first_param)] if first_param else []
+  q = [urllib.quote(first_param)] if first_param else []
   q.extend(['%s:%s' % (key, val) for key, val in params])
   return '+'.join(q)
 
 
+def GetConnectionObject(protocol=None):
+  if protocol is None:
+    protocol = GERRIT_PROTOCOL
+  if protocol in ('http', 'https'):
+    return httplib2.Http()
+  else:
+    raise RuntimeError(
+        "Don't know how to work with protocol '%s'" % protocol)
+
+
 class Authenticator(object):
   """Base authenticator class for authenticator implementations to subclass."""
 
@@ -298,13 +300,11 @@ class GceAuthenticator(Authenticator):
   def _get(url, **kwargs):
     next_delay_sec = 1
     for i in xrange(TRY_LIMIT):
-      p = urllib.parse.urlparse(url)
-      if p.scheme not in ('http', 'https'):
-        raise RuntimeError(
-            "Don't know how to work with protocol '%s'" % protocol)
-      resp, contents = httplib2.Http().request(url, 'GET', **kwargs)
+      p = urlparse.urlparse(url)
+      c = GetConnectionObject(protocol=p.scheme)
+      resp, contents = c.request(url, 'GET', **kwargs)
       LOGGER.debug('GET [%s] #%d/%d (%d)', url, i+1, TRY_LIMIT, resp.status)
-      if resp.status < 500:
+      if resp.status < httplib.INTERNAL_SERVER_ERROR:
         return (resp, contents)
 
       # Retry server error status codes.
@@ -312,7 +312,7 @@ class GceAuthenticator(Authenticator):
       if TRY_LIMIT - i > 1:
         LOGGER.info('Will retry in %d seconds (%d more times)...',
                     next_delay_sec, TRY_LIMIT - i - 1)
-        time_sleep(next_delay_sec)
+        time.sleep(next_delay_sec)
         next_delay_sec *= 2
 
   @classmethod
@@ -323,7 +323,7 @@ class GceAuthenticator(Authenticator):
         return cls._token_cache
 
     resp, contents = cls._get(cls._ACQUIRE_URL, headers=cls._ACQUIRE_HEADERS)
-    if resp.status != 200:
+    if resp.status != httplib.OK:
       return None
     cls._token_cache = json.loads(contents)
     cls._token_expiration = cls._token_cache['expires_in'] + time.time()
@@ -370,7 +370,7 @@ def CreateHttpConn(host, path, reqtype='GET', headers=None, body=None):
     url = '/a%s' % url
 
   if body:
-    body = json.dumps(body, sort_keys=True)
+    body = json.JSONEncoder().encode(body)
     headers.setdefault('Content-Type', 'application/json')
   if LOGGER.isEnabledFor(logging.DEBUG):
     LOGGER.debug('%s %s://%s%s' % (reqtype, GERRIT_PROTOCOL, host, url))
@@ -380,12 +380,12 @@ def CreateHttpConn(host, path, reqtype='GET', headers=None, body=None):
       LOGGER.debug('%s: %s' % (key, val))
     if body:
       LOGGER.debug(body)
-  conn = httplib2.Http()
-  # HACK: httplib2.Http has no such attribute; we store req_host here for later
+  conn = GetConnectionObject()
+  # HACK: httplib.Http has no such attribute; we store req_host here for later
   # use in ReadHttpResponse.
   conn.req_host = host
   conn.req_params = {
-      'uri': urllib.parse.urljoin('%s://%s' % (GERRIT_PROTOCOL, host), url),
+      'uri': urlparse.urljoin('%s://%s' % (GERRIT_PROTOCOL, host), url),
       'method': reqtype,
       'headers': headers,
       'body': body,
@@ -406,7 +406,6 @@ def ReadHttpResponse(conn, accept_statuses=frozenset([200])):
   for idx in range(TRY_LIMIT):
     before_response = time.time()
     response, contents = conn.request(**conn.req_params)
-    contents = contents.decode('utf-8', 'replace')
 
     response_time = time.time() - before_response
     metrics.collector.add_repeated(
@@ -415,12 +414,21 @@ def ReadHttpResponse(conn, accept_statuses=frozenset([200])):
             conn.req_params['uri'], conn.req_params['method'], response.status,
             response_time))
 
-    # If response.status is an accepted status,
-    # or response.status < 500 then the result is final; break retry loop.
-    # If the response is 404/409 it might be because of replication lag,
-    # so keep trying anyway.
-    if (response.status in accept_statuses
-        or response.status < 500 and response.status not in [404, 409]):
+    # Check if this is an authentication issue.
+    www_authenticate = response.get('www-authenticate')
+    if (response.status in (httplib.UNAUTHORIZED, httplib.FOUND) and
+        www_authenticate):
+      auth_match = re.search('realm="([^"]+)"', www_authenticate, re.I)
+      host = auth_match.group(1) if auth_match else conn.req_host
+      reason = ('Authentication failed. Please make sure your .gitcookies file '
+                'has credentials for %s' % host)
+      raise GerritAuthenticationError(response.status, reason)
+
+    # If response.status < 500 then the result is final; break retry loop.
+    # If the response is 404/409, it might be because of replication lag, so
+    # keep trying anyway.
+    if ((response.status < 500 and response.status not in [404, 409])
+        or response.status in accept_statuses):
       LOGGER.debug('got response %d for %s %s', response.status,
                    conn.req_params['method'], conn.req_params['uri'])
       # If 404 was in accept_statuses, then it's expected that the file might
@@ -429,7 +437,6 @@ def ReadHttpResponse(conn, accept_statuses=frozenset([200])):
       if response.status == 404:
         contents = ''
       break
-
     # A status >=500 is assumed to be a possible transient error; retry.
     http_version = 'HTTP/%s' % ('1.1' if response.version == 11 else '1.0')
     LOGGER.warn('A transient error occurred while querying %s:\n'
@@ -439,29 +446,19 @@ def ReadHttpResponse(conn, accept_statuses=frozenset([200])):
                 conn.req_params['uri'],
                 http_version, http_version, response.status, response.reason)
 
-    if idx < TRY_LIMIT - 1:
+    if TRY_LIMIT - idx > 1:
       LOGGER.info('Will retry in %d seconds (%d more times)...',
                   sleep_time, TRY_LIMIT - idx - 1)
-      time_sleep(sleep_time)
+      time.sleep(sleep_time)
       sleep_time = sleep_time * 2
   # end of retries loop
-
-  if response.status in accept_statuses:
-    return StringIO(contents)
-
-  if response.status in (302, 401, 403):
-    www_authenticate = response.get('www-authenticate')
-    if not www_authenticate:
-      print('Your Gerrit credentials might be misconfigured.')
-    else:
-      auth_match = re.search('realm="([^"]+)"', www_authenticate, re.I)
-      host = auth_match.group(1) if auth_match else conn.req_host
-      print('Authentication failed. Please make sure your .gitcookies '
-            'file has credentials for %s.' % host)
-    print('Try:\n  git cl creds-check')
-
-  reason = '%s: %s' % (response.reason, contents)
-  raise GerritError(response.status, reason)
+  if response.status not in accept_statuses:
+    if response.status in (401, 403):
+      print('Your Gerrit credentials might be misconfigured. Try: \n'
+            '  git cl creds-check')
+    reason = '%s: %s' % (response.reason, contents)
+    raise GerritError(response.status, reason)
+  return StringIO(contents)
 
 
 def ReadHttpJsonResponse(conn, accept_statuses=frozenset([200])):
@@ -578,7 +575,7 @@ def MultiQueryChanges(host, params, change_list, limit=None, o_params=None,
   if not change_list:
     raise RuntimeError(
         "MultiQueryChanges requires a list of change numbers/id's")
-  q = ['q=%s' % '+OR+'.join([urllib.parse.quote(str(x)) for x in change_list])]
+  q = ['q=%s' % '+OR+'.join([urllib.quote(str(x)) for x in change_list])]
   if params:
     q.append(_QueryString(params))
   if limit:
@@ -604,8 +601,7 @@ def GetGerritFetchUrl(host):
 def GetCodeReviewTbrScore(host, project):
   """Given a Gerrit host name and project, return the Code-Review score for TBR.
   """
-  conn = CreateHttpConn(
-      host, '/projects/%s' % urllib.parse.quote(project, ''))
+  conn = CreateHttpConn(host, '/projects/%s' % urllib.quote(project, safe=''))
   project = ReadHttpJsonResponse(conn)
   if ('labels' not in project
       or 'Code-Review' not in project['labels']
@@ -992,4 +988,4 @@ def ChangeIdentifier(project, change_number):
   comparing to specifying just change_number.
   """
   assert int(change_number)
-  return '%s~%s' % (urllib.parse.quote(project, ''), change_number)
+  return '%s~%s' % (urllib.quote(project, safe=''), change_number)

tests/gerrit_util_test.py

@@ -1,4 +1,4 @@
-#!/usr/bin/env vpython3
+#!/usr/bin/env vpython
 # coding=utf-8
 # Copyright (c) 2019 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
@@ -9,7 +9,6 @@ from __future__ import unicode_literals
 
 
 import base64
-import json
 import os
 import sys
 import unittest
@@ -20,15 +19,8 @@ from third_party import mock
 
 import gerrit_util
 import gclient_utils
-import metrics
-import metrics_utils
 import subprocess2
 
-if sys.version_info.major == 2:
-  from cStringIO import StringIO
-else:
-  from io import StringIO
-
 
 class CookiesAuthenticatorTest(unittest.TestCase):
   _GITCOOKIES = '\n'.join([
@@ -175,214 +167,5 @@ class CookiesAuthenticatorTest(unittest.TestCase):
     self.assertIsNone(auth.get_auth_email('some-review.example.com'))
 
 
-class GerritUtilTest(unittest.TestCase):
-  def setUp(self):
-    super(GerritUtilTest, self).setUp()
-    mock.patch('gerrit_util.LOGGER').start()
-    mock.patch('gerrit_util.time_sleep').start()
-    mock.patch('metrics.collector').start()
-    mock.patch(
-        'metrics_utils.extract_http_metrics',
-        return_value='http_metrics').start()
-    self.addCleanup(mock.patch.stopall)
-
-  def testQueryString(self):
-    self.assertEqual('', gerrit_util._QueryString([]))
-    self.assertEqual(
-        'first%20param%2B', gerrit_util._QueryString([], 'first param+'))
-    self.assertEqual(
-        'key:val+foo:bar',
-         gerrit_util._QueryString([('key', 'val'), ('foo', 'bar')]))
-    self.assertEqual(
-        'first%20param%2B+key:val+foo:bar',
-         gerrit_util._QueryString(
-            [('key', 'val'), ('foo', 'bar')], 'first param+'))
-
-  @mock.patch('gerrit_util.Authenticator')
-  def testCreateHttpConn_Basic(self, mockAuth):
-    mockAuth.get().get_auth_header.return_value = None
-    conn = gerrit_util.CreateHttpConn('host.example.com', 'foo/bar')
-    self.assertEqual('host.example.com', conn.req_host)
-    self.assertEqual({
-        'uri': 'https://host.example.com/foo/bar',
-        'method': 'GET',
-        'headers': {},
-        'body': None,
-    }, conn.req_params)
-
-  @mock.patch('gerrit_util.Authenticator')
-  def testCreateHttpConn_Authenticated(self, mockAuth):
-    mockAuth.get().get_auth_header.return_value = 'Bearer token'
-    conn = gerrit_util.CreateHttpConn(
-        'host.example.com', 'foo/bar', headers={'header': 'value'})
-    self.assertEqual('host.example.com', conn.req_host)
-    self.assertEqual({
-        'uri': 'https://host.example.com/a/foo/bar',
-        'method': 'GET',
-        'headers': {'Authorization': 'Bearer token', 'header': 'value'},
-        'body': None,
-    }, conn.req_params)
-
-  @mock.patch('gerrit_util.Authenticator')
-  def testCreateHttpConn_Body(self, mockAuth):
-    mockAuth.get().get_auth_header.return_value = None
-    conn = gerrit_util.CreateHttpConn(
-        'host.example.com', 'foo/bar', body={'l': [1, 2, 3], 'd': {'k': 'v'}})
-    self.assertEqual('host.example.com', conn.req_host)
-    self.assertEqual({
-        'uri': 'https://host.example.com/foo/bar',
-        'method': 'GET',
-        'headers': {'Content-Type': 'application/json'},
-        'body': '{"d": {"k": "v"}, "l": [1, 2, 3]}',
-    }, conn.req_params)
-
-  def testReadHttpResponse_200(self):
-    conn = mock.Mock()
-    conn.req_params = {'uri': 'uri', 'method': 'method'}
-    conn.request.return_value = (mock.Mock(status=200), b'content')
-
-    content = gerrit_util.ReadHttpResponse(conn)
-    self.assertEqual('content', content.getvalue())
-    metrics.collector.add_repeated.assert_called_once_with(
-        'http_requests', 'http_metrics')
-
-  def testReadHttpResponse_AuthenticationIssue(self):
-    for status in (302, 401, 403):
-      response = mock.Mock(status=status)
-      response.get.return_value = None
-      conn = mock.Mock(req_params={'uri': 'uri', 'method': 'method'})
-      conn.request.return_value = (response, b'')
-
-      with mock.patch('sys.stdout', StringIO()):
-        with self.assertRaises(gerrit_util.GerritError) as cm:
-          gerrit_util.ReadHttpResponse(conn)
-
-        self.assertEqual(status, cm.exception.http_status)
-        self.assertIn(
-            'Your Gerrit credentials might be misconfigured',
-            sys.stdout.getvalue())
-
-  def testReadHttpResponse_ClientError(self):
-    conn = mock.Mock(req_params={'uri': 'uri', 'method': 'method'})
-    conn.request.return_value = (mock.Mock(status=404), b'')
-
-    with self.assertRaises(gerrit_util.GerritError) as cm:
-      gerrit_util.ReadHttpResponse(conn)
-
-    self.assertEqual(404, cm.exception.http_status)
-
-  def testReadHttpResponse_ServerError(self):
-    conn = mock.Mock(req_params={'uri': 'uri', 'method': 'method'})
-    conn.request.return_value = (mock.Mock(status=500), b'')
-
-    with self.assertRaises(gerrit_util.GerritError) as cm:
-      gerrit_util.ReadHttpResponse(conn)
-
-    self.assertEqual(500, cm.exception.http_status)
-    self.assertEqual(gerrit_util.TRY_LIMIT, len(conn.request.mock_calls))
-    self.assertEqual(
-        [mock.call(1.5), mock.call(3)], gerrit_util.time_sleep.mock_calls)
-
-  def testReadHttpResponse_ServerErrorAndSuccess(self):
-    conn = mock.Mock(req_params={'uri': 'uri', 'method': 'method'})
-    conn.request.side_effect = [
-        (mock.Mock(status=500), b''),
-        (mock.Mock(status=200), b'content'),
-    ]
-
-    self.assertEqual('content', gerrit_util.ReadHttpResponse(conn).getvalue())
-    self.assertEqual(2, len(conn.request.mock_calls))
-    gerrit_util.time_sleep.assert_called_once_with(1.5)
-
-  def testReadHttpResponse_Expected404(self):
-    conn = mock.Mock()
-    conn.req_params = {'uri': 'uri', 'method': 'method'}
-    conn.request.return_value = (mock.Mock(status=404), b'content')
-
-    content = gerrit_util.ReadHttpResponse(conn, (404,))
-    self.assertEqual('', content.getvalue())
-
-  @mock.patch('gerrit_util.ReadHttpResponse')
-  def testReadHttpJsonResponse_NotJSON(self, mockReadHttpResponse):
-    mockReadHttpResponse.return_value = StringIO('not json')
-    with self.assertRaises(gerrit_util.GerritError) as cm:
-      gerrit_util.ReadHttpJsonResponse(None)
-    self.assertEqual(cm.exception.http_status, 200)
-    self.assertEqual(
-        cm.exception.message, '(200) Unexpected json output: not json')
-
-  @mock.patch('gerrit_util.ReadHttpResponse')
-  def testReadHttpJsonResponse_EmptyValue(self, mockReadHttpResponse):
-    mockReadHttpResponse.return_value = StringIO(')]}\'')
-    self.assertIsNone(gerrit_util.ReadHttpJsonResponse(None))
-
-  @mock.patch('gerrit_util.ReadHttpResponse')
-  def testReadHttpJsonResponse_JSON(self, mockReadHttpResponse):
-    expected_value = {'foo': 'bar', 'baz': [1, '2', 3]}
-    mockReadHttpResponse.return_value = StringIO(
-        ')]}\'\n' + json.dumps(expected_value))
-    self.assertEqual(expected_value, gerrit_util.ReadHttpJsonResponse(None))
-
-  @mock.patch('gerrit_util.CreateHttpConn')
-  @mock.patch('gerrit_util.ReadHttpJsonResponse')
-  def testQueryChanges(self, mockJsonResponse, mockCreateHttpConn):
-    gerrit_util.QueryChanges(
-        'host', [('key', 'val'), ('foo', 'bar')], 'first param', limit=500,
-        o_params=['PARAM_A', 'PARAM_B'], start='start')
-    mockCreateHttpConn.assert_called_once_with(
-        'host',
-        ('changes/?q=first%20param+key:val+foo:bar'
-         '&start=start'
-         '&n=500'
-         '&o=PARAM_A'
-         '&o=PARAM_B'))
-
-  def testQueryChanges_NoParams(self):
-    self.assertRaises(RuntimeError, gerrit_util.QueryChanges, 'host', [])
-
-  @mock.patch('gerrit_util.QueryChanges')
-  def testGenerateAllChanges(self, mockQueryChanges):
-    mockQueryChanges.side_effect = [
-        # First results page
-        [
-            {'_number': '4'},
-            {'_number': '3'},
-            {'_number': '2', '_more_changes': True},
-        ],
-        # Second results page, there are new changes, so second page includes
-        # some results from the first page.
-        [
-            {'_number': '2'},
-            {'_number': '1'},
-        ],
-        # GenerateAllChanges queries again from the start to get any new
-        # changes (5 in this case).
-        [
-            {'_number': '5'},
-            {'_number': '4'},
-            {'_number': '3', '_more_changes': True},
-
-        ],
-    ]
-
-    changes = list(gerrit_util.GenerateAllChanges('host', 'params'))
-    self.assertEqual(
-        [
-            {'_number': '4'},
-            {'_number': '3'},
-            {'_number': '2', '_more_changes': True},
-            {'_number': '1'},
-            {'_number': '5'},
-        ],
-        changes)
-    self.assertEqual(
-        [
-            mock.call('host', 'params', None, 500, None, 0),
-            mock.call('host', 'params', None, 500, None, 3),
-            mock.call('host', 'params', None, 500, None, 0),
-        ],
-        mockQueryChanges.mock_calls)
-
-
 if __name__ == '__main__':
   unittest.main()