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

Proposed by Alex Meade
Status: Merged
Approved by: Brian Waldon
Approved revision: 1181
Merged at revision: 1181
Proposed branch: lp:~rackspace-titan/nova/lp795029
Merge into: lp:~hudson-openstack/nova/trunk
Diff against target: 112 lines (+43/-3)
3 files modified
nova/api/openstack/wsgi.py (+18/-3)
nova/exception.py (+4/-0)
nova/tests/api/openstack/test_api.py (+21/-0)
To merge this branch: bzr merge lp:~rackspace-titan/nova/lp795029
Reviewer Review Type Date Requested Status
Ed Leafe (community) Approve
Brian Waldon (community) Approve
Review via email: mp+64558@code.launchpad.net

Description of the change

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

To post a comment you must log in.
Revision history for this message
Brian Waldon (bcwaldon) wrote :

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
Revision history for this message
Brian Waldon (bcwaldon) wrote :

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."

Revision history for this message
Ed Leafe (ed-leafe) wrote :

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
Revision history for this message
Ed Leafe (ed-leafe) wrote :

Changes look good.

review: Approve
Revision history for this message
Brian Waldon (bcwaldon) wrote :

Awesome.

Awesome to the max.

review: Approve
Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

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

Revision history for this message
Ed Leafe (ed-leafe) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'nova/api/openstack/wsgi.py'
2--- nova/api/openstack/wsgi.py 2011-06-13 20:00:14 +0000
3+++ nova/api/openstack/wsgi.py 2011-06-14 17:29:59 +0000
4@@ -2,8 +2,10 @@
5 import json
6 import webob
7 from xml.dom import minidom
8+from xml.parsers.expat import ExpatError
9
10 from nova import exception
11+import faults
12 from nova import log as logging
13 from nova import utils
14 from nova import wsgi
15@@ -71,7 +73,11 @@
16 class JSONDeserializer(TextDeserializer):
17
18 def default(self, datastring):
19- return utils.loads(datastring)
20+ try:
21+ return utils.loads(datastring)
22+ except ValueError:
23+ raise exception.MalformedRequestBody(
24+ reason=_("malformed JSON in request body"))
25
26
27 class XMLDeserializer(TextDeserializer):
28@@ -86,8 +92,13 @@
29
30 def default(self, datastring):
31 plurals = set(self.metadata.get('plurals', {}))
32- node = minidom.parseString(datastring).childNodes[0]
33- return {node.nodeName: self._from_xml_node(node, plurals)}
34+
35+ try:
36+ node = minidom.parseString(datastring).childNodes[0]
37+ return {node.nodeName: self._from_xml_node(node, plurals)}
38+ except ExpatError:
39+ raise exception.MalformedRequestBody(
40+ reason=_("malformed XML in request body"))
41
42 def _from_xml_node(self, node, listnames):
43 """Convert a minidom node to a simple Python type.
44@@ -353,6 +364,10 @@
45 request)
46 except exception.InvalidContentType:
47 return webob.exc.HTTPBadRequest(_("Unsupported Content-Type"))
48+ except exception.MalformedRequestBody:
49+ explanation = _("Malformed request body")
50+ return faults.Fault(webob.exc.HTTPBadRequest(
51+ explanation=explanation))
52
53 action_result = self.dispatch(request, action, action_args)
54
55
56=== modified file 'nova/exception.py'
57--- nova/exception.py 2011-06-11 01:31:10 +0000
58+++ nova/exception.py 2011-06-14 17:29:59 +0000
59@@ -585,3 +585,7 @@
60
61 class MigrationError(NovaException):
62 message = _("Migration error") + ": %(reason)s"
63+
64+
65+class MalformedRequestBody(NovaException):
66+ message = _("Malformed message body: %(reason)s")
67
68=== modified file 'nova/tests/api/openstack/test_api.py'
69--- nova/tests/api/openstack/test_api.py 2011-04-07 18:55:42 +0000
70+++ nova/tests/api/openstack/test_api.py 2011-06-14 17:29:59 +0000
71@@ -15,6 +15,8 @@
72 # License for the specific language governing permissions and limitations
73 # under the License.
74
75+import json
76+
77 import webob.exc
78 import webob.dec
79
80@@ -23,6 +25,7 @@
81 from nova import test
82 from nova.api import openstack
83 from nova.api.openstack import faults
84+from nova.tests.api.openstack import fakes
85
86
87 class APITest(test.TestCase):
88@@ -31,6 +34,24 @@
89 # simpler version of the app than fakes.wsgi_app
90 return openstack.FaultWrapper(inner_app)
91
92+ def test_malformed_json(self):
93+ req = webob.Request.blank('/')
94+ req.method = 'POST'
95+ req.body = '{'
96+ req.headers["content-type"] = "application/json"
97+
98+ res = req.get_response(fakes.wsgi_app())
99+ self.assertEqual(res.status_int, 400)
100+
101+ def test_malformed_xml(self):
102+ req = webob.Request.blank('/')
103+ req.method = 'POST'
104+ req.body = '<hi im not xml>'
105+ req.headers["content-type"] = "application/xml"
106+
107+ res = req.get_response(fakes.wsgi_app())
108+ self.assertEqual(res.status_int, 400)
109+
110 def test_exceptions_are_converted_to_faults(self):
111
112 @webob.dec.wsgify