Merge lp:~reldan/nova/lp766282 into lp:~hudson-openstack/nova/trunk

[Eldar Nugaev]

Fix bug with content-type and small OpenStack API actions refactor

[Brian Waldon (bcwaldon)]

157: This test is a bit misleading. You set the content-type to json, but you have an xml body. Was this the intention, or was it just a copy/paste mistake?

189-201: Would you mind using self.content_type rather than parsing it out yourself with regex? The class you are working in is a webob.Request, so you should be able to take this approach: http://pythonpaste.org/webob/reference.html#id4

I'm also not too keen on the removal of all the try/except's from the _action_... methods. Why did you do this? You really shouldn't sneak in changes that aren't directly related to the bug you are fixing. If you feel differently, I would love to hear your thoughts.

review: Needs Fixing

[Eldar Nugaev (reldan)]

Thank you for the review!

> 157: This test is a bit misleading. You set the content-type to json, but you
> have an xml body. Was this the intention, or was it just a copy/paste mistake?
>
Just removed this line.

> 189-201: Would you mind using self.content_type rather than parsing it out
> yourself with regex? The class you are working in is a webob.Request, so you
> should be able to take this approach:
> http://pythonpaste.org/webob/reference.html#id4

Thank you. Fixed.
>
> I'm also not too keen on the removal of all the try/except's from the
> _action_... methods. Why did you do this? You really shouldn't sneak in
> changes that aren't directly related to the bug you are fixing. If you feel
> differently, I would love to hear your thoughts.

Just cleaning some old ugly code. If you really against it let me just revert this changes.

[Brian Waldon (bcwaldon)]

Thanks for the fixes.

> > I'm also not too keen on the removal of all the try/except's from the
> > _action_... methods. Why did you do this? You really shouldn't sneak in
> > changes that aren't directly related to the bug you are fixing. If you feel
> > differently, I would love to hear your thoughts.
>
> Just cleaning some old ugly code. If you really against it let me just revert
> this changes.

Like I said, I think we should really try and respect the scope defined in the bug report. Let's see if we can get somebody else to weigh in on this.

[Jay Pipes (jaypipes)]

Hi!

I can understand why you wanted to clean up the try/except redundant blocks of code, Eldar, and you did a good job for the most part. However, in this particular block, you altered the logic of the code:

72 - def _action_reboot(self, input_dict, req, id):
73 - try:
74 - reboot_type = input_dict['reboot']['type']
75 - except Exception:
76 - raise faults.Fault(exc.HTTPNotImplemented())
77 - try:
78 - # TODO(gundlach): pass reboot_type, support soft reboot in
79 - # virt driver
80 - self.compute_api.reboot(req.environ['nova.context'], id)
81 - except:
82 - return faults.Fault(exc.HTTPUnprocessableEntity())
83 + def _action_reboot(self, context, input_dict, id):
84 + reboot_type = input_dict['reboot']['type']
85 + # TODO(gundlach): pass reboot_type, support soft reboot in
86 + # virt driver
87 + self.compute_api.reboot(context, id)
88 return exc.HTTPAccepted()

Note that originally, if the input_dict did not contain the nested key ['reboot']['type'], an HTTPNotImplemented would be thrown. However, in the updated code, a KeyError will be thrown :)

So, there is a bit of a danger in these kind of cleanups, which is generally why it's good to make them a separate patch... in this particular case, due to the change in logic demonstrated above, I think it's best to follow Brian's request and remove the specific cleanups and leave just the bug fix. Then, submit a separate patch with the code cleanups (just remember to fix the above logic error! ;)

Cheers!
jay

review: Needs Fixing

[Brian Waldon (bcwaldon)]

Good catch, Jay. These kinds of issues are exactly what I was thinking about.

Eldar, I would still greatly encourage you to submit your refactoring as a separate patch. Any improvements to the code base will be welcomed with open arms!

[Eldar Nugaev (reldan)]

Hi Jay.
Glad to see you!

Yeap, I'll do it by the separate path. Let'me fix it.
But there are no error in my code :)

1) We should not raise HTTPNotImplemented exception here. We have malformed request without 'type' field and should raise BadRequest

73 - try:
74 - reboot_type = input_dict['reboot']['type']
75 - except Exception:
76 - raise faults.Fault(exc.HTTPNotImplemented())

2) More over - we should not raise faults.Fault(exc.HTTPNotImplemented()), we should return faults.Fault(exc.HTTPNotImplemented()) instead of.

