Merge lp:~johannes.erdfelt/nova/lp844910 into lp:~hudson-openstack/nova/trunk

[Johannes Erdfelt]

The 1.1 API specifies that two vendor content types are allowed in addition to the standard JSON and XML content types.

This branch adds support for application/vnd.openstack.compute+json and application/vnd.openstack.compute+xml.

[Brian Waldon (bcwaldon)]

Personally, I feel like these new content types are just aliases to their simpler counterparts. I don't really think it is necessary to map them individually for (de)serialization. Did you think about writing a simple middleware that just simplifies the content types?

[Johannes Erdfelt (johannes.erdfelt)]

I had thought about that, but if you alias the values, then the content-type returned won't necessarily match what is requested in the accept header.

[Brian Waldon (bcwaldon)]

Being middleware, you could remember that you aliased the value in the request and set it back in the response.

[Johannes Erdfelt (johannes.erdfelt)]

True, but the code would be relatively complicated. Much of the code in nova/api/openstack/wsgi.py deals with determining the best match for the accept header. This isn't trivial since content types can be weighted.

To handle the case of "application/vnd.openstack.compute+xml; q=0.2, application/xml; q=0.8" would require some fairly sophisticated code to parse out the accept, choose the best match, possibly replace/remove it from the accept header and then possibly replace the content-type on the response path.

It would duplicate much of the existing code.

The proposed branch is extremely straight forward and has no tricky code in it. I still think it's the better option.

[Jay Pipes (jaypipes)]

> True, but the code would be relatively complicated. Much of the code in
> nova/api/openstack/wsgi.py deals with determining the best match for the
> accept header. This isn't trivial since content types can be weighted.
>
> To handle the case of "application/vnd.openstack.compute+xml; q=0.2,
> application/xml; q=0.8" would require some fairly sophisticated code to parse
> out the accept, choose the best match, possibly replace/remove it from the
> accept header and then possibly replace the content-type on the response path.

Sophisticated code that's actually already written :)

http://docs.webob.org/en/latest/reference.html#accept-headers

-jay

[Johannes Erdfelt (johannes.erdfelt)]

The sophisticated code I meant was the code to wrap up the webob best match code with code to modify the Accept header.

The webob code makes matching against the Accept header easy, but not modifying it. That's the sophisticated part.

[Jay Pipes (jaypipes)]

OK, I don't really see why you would need to modify the Accept header at all (is that even advisable/legal?)

[Jay Pipes (jaypipes)]

In any case, my remark is not consequential to this patch being good, which it is ;)

review: Approve

[Johannes Erdfelt (johannes.erdfelt)]

I've made changes to simplify the code a little bit and fix a bug that snuck during a merge. The vendor types will be mapped to the standard types when the code goes to serialize or deserialize. This simplifies the code while minimizing the changes to all of the other code that handles XML serialization.

I also added a couple of more tests and a missing copyright.

[Brian Waldon (bcwaldon)]

This is great, Johannes.

review: Approve

[Dan Prince (dan-prince)]

This looks good.

review: Approve

=== modified file 'nova/api/openstack/versions.py'
--- nova/api/openstack/versions.py	2011-09-02 05:07:30 +0000
+++ nova/api/openstack/versions.py	2011-09-14 17:58:40 +0000
@@ -107,7 +107,9 @@
             headers_serializer=headers_serializer)
 
         supported_content_types = ('application/json',
+                                   'application/vnd.openstack.compute+json',
                                    'application/xml',
+                                   'application/vnd.openstack.compute+xml',
                                    'application/atom+xml')
         deserializer = VersionsRequestDeserializer(
             supported_content_types=supported_content_types)
@@ -308,7 +310,9 @@
     serializer = wsgi.ResponseSerializer(body_serializers)
 
     supported_content_types = ('application/json',
+                               'application/vnd.openstack.compute+json',
                                'application/xml',
+                               'application/vnd.openstack.compute+xml',
                                'application/atom+xml')
     deserializer = wsgi.RequestDeserializer(
         supported_content_types=supported_content_types)

=== modified file 'nova/api/openstack/wsgi.py'
--- nova/api/openstack/wsgi.py	2011-08-25 20:35:04 +0000
+++ nova/api/openstack/wsgi.py	2011-09-14 17:58:40 +0000
@@ -1,3 +1,19 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2011 OpenStack LLC.
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
 
 import json
 from lxml import etree
@@ -19,6 +35,21 @@
 
 LOG = logging.getLogger('nova.api.openstack.wsgi')
 
+# The vendor content types should serialize identically to the non-vendor
+# content types. So to avoid littering the code with both options, we
+# map the vendor to the other when looking up the type
+_CONTENT_TYPE_MAP = {
+    'application/vnd.openstack.compute+json': 'application/json',
+    'application/vnd.openstack.compute+xml': 'application/xml',
+}
+
+_SUPPORTED_CONTENT_TYPES = (
+    'application/json',
+    'application/vnd.openstack.compute+json',
+    'application/xml',
+    'application/vnd.openstack.compute+xml',
+)
+
 
 class Request(webob.Request):
     """Add some Openstack API-specific logic to the base webob.Request."""
@@ -30,7 +61,7 @@
 
         """
         supported_content_types = supported_content_types or \
-            ('application/json', 'application/xml')
+            _SUPPORTED_CONTENT_TYPES
 
         parts = self.path.rsplit('.', 1)
         if len(parts) > 1:
@@ -52,7 +83,7 @@
         if not "Content-Type" in self.headers:
             return None
 
-        allowed_types = ("application/xml", "application/json")
+        allowed_types = _SUPPORTED_CONTENT_TYPES
         content_type = self.content_type
 
         if content_type not in allowed_types:
@@ -192,7 +223,7 @@
                  supported_content_types=None):
 
         self.supported_content_types = supported_content_types or \
-                ('application/json', 'application/xml')
+                _SUPPORTED_CONTENT_TYPES
 
         self.body_deserializers = {
             'application/xml': XMLDeserializer(),
@@ -250,7 +281,8 @@
 
     def get_body_deserializer(self, content_type):
         try:
-            return self.body_deserializers[content_type]
+            ctype = _CONTENT_TYPE_MAP.get(content_type, content_type)
+            return self.body_deserializers[ctype]
         except (KeyError, TypeError):
             raise exception.InvalidContentType(content_type=content_type)
 
@@ -444,7 +476,8 @@
 
     def get_body_serializer(self, content_type):
         try:
-            return self.body_serializers[content_type]
+            ctype = _CONTENT_TYPE_MAP.get(content_type, content_type)
+            return self.body_serializers[ctype]
         except (KeyError, TypeError):
             raise exception.InvalidContentType(content_type=content_type)
 

=== modified file 'nova/tests/api/openstack/test_api.py'
--- nova/tests/api/openstack/test_api.py	2011-06-14 14:16:51 +0000
+++ nova/tests/api/openstack/test_api.py	2011-09-14 17:58:40 +0000
@@ -20,6 +20,7 @@
 import webob.exc
 import webob.dec
 
+from lxml import etree
 from webob import Request
 
 from nova import test
@@ -52,6 +53,30 @@
         res = req.get_response(fakes.wsgi_app())
         self.assertEqual(res.status_int, 400)
 
+    def test_vendor_content_type_json(self):
+        ctype = 'application/vnd.openstack.compute+json'
+
+        req = webob.Request.blank('/')
+        req.headers['Accept'] = ctype
+
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 200)
+        self.assertEqual(res.content_type, ctype)
+
+        body = json.loads(res.body)
+
+    def test_vendor_content_type_xml(self):
+        ctype = 'application/vnd.openstack.compute+xml'
+
+        req = webob.Request.blank('/')
+        req.headers['Accept'] = ctype
+
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 200)
+        self.assertEqual(res.content_type, ctype)
+
+        body = etree.XML(res.body)
+
     def test_exceptions_are_converted_to_faults(self):
 
         @webob.dec.wsgify

=== modified file 'nova/tests/api/openstack/test_wsgi.py'
--- nova/tests/api/openstack/test_wsgi.py	2011-07-25 19:56:23 +0000
+++ nova/tests/api/openstack/test_wsgi.py	2011-09-14 17:58:40 +0000
@@ -27,17 +27,17 @@
         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"
-        result = request.best_match_content_type()
-        self.assertEqual(result, "application/xml")
-
-        request = wsgi.Request.blank('/tests/123')
-        request.headers["Accept"] = "application/json"
-        result = request.best_match_content_type()
-        self.assertEqual(result, "application/json")
-
+    def test_content_type_from_accept(self):
+        for content_type in ('application/xml',
+                             'application/vnd.openstack.compute+xml',
+                             'application/json',
+                             'application/vnd.openstack.compute+json'):
+            request = wsgi.Request.blank('/tests/123')
+            request.headers["Accept"] = content_type
+            result = request.best_match_content_type()
+            self.assertEqual(result, content_type)
+
+    def test_content_type_from_accept_best(self):
         request = wsgi.Request.blank('/tests/123')
         request.headers["Accept"] = "application/xml, application/json"
         result = request.best_match_content_type()
@@ -231,7 +231,7 @@
 
         self.body_serializers = {
             'application/json': JSONSerializer(),
-            'application/XML': XMLSerializer(),
+            'application/xml': XMLSerializer(),
         }
 
         self.serializer = wsgi.ResponseSerializer(self.body_serializers,
@@ -250,15 +250,24 @@
                           self.serializer.get_body_serializer,
                           'application/unknown')
 
-    def test_serialize_response(self):
-        response = self.serializer.serialize({}, 'application/json')
-        self.assertEqual(response.headers['Content-Type'], 'application/json')
-        self.assertEqual(response.body, 'pew_json')
-        self.assertEqual(response.status_int, 404)
+    def test_serialize_response_json(self):
+        for content_type in ('application/json',
+                             'application/vnd.openstack.compute+json'):
+            response = self.serializer.serialize({}, content_type)
+            self.assertEqual(response.headers['Content-Type'], content_type)
+            self.assertEqual(response.body, 'pew_json')
+            self.assertEqual(response.status_int, 404)
+
+    def test_serialize_response_xml(self):
+        for content_type in ('application/xml',
+                             'application/vnd.openstack.compute+xml'):
+            response = self.serializer.serialize({}, content_type)
+            self.assertEqual(response.headers['Content-Type'], content_type)
+            self.assertEqual(response.body, 'pew_xml')
+            self.assertEqual(response.status_int, 404)
 
     def test_serialize_response_None(self):
         response = self.serializer.serialize(None, 'application/json')
-        print response
         self.assertEqual(response.headers['Content-Type'], 'application/json')
         self.assertEqual(response.body, '')
         self.assertEqual(response.status_int, 404)
@@ -281,7 +290,7 @@
 
         self.body_deserializers = {
             'application/json': JSONDeserializer(),
-            'application/XML': XMLDeserializer(),
+            'application/xml': XMLDeserializer(),
         }
 
         self.deserializer = wsgi.RequestDeserializer(self.body_deserializers)
@@ -290,8 +299,9 @@
         pass
 
     def test_get_deserializer(self):
-        expected = self.deserializer.get_body_deserializer('application/json')
-        self.assertEqual(expected, self.body_deserializers['application/json'])
+        ctype = 'application/json'
+        expected = self.deserializer.get_body_deserializer(ctype)
+        self.assertEqual(expected, self.body_deserializers[ctype])
 
     def test_get_deserializer_unknown_content_type(self):
         self.assertRaises(exception.InvalidContentType,
@@ -299,10 +309,11 @@
                           'application/unknown')
 
     def test_get_expected_content_type(self):
+        ctype = 'application/json'
         request = wsgi.Request.blank('/')
-        request.headers['Accept'] = 'application/json'
+        request.headers['Accept'] = ctype
         self.assertEqual(self.deserializer.get_expected_content_type(request),
-                         'application/json')
+                         ctype)
 
     def test_get_action_args(self):
         env = {