diff --git a/gerrit_util.py b/gerrit_util.py index 1e9b61b51f..0aef46d7b5 100755 --- 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): diff --git a/git_cl.py b/git_cl.py index ac3cea8819..96c50c29ef 100755 --- 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()) diff --git a/presubmit_support.py b/presubmit_support.py index 4056f5b966..ab43d97f69 100755 --- 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 ' diff --git a/testing_support/gerrit_test_case.py b/testing_support/gerrit_test_case.py index bc95694f2b..63a8dae7c7 100644 --- 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 diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 6e5d36998b..7c44f1578a 100755 --- 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)