Refactor wsgi.Serializer away from handling Requests directly; now require Content-Type in all requests; fix tests according to new code

13 files changed, 152 insertions, 90 deletions

diff --git a/nova/api/direct.py b/nova/api/direct.py
index cd237f649..1d699f947 100644
--- a/nova/api/direct.py
+++ b/nova/api/direct.py
@@ -206,7 +206,7 @@ class ServiceWrapper(wsgi.Controller):
         params = dict([(str(k), v) for (k, v) in params.iteritems()])
         result = method(context, **params)
         if type(result) is dict or type(result) is list:
-            return self._serialize(result, req)
+            return self._serialize(result, req.best_match())
         else:
             return result
 
diff --git a/nova/api/openstack/__init__.py b/nova/api/openstack/__init__.py
index b5439788d..6e1a2a06c 100644
--- a/nova/api/openstack/__init__.py
+++ b/nova/api/openstack/__init__.py
@@ -124,4 +124,6 @@ class Versions(wsgi.Application):
         metadata = {
             "application/xml": {
                 "attributes": dict(version=["status", "id"])}}
-        return wsgi.Serializer(req.environ, metadata).to_content_type(response)
+
+        content_type = req.best_match()
+        return wsgi.Serializer(metadata).serialize(response, content_type)
diff --git a/nova/api/openstack/consoles.py b/nova/api/openstack/consoles.py
index 9ebdbe710..8c291c2eb 100644
--- a/nova/api/openstack/consoles.py
+++ b/nova/api/openstack/consoles.py
@@ -65,7 +65,7 @@ class Controller(wsgi.Controller):
 
     def create(self, req, server_id):
         """Creates a new console"""
-        #info = self._deserialize(req.body, req)
+        #info = self._deserialize(req.body, req.get_content_type())
         self.console_api.create_console(
                                 req.environ['nova.context'],
                                 int(server_id))
diff --git a/nova/api/openstack/faults.py b/nova/api/openstack/faults.py
index c70b00fa3..075fdb997 100644
--- a/nova/api/openstack/faults.py
+++ b/nova/api/openstack/faults.py
@@ -57,6 +57,7 @@ class Fault(webob.exc.HTTPException):
             fault_data[fault_name]['retryAfter'] = retry
         # 'code' is an attribute on the fault tag itself
         metadata = {'application/xml': {'attributes': {fault_name: 'code'}}}
-        serializer = wsgi.Serializer(req.environ, metadata)
-        self.wrapped_exc.body = serializer.to_content_type(fault_data)
+        serializer = wsgi.Serializer(metadata)
+        content_type = req.best_match()
+        self.wrapped_exc.body = serializer.serialize(fault_data, content_type)
         return self.wrapped_exc
diff --git a/nova/api/openstack/images.py b/nova/api/openstack/images.py
index cf85a496f..98f0dd96b 100644
--- a/nova/api/openstack/images.py
+++ b/nova/api/openstack/images.py
@@ -151,7 +151,7 @@ class Controller(wsgi.Controller):
 
     def create(self, req):
         context = req.environ['nova.context']
-        env = self._deserialize(req.body, req)
+        env = self._deserialize(req.body, req.get_content_type())
         instance_id = env["image"]["serverId"]
         name = env["image"]["name"]
 
diff --git a/nova/api/openstack/servers.py b/nova/api/openstack/servers.py
index 08b95b46a..24d2826af 100644
--- a/nova/api/openstack/servers.py
+++ b/nova/api/openstack/servers.py
@@ -141,7 +141,7 @@ class Controller(wsgi.Controller):
 
     def create(self, req):
         """ Creates a new server for a given user """
-        env = self._deserialize(req.body, req)
+        env = self._deserialize(req.body, req.get_content_type())
         if not env:
             return faults.Fault(exc.HTTPUnprocessableEntity())
 
@@ -182,7 +182,10 @@ class Controller(wsgi.Controller):
 
     def update(self, req, id):
         """ Updates the server name or password """
-        inst_dict = self._deserialize(req.body, req)
+        if len(req.body) == 0:
+            raise exc.HTTPUnprocessableEntity()
+
+        inst_dict = self._deserialize(req.body, req.get_content_type())
         if not inst_dict:
             return faults.Fault(exc.HTTPUnprocessableEntity())
 
@@ -205,7 +208,7 @@ class Controller(wsgi.Controller):
     def action(self, req, id):
         """ Multi-purpose method used to reboot, rebuild, and
         resize a server """
-        input_dict = self._deserialize(req.body, req)
+        input_dict = self._deserialize(req.body, req.get_content_type())
         #TODO(sandy): rebuild/resize not supported.
         try:
             reboot_type = input_dict['reboot']['type']
diff --git a/nova/api/openstack/zones.py b/nova/api/openstack/zones.py
index d5206da20..cf6cd789f 100644
--- a/nova/api/openstack/zones.py
+++ b/nova/api/openstack/zones.py
@@ -67,13 +67,13 @@ class Controller(wsgi.Controller):
 
     def create(self, req):
         context = req.environ['nova.context']
-        env = self._deserialize(req.body, req)
+        env = self._deserialize(req.body, req.get_content_type())
         zone = db.zone_create(context, env["zone"])
         return dict(zone=_scrub_zone(zone))
 
     def update(self, req, id):
         context = req.environ['nova.context']
-        env = self._deserialize(req.body, req)
+        env = self._deserialize(req.body, req.get_content_type())
         zone_id = int(id)
         zone = db.zone_update(context, zone_id, env["zone"])
         return dict(zone=_scrub_zone(zone))
diff --git a/nova/exception.py b/nova/exception.py
index 7d65bd6a5..93c5fe3d7 100644
--- a/nova/exception.py
+++ b/nova/exception.py
@@ -88,6 +88,10 @@ class InvalidInputException(Error):
     pass
 
 
+class InvalidContentType(Error):
+    pass
+
+
 class TimeoutException(Error):
     pass
 
diff --git a/nova/tests/api/openstack/test_servers.py b/nova/tests/api/openstack/test_servers.py
index 78beb7df9..fae08d0be 100644
--- a/nova/tests/api/openstack/test_servers.py
+++ b/nova/tests/api/openstack/test_servers.py
@@ -227,6 +227,7 @@ class ServersTest(test.TestCase):
         req = webob.Request.blank('/v1.0/servers')
         req.method = 'POST'
         req.body = json.dumps(body)
+        req.headers["Content-Type"] = "application/json"
 
         res = req.get_response(fakes.wsgi_app())
 
diff --git a/nova/tests/api/openstack/test_zones.py b/nova/tests/api/openstack/test_zones.py
index 555b206b9..d0da8eaaf 100644
--- a/nova/tests/api/openstack/test_zones.py
+++ b/nova/tests/api/openstack/test_zones.py
@@ -86,24 +86,27 @@ class ZonesTest(test.TestCase):
 
     def test_get_zone_list(self):
         req = webob.Request.blank('/v1.0/zones')
+        req.headers["Content-Type"] = "application/json"
         res = req.get_response(fakes.wsgi_app())
-        res_dict = json.loads(res.body)
 
         self.assertEqual(res.status_int, 200)
+        res_dict = json.loads(res.body)
         self.assertEqual(len(res_dict['zones']), 2)
 
     def test_get_zone_by_id(self):
         req = webob.Request.blank('/v1.0/zones/1')
+        req.headers["Content-Type"] = "application/json"
         res = req.get_response(fakes.wsgi_app())
-        res_dict = json.loads(res.body)
 
+        self.assertEqual(res.status_int, 200)
+        res_dict = json.loads(res.body)
         self.assertEqual(res_dict['zone']['id'], 1)
         self.assertEqual(res_dict['zone']['api_url'], 'http://foo.com')
         self.assertFalse('password' in res_dict['zone'])
-        self.assertEqual(res.status_int, 200)
 
     def test_zone_delete(self):
         req = webob.Request.blank('/v1.0/zones/1')
+        req.headers["Content-Type"] = "application/json"
         res = req.get_response(fakes.wsgi_app())
 
         self.assertEqual(res.status_int, 200)
@@ -112,13 +115,14 @@ class ZonesTest(test.TestCase):
         body = dict(zone=dict(api_url='http://blah.zoo', username='fred',
                         password='fubar'))
         req = webob.Request.blank('/v1.0/zones')
+        req.headers["Content-Type"] = "application/json"
         req.method = 'POST'
         req.body = json.dumps(body)
 
         res = req.get_response(fakes.wsgi_app())
-        res_dict = json.loads(res.body)
 
         self.assertEqual(res.status_int, 200)
+        res_dict = json.loads(res.body)
         self.assertEqual(res_dict['zone']['id'], 1)
         self.assertEqual(res_dict['zone']['api_url'], 'http://blah.zoo')
         self.assertFalse('username' in res_dict['zone'])
@@ -126,13 +130,14 @@ class ZonesTest(test.TestCase):
     def test_zone_update(self):
         body = dict(zone=dict(username='zeb', password='sneaky'))
         req = webob.Request.blank('/v1.0/zones/1')
+        req.headers["Content-Type"] = "application/json"
         req.method = 'PUT'
         req.body = json.dumps(body)
 
         res = req.get_response(fakes.wsgi_app())
-        res_dict = json.loads(res.body)
 
         self.assertEqual(res.status_int, 200)
+        res_dict = json.loads(res.body)
         self.assertEqual(res_dict['zone']['id'], 1)
         self.assertEqual(res_dict['zone']['api_url'], 'http://foo.com')
         self.assertFalse('username' in res_dict['zone'])
diff --git a/nova/tests/api/test_wsgi.py b/nova/tests/api/test_wsgi.py
index cf2d0e297..7c0135656 100644
--- a/nova/tests/api/test_wsgi.py
+++ b/nova/tests/api/test_wsgi.py
@@ -93,29 +93,29 @@ class ControllerTest(test.TestCase):
         result = request.get_response(self.TestRouter())
         self.assertEqual(json.loads(result.body), {"test": {"id": "123"}})
 
-    def test_content_type_from_accept_xml(self):
+    def test_response_content_type_from_accept_xml(self):
         request = webob.Request.blank('/tests/123')
         request.headers["Accept"] = "application/xml"
         result = request.get_response(self.TestRouter())
         self.assertEqual(result.headers["Content-Type"], "application/xml")
 
-    def test_content_type_from_accept_json(self):
+    def test_response_content_type_from_accept_json(self):
         request = wsgi.Request.blank('/tests/123')
         request.headers["Accept"] = "application/json"
         result = request.get_response(self.TestRouter())
         self.assertEqual(result.headers["Content-Type"], "application/json")
 
-    def test_content_type_from_query_extension_xml(self):
+    def test_response_content_type_from_query_extension_xml(self):
         request = wsgi.Request.blank('/tests/123.xml')
         result = request.get_response(self.TestRouter())
         self.assertEqual(result.headers["Content-Type"], "application/xml")
 
-    def test_content_type_from_query_extension_json(self):
+    def test_response_content_type_from_query_extension_json(self):
         request = wsgi.Request.blank('/tests/123.json')
         result = request.get_response(self.TestRouter())
         self.assertEqual(result.headers["Content-Type"], "application/json")
 
-    def test_content_type_default_when_unsupported(self):
+    def test_response_content_type_default_when_unsupported(self):
         request = wsgi.Request.blank('/tests/123.unsupported')
         request.headers["Accept"] = "application/unsupported1"
         result = request.get_response(self.TestRouter())
@@ -125,6 +125,17 @@ class ControllerTest(test.TestCase):
 
 class RequestTest(test.TestCase):
 
+    def test_request_content_type_missing(self):
+        request = wsgi.Request.blank('/tests/123')
+        request.body = "<body />"
+        self.assertRaises(webob.exc.HTTPBadRequest, request.get_content_type)
+
+    def test_request_content_type_unsupported(self):
+        request = wsgi.Request.blank('/tests/123')
+        request.headers["Content-Type"] = "text/html"
+        request.body = "asdf<br />"
+        self.assertRaises(webob.exc.HTTPBadRequest, request.get_content_type)
+
     def test_content_type_from_accept_xml(self):
         request = wsgi.Request.blank('/tests/123')
         request.headers["Accept"] = "application/xml"
@@ -173,40 +184,48 @@ class RequestTest(test.TestCase):
         self.assertEqual(result, "application/json")
 
 
-
-
-
 class SerializerTest(test.TestCase):
 
-    def match(self, url, accept, expect):
+    def test_xml(self):
         input_dict = dict(servers=dict(a=(2, 3)))
         expected_xml = '<servers><a>(2,3)</a></servers>'
+        serializer = wsgi.Serializer()
+        result = serializer.serialize(input_dict, "application/xml")
+        result = result.replace('\n', '').replace(' ', '')
+        self.assertEqual(result, expected_xml)
+
+    def test_json(self):
+        input_dict = dict(servers=dict(a=(2, 3)))
         expected_json = '{"servers":{"a":[2,3]}}'
-        req = webob.Request.blank(url, headers=dict(Accept=accept))
-        result = wsgi.Serializer(req.environ).to_content_type(input_dict)
+        serializer = wsgi.Serializer()
+        result = serializer.serialize(input_dict, "application/json")
         result = result.replace('\n', '').replace(' ', '')
-        if expect == 'xml':
-            self.assertEqual(result, expected_xml)
-        elif expect == 'json':
-            self.assertEqual(result, expected_json)
-        else:
-            raise "Bad expect value"
-
-    def test_basic(self):
-        self.match('/servers/4.json', None, expect='json')
-        self.match('/servers/4', 'application/json', expect='json')
-        self.match('/servers/4', 'application/xml', expect='xml')
-        self.match('/servers/4.xml', None, expect='xml')
-
-    def test_defaults_to_json(self):
-        self.match('/servers/4', None, expect='json')
-        self.match('/servers/4', 'text/html', expect='json')
-
-    def test_suffix_takes_precedence_over_accept_header(self):
-        self.match('/servers/4.xml', 'application/json', expect='xml')
-        self.match('/servers/4.xml.', 'application/json', expect='json')
-
-    def test_deserialize(self):
+        self.assertEqual(result, expected_json)
+
+    def test_unsupported_content_type(self):
+        serializer = wsgi.Serializer()
+        self.assertRaises(exception.InvalidContentType, serializer.serialize,
+                          {}, "text/null")
+
+    def test_deserialize_json(self):
+        data = """{"a": {
+                "a1": "1",
+                "a2": "2",
+                "bs": ["1", "2", "3", {"c": {"c1": "1"}}],
+                "d": {"e": "1"},
+                "f": "1"}}"""
+        as_dict = dict(a={
+                'a1': '1',
+                'a2': '2',
+                'bs': ['1', '2', '3', {'c': dict(c1='1')}],
+                'd': {'e': '1'},
+                'f': '1'})
+        metadata = {}
+        serializer = wsgi.Serializer(metadata)
+        self.assertEqual(serializer.deserialize(data, "application/json"),
+                         as_dict)
+
+    def test_deserialize_xml(self):
         xml = """
             <a a1="1" a2="2">
               <bs><b>1</b><b>2</b><b>3</b><b><c c1="1"/></b></bs>
@@ -221,11 +240,13 @@ class SerializerTest(test.TestCase):
                 'd': {'e': '1'},
                 'f': '1'})
         metadata = {'application/xml': dict(plurals={'bs': 'b', 'ts': 't'})}
-        serializer = wsgi.Serializer({}, metadata)
-        self.assertEqual(serializer.deserialize(xml), as_dict)
+        serializer = wsgi.Serializer(metadata)
+        self.assertEqual(serializer.deserialize(xml, "application/xml"),
+                         as_dict)
 
     def test_deserialize_empty_xml(self):
         xml = """<a></a>"""
         as_dict = {"a": {}}
-        serializer = wsgi.Serializer({})
-        self.assertEqual(serializer.deserialize(xml), as_dict)
+        serializer = wsgi.Serializer()
+        self.assertEqual(serializer.deserialize(xml, "application/xml"),
+                         as_dict)
diff --git a/nova/tests/test_direct.py b/nova/tests/test_direct.py
index b6bfab534..85bfcfd85 100644
--- a/nova/tests/test_direct.py
+++ b/nova/tests/test_direct.py
@@ -59,6 +59,7 @@ class DirectTestCase(test.TestCase):
         req.headers['X-OpenStack-User'] = 'user1'
         req.headers['X-OpenStack-Project'] = 'proj1'
         resp = req.get_response(self.auth_router)
+        self.assertEqual(resp.status_int, 200)
         data = json.loads(resp.body)
         self.assertEqual(data['user'], 'user1')
         self.assertEqual(data['project'], 'proj1')
@@ -69,6 +70,7 @@ class DirectTestCase(test.TestCase):
         req.method = 'POST'
         req.body = 'json=%s' % json.dumps({'data': 'foo'})
         resp = req.get_response(self.router)
+        self.assertEqual(resp.status_int, 200)
         resp_parsed = json.loads(resp.body)
         self.assertEqual(resp_parsed['data'], 'foo')
 
@@ -78,6 +80,7 @@ class DirectTestCase(test.TestCase):
         req.method = 'POST'
         req.body = 'data=foo'
         resp = req.get_response(self.router)
+        self.assertEqual(resp.status_int, 200)
         resp_parsed = json.loads(resp.body)
         self.assertEqual(resp_parsed['data'], 'foo')
 
diff --git a/nova/wsgi.py b/nova/wsgi.py
index 4577439cb..c3e08522d 100644
--- a/nova/wsgi.py
+++ b/nova/wsgi.py
@@ -36,6 +36,7 @@ import webob.exc
 
 from paste import deploy
 
+from nova import exception
 from nova import flags
 from nova import log as logging
 from nova import utils
@@ -102,6 +103,14 @@ class Request(webob.Request):
 
         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")
+
 
 class Application(object):
     """Base WSGI application wrapper. Subclasses need to implement __call__."""
@@ -343,30 +352,41 @@ class Controller(object):
             del arg_dict['format']
         arg_dict['req'] = req
         result = method(**arg_dict)
+
         if type(result) is dict:
-            return self._serialize(result, req)
+            content_type = req.best_match()
+            body = self._serialize(result, content_type)
+
+            response = webob.Response()
+            response.headers["Content-Type"] = content_type
+            response.body = body
+            return response
+
         else:
             return result
 
-    def _serialize(self, data, request):
+    def _serialize(self, data, content_type):
         """
-        Serialize the given dict to the response type requested in request.
+        Serialize the given dict to the provided content_type.
         Uses self._serialization_metadata if it exists, which is a dict mapping
         MIME types to information needed to serialize to that type.
         """
         _metadata = getattr(type(self), "_serialization_metadata", {})
-        serializer = Serializer(request.environ, _metadata)
-        return serializer.to_content_type(data)
+        serializer = Serializer(_metadata)
+        try:
+            return serializer.serialize(data, content_type)
+        except exception.InvalidContentType:
+            raise webob.exc.HTTPNotAcceptable()
 
-    def _deserialize(self, data, request):
+    def _deserialize(self, data, content_type):
         """
-        Deserialize the request body to the response type requested in request.
+        Deserialize the request body to the specefied content type.
         Uses self._serialization_metadata if it exists, which is a dict mapping
         MIME types to information needed to serialize to that type.
         """
         _metadata = getattr(type(self), "_serialization_metadata", {})
-        serializer = Serializer(request.environ, _metadata)
-        return serializer.deserialize(data)
+        serializer = Serializer(_metadata)
+        return serializer.deserialize(data, content_type)
 
 
 class Serializer(object):
@@ -374,50 +394,52 @@ class Serializer(object):
     Serializes and deserializes dictionaries to certain MIME types.
     """
 
-    def __init__(self, environ, metadata=None):
+    def __init__(self, metadata=None):
         """
         Create a serializer based on the given WSGI environment.
         'metadata' is an optional dict mapping MIME types to information
         needed to serialize a dictionary to that type.
         """
         self.metadata = metadata or {}
-        req = Request.blank('', environ)
-        suffix = req.path_info.split('.')[-1].lower()
-        if suffix == 'json':
-            self.handler = self._to_json
-        elif suffix == 'xml':
-            self.handler = self._to_xml
-        elif 'application/json' in req.accept:
-            self.handler = self._to_json
-        elif 'application/xml' in req.accept:
-            self.handler = self._to_xml
-        else:
-            # This is the default
-            self.handler = self._to_json
 
-    def to_content_type(self, data):
-        """
-        Serialize a dictionary into a string.
+    def _get_serialize_handler(self, content_type):
+        handlers = {
+            "application/json": self._to_json,
+            "application/xml": self._to_xml,
+        }
+
+        try:
+            return handlers[content_type]
+        except Exception:
+            raise exception.InvalidContentType()
 
-        The format of the string will be decided based on the Content Type
-        requested in self.environ: by Accept: header, or by URL suffix.
+    def serialize(self, data, content_type):
+        """
+        Serialize a dictionary into a string of the specified content type.
         """
-        return self.handler(data)
+        return self._get_serialize_handler(content_type)(data)
 
-    def deserialize(self, datastring):
+    def deserialize(self, datastring, content_type):
         """
         Deserialize a string to a dictionary.
 
         The string must be in the format of a supported MIME type.
         """
-        datastring = datastring.strip()
+        return self.get_deserialize_handler(content_type)(datastring)
+
+    def get_deserialize_handler(self, content_type):
+        handlers = {
+            "application/json": self._from_json,
+            "application/xml": self._from_xml,
+        }
+
         try:
-            is_xml = (datastring[0] == '<')
-            if not is_xml:
-                return utils.loads(datastring)
-            return self._from_xml(datastring)
-        except:
-            return None
+            return handlers[content_type]
+        except Exception:
+            raise exception.InvalidContentType()
+
+    def _from_json(self, datastring):
+        return utils.loads(datastring)
 
     def _from_xml(self, datastring):
         xmldata = self.metadata.get('application/xml', {})