diff --git a/rbtools/postreview.py b/rbtools/postreview.py index effa8012..3b026d2a 100755 --- a/rbtools/postreview.py +++ b/rbtools/postreview.py @@ -113,7 +113,22 @@ class APIError(Exception): - pass + def __init__(self, http_status, error_code, rsp=None, *args, **kwargs): + Exception.__init__(self, *args, **kwargs) + self.http_status = http_status + self.error_code = error_code + self.rsp = rsp + + def __str__(self): + code_str = "HTTP %d" % self.http_status + + if self.error_code: + code_str += ', API Error %d' % self.error_code + + if 'err' in self.rsp: + return '%s (%s)' % (self.rsp['err']['msg'], code_str) + else: + return code_str class RepositoryInfo: @@ -194,8 +209,7 @@ def _get_repository_info(self, server, repository): # If the server couldn't fetch the repository info, it will return # code 210. Ignore those. # Other more serious errors should still be raised, though. - rsp = e.args[0] - if rsp['err']['code'] == 210: + if e.error_code == 210: return None raise e @@ -327,10 +341,7 @@ def login(self, force=False): 'password': password, }) except APIError, e: - rsp, = e.args - - die("Unable to log in: %s (%s)" % (rsp["err"]["msg"], - rsp["err"]["code"])) + die("Unable to log in: %s" % e) debug("Logged in.") @@ -405,9 +416,7 @@ def new_review_request(self, changenum, submit_as=None): rsp = self.api_post('api/json/reviewrequests/new/', data) except APIError, e: - rsp, = e.args - - if rsp['err']['code'] == 204: # Change number in use + if e.error_code == 204: # Change number in use if options.diff_only: # In this case, fall through and return to tempt_fate. debug("Review request already exists.") @@ -517,10 +526,26 @@ def process_json(self, data): rsp = json.loads(data) if rsp['stat'] == 'fail': - raise APIError, rsp + self.process_error(200, data) return rsp + def process_error(self, http_status, data): + """Processes an error, raising an APIError with the information.""" + try: + rsp = json.loads(data) + + assert rsp['stat'] == 'fail' + + debug("Got API Error %d (HTTP code %d): %s" % + (rsp['err']['code'], http_status, rsp['err']['msg'])) + debug("Error data: %r" % rsp) + raise APIError(http_status, rsp['err']['code'], rsp, + rsp['err']['msg']) + except ValueError: + debug("Got HTTP error: %s: %s" % (http_status, data)) + raise APIError(http_status, None, None, data) + def http_get(self, path): """ Performs an HTTP GET on the specified path, storing any cookies that @@ -529,19 +554,9 @@ def http_get(self, path): debug('HTTP GETting %s' % path) url = self._make_url(path) - - try: - rsp = urllib2.urlopen(url).read() - self.cookie_jar.save(self.cookie_file) - return rsp - except urllib2.HTTPError, e: - print "Unable to access %s (%s). The host path may be invalid" % \ - (url, e.code) - try: - debug(e.read()) - except AttributeError: - pass - die() + rsp = urllib2.urlopen(url).read() + self.cookie_jar.save(self.cookie_file) + return rsp def _make_url(self, path): """Given a path on the server returns a full http:// style url""" @@ -559,7 +574,10 @@ def api_get(self, path): """ Performs an API call using HTTP GET at the specified path. """ - return self.process_json(self.http_get(path)) + try: + return self.process_json(self.http_get(path)) + except urllib2.HTTPError, e: + self.process_error(e.code, e.read()) def http_post(self, path, fields, files=None): """ @@ -595,15 +613,15 @@ def http_post(self, path, fields, files=None): die("Unable to access %s. The host path may be invalid\n%s" % \ (url, e)) - except urllib2.HTTPError, e: - die("Unable to access %s (%s). The host path may be invalid\n%s" % \ - (url, e.code, e.read())) def api_post(self, path, fields=None, files=None): """ Performs an API call using HTTP POST at the specified path. """ - return self.process_json(self.http_post(path, fields, files)) + try: + return self.process_json(self.http_post(path, fields, files)) + except urllib2.HTTPError, e: + self.process_error(e.code, e.read()) def _encode_multipart_formdata(self, fields, files): """ @@ -2430,8 +2448,7 @@ def tempt_fate(server, tool, changenum, diff_content=None, server.set_review_request_field(review_request, 'testing_done', options.testing_done) except APIError, e: - rsp, = e.args - if rsp['err']['code'] == 103: # Not logged in + if e.error_code == 103: # Not logged in retries = retries - 1 # We had an odd issue where the server ended up a couple of @@ -2446,11 +2463,9 @@ def tempt_fate(server, tool, changenum, diff_content=None, return if options.rid: - die("Error getting review request %s: %s (code %s)" % \ - (options.rid, rsp['err']['msg'], rsp['err']['code'])) + die("Error getting review request %s: %s" % (options.rid, e)) else: - die("Error creating review request: %s (code %s)" % \ - (rsp['err']['msg'], rsp['err']['code'])) + die("Error creating review request: %s" % e) if not server.info.supports_changesets or not options.change_only: @@ -2458,10 +2473,7 @@ def tempt_fate(server, tool, changenum, diff_content=None, server.upload_diff(review_request, diff_content, parent_diff_content) except APIError, e: - rsp, = e.args - print "Error uploading diff: %s (%s)" % (rsp['err']['msg'], - rsp['err']['code']) - debug(rsp) + print "Error uploading diff: %s" % e die("Your review request still exists, but the diff is not " + "attached.") diff --git a/rbtools/tests.py b/rbtools/tests.py index f0bc369d..f0906e3c 100644 --- a/rbtools/tests.py +++ b/rbtools/tests.py @@ -4,9 +4,21 @@ import sys import tempfile import unittest +import urllib2 + +try: + from cStringIO import StringIO +except ImportError: + from StringIO import StringIO + +try: + import json +except ImportError: + import simplejson as json from rbtools.postreview import execute, load_config_file -from rbtools.postreview import GitClient, RepositoryInfo +from rbtools.postreview import APIError, GitClient, RepositoryInfo, \ + ReviewBoardServer import rbtools.postreview @@ -87,6 +99,32 @@ def is_exe_in_path(name): return False +class MockHttpUnitTest(unittest.TestCase): + def setUp(self): + # Save the old http_get and http_post + self.saved_http_get = ReviewBoardServer.http_get + self.saved_http_post = ReviewBoardServer.http_post + + self.server = ReviewBoardServer('http://localhost:8080/', + RepositoryInfo(), None) + ReviewBoardServer.http_get = self._http_method + ReviewBoardServer.http_post = self._http_method + + self.http_response = "" + + rbtools.postreview.options = OptionsStub() + + def tearDown(self): + ReviewBoardServer.http_get = self.saved_http_get + ReviewBoardServer.http_post = self.saved_http_post + + def _http_method(self, *args, **kwargs): + if isinstance(self.http_response, Exception): + raise self.http_response + else: + return self.http_response + + class OptionsStub(object): def __init__(self): self.debug = True @@ -377,3 +415,77 @@ def test_diff_tracking_override(self): ri = self.client.get_repository_info() self.assertEqual(self.client.diff(None), (diff, None)) + +class ApiTests(MockHttpUnitTest): + SAMPLE_ERROR_STR = json.dumps({ + 'stat': 'fail', + 'err': { + 'code': 100, + 'msg': 'This is a test failure', + } + }) + + def test_parse_get_error_http_200(self): + self.http_response = self.SAMPLE_ERROR_STR + + try: + data = self.server.api_get('/foo/') + + # Shouldn't be reached + self._assert(False) + except APIError, e: + self.assertEqual(e.http_status, 200) + self.assertEqual(e.error_code, 100) + self.assertEqual(e.rsp['stat'], 'fail') + self.assertEqual(str(e), + 'This is a test failure (HTTP 200, API Error 100)') + + def test_parse_post_error_http_200(self): + self.http_response = self.SAMPLE_ERROR_STR + + try: + data = self.server.api_post('/foo/') + + # Shouldn't be reached + self._assert(False) + except APIError, e: + self.assertEqual(e.http_status, 200) + self.assertEqual(e.error_code, 100) + self.assertEqual(e.rsp['stat'], 'fail') + self.assertEqual(str(e), + 'This is a test failure (HTTP 200, API Error 100)') + + def test_parse_get_error_http_400(self): + self.http_response = self._make_http_error('/foo/', 400, + self.SAMPLE_ERROR_STR) + + try: + data = self.server.api_get('/foo/') + + # Shouldn't be reached + self._assert(False) + except APIError, e: + self.assertEqual(e.http_status, 400) + self.assertEqual(e.error_code, 100) + self.assertEqual(e.rsp['stat'], 'fail') + self.assertEqual(str(e), + 'This is a test failure (HTTP 400, API Error 100)') + + def test_parse_post_error_http_400(self): + self.http_response = self._make_http_error('/foo/', 400, + self.SAMPLE_ERROR_STR) + + try: + data = self.server.api_post('/foo/') + + # Shouldn't be reached + self._assert(False) + except APIError, e: + self.assertEqual(e.http_status, 400) + self.assertEqual(e.error_code, 100) + self.assertEqual(e.rsp['stat'], 'fail') + self.assertEqual(str(e), + 'This is a test failure (HTTP 400, API Error 100)') + + def _make_http_error(self, url, code, body): + return urllib2.HTTPError(url, code, body, {}, StringIO(body))