Merge lp:~rackspace-titan/nova/lp795029 into lp:~hudson-openstack/nova/trunk

[Alex Meade]

Changed requests with malformed bodies to return a HTTP 400 Bad Request instead of a HTTP 500 error.

[Brian Waldon (bcwaldon)]

This looks great, Alex. Could you actually wrap the 400 you raise with faults.Fault()? This will get us proper response formats. You can import faults in nova/api/openstack/wsgi.py

review: Needs Fixing

[Brian Waldon (bcwaldon)]

23, 38: You need to give these exceptions the proper keyword arguments. Looks like you defined MalformedRequestBody to take 'reason'.

47: How about we be a bit more specific and tell the user it was a "Malformed request body", not just a "Malformed request."

[Ed Leafe (ed-leafe)]

Adding to Brian's comment: when you raise the new exception, you need to pass the argument as a keyword; e.g.:
    raise exception.MalformedRequestBody(reason=_("malformed JSON in request body"))

Line 40 also has a capitalization inconsistency: 'Body' should be lowercase 'body'.

Line 66 can be more simply written as:
    message = _("Malformed message body: %(reason)s)"

review: Needs Fixing

[Ed Leafe (ed-leafe)]

Changes look good.

review: Approve

[Brian Waldon (bcwaldon)]

Awesome.

Awesome to the max.

review: Approve

[OpenStack Infra (hudson-openstack)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

[Ed Leafe (ed-leafe)]

review: Approve

=== modified file 'nova/api/openstack/wsgi.py'
--- nova/api/openstack/wsgi.py	2011-06-13 20:00:14 +0000
+++ nova/api/openstack/wsgi.py	2011-06-14 17:29:59 +0000
@@ -2,8 +2,10 @@
 import json
 import webob
 from xml.dom import minidom
+from xml.parsers.expat import ExpatError
 
 from nova import exception
+import faults
 from nova import log as logging
 from nova import utils
 from nova import wsgi
@@ -71,7 +73,11 @@
 class JSONDeserializer(TextDeserializer):
 
     def default(self, datastring):
-        return utils.loads(datastring)
+        try:
+            return utils.loads(datastring)
+        except ValueError:
+            raise exception.MalformedRequestBody(
+                               reason=_("malformed JSON in request body"))
 
 
 class XMLDeserializer(TextDeserializer):
@@ -86,8 +92,13 @@
 
     def default(self, datastring):
         plurals = set(self.metadata.get('plurals', {}))
-        node = minidom.parseString(datastring).childNodes[0]
-        return {node.nodeName: self._from_xml_node(node, plurals)}
+
+        try:
+            node = minidom.parseString(datastring).childNodes[0]
+            return {node.nodeName: self._from_xml_node(node, plurals)}
+        except ExpatError:
+            raise exception.MalformedRequestBody(
+                                    reason=_("malformed XML in request body"))
 
     def _from_xml_node(self, node, listnames):
         """Convert a minidom node to a simple Python type.
@@ -353,6 +364,10 @@
                                                                       request)
         except exception.InvalidContentType:
             return webob.exc.HTTPBadRequest(_("Unsupported Content-Type"))
+        except exception.MalformedRequestBody:
+            explanation = _("Malformed request body")
+            return faults.Fault(webob.exc.HTTPBadRequest(
+                                            explanation=explanation))
 
         action_result = self.dispatch(request, action, action_args)
 

=== modified file 'nova/exception.py'
--- nova/exception.py	2011-06-11 01:31:10 +0000
+++ nova/exception.py	2011-06-14 17:29:59 +0000
@@ -585,3 +585,7 @@
 
 class MigrationError(NovaException):
     message = _("Migration error") + ": %(reason)s"
+
+
+class MalformedRequestBody(NovaException):
+    message = _("Malformed message body: %(reason)s")

=== modified file 'nova/tests/api/openstack/test_api.py'
--- nova/tests/api/openstack/test_api.py	2011-04-07 18:55:42 +0000
+++ nova/tests/api/openstack/test_api.py	2011-06-14 17:29:59 +0000
@@ -15,6 +15,8 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import json
+
 import webob.exc
 import webob.dec
 
@@ -23,6 +25,7 @@
 from nova import test
 from nova.api import openstack
 from nova.api.openstack import faults
+from nova.tests.api.openstack import fakes
 
 
 class APITest(test.TestCase):
@@ -31,6 +34,24 @@
         # simpler version of the app than fakes.wsgi_app
         return openstack.FaultWrapper(inner_app)
 
+    def test_malformed_json(self):
+        req = webob.Request.blank('/')
+        req.method = 'POST'
+        req.body = '{'
+        req.headers["content-type"] = "application/json"
+
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 400)
+
+    def test_malformed_xml(self):
+        req = webob.Request.blank('/')
+        req.method = 'POST'
+        req.body = '<hi im not xml>'
+        req.headers["content-type"] = "application/xml"
+
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 400)
+
     def test_exceptions_are_converted_to_faults(self):
 
         @webob.dec.wsgify