Skip to content

Commit

Permalink
Be more flexible with HTTP response codes.
Browse files Browse the repository at this point in the history 
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/
  • Loading branch information
@chipx86
chipx86 committed Feb 6, 2010
1 parent a822ccf commit 5968f84
Show file tree
Hide file tree
Showing 2 changed files with 164 additions and 40 deletions.
90 changes: 51 additions & 39 deletions rbtools/postreview.py
View file
Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.")

Expand Down Expand Up @@ -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.")
Expand Down Expand Up @@ -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
Expand All @@ -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"""
Expand All @@ -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):
"""
Expand Down Expand Up @@ -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):
"""
Expand Down Expand Up @@ -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
Expand All @@ -2446,22 +2463,17 @@ 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:
try:
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.")

Expand Down
114 changes: 113 additions & 1 deletion rbtools/tests.py
View file
Expand Up @@ -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


Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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))

0 comments on commit 5968f84

Please sign in to comment.