From 20b54fbefa94c545bea868dc1fecafd99649b9ba Mon Sep 17 00:00:00 2001
From: Joe Gregorio <jcgregorio@google.com>
Date: Thu, 26 Jul 2012 09:59:35 -0400
Subject: [PATCH] More robust picking up JSON error responses.

Fixes issue #169.

Another CL towards fixing 169, by making sure we parse the JSON error response
in more cases.

Reviewed in http://codereview.appspot.com/6447045/.
---
 apiclient/errors.py      | 14 ++++++--------
 tests/test_errors.py     | 30 ++++++++++++++++++------------
 tests/test_http.py       | 24 +++++++++++-------------
 tests/test_json_model.py |  2 +-
 4 files changed, 36 insertions(+), 34 deletions(-)

diff --git a/apiclient/errors.py b/apiclient/errors.py
index b58d97f..e744519 100644
--- a/apiclient/errors.py
+++ b/apiclient/errors.py
@@ -41,14 +41,12 @@ class HttpError(Error):
 
   def _get_reason(self):
     """Calculate the reason for the error from the response content."""
-    if self.resp.get('content-type', '').startswith('application/json'):
-      try:
-        data = simplejson.loads(self.content)
-        reason = data['error']['message']
-      except (ValueError, KeyError):
-        reason = self.content
-    else:
-      reason = self.resp.reason
+    reason = self.resp.reason
+    try:
+      data = simplejson.loads(self.content)
+      reason = data['error']['message']
+    except (ValueError, KeyError):
+      pass
     return reason
 
   def __repr__(self):
diff --git a/tests/test_errors.py b/tests/test_errors.py
index ab0be28..25e5d2c 100644
--- a/tests/test_errors.py
+++ b/tests/test_errors.py
@@ -45,8 +45,10 @@ JSON_ERROR_CONTENT = """
 }
 """
 
-def fake_response(data, headers):
-  return httplib2.Response(headers), data
+def fake_response(data, headers, reason='Ok'):
+  response = httplib2.Response(headers)
+  response.reason = reason
+  return response, data
 
 
 class Error(unittest.TestCase):
@@ -55,33 +57,37 @@ class Error(unittest.TestCase):
   def test_json_body(self):
     """Test a nicely formed, expected error response."""
     resp, content = fake_response(JSON_ERROR_CONTENT,
-        {'status':'400', 'content-type': 'application/json'})
-    error = HttpError(resp, content)
-    self.assertEqual(str(error), '<HttpError 400 "country is required">')
+        {'status':'400', 'content-type': 'application/json'},
+        reason='Failed')
+    error = HttpError(resp, content, 'http://example.org')
+    self.assertEqual(str(error), '<HttpError 400 when requesting http://example.org returned "country is required">')
 
   def test_bad_json_body(self):
     """Test handling of bodies with invalid json."""
     resp, content = fake_response('{',
-        {'status':'400', 'content-type': 'application/json'})
+        { 'status':'400', 'content-type': 'application/json'},
+        reason='Failed')
     error = HttpError(resp, content)
-    self.assertEqual(str(error), '<HttpError 400 "{">')
+    self.assertEqual(str(error), '<HttpError 400 "Failed">')
 
   def test_with_uri(self):
     """Test handling of passing in the request uri."""
     resp, content = fake_response('{',
-        {'status':'400', 'content-type': 'application/json'})
+        {'status':'400', 'content-type': 'application/json'},
+        reason='Failure')
     error = HttpError(resp, content, 'http://example.org')
-    self.assertEqual(str(error), '<HttpError 400 when requesting http://example.org returned "{">')
+    self.assertEqual(str(error), '<HttpError 400 when requesting http://example.org returned "Failure">')
 
   def test_missing_message_json_body(self):
     """Test handling of bodies with missing expected 'message' element."""
     resp, content = fake_response('{}',
-        {'status':'400', 'content-type': 'application/json'})
+        {'status':'400', 'content-type': 'application/json'},
+        reason='Failed')
     error = HttpError(resp, content)
-    self.assertEqual(str(error), '<HttpError 400 "{}">')
+    self.assertEqual(str(error), '<HttpError 400 "Failed">')
 
   def test_non_json(self):
     """Test handling of non-JSON bodies"""
-    resp, content = fake_response('NOT OK', {'status':'400'})
+    resp, content = fake_response('}NOT OK', {'status':'400'})
     error = HttpError(resp, content)
     self.assertEqual(str(error), '<HttpError 400 "Ok">')
diff --git a/tests/test_http.py b/tests/test_http.py
index 95de47a..d067afe 100644
--- a/tests/test_http.py
+++ b/tests/test_http.py
@@ -354,7 +354,7 @@ content-length: 0\r\n\r\n"""
 
 
 RESPONSE = """HTTP/1.1 200 OK
-Content-Type application/json
+Content-Type: application/json
 Content-Length: 14
 ETag: "etag/pony"\r\n\r\n{"answer": 42}"""
 
@@ -365,7 +365,7 @@ Content-Transfer-Encoding: binary
 Content-ID: <randomness+1>
 
 HTTP/1.1 200 OK
-Content-Type application/json
+Content-Type: application/json
 Content-Length: 14
 ETag: "etag/pony"\r\n\r\n{"foo": 42}
 
@@ -375,7 +375,7 @@ Content-Transfer-Encoding: binary
 Content-ID: <randomness+2>
 
 HTTP/1.1 200 OK
-Content-Type application/json
+Content-Type: application/json
 Content-Length: 14
 ETag: "etag/sheep"\r\n\r\n{"baz": "qux"}
 --batch_foobarbaz--"""
@@ -387,7 +387,7 @@ Content-Transfer-Encoding: binary
 Content-ID: <randomness+1>
 
 HTTP/1.1 200 OK
-Content-Type application/json
+Content-Type: application/json
 Content-Length: 14
 ETag: "etag/pony"\r\n\r\n{"foo": 42}
 
@@ -397,11 +397,9 @@ Content-Transfer-Encoding: binary
 Content-ID: <randomness+2>
 
 HTTP/1.1 403 Access Not Configured
-Content-Type application/json
-Content-Length: 14
-ETag: "etag/sheep"\r\n\r\nContent-Length: 245
-
-{
+Content-Type: application/json
+Content-Length: 245
+ETag: "etag/sheep"\r\n\r\n{
  "error": {
   "errors": [
    {
@@ -425,7 +423,7 @@ Content-Transfer-Encoding: binary
 Content-ID: <randomness+1>
 
 HTTP/1.1 401 Authorization Required
-Content-Type application/json
+Content-Type: application/json
 Content-Length: 14
 ETag: "etag/pony"\r\n\r\n{"error": {"message":
   "Authorizaton failed."}}
@@ -436,7 +434,7 @@ Content-Transfer-Encoding: binary
 Content-ID: <randomness+2>
 
 HTTP/1.1 200 OK
-Content-Type application/json
+Content-Type: application/json
 Content-Length: 14
 ETag: "etag/sheep"\r\n\r\n{"baz": "qux"}
 --batch_foobarbaz--"""
@@ -448,7 +446,7 @@ Content-Transfer-Encoding: binary
 Content-ID: <randomness+1>
 
 HTTP/1.1 200 OK
-Content-Type application/json
+Content-Type: application/json
 Content-Length: 14
 ETag: "etag/pony"\r\n\r\n{"foo": 42}
 --batch_foobarbaz--"""
@@ -708,7 +706,7 @@ class TestBatch(unittest.TestCase):
     self.assertEqual({'foo': 42}, callbacks.responses['1'])
     self.assertEqual({'baz': 'qux'}, callbacks.responses['2'])
 
-  def test_execute_http_error(self):
+  def test_execute_batch_http_error(self):
     callbacks = Callbacks()
     batch = BatchHttpRequest(callback=callbacks.f)
 
diff --git a/tests/test_json_model.py b/tests/test_json_model.py
index 43b0421..ececdab 100644
--- a/tests/test_json_model.py
+++ b/tests/test_json_model.py
@@ -147,7 +147,7 @@ class Model(unittest.TestCase):
       content = model.response(resp, content)
       self.fail('Should have thrown an exception')
     except HttpError, e:
-      self.assertTrue('Unauthorized' in str(e))
+      self.assertTrue('not authorized' in str(e))
 
     resp['content-type'] = 'application/json'