Revert "Refactor ReadHttpResponse to be error-friendlier"

This reverts commit 6d7ab1bfe5. Reason for revert: Stacktrace: File "/s/depot_tools/gerrit_util.py", line 816, in GetAccountDetails return ReadHttpJsonResponse(conn) File "/s/depot_tools/gerrit_util.py", line 376, in ReadHttpJsonResponse fh = ReadHttpResponse(conn, accept_statuses) File "/s/depot_tools/gerrit_util.py", line 365, in ReadHttpResponse if response.status not in accept_statuses: TypeError: argument of type 'NoneType' is not iterable Original change's description: > Refactor ReadHttpResponse to be error-friendlier > > Gerrit sometimes returns a full response json object at > the same time as returning a non-200 status code. This > refactor makes it easier for calling code to request > access to that object and handle error cases on its own. > > Bug: 710028 > Change-Id: Id1017d580d2fb843d5ca6287efcfed8775c52cd6 > Reviewed-on: https://chromium-review.googlesource.com/479450 > Commit-Queue: Aaron Gable <agable@chromium.org> > Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org> > TBR=agable@chromium.org,tandrii@chromium.org,chromium-reviews@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true Change-Id: Ia9d9ce835e207a32e7cc8ee35c0cf40c823c7b78 Reviewed-on: https://chromium-review.googlesource.com/481059 Reviewed-by: Aaron Gable <agable@chromium.org> Commit-Queue: Aaron Gable <agable@chromium.org>
9 years ago · 382674be12
parent 6c98dc69b2
commit 382674be12
5 changed files with 62 additions and 44 deletions
--- a/gerrit_util.py
+++ b/gerrit_util.py
@ -323,15 +323,18 @@ def CreateHttpConn(host, path, reqtype='GET', headers=None, body=None):
  return conn


-def ReadHttpResponse(conn, accept_statuses=frozenset([200, 404])):
+def ReadHttpResponse(conn, expect_status=200, ignore_404=True):
  """Reads an http response from a connection into a string buffer.

  Args:
    conn: An Http object created by CreateHttpConn above.
-    accept_statuses: Treat any of these statuses as success. Default: [200, 404]
-                     Common additions include 204 and 400.
+    expect_status: Success is indicated by this status in the response.
+    ignore_404: For many requests, gerrit-on-borg will return 404 if the request
+                doesn't match the database contents.  In most such cases, we
+                want the API to return None rather than raise an Exception.
  Returns: A string buffer containing the connection's reply.
  """
+
  sleep_time = 0.5
  for idx in range(TRY_LIMIT):
    response, contents = conn.request(**conn.req_params)
@ -342,7 +345,7 @@ def ReadHttpResponse(conn, accept_statuses=frozenset([200, 404])):
        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 '
+      reason = ('Authentication failed. Please make sure your .netrc file '
                'has credentials for %s' % host)
      raise GerritAuthenticationError(response.status, reason)

@ -362,7 +365,9 @@ def ReadHttpResponse(conn, accept_statuses=frozenset([200, 404])):
      LOGGER.warn('... will retry %d more times.', TRY_LIMIT - idx - 1)
      time.sleep(sleep_time)
      sleep_time = sleep_time * 2
-  if response.status not in accept_statuses:
+  if ignore_404 and response.status == 404:
+    return StringIO()
+  if response.status != expect_status:
    if response.status in (401, 403):
      print('Your Gerrit credentials might be misconfigured. Try: \n'
            '  git cl creds-check')
@ -371,9 +376,10 @@ def ReadHttpResponse(conn, accept_statuses=frozenset([200, 404])):
  return StringIO(contents)


-def ReadHttpJsonResponse(conn, accept_statuses=None):
+def ReadHttpJsonResponse(conn, expect_status=200, ignore_404=True):
  """Parses an https response as json."""
-  fh = ReadHttpResponse(conn, accept_statuses)
+  fh = ReadHttpResponse(
+      conn, expect_status=expect_status, ignore_404=ignore_404)
  # The first line of the response should always be: )]}'
  s = fh.readline()
  if s and s.rstrip() != ")]}'":
@ -411,7 +417,7 @@ def QueryChanges(host, param_dict, first_param=None, limit=None, o_params=None,
  if o_params:
    path = '%s&%s' % (path, '&'.join(['o=%s' % p for p in o_params]))
  # Don't ignore 404; a query should always return a list, even if it's empty.
-  return ReadHttpJsonResponse(CreateHttpConn(host, path), accept_statuses=[200])
+  return ReadHttpJsonResponse(CreateHttpConn(host, path), ignore_404=False)


 def GenerateAllChanges(host, param_dict, first_param=None, limit=500,
@ -494,8 +500,7 @@ def MultiQueryChanges(host, param_dict, change_list, limit=None, o_params=None,
    q.extend(['o=%s' % p for p in o_params])
  path = 'changes/?%s' % '&'.join(q)
  try:
-    result = ReadHttpJsonResponse(
-        CreateHttpConn(host, path), accept_statuses=[200])
+    result = ReadHttpJsonResponse(CreateHttpConn(host, path), ignore_404=False)
  except GerritError as e:
    msg = '%s:\n%s' % (e.message, path)
    raise GerritError(e.http_status, msg)
@ -523,13 +528,13 @@ def GetChange(host, change):
  return ReadHttpJsonResponse(CreateHttpConn(host, path))


-def GetChangeDetail(host, change, o_params=None, accept_statuses=None):
+def GetChangeDetail(host, change, o_params=None, ignore_404=True):
  """Query a gerrit server for extended information about a single change."""
+  # TODO(tandrii): cahnge ignore_404 to False by default.
  path = 'changes/%s/detail' % change
  if o_params:
    path += '?%s' % '&'.join(['o=%s' % p for p in o_params])
-  return ReadHttpJsonResponse(
-      CreateHttpConn(host, path), accept_statuses=accept_statuses)
+  return ReadHttpJsonResponse(CreateHttpConn(host, path), ignore_404=ignore_404)


 def GetChangeCommit(host, change, revision='current'):
@ -566,7 +571,7 @@ def AbandonChange(host, change, msg=''):
  path = 'changes/%s/abandon' % change
  body = {'message': msg} if msg else {}
  conn = CreateHttpConn(host, path, reqtype='POST', body=body)
-  return ReadHttpJsonResponse(conn, accept_statuses=[200])
+  return ReadHttpJsonResponse(conn, ignore_404=False)


 def RestoreChange(host, change, msg=''):
@ -574,7 +579,7 @@ def RestoreChange(host, change, msg=''):
  path = 'changes/%s/restore' % change
  body = {'message': msg} if msg else {}
  conn = CreateHttpConn(host, path, reqtype='POST', body=body)
-  return ReadHttpJsonResponse(conn, accept_statuses=[200])
+  return ReadHttpJsonResponse(conn, ignore_404=False)


 def SubmitChange(host, change, wait_for_merge=True):
@ -582,26 +587,31 @@ def SubmitChange(host, change, wait_for_merge=True):
  path = 'changes/%s/submit' % change
  body = {'wait_for_merge': wait_for_merge}
  conn = CreateHttpConn(host, path, reqtype='POST', body=body)
-  return ReadHttpJsonResponse(conn, accept_statuses=[200])
+  return ReadHttpJsonResponse(conn, ignore_404=False)


 def HasPendingChangeEdit(host, change):
  conn = CreateHttpConn(host, 'changes/%s/edit' % change)
  try:
-    ReadHttpResponse(conn, accept_statuses=[200])
+    ReadHttpResponse(conn, ignore_404=False)
  except GerritError as e:
-    # 204 No Content means no pending change.
-    if e.http_status == 204:
-      return False
-    raise
-  return True
+    # On success, gerrit returns status 204; anything else is an error.
+    if e.http_status != 204:
+      raise
+    return False
+  else:
+    return True


 def DeletePendingChangeEdit(host, change):
  conn = CreateHttpConn(host, 'changes/%s/edit' % change, reqtype='DELETE')
-  # On success, gerrit returns status 204; if the edit was already deleted it
-  # returns 404.  Anything else is an error.
-  ReadHttpResponse(conn, accept_statuses=[204, 404])
+  try:
+    ReadHttpResponse(conn, ignore_404=False)
+  except GerritError as e:
+    # On success, gerrit returns status 204; if the edit was already deleted it
+    # returns 404.  Anything else is an error.
+    if e.http_status not in (204, 404):
+      raise


 def SetCommitMessage(host, change, description, notify='ALL'):
@ -613,24 +623,29 @@ def SetCommitMessage(host, change, description, notify='ALL'):
    body = {'message': description}
    conn = CreateHttpConn(host, path, reqtype='PUT', body=body)
    try:
-      ReadHttpResponse(conn, accept_statuses=[204])
+      ReadHttpResponse(conn, ignore_404=False)
    except GerritError as e:
+      # On success, gerrit returns status 204; anything else is an error.
+      if e.http_status != 204:
+        raise
+    else:
      raise GerritError(
-          e.http_status,
-          'Received unexpected http status while editing message '
-          'in change %s' % change)
+          'Unexpectedly received a 200 http status while editing message in '
+          'change %s' % change)

    # And then publish it.
    path = 'changes/%s/edit:publish' % change
    conn = CreateHttpConn(host, path, reqtype='POST', body={'notify': notify})
    try:
-      ReadHttpResponse(conn, accept_statuses=[204])
+      ReadHttpResponse(conn, ignore_404=False)
    except GerritError as e:
+      # On success, gerrit returns status 204; anything else is an error.
+      if e.http_status != 204:
+        raise
+    else:
      raise GerritError(
-          e.http_status,
-          'Received unexpected http status while publishing message '
-          'in change %s' % change)
-
+          'Unexpectedly received a 200 http status while publishing message '
+          'change in %s' % change)
  except (GerritError, KeyboardInterrupt) as e:
    # Something went wrong with one of the two calls, so we want to clean up
    # after ourselves before returning.
@ -673,7 +688,7 @@ def AddReviewers(host, change, add=None, is_reviewer=True, notify=True):
    }
    try:
      conn = CreateHttpConn(host, path, reqtype='POST', body=body)
-      _ = ReadHttpJsonResponse(conn, accept_statuses=[200])
+      _ = ReadHttpJsonResponse(conn, ignore_404=False)
    except GerritError as e:
      if e.http_status == 422:  # "Unprocessable Entity"
        LOGGER.warn('Note: "%s" not added as a %s' % (r, state.lower()))
@ -693,12 +708,15 @@ def RemoveReviewers(host, change, remove=None):
    path = 'changes/%s/reviewers/%s' % (change, r)
    conn = CreateHttpConn(host, path, reqtype='DELETE')
    try:
-      ReadHttpResponse(conn, accept_statuses=[204])
+      ReadHttpResponse(conn, ignore_404=False)
    except GerritError as e:
+      # On success, gerrit returns status 204; anything else is an error.
+      if e.http_status != 204:
+        raise
+    else:
      raise GerritError(
-          e.http_status,
-          'Received unexpected http status while deleting reviewer "%s" '
-          'from change %s' % (r, change))
+          'Unexpectedly received a 200 http status while deleting reviewer "%s"'
+          ' from change %s' % (r, change))


 def SetReview(host, change, msg=None, labels=None, notify=None):
--- a/git_cl.py
+++ b/git_cl.py
@ -2641,8 +2641,8 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
          return data

    try:
-      data = gerrit_util.GetChangeDetail(
-          self._GetGerritHost(), str(issue), options, accept_statuses=[200])
+      data = gerrit_util.GetChangeDetail(self._GetGerritHost(), str(issue),
+                                         options, ignore_404=False)
    except gerrit_util.GerritError as e:
      if e.http_status == 404:
        raise GerritChangeNotExists(issue, self.GetCodereviewServer())
--- a/presubmit_support.py
+++ b/presubmit_support.py
@ -202,7 +202,7 @@ class GerritAccessor(object):
      return gerrit_util.GetChangeDetail(
          self.host, str(issue),
          ['ALL_REVISIONS', 'DETAILED_LABELS', 'ALL_COMMITS'],
-          accept_statuses=[200])
+          ignore_404=False)
    except gerrit_util.GerritError as e:
      if e.http_status == 404:
        raise Exception('Either Gerrit issue %s doesn\'t exist, or '
--- a/testing_support/gerrit_test_case.py
+++ b/testing_support/gerrit_test_case.py
@ -208,7 +208,7 @@ class GerritTestCase(unittest.TestCase):
    path = 'projects/%s' % urllib.quote(name, '')
    conn = gerrit_util.CreateHttpConn(
        cls.gerrit_instance.gerrit_host, path, reqtype='PUT', body=body)
-    jmsg = gerrit_util.ReadHttpJsonResponse(conn, accept_statuses=[200, 201])
+    jmsg = gerrit_util.ReadHttpJsonResponse(conn, expect_status=201)
    assert jmsg['name'] == name

  @classmethod
--- a/tests/git_cl_test.py
+++ b/tests/git_cl_test.py
@ -2200,7 +2200,7 @@ class TestGitCl(TestCase):

  def test_patch_gerrit_not_exists(self):
    def notExists(_issue, *_, **kwargs):
-      self.assertNotIn(404, kwargs['accept_statuses'])
+      self.assertFalse(kwargs['ignore_404'])
      raise git_cl.gerrit_util.GerritError(404, '')
    self.mock(git_cl.gerrit_util, 'GetChangeDetail', notExists)