Be more flexible with HTTP response codes.

post-review's HTTP GET/POST code always assumed that any Review Board API
error code would be in an HTTP 200, and that anything else was an unknown
failure. It's now more flexible and understands that any HTTP status code
could have a JSON payload in it, and processes them accordingly.

This introduces some new unit tests for testing the error parsing code
paths.

Reviewed at http://reviews.reviewboard.org/r/1375/

2 files changed, 164 insertions, 40 deletions

diff --git a/rbtools/postreview.py b/rbtools/postreview.py
index effa801..3b026d2 100755
--- a/rbtools/postreview.py
+++ b/rbtools/postreview.py
@@ -113,7 +113,22 @@ options = None
 
 
 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 @@ class SvnRepositoryInfo(RepositoryInfo):
             # 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 @@ class ReviewBoardServer(object):
                 '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 @@ class ReviewBoardServer(object):
 
             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 @@ class ReviewBoardServer(object):
         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 @@ class ReviewBoardServer(object):
         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 @@ class ReviewBoardServer(object):
         """
         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 @@ class ReviewBoardServer(object):
 
             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 f0bc369..f0906e3 100644
--- a/rbtools/tests.py
+++ b/rbtools/tests.py
@@ -4,9 +4,21 @@ import shutil
 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 @@ class GitClientTests(unittest.TestCase):
         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))