3) I catch KeyError in this place, create right log and return the right fault - HttpBadRequest()

13 + try:
14 + context = req.environ['nova.context']
15 + return actions[key](context, input_dict, id)
16 + except Exception, e:
17 + LOG.exception(_("Error in action %(key)s: %(e)s") %
18 + locals())
19 + return faults.Fault(exc.HTTPBadRequest())

[Jay Pipes (jaypipes)]

> Hi Jay.
> Glad to see you!
>
> Yeap, I'll do it by the separate path. Let'me fix it.
> But there are no error in my code :)

Cool, thanks :)

> 1) We should not raise HTTPNotImplemented exception here. We have malformed
> request without 'type' field and should raise BadRequest
>
> 73 - try:
> 74 - reboot_type = input_dict['reboot']['type']
> 75 - except Exception:
> 76 - raise faults.Fault(exc.HTTPNotImplemented())
>
> 2) More over - we should not raise faults.Fault(exc.HTTPNotImplemented()), we
> should return faults.Fault(exc.HTTPNotImplemented()) instead of.
>
> 3) I catch KeyError in this place, create right log and return the right fault
> - HttpBadRequest()
>
> 13 + try:
> 14 + context = req.environ['nova.context']
> 15 + return actions[key](context, input_dict, id)
> 16 + except Exception, e:
> 17 + LOG.exception(_("Error in action %(key)s: %(e)s")
> %
> 18 + locals())
> 19 + return faults.Fault(exc.HTTPBadRequest())

Well, you may assert the above is the correct behaviour, and it may well be, but I think you can acknowledge that your patch *changes* the existing behaviour (changes from returning a HTTPNotImplementedError to an HTTPBadRequest). We can discuss what's the "correct" behaviour in your future patch, since it's tangential to this particular bug :)

See you on the other merge prop! ;)

-jay

[Jay Pipes (jaypipes)]

lgtm.

review: Approve

[Brian Waldon (bcwaldon)]

Looks good!

Just a word of advice: you don't always need to resubmit the merge prop by like this. The best process is to toggle the status to "Work in Progress", then back to "Needs Review."

review: Approve

[Devin Carlen (devcamcar)]

lgtm

review: Approve

[OpenStack Infra (hudson-openstack)]

Attempt to merge into lp:nova failed due to conflicts:

text conflict in nova/wsgi.py

[lp:~reldan/nova/lp766282]

1010. By Eldar Nugaev on 2011-04-21

merge with trunk

[Eldar Nugaev (reldan)]

Fixed text conflict

=== modified file 'nova/tests/api/test_wsgi.py'
--- nova/tests/api/test_wsgi.py	2011-03-18 13:56:05 +0000
+++ nova/tests/api/test_wsgi.py	2011-04-21 23:26:45 +0000
@@ -136,6 +136,12 @@
         request.body = "asdf<br />"
         self.assertRaises(webob.exc.HTTPBadRequest, request.get_content_type)
 
+    def test_request_content_type_with_charset(self):
+        request = wsgi.Request.blank('/tests/123')
+        request.headers["Content-Type"] = "application/json; charset=UTF-8"
+        result = request.get_content_type()
+        self.assertEqual(result, "application/json")
+
     def test_content_type_from_accept_xml(self):
         request = wsgi.Request.blank('/tests/123')
         request.headers["Accept"] = "application/xml"

=== modified file 'nova/wsgi.py'
--- nova/wsgi.py	2011-04-20 19:08:24 +0000
+++ nova/wsgi.py	2011-04-21 23:26:45 +0000
@@ -102,12 +102,16 @@
         return bm or 'application/json'
 
     def get_content_type(self):
-        try:
-            ct = self.headers['Content-Type']
-            assert ct in ('application/xml', 'application/json')
-            return ct
-        except Exception:
-            raise webob.exc.HTTPBadRequest('Invalid content type')
+        allowed_types = ("application/xml", "application/json")
+        if not "Content-Type" in self.headers:
+            msg = _("Missing Content-Type")
+            LOG.debug(msg)
+            raise webob.exc.HTTPBadRequest(msg)
+        type = self.content_type
+        if type in allowed_types:
+            return type
+        LOG.debug(_("Wrong Content-Type: %s") % type)
+        raise webob.exc.HTTPBadRequest("Invalid content type")
 
 
 class Application(object):