Merge lp:~rackspace-titan/glance/wsgi-refactor into lp:~hudson-openstack/glance/trunk

[Brian Waldon]

- refactoring wsgi code to divide deserialization, controller, serialization among different objects
- Resource object acts as coordinator

- tests are coming, this is for review purposes

[Brian Waldon (bcwaldon)]

I can't figure out why tests/functional/test_curl_api.py:TestCurlApi.test_traceback_not_consumed is failing. It is "turned off" in my merge prop but I would like to fix it before I send it in.

[Jay Pipes (jaypipes)]

Hey Brian,

The reason that test is failing is because of this change:

148 - image_id = image_meta['id']
149 - content_type = req.headers.get('content-type', 'notset')
150 - if content_type != 'application/octet-stream':
151 - self._safe_kill(req, image_id)
152 - msg = ("Content-Type must be application/octet-stream")
153 + try:
154 + req.get_content_type('application/octet-stream')
155 + except exception.InvalidContentType:
156 + msg = "Content-Type must be application/octet-stream"

The issue is that the image is registered and set to a 'queued' status before this code is run. The call to _safe_kill() is what (correctly) sets the image status to 'killed' and returns the error message. You need to add the call to _safe_kill() back to the new except InvalidContentType block.

Hope that helps,

jay

[Brian Waldon (bcwaldon)]

Fixed the test case.

[Jay Pipes (jaypipes)]

Hey again Brian, here's a full review...

Overall, excellent and clean work. As mentioned on IRC, in the future, try to submit style/formatting-only stuff in a separate patch.

Design Discussion
-----------------

The one big design/architecture thing that I'd like to suggest a change for is making the responsibility of the Serializer subclasses very specific to transforming the dicts that may compose the body of a Response object. As they currently stand, the Serializer subclasses look to be doing more work that may more appropriately belong in the Controller.

I'll work through an example.

Consider the ImageSerializer class (shortened for brevity):

357 +class ImageSerializer(wsgi.JSONResponseSerializer):
...
360 + def _inject_location_header(self, response, image_meta):
361 + location = self._get_image_location(image_meta)
362 + response.headers.add('Location', location)
363 +
364 + def _inject_checksum_header(self, response, image_meta):
365 + response.headers.add('ETag', image_meta['checksum'])
366 +
367 + def _inject_image_meta_headers(self, response, image_meta):
...
379 + headers = utils.image_meta_to_http_headers(image_meta)
380 +
381 + for k, v in headers.items():
382 + response.headers.add(k, v)
383 +
384 + def _get_image_location(self, image_meta):
...
386 + return "/v1/images/%s" % image_meta['id']
...
396 + def show(self, result):
397 + image_meta = result['image_meta']
398 +
399 + response = webob.Response(app_iter=result['image_iterator'])
400 + # Using app_iter blanks content-length, so we set it here...
401 + response.headers.add('Content-Length', image_meta['size'])
402 + response.headers.add('Content-Type', 'application/octet-stream')
403 +
404 + self._inject_image_meta_headers(response, image_meta)
405 + self._inject_location_header(response, image_meta)
406 + self._inject_checksum_header(response, image_meta)
...
419 + def create(self, result):
420 + image_meta = result['image_meta']
421 + response = webob.Response()
422 + response.status = httplib.CREATED
423 + response.headers.add('Content-Type', 'application/json')
424 + response.body = self.to_json(dict(image=image_meta))
425 + self._inject_location_header(response, image_meta)
426 + self._inject_checksum_header(response, image_meta)
427 + return response

I think that I would prefer that the ImageSerializer class NOT be responsible for:

a) creating a webob.Response object
b) modifying any headers of the Response object other than Content-Type

I think that all the Serializer subclasses should be doing is transforming the Response object's body attribute to the necessary serialized format, and setting the Content-Type header appropriately.

All the other activities that are happening in ImageSerializer I believe should be in the Controller object, since the Controller is responsible for setting various headers (ETag, Location, etc) and for creating the Response object itself...

So, I think I'd prefer the ImageSerializer to look like this:

class ImageSerializer(wsgi.JSONResponseSerializer):
    def _serialize(self, response):
        response.content_type = 'application/json'
        meta = response....

review: Needs Fixing

[Mark Washenberger (markwash)]

>
> I think that I would prefer that the ImageSerializer class NOT be responsible
> for:
>
> a) creating a webob.Response object
> b) modifying any headers of the Response object other than Content-Type
>
> I think that all the Serializer subclasses should be doing is transforming the
> Response object's body attribute to the necessary serialized format, and
> setting the Content-Type header appropriately.
>
> All the other activities that are happening in ImageSerializer I believe
> should be in the Controller object, since the Controller is responsible for
> setting various headers (ETag, Location, etc) and for creating the Response
> object itself...
>

I think it is fitting for the Serializer to be responsible for creating the Webob Response. It seems like part of the idea here is to take some of the wsgi- and http api- specific responsibilities away from the controller so that it can focus on its responsibilities rather than its context. In my mind I see the ImageSerializer as responsible for translating image responses (which are really domain-model objects) into something that a wsgi server can present.

>
> Also, have you thought about how you will deal with multiple serializers? How
> do you plan to support a resource that can be translated into both JSON and
> XML?

I believe Waldon has given this a good bit of thought, but I actually encouraged him to leave out code going towards this direction. Let's XP it up: do only the minimum required for the features needed today. I believe the design to incorporate xml will be cleaner if we do it when we are actually trying to support it.

Cheers :-)

[Jay Pipes (jaypipes)]

> >
> > I think that I would prefer that the ImageSerializer class NOT be
> responsible
> > for:
> >
> > a) creating a webob.Response object
> > b) modifying any headers of the Response object other than Content-Type
> >
> > I think that all the Serializer subclasses should be doing is transforming
> the
> > Response object's body attribute to the necessary serialized format, and
> > setting the Content-Type header appropriately.
> >
> > All the other activities that are happening in ImageSerializer I believe
> > should be in the Controller object, since the Controller is responsible for
> > setting various headers (ETag, Location, etc) and for creating the Response
> > object itself...
> >
>
> I think it is fitting for the Serializer to be responsible for creating the
> Webob Response. It seems like part of the idea here is to take some of the
> wsgi- and http api- specific responsibilities away from the controller so that
> it can focus on its responsibilities rather than its context. In my mind I see
> the ImageSerializer as responsible for translating image responses (which are
> really domain-model objects) into something that a wsgi server can present.

I strongly disagree :) Serializers should serialize. Nothing more. Controllers *control* things. They have the requisite information to set non-content-type headers, to create the appropriate Response or HTTPException objects. Moving that code into the Serializer class just doesn't make much sense to me...

> > Also, have you thought about how you will deal with multiple serializers?
> How
> > do you plan to support a resource that can be translated into both JSON and
> > XML?
>
> I believe Waldon has given this a good bit of thought, but I actually
> encouraged him to leave out code going towards this direction. Let's XP it up:
> do only the minimum required for the features needed today. I believe the
> design to incorporate xml will be cleaner if we do it when we are actually
> trying to support it.

Sure, no probs. I was just wondering if Brian had considered it :)

-jay

[Mark Washenberger (markwash)]

> I strongly disagree :) Serializers should serialize. Nothing more. Controllers
> *control* things. They have the requisite information to set non-content-type
> headers, to create the appropriate Response or HTTPException objects. Moving
> that code into the Serializer class just doesn't make much sense to me...
>

It seems that there are two issues: proper naming, and a proper division of responsibility.

Perhaps Serializer is not the right name for this set of functionality. But I hope you will agree that "Controller" is a bit too broad of a term to fit into a system with a good division of responsibility. I am certainly open to renaming both. But maybe we ought not rename Controller, so that we remain consistent with other such units across Openstack. However, our naming mistake doesn't require us to make a similar mistake in the division of responsibility just to conform.

Although the naming is extremely important, if we set it aside I believe that leaving it to the "postprocessor" of the controller's output (currently the serializer) to create the WebOb response is a powerful approach--it lets the Controller focus on the problem domain rather than on wsgi details.

I hope some of that makes sense! :-)

[Jay Pipes (jaypipes)]

> Perhaps Serializer is not the right name for this set of functionality. But I
> hope you will agree that "Controller" is a bit too broad of a term to fit into
> a system with a good division of responsibility.

Not really... it's called MVC for a reason... The Controller deals with logic and business rules, and that's what *most* of the stuff that was placed into the ImageSerializer was doing. And I think that is incorrect. Logic should stay in the Controller. The Controller should create and populate a Response object and return it. Middleware (both paste-constructed and manually created as in the case of serializers) should modify the Response object. That's it. No logic.

> I am certainly open to
> renaming both. But maybe we ought not rename Controller, so that we remain
> consistent with other such units across Openstack.

I wasn't suggesting we rename Controller :) Like I said, it's an integral part of the MVC pattern.

> However, our naming mistake
> doesn't require us to make a similar mistake in the division of responsibility
> just to conform.

I don't understand this, sorry. I think I've explained why a piece of middleware should not be constructing the Response object. That's the responsibility of the center-most object in the chain: the Controller.

> Although the naming is extremely important, if we set it aside I believe that
> leaving it to the "postprocessor" of the controller's output (currently the
> serializer) to create the WebOb response is a powerful approach--it lets the
> Controller focus on the problem domain rather than on wsgi details.

Postprocessing what? Oh, that's right... postprocessing the *Response* object :) Postprocessing implies that you are processing a previously-constructed object, no? ;)

> I hope some of that makes sense! :-)

Not really, but I still dig you man :)

-jay

[Brian Waldon (bcwaldon)]

> I think that I would prefer that the ImageSerializer class NOT be responsible
> for:
>
> a) creating a webob.Response object

I moved this into wsgi.Resource

> b) modifying any headers of the Response object other than Content-Type

I'm going to take some time to figure this out. I'll have another commit for you later today.

> Tiny Nits/Fixes
> ---------------
>
> Here are some little nits to correct before an approve.
>
> 1) docstring stuff
>
> 445 + """Base Nova Exception
> 446 +
> 447 + To correctly use this class, inherit from it and define
> 448 + a 'message' property. That message will get printf'd
> 449 + with the keyword arguments provided to the constructor.
> 450 +
> 451 + """
>
> Please use this style instead:
>
> """
> Base Nova Exception
>
> To correctly use this class, inherit from it and define
> a 'message' property. That message will get printf'd
> with the keyword arguments provided to the constructor.
> """
>
> No need for extra newline at the end, and please put the short description on
> a new line after the opening """
>
> Some of the new docstrings are correct, others aren't... please do a quick
> run-through to check they are all consistent like the above style.
>
> Also, we're Glance, not Nova ;P

Copy/paste error. Sorry for that. I fixed the problems I introduced. There are a bunch of docstrings that don't follow your suggestion in registry/server.py (probably elsewhere as well) that I did not fix because I did not introduce them in this merge prop. Maybe a style cleanup is in order?

> 2)
>
> 475 import datetime
> 476 -
> 477 import eventlet
>
> eventlet is a 3rd party library. We separate stdlib modules from 3rd party
> modules with a single newline.

Ah, I thought it was just separation of non-glance and glance modules. Fixed that.

> 3) few little typos
>
> 67 + :retval similiar to 'show' method but without image_data
>
> s/similiar/similar

Fixed. I do that all the time :(

> 4)
>
> 133 + if 'location' in image_meta and image_meta['location'] is not None:
> 264 + if 'location' in image_meta and image_meta['location'] is not None:
>
> can be shortened to:
>
> if image_meta.get('location'):

Fixed. I took it a step farther and stored location in a variable before the conditional. We needed to use it again immediately anyways, so I removed the duplication of reading from the image_meta dict.

> 5)
>
> 342 + def create(self, request):
> 343 + result = {}
> 344 + result['image_meta'] = utils.get_image_meta_from_headers(request)
> 345 + data = request.body if self.has_body(request) else None
> 346 + result['image_data'] = data
> 347 + return result
> 348 +
> 349 + def update(self, request):
> 350 + result = {}
> 351 + result['image_meta'] = utils.get_image_meta_from_headers(request)
> 352 + data = request.body if self.has_body(request) else None
> 353 + result['image_data'] = data
> 354 + return result
>
> Little bit of DRY cleanup needed there... something like this would do:

Good catch. I didn't even notice that.

> 6) the wsgi.Resource constructor
>
> 665 + def __init__(self, des...

[Brian Waldon (bcwaldon)]

Looking for suggestions on separating out header "serialization" code.

[Jay Pipes (jaypipes)]

Let's work on cleaning up the header serialization code in a later patch. Approved from me.

review: Approve

[Rick Harris (rconradharris)]

Overall looks good. Some thoughts:

> 425 + self._inject_location_header(response, image_meta)
> 426 + self._inject_checksum_header(response, image_meta)
> 427 + return response

Super nit-picky here, but might be worth DRY'ing up some of this code since
it's repeated quite a bit.

> 582 + return (request.content_length and request.content_length > 0) \
> 583 + or 'transfer-encoding' in request.headers

This statement was a little hard to read at-first. Once I understood it, it
was clear enough. That said, given that we're, for the most part, optimizing
for readability, maybe it would be clearer as:

  if 'transfer-encoding' in request.headers:
    return True
  elif request.content_length is None:
    raise Exception("Content-Length was not specified and we're not "
                    "Transfer-Encoding")

  return request.content_length > 0:

> 683 + if type(action_result) is dict:

Duck-typing might be preferable here. Do you know specifically what dict
methods the serializer is using; if you're not sure, you could always check
for presence of `keys()`

  if hasattr(action_result, 'keys'):
    blah

(This is a really small nit, but if we ever need to do something tricky by
subclassing `dict`, this would keep the code working)

> + """Find action-spefic method on self and call it."""
s/spefic/specific/

> 695 + except Exception:
> 696 + method = getattr(obj, 'default')

Should this handler be this broad? Would AttributeError work here?

If that's the case, we could do:

  method = getattr(obj, action, 'default')

[Brian Waldon (bcwaldon)]

> Overall looks good. Some thoughts:
>
> > 425 + self._inject_location_header(response, image_meta)
> > 426 + self._inject_checksum_header(response, image_meta)
> > 427 + return response
>
> Super nit-picky here, but might be worth DRY'ing up some of this code since
> it's repeated quite a bit.

I would factor this out a bit more if it were clear exactly how to do it. Now, I could make a method called inject_location_and_checksum_headers, but I don't see much value in that. I already factored out quite a bit with regards to these two functions. If you still feel strongly about this, I'll do it for you.

> > 582 + return (request.content_length and request.content_length >
> 0) \
> > 583 + or 'transfer-encoding' in request.headers
>
> This statement was a little hard to read at-first. Once I understood it, it
> was clear enough. That said, given that we're, for the most part, optimizing
> for readability, maybe it would be clearer as:
>
>
> if 'transfer-encoding' in request.headers:
> return True
> elif request.content_length is None:
> raise Exception("Content-Length was not specified and we're not "
> "Transfer-Encoding")
>
> return request.content_length > 0:

I cleaned it up with a longer if/elif block, while keeping the same logic. This is a boolean check, so I didn't want to throw an exception.

> > 683 + if type(action_result) is dict:
>
> Duck-typing might be preferable here. Do you know specifically what dict
> methods the serializer is using; if you're not sure, you could always check
> for presence of `keys()`
>
> if hasattr(action_result, 'keys'):
> blah
>
> (This is a really small nit, but if we ever need to do something tricky by
> subclassing `dict`, this would keep the code working)

How about I just assume I can serialize it, and if there is an exception I will just return the original results? I checked in a possible implementation of this. Tell me what you think.

> > + """Find action-spefic method on self and call it."""
> s/spefic/specific/

Good catch. Fixed.

> > 695 + except Exception:
> > 696 + method = getattr(obj, 'default')
>
> Should this handler be this broad? Would AttributeError work here?
>
> If that's the case, we could do:
>
> method = getattr(obj, action, 'default')

I added AttributeError to the except statement. I can't use 'default' as a third argument there because it expects a callable. I would have to pass in obj.default, and I can't assume that exists.

I also caught some issues in the wsgi.Resource docstring and cleaned that up.

[lp:~rackspace-titan/glance/wsgi-refactor]

147. By Brian Waldon on 2011-06-17

refactoring from Rick's comments

[Brian Lamar (blamar)]

164: HTTPBadRequest needs explanation=msg to actually display the passed-in message.

680: Not sure if it will hinder readability, but this blank line seems out of place.

716 + try:
717 + del args['controller']
718 + except KeyError:
719 + pass

I've been playing with this replacement logic/syntax:

args.pop("controller", None)

It's not as explicit, which I don't like, but it gets the job done and isn't too cryptic.

Starting at 1064, should those each be it's own test?

I like your style/spacing! :)

review: Needs Fixing

[Brian Waldon (bcwaldon)]

> 164: HTTPBadRequest needs explanation=msg to actually display the passed-in
> message.

Fixed.

> 680: Not sure if it will hinder readability, but this blank line seems out of
> place.

Fixed.

> 716 + try:
> 717 + del args['controller']
> 718 + except KeyError:
> 719 + pass
>
> I've been playing with this replacement logic/syntax:
>
> args.pop("controller", None)
>
> It's not as explicit, which I don't like, but it gets the job done and isn't
> too cryptic.

I could go either way, and originally I did have it this way. When I merge prop'd this code in Nova, I was asked to change it to the try/except style. I hope it's okay to leave it.

> Starting at 1064, should those each be it's own test?

Yes, sir.

> I like your style/spacing! :)

I like your style, too.

[lp:~rackspace-titan/glance/wsgi-refactor]

148. By Brian Waldon on 2011-06-17

refactoring for Brian

[Brian Lamar (blamar)]

review: Approve

[Rick Harris (rconradharris)]

Looks good, nice job Brian.

review: Approve

=== modified file 'glance/api/v1/__init__.py'
--- glance/api/v1/__init__.py	2011-05-11 23:03:51 +0000
+++ glance/api/v1/__init__.py	2011-06-17 20:46:38 +0000
@@ -32,11 +32,11 @@
     def __init__(self, options):
         self.options = options
         mapper = routes.Mapper()
-        controller = images.Controller(options)
-        mapper.resource("image", "images", controller=controller,
+        resource = images.create_resource(options)
+        mapper.resource("image", "images", controller=resource,
                        collection={'detail': 'GET'})
-        mapper.connect("/", controller=controller, action="index")
-        mapper.connect("/images/{id}", controller=controller, action="meta",
+        mapper.connect("/", controller=resource, action="index")
+        mapper.connect("/images/{id}", controller=resource, action="meta",
                        conditions=dict(method=["HEAD"]))
         super(API, self).__init__(mapper)
 
 
=== modified file 'glance/api/v1/images.py'
--- glance/api/v1/images.py	2011-05-17 13:34:42 +0000
+++ glance/api/v1/images.py	2011-06-17 20:46:38 +0000
@@ -24,7 +24,7 @@
 import logging
 import sys
 
-from webob import Response
+import webob
 from webob.exc import (HTTPNotFound,
                        HTTPConflict,
                        HTTPBadRequest)
@@ -46,7 +46,7 @@
                      'size_min', 'size_max']
 
 
-class Controller(wsgi.Controller):
+class Controller(object):
 
     """
     WSGI controller for images resource in Glance v1 API
@@ -80,7 +80,7 @@
             * checksum -- MD5 checksum of the image data
             * size -- Size of image data in bytes
 
-        :param request: The WSGI/Webob Request object
+        :param req: The WSGI/Webob Request object
         :retval The response body is a mapping of the following form::
 
             {'images': [
@@ -107,7 +107,7 @@
         """
         Returns detailed information for all public, available images
 
-        :param request: The WSGI/Webob Request object
+        :param req: The WSGI/Webob Request object
         :retval The response body is a mapping of the following form::
 
             {'images': [
@@ -155,29 +155,22 @@
         Returns metadata about an image in the HTTP headers of the
         response object
 
-        :param request: The WSGI/Webob Request object
+        :param req: The WSGI/Webob Request object
         :param id: The opaque image identifier
+        :retval similar to 'show' method but without image_data
 
         :raises HTTPNotFound if image metadata is not available to user
         """
-        image = self.get_image_meta_or_404(req, id)
-
-        res = Response(request=req)
-        utils.inject_image_meta_into_headers(res, image)
-        res.headers.add('Location', "/v1/images/%s" % id)
-        res.headers.add('ETag', image['checksum'])
-
-        return req.get_response(res)
+        return {
+            'image_meta': self.get_image_meta_or_404(req, id),
+        }
 
     def show(self, req, id):
         """
-        Returns an iterator as a Response object that
-        can be used to retrieve an image's data. The
-        content-type of the response is the content-type
-        of the image, or application/octet-stream if none
-        is known or found.
+        Returns an iterator that can be used to retrieve an image's
+        data along with the image metadata.
 
-        :param request: The WSGI/Webob Request object
+        :param req: The WSGI/Webob Request object
         :param id: The opaque image identifier
 
         :raises HTTPNotFound if image is not available to user
@@ -192,31 +185,26 @@
             for chunk in chunks:
                 yield chunk
 
-        res = Response(app_iter=image_iterator(),
-                       content_type="application/octet-stream")
-        # Using app_iter blanks content-length, so we set it here...
-        res.headers.add('Content-Length', image['size'])
-        utils.inject_image_meta_into_headers(res, image)
-        res.headers.add('Location', "/v1/images/%s" % id)
-        res.headers.add('ETag', image['checksum'])
-        return req.get_response(res)
+        return {
+            'image_iterator': image_iterator(),
+            'image_meta': image,
+        }
 
-    def _reserve(self, req):
+    def _reserve(self, req, image_meta):
         """
         Adds the image metadata to the registry and assigns
         an image identifier if one is not supplied in the request
-        headers. Sets the image's status to `queued`
+        headers. Sets the image's status to `queued`.
 
-        :param request: The WSGI/Webob Request object
+        :param req: The WSGI/Webob Request object
         :param id: The opaque image identifier
 
         :raises HTTPConflict if image already exists
         :raises HTTPBadRequest if image metadata is not valid
         """
-        image_meta = utils.get_image_meta_from_headers(req)
-
-        if 'location' in image_meta:
-            store = get_store_from_location(image_meta['location'])
+        location = image_meta.get('location')
+        if location:
+            store = get_store_from_location(location)
             # check the store exists before we hit the registry, but we
             # don't actually care what it is at this point
             self.get_store_or_400(req, store)
@@ -250,28 +238,27 @@
         will attempt to use that store, if not, Glance will use the
         store set by the flag `default_store`.
 
-        :param request: The WSGI/Webob Request object
+        :param req: The WSGI/Webob Request object
         :param image_meta: Mapping of metadata about image
 
         :raises HTTPConflict if image already exists
         :retval The location where the image was stored
         """
-        image_id = image_meta['id']
-        content_type = req.headers.get('content-type', 'notset')
-        if content_type != 'application/octet-stream':
-            self._safe_kill(req, image_id)
-            msg = ("Content-Type must be application/octet-stream")
+        try:
+            req.get_content_type('application/octet-stream')
+        except exception.InvalidContentType:
+            self._safe_kill(req, image_meta['id'])
+            msg = "Content-Type must be application/octet-stream"
             logger.error(msg)
-            raise HTTPBadRequest(msg, content_type="text/plain",
-                                 request=req)
+            raise HTTPBadRequest(explanation=msg)
 
-        store_name = req.headers.get(
-            'x-image-meta-store', self.options['default_store'])
+        store_name = req.headers.get('x-image-meta-store',
+                                     self.options['default_store'])
 
         store = self.get_store_or_400(req, store_name)
 
-        logger.debug("Setting image %s to status 'saving'"
-                     % image_id)
+        image_id = image_meta['id']
+        logger.debug("Setting image %s to status 'saving'" % image_id)
         registry.update_image_metadata(self.options, image_id,
                                        {'status': 'saving'})
         try:
@@ -304,11 +291,13 @@
                                             'size': size})
 
             return location
+
         except exception.Duplicate, e:
             msg = ("Attempt to upload duplicate image: %s") % str(e)
             logger.error(msg)
             self._safe_kill(req, image_id)
             raise HTTPConflict(msg, request=req)
+
         except Exception, e:
             msg = ("Error uploading image: %s") % str(e)
             logger.error(msg)
@@ -320,8 +309,8 @@
         Sets the image status to `active` and the image's location
         attribute.
 
-        :param request: The WSGI/Webob Request object
-        :param image_meta: Mapping of metadata about image
+        :param req: The WSGI/Webob Request object
+        :param image_id: Opaque image identifier
         :param location: Location of where Glance stored this image
         """
         image_meta = {}
@@ -333,9 +322,9 @@
 
     def _kill(self, req, image_id):
         """
-        Marks the image status to `killed`
+        Marks the image status to `killed`.
 
-        :param request: The WSGI/Webob Request object
+        :param req: The WSGI/Webob Request object
         :param image_id: Opaque image identifier
         """
         registry.update_image_metadata(self.options,
@@ -349,7 +338,7 @@
         Since _kill is meant to be called from exceptions handlers, it should
         not raise itself, rather it should just log its error.
 
-        :param request: The WSGI/Webob Request object
+        :param req: The WSGI/Webob Request object
         :param image_id: Opaque image identifier
         """
         try:
@@ -364,7 +353,7 @@
         and activates the image in the registry after a successful
         upload.
 
-        :param request: The WSGI/Webob Request object
+        :param req: The WSGI/Webob Request object
         :param image_meta: Mapping of metadata about image
 
         :retval Mapping of updated image data
@@ -373,7 +362,7 @@
         location = self._upload(req, image_meta)
         return self._activate(req, image_id, location)
 
-    def create(self, req):
+    def create(self, req, image_meta, image_data):
         """
         Adds a new image to Glance. Three scenarios exist when creating an
         image:
@@ -399,32 +388,27 @@
         containing metadata about the image is returned, including its
         opaque identifier.
 
-        :param request: The WSGI/Webob Request object
+        :param req: The WSGI/Webob Request object
+        :param image_meta: Mapping of metadata about image
+        :param image_data: Actual image data that is to be stored
 
-        :raises HTTPBadRequest if no x-image-meta-location is missing
+        :raises HTTPBadRequest if x-image-meta-location is missing
                 and the request body is not application/octet-stream
                 image data.
         """
-        image_meta = self._reserve(req)
+        image_meta = self._reserve(req, image_meta)
         image_id = image_meta['id']
 
-        if utils.has_body(req):
+        if image_data is not None:
             image_meta = self._upload_and_activate(req, image_meta)
         else:
-            if 'x-image-meta-location' in req.headers:
-                location = req.headers['x-image-meta-location']
+            location = image_meta.get('location')
+            if location:
                 image_meta = self._activate(req, image_id, location)
 
-        # APP states we should return a Location: header with the edit
-        # URI of the resource newly-created.
-        res = Response(request=req, body=json.dumps(dict(image=image_meta)),
-                       status=httplib.CREATED, content_type="text/plain")
-        res.headers.add('Location', "/v1/images/%s" % image_id)
-        res.headers.add('ETag', image_meta['checksum'])
-
-        return req.get_response(res)
-
-    def update(self, req, id):
+        return {'image_meta': image_meta}
+
+    def update(self, req, id, image_meta, image_data):
         """
         Updates an existing image with the registry.
 
@@ -433,29 +417,17 @@
 
         :retval Returns the updated image information as a mapping
         """
-        has_body = utils.has_body(req)
-
         orig_image_meta = self.get_image_meta_or_404(req, id)
         orig_status = orig_image_meta['status']
 
-        if has_body and orig_status != 'queued':
+        if image_data is not None and orig_status != 'queued':
             raise HTTPConflict("Cannot upload to an unqueued image")
 
-        new_image_meta = utils.get_image_meta_from_headers(req)
         try:
-            image_meta = registry.update_image_metadata(self.options,
-                                                        id,
-                                                        new_image_meta,
-                                                        True)
-            if has_body:
+            image_meta = registry.update_image_metadata(self.options, id,
+                                                         image_meta, True)
+            if image_data is not None:
                 image_meta = self._upload_and_activate(req, image_meta)
-
-            res = Response(request=req,
-                           body=json.dumps(dict(image=image_meta)),
-                           content_type="text/plain")
-            res.headers.add('Location', "/images/%s" % id)
-            res.headers.add('ETag', image_meta['checksum'])
-            return res
         except exception.Invalid, e:
             msg = ("Failed to update image metadata. Got error: %(e)s"
                    % locals())
@@ -463,11 +435,13 @@
                 logger.error(line)
             raise HTTPBadRequest(msg, request=req, content_type="text/plain")
 
+        return {'image_meta': image_meta}
+
     def delete(self, req, id):
         """
         Deletes the image and all its chunks from the Glance
 
-        :param request: The WSGI/Webob Request object
+        :param req: The WSGI/Webob Request object
         :param id: The opaque image identifier
 
         :raises HttpBadRequest if image registry is invalid
@@ -527,3 +501,97 @@
             logger.error(msg)
             raise HTTPBadRequest(msg, request=request,
                                  content_type='text/plain')
+
+
+class ImageDeserializer(wsgi.JSONRequestDeserializer):
+    """Handles deserialization of specific controller method requests."""
+
+    def _deserialize(self, request):
+        result = {}
+        result['image_meta'] = utils.get_image_meta_from_headers(request)
+        data = request.body if self.has_body(request) else None
+        result['image_data'] = data
+        return result
+
+    def create(self, request):
+        return self._deserialize(request)
+
+    def update(self, request):
+        return self._deserialize(request)
+
+
+class ImageSerializer(wsgi.JSONResponseSerializer):
+    """Handles serialization of specific controller method responses."""
+
+    def _inject_location_header(self, response, image_meta):
+        location = self._get_image_location(image_meta)
+        response.headers.add('Location', location)
+
+    def _inject_checksum_header(self, response, image_meta):
+        response.headers.add('ETag', image_meta['checksum'])
+
+    def _inject_image_meta_headers(self, response, image_meta):
+        """
+        Given a response and mapping of image metadata, injects
+        the Response with a set of HTTP headers for the image
+        metadata. Each main image metadata field is injected
+        as a HTTP header with key 'x-image-meta-<FIELD>' except
+        for the properties field, which is further broken out
+        into a set of 'x-image-meta-property-<KEY>' headers
+
+        :param response: The Webob Response object
+        :param image_meta: Mapping of image metadata
+        """
+        headers = utils.image_meta_to_http_headers(image_meta)
+
+        for k, v in headers.items():
+            response.headers.add(k, v)
+
+    def _get_image_location(self, image_meta):
+        """Build a relative url to reach the image defined by image_meta."""
+        return "/v1/images/%s" % image_meta['id']
+
+    def meta(self, response, result):
+        image_meta = result['image_meta']
+        self._inject_image_meta_headers(response, image_meta)
+        self._inject_location_header(response, image_meta)
+        self._inject_checksum_header(response, image_meta)
+        return response
+
+    def show(self, response, result):
+        image_meta = result['image_meta']
+
+        response.app_iter = result['image_iterator']
+        # Using app_iter blanks content-length, so we set it here...
+        response.headers.add('Content-Length', image_meta['size'])
+        response.headers.add('Content-Type', 'application/octet-stream')
+
+        self._inject_image_meta_headers(response, image_meta)
+        self._inject_location_header(response, image_meta)
+        self._inject_checksum_header(response, image_meta)
+
+        return response
+
+    def update(self, response, result):
+        image_meta = result['image_meta']
+        response.body = self.to_json(dict(image=image_meta))
+        response.headers.add('Content-Type', 'application/json')
+        self._inject_location_header(response, image_meta)
+        self._inject_checksum_header(response, image_meta)
+        return response
+
+    def create(self, response, result):
+        image_meta = result['image_meta']
+        response.status = httplib.CREATED
+        response.headers.add('Content-Type', 'application/json')
+        response.body = self.to_json(dict(image=image_meta))
+        self._inject_location_header(response, image_meta)
+        self._inject_checksum_header(response, image_meta)
+        return response
+
+
+def create_resource(options):
+    """Images resource factory method"""
+    deserializer = ImageDeserializer()
+    serializer = ImageSerializer()
+    return wsgi.Resource(Controller(options), deserializer, serializer)
 
=== modified file 'glance/common/exception.py'
--- glance/common/exception.py	2011-02-02 01:43:16 +0000
+++ glance/common/exception.py	2011-06-17 20:46:38 +0000
@@ -96,3 +96,29 @@
             raise
     _wrap.func_name = f.func_name
     return _wrap
+
+
+class GlanceException(Exception):
+    """
+    Base Glance Exception
+
+    To correctly use this class, inherit from it and define
+    a 'message' property. That message will get printf'd
+    with the keyword arguments provided to the constructor.
+    """
+    message = "An unknown exception occurred"
+
+    def __init__(self, **kwargs):
+        try:
+            self._error_string = self.message % kwargs
+
+        except Exception:
+            # at least get the core message out if something happened
+            self._error_string = self.message
+
+    def __str__(self):
+        return self._error_string
+
+
+class InvalidContentType(GlanceException):
+    message = "Invalid content type %(content_type)s"
 
=== modified file 'glance/common/wsgi.py'
--- glance/common/wsgi.py	2011-05-18 00:11:46 +0000
+++ glance/common/wsgi.py	2011-06-17 20:46:38 +0000
@@ -34,6 +34,8 @@
 import webob.dec
 import webob.exc
 
+from glance.common import exception
+
 
 class WritableLogger(object):
     """A thin wrapper that responds to `write` and logs."""
@@ -205,73 +207,55 @@
         return app
 
 
-class Controller(object):
-    """
-    WSGI app that reads routing information supplied by RoutesMiddleware
-    and calls the requested action method upon itself.  All action methods
-    must, in addition to their normal parameters, accept a 'req' argument
-    which is the incoming webob.Request.  They raise a webob.exc exception,
-    or return a dict which will be serialized by requested content type.
-    """
-
-    @webob.dec.wsgify
-    def __call__(self, req):
-        """
-        Call the method specified in req.environ by RoutesMiddleware.
-        """
-        arg_dict = req.environ['wsgiorg.routing_args'][1]
-        action = arg_dict['action']
-        method = getattr(self, action)
-        del arg_dict['controller']
-        del arg_dict['action']
-        arg_dict['req'] = req
-        result = method(**arg_dict)
-        if type(result) is dict:
-            return self._serialize(result, req)
-        else:
-            return result
-
-    def _serialize(self, data, request):
-        """
-        Serialize the given dict to the response type requested in request.
-        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)
-
-
-class Serializer(object):
-    """
-    Serializes a dictionary to a Content Type specified by a WSGI environment.
-    """
-
-    def __init__(self, environ, 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.environ = environ
-        self.metadata = metadata or {}
-        self._methods = {
-            'application/json': self._to_json,
-            'application/xml': self._to_xml}
-
-    def to_content_type(self, data):
-        """
-        Serialize a dictionary into a string.  The format of the string
-        will be decided based on the Content Type requested in self.environ:
-        by Accept: header, or by URL suffix.
-        """
-        # FIXME(sirp): for now, supporting json only
-        #mimetype = 'application/xml'
-        mimetype = 'application/json'
-        # TODO(gundlach): determine mimetype from request
-        return self._methods.get(mimetype, repr)(data)
-
-    def _to_json(self, data):
+class Request(webob.Request):
+    """Add some Openstack API-specific logic to the base webob.Request."""
+
+    def best_match_content_type(self):
+        """Determine the requested response content-type."""
+        supported = ('application/json',)
+        bm = self.accept.best_match(supported)
+        return bm or 'application/json'
+
+    def get_content_type(self, allowed_content_types):
+        """Determine content type of the request body."""
+        if not "Content-Type" in self.headers:
+            raise exception.InvalidContentType(content_type=None)
+
+        content_type = self.content_type
+
+        if content_type not in allowed_content_types:
+            raise exception.InvalidContentType(content_type=content_type)
+        else:
+            return content_type
+
+
+class JSONRequestDeserializer(object):
+    def has_body(self, request):
+        """
+        Returns whether a Webob.Request object will possess an entity body.
+
+        :param request:  Webob.Request object
+        """
+        if 'transfer-encoding' in request.headers:
+            return True
+        elif request.content_length > 0:
+            return True
+
+        return False
+
+    def from_json(self, datastring):
+        return json.loads(datastring)
+
+    def default(self, request):
+        if self.has_body(request):
+            return {'body': self.from_json(request.body)}
+        else:
+            return {}
+
+
+class JSONResponseSerializer(object):
+
+    def to_json(self, data):
         def sanitizer(obj):
             if isinstance(obj, datetime.datetime):
                 return obj.isoformat()
@@ -279,37 +263,85 @@
 
         return json.dumps(data, default=sanitizer)
 
-    def _to_xml(self, data):
-        metadata = self.metadata.get('application/xml', {})
-        # We expect data to contain a single key which is the XML root.
-        root_key = data.keys()[0]
-        from xml.dom import minidom
-        doc = minidom.Document()
-        node = self._to_xml_node(doc, metadata, root_key, data[root_key])
-        return node.toprettyxml(indent='    ')
-
-    def _to_xml_node(self, doc, metadata, nodename, data):
-        """Recursive method to convert data members to XML nodes."""
-        result = doc.createElement(nodename)
-        if type(data) is list:
-            singular = metadata.get('plurals', {}).get(nodename, None)
-            if singular is None:
-                if nodename.endswith('s'):
-                    singular = nodename[:-1]
-                else:
-                    singular = 'item'
-            for item in data:
-                node = self._to_xml_node(doc, metadata, singular, item)
-                result.appendChild(node)
-        elif type(data) is dict:
-            attrs = metadata.get('attributes', {}).get(nodename, {})
-            for k, v in data.items():
-                if k in attrs:
-                    result.setAttribute(k, str(v))
-                else:
-                    node = self._to_xml_node(doc, metadata, k, v)
-                    result.appendChild(node)
-        else:  # atom
-            node = doc.createTextNode(str(data))
-            result.appendChild(node)
-        return result
+    def default(self, response, result):
+        response.headers.add('Content-Type', 'application/json')
+        response.body = self.to_json(result)
+
+
+class Resource(object):
+    """
+    WSGI app that handles (de)serialization and controller dispatch.
+
+    Reads routing information supplied by RoutesMiddleware and calls
+    the requested action method upon its deserializer, controller,
+    and serializer. Those three objects may implement any of the basic
+    controller action methods (create, update, show, index, delete)
+    along with any that may be specified in the api router. A 'default'
+    method may also be implemented to be used in place of any
+    non-implemented actions. Deserializer methods must accept a request
+    argument and return a dictionary. Controller methods must accept a
+    request argument. Additionally, they must also accept keyword
+    arguments that represent the keys returned by the Deserializer. They
+    may raise a webob.exc exception or return a dict, which will be
+    serialized by requested content type.
+    """
+    def __init__(self, controller, deserializer, serializer):
+        """
+        :param controller: object that implement methods created by routes lib
+        :param deserializer: object that supports webob request deserialization
+                             through controller-like actions
+        :param serializer: object that supports webob response serialization
+                           through controller-like actions
+        """
+        self.controller = controller
+        self.serializer = serializer
+        self.deserializer = deserializer
+
+    @webob.dec.wsgify(RequestClass=Request)
+    def __call__(self, request):
+        """WSGI method that controls (de)serialization and method dispatch."""
+        action_args = self.get_action_args(request.environ)
+        action = action_args.pop('action', None)
+
+        deserialized_request = self.dispatch(self.deserializer,
+                                             action, request)
+        action_args.update(deserialized_request)
+
+        action_result = self.dispatch(self.controller, action,
+                                      request, **action_args)
+        try:
+            response = webob.Response()
+            self.dispatch(self.serializer, action, response, action_result)
+            return response
+
+        # return unserializable result (typically a webob exc)
+        except Exception:
+            return action_result
+
+    def dispatch(self, obj, action, *args, **kwargs):
+        """Find action-specific method on self and call it."""
+        try:
+            method = getattr(obj, action)
+        except AttributeError:
+            method = getattr(obj, 'default')
+
+        return method(*args, **kwargs)
+
+    def get_action_args(self, request_environment):
+        """Parse dictionary created by routes library."""
+        try:
+            args = request_environment['wsgiorg.routing_args'][1].copy()
+        except Exception:
+            return {}
+
+        try:
+            del args['controller']
+        except KeyError:
+            pass
+
+        try:
+            del args['format']
+        except KeyError:
+            pass
+
+        return args
 
=== modified file 'glance/registry/client.py'
--- glance/registry/client.py	2011-05-27 19:48:51 +0000
+++ glance/registry/client.py	2011-06-17 20:46:38 +0000
@@ -94,10 +94,16 @@
         """
         Tells registry about an image's metadata
         """
+        headers = {
+            'Content-Type': 'application/json',
+        }
+
         if 'image' not in image_metadata.keys():
             image_metadata = dict(image=image_metadata)
+
         body = json.dumps(image_metadata)
-        res = self.do_request("POST", "/images", body)
+
+        res = self.do_request("POST", "/images", body, headers=headers)
         # Registry returns a JSONified dict(image=image_info)
         data = json.loads(res.read())
         return data['image']
@@ -111,9 +117,13 @@
 
         body = json.dumps(image_metadata)
 
-        headers = {}
+        headers = {
+            'Content-Type': 'application/json',
+        }
+
         if purge_props:
             headers["X-Glance-Registry-Purge-Props"] = "true"
+
         res = self.do_request("PUT", "/images/%s" % image_id, body, headers)
         data = json.loads(res.read())
         image = data['image']
 
=== modified file 'glance/registry/server.py'
--- glance/registry/server.py	2011-05-18 00:19:44 +0000
+++ glance/registry/server.py	2011-06-17 20:46:38 +0000
@@ -42,7 +42,7 @@
 MAX_ITEM_LIMIT = 25
 
 
-class Controller(wsgi.Controller):
+class Controller(object):
     """Controller for the reference implementation registry server"""
 
     def __init__(self, options):
@@ -167,7 +167,7 @@
         """
         Deletes an existing image with the registry.
 
-        :param req: Request body.  Ignored.
+        :param req: wsgi Request object
         :param id:  The opaque internal identifier for the image
 
         :retval Returns 200 if delete was successful, a fault if not.
@@ -179,19 +179,19 @@
         except exception.NotFound:
             return exc.HTTPNotFound()
 
-    def create(self, req):
+    def create(self, req, body):
         """
         Registers a new image with the registry.
 
-        :param req: Request body.  A JSON-ified dict of information about
-                    the image.
+        :param req: wsgi Request object
+        :param body: Dictionary of information about the image
 
         :retval Returns the newly-created image information as a mapping,
                 which will include the newly-created image's internal id
                 in the 'id' field
 
         """
-        image_data = json.loads(req.body)['image']
+        image_data = body['image']
 
         # Ensure the image has a status set
         image_data.setdefault('status', 'active')
@@ -209,18 +209,17 @@
             logger.error(msg)
             return exc.HTTPBadRequest(msg)
 
-    def update(self, req, id):
+    def update(self, req, id, body):
         """Updates an existing image with the registry.
 
-        :param req: Request body.  A JSON-ified dict of information about
-                    the image.  This will replace the information in the
-                    registry about this image
+        :param req: wsgi Request object
+        :param body: Dictionary of information about the image
         :param id:  The opaque internal identifier for the image
 
         :retval Returns the updated image information as a mapping,
 
         """
-        image_data = json.loads(req.body)['image']
+        image_data = body['image']
 
         purge_props = req.headers.get("X-Glance-Registry-Purge-Props", "false")
         context = None
@@ -244,15 +243,22 @@
                                content_type='text/plain')
 
 
+def create_resource(options):
+    """Images resource factory method."""
+    deserializer = wsgi.JSONRequestDeserializer()
+    serializer = wsgi.JSONResponseSerializer()
+    return wsgi.Resource(Controller(options), deserializer, serializer)
+
+
 class API(wsgi.Router):
     """WSGI entry point for all Registry requests."""
 
     def __init__(self, options):
         mapper = routes.Mapper()
-        controller = Controller(options)
-        mapper.resource("image", "images", controller=controller,
+        resource = create_resource(options)
+        mapper.resource("image", "images", controller=resource,
                        collection={'detail': 'GET'})
-        mapper.connect("/", controller=controller, action="index")
+        mapper.connect("/", controller=resource, action="index")
         super(API, self).__init__(mapper)
 
 
 
=== modified file 'glance/utils.py'
--- glance/utils.py	2011-04-12 21:45:16 +0000
+++ glance/utils.py	2011-06-17 20:46:38 +0000
@@ -43,24 +43,6 @@
     return headers
 
 
-def inject_image_meta_into_headers(response, image_meta):
-    """
-    Given a response and mapping of image metadata, injects
-    the Response with a set of HTTP headers for the image
-    metadata. Each main image metadata field is injected
-    as a HTTP header with key 'x-image-meta-<FIELD>' except
-    for the properties field, which is further broken out
-    into a set of 'x-image-meta-property-<KEY>' headers
-
-    :param response: The Webob Response object
-    :param image_meta: Mapping of image metadata
-    """
-    headers = image_meta_to_http_headers(image_meta)
-
-    for k, v in headers.items():
-        response.headers.add(k, v)
-
-
 def get_image_meta_from_headers(response):
     """
     Processes HTTP headers from a supplied response that
 
=== modified file 'tests/functional/test_curl_api.py'
--- tests/functional/test_curl_api.py	2011-05-27 19:48:51 +0000
+++ tests/functional/test_curl_api.py	2011-06-17 20:46:38 +0000
@@ -773,7 +773,7 @@
         with tempfile.NamedTemporaryFile() as test_data_file:
             test_data_file.write("XXX")
             test_data_file.flush()
-            cmd = ("curl -i -X POST --upload-file %s "
+            cmd = ("curl -i -X POST --upload-file %s -H 'Expect: ' "
                    "http://0.0.0.0:%d/v1/images") % (test_data_file.name,
                                                   api_port)
 
 
=== modified file 'tests/unit/test_api.py'
--- tests/unit/test_api.py	2011-05-31 19:32:17 +0000
+++ tests/unit/test_api.py	2011-06-17 20:46:38 +0000
@@ -678,6 +678,7 @@
         req = webob.Request.blank('/images')
 
         req.method = 'POST'
+        req.content_type = 'application/json'
         req.body = json.dumps(dict(image=fixture))
 
         res = req.get_response(self.api)
@@ -706,6 +707,7 @@
         req = webob.Request.blank('/images')
 
         req.method = 'POST'
+        req.content_type = 'application/json'
         req.body = json.dumps(dict(image=fixture))
 
         res = req.get_response(self.api)
@@ -723,6 +725,7 @@
         req = webob.Request.blank('/images')
 
         req.method = 'POST'
+        req.content_type = 'application/json'
         req.body = json.dumps(dict(image=fixture))
 
         res = req.get_response(self.api)
@@ -739,6 +742,7 @@
         req = webob.Request.blank('/images')
 
         req.method = 'POST'
+        req.content_type = 'application/json'
         req.body = json.dumps(dict(image=fixture))
 
         res = req.get_response(self.api)
@@ -758,6 +762,7 @@
         req = webob.Request.blank('/images')
 
         req.method = 'POST'
+        req.content_type = 'application/json'
         req.body = json.dumps(dict(image=fixture))
 
         res = req.get_response(self.api)
@@ -772,6 +777,7 @@
         req = webob.Request.blank('/images/2')
 
         req.method = 'PUT'
+        req.content_type = 'application/json'
         req.body = json.dumps(dict(image=fixture))
 
         res = req.get_response(self.api)
@@ -791,6 +797,7 @@
         req = webob.Request.blank('/images/3')
 
         req.method = 'PUT'
+        req.content_type = 'application/json'
         req.body = json.dumps(dict(image=fixture))
 
         res = req.get_response(self.api)
@@ -804,6 +811,7 @@
         req = webob.Request.blank('/images/2')
 
         req.method = 'PUT'
+        req.content_type = 'application/json'
         req.body = json.dumps(dict(image=fixture))
 
         res = req.get_response(self.api)
@@ -817,6 +825,7 @@
         req = webob.Request.blank('/images/2')
 
         req.method = 'PUT'
+        req.content_type = 'application/json'
         req.body = json.dumps(dict(image=fixture))
 
         res = req.get_response(self.api)
@@ -830,6 +839,7 @@
         req = webob.Request.blank('/images/2')
 
         req.method = 'PUT'
+        req.content_type = 'application/json'
         req.body = json.dumps(dict(image=fixture))
 
         res = req.get_response(self.api)
@@ -844,6 +854,7 @@
         req = webob.Request.blank('/images/2')  # Image 2 has disk format 'vhd'
 
         req.method = 'PUT'
+        req.content_type = 'application/json'
         req.body = json.dumps(dict(image=fixture))
 
         res = req.get_response(self.api)
@@ -972,6 +983,21 @@
         res_body = json.loads(res.body)['image']
         self.assertEquals('queued', res_body['status'])
 
+    def test_add_image_no_location_no_content_type(self):
+        """Tests creates a queued image for no body and no loc header"""
+        fixture_headers = {'x-image-meta-store': 'file',
+                           'x-image-meta-disk-format': 'vhd',
+                           'x-image-meta-container-format': 'ovf',
+                           'x-image-meta-name': 'fake image #3'}
+
+        req = webob.Request.blank("/images")
+        req.body = "chunk00000remainder"
+        req.method = 'POST'
+        for k, v in fixture_headers.iteritems():
+            req.headers[k] = v
+        res = req.get_response(self.api)
+        self.assertEquals(res.status_int, 400)
+
     def test_add_image_bad_store(self):
         """Tests raises BadRequest for invalid store header"""
         fixture_headers = {'x-image-meta-store': 'bad',
 
=== added file 'tests/unit/test_wsgi.py'
--- tests/unit/test_wsgi.py	1970-01-01 00:00:00 +0000
+++ tests/unit/test_wsgi.py	2011-06-17 20:46:38 +0000
@@ -0,0 +1,182 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2010-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 unittest
+import webob
+
+from glance.common import wsgi
+from glance.common import exception
+
+
+class RequestTest(unittest.TestCase):
+    def test_content_type_missing(self):
+        request = wsgi.Request.blank('/tests/123')
+        request.body = "<body />"
+        self.assertRaises(exception.InvalidContentType,
+                          request.get_content_type, ('application/xml'))
+
+    def test_content_type_unsupported(self):
+        request = wsgi.Request.blank('/tests/123')
+        request.headers["Content-Type"] = "text/html"
+        request.body = "asdf<br />"
+        self.assertRaises(exception.InvalidContentType,
+                          request.get_content_type, ('application/xml'))
+
+    def test_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(('application/json'))
+        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/json")
+
+    def test_content_type_from_accept_json(self):
+        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_xml_json(self):
+        request = wsgi.Request.blank('/tests/123')
+        request.headers["Accept"] = "application/xml, application/json"
+        result = request.best_match_content_type()
+        self.assertEqual(result, "application/json")
+
+    def test_content_type_from_accept_json_xml_quality(self):
+        request = wsgi.Request.blank('/tests/123')
+        request.headers["Accept"] = \
+            "application/json; q=0.3, application/xml; q=0.9"
+        result = request.best_match_content_type()
+        self.assertEqual(result, "application/json")
+
+    def test_content_type_accept_default(self):
+        request = wsgi.Request.blank('/tests/123.unsupported')
+        request.headers["Accept"] = "application/unsupported1"
+        result = request.best_match_content_type()
+        self.assertEqual(result, "application/json")
+
+
+class ResourceTest(unittest.TestCase):
+    def test_get_action_args(self):
+        env = {
+            'wsgiorg.routing_args': [
+                None,
+                {
+                    'controller': None,
+                    'format': None,
+                    'action': 'update',
+                    'id': 12,
+                },
+            ],
+        }
+
+        expected = {'action': 'update', 'id': 12}
+        actual = wsgi.Resource(None, None, None).get_action_args(env)
+
+        self.assertEqual(actual, expected)
+
+    def test_dispatch(self):
+        class Controller(object):
+            def index(self, shirt, pants=None):
+                return (shirt, pants)
+
+        resource = wsgi.Resource(None, None, None)
+        actual = resource.dispatch(Controller(), 'index', 'on', pants='off')
+        expected = ('on', 'off')
+        self.assertEqual(actual, expected)
+
+    def test_dispatch_default(self):
+        class Controller(object):
+            def default(self, shirt, pants=None):
+                return (shirt, pants)
+
+        resource = wsgi.Resource(None, None, None)
+        actual = resource.dispatch(Controller(), 'index', 'on', pants='off')
+        expected = ('on', 'off')
+        self.assertEqual(actual, expected)
+
+    def test_dispatch_no_default(self):
+        class Controller(object):
+            def show(self, shirt, pants=None):
+                return (shirt, pants)
+
+        resource = wsgi.Resource(None, None, None)
+        self.assertRaises(AttributeError, resource.dispatch, Controller(),
+                          'index', 'on', pants='off')
+
+
+class JSONResponseSerializerTest(unittest.TestCase):
+    def test_to_json(self):
+        fixture = {"key": "value"}
+        expected = '{"key": "value"}'
+        actual = wsgi.JSONResponseSerializer().to_json(fixture)
+        self.assertEqual(actual, expected)
+
+    def test_default(self):
+        fixture = {"key": "value"}
+        response = webob.Response()
+        wsgi.JSONResponseSerializer().default(response, fixture)
+        self.assertEqual(response.status_int, 200)
+        self.assertEqual(response.content_type, 'application/json')
+        self.assertEqual(response.body, '{"key": "value"}')
+
+
+class JSONRequestDeserializerTest(unittest.TestCase):
+    def test_has_body_no_content_length(self):
+        request = wsgi.Request.blank('/')
+        request.body = 'asdf'
+        request.headers.pop('Content-Length')
+        self.assertFalse(wsgi.JSONRequestDeserializer().has_body(request))
+
+    def test_has_body_zero_content_length(self):
+        request = wsgi.Request.blank('/')
+        request.body = 'asdf'
+        request.headers['Content-Length'] = 0
+        self.assertFalse(wsgi.JSONRequestDeserializer().has_body(request))
+
+    def test_has_body_has_content_length(self):
+        request = wsgi.Request.blank('/')
+        request.body = 'asdf'
+        self.assertTrue('Content-Length' in request.headers)
+        self.assertTrue(wsgi.JSONRequestDeserializer().has_body(request))
+
+    def test_no_body_no_content_length(self):
+        request = wsgi.Request.blank('/')
+        self.assertFalse(wsgi.JSONRequestDeserializer().has_body(request))
+
+    def test_from_json(self):
+        fixture = '{"key": "value"}'
+        expected = {"key": "value"}
+        actual = wsgi.JSONRequestDeserializer().from_json(fixture)
+        self.assertEqual(actual, expected)
+
+    def test_default_no_body(self):
+        request = wsgi.Request.blank('/')
+        actual = wsgi.JSONRequestDeserializer().default(request)
+        expected = {}
+        self.assertEqual(actual, expected)
+
+    def test_default_with_body(self):
+        request = wsgi.Request.blank('/')
+        request.body = '{"key": "value"}'
+        actual = wsgi.JSONRequestDeserializer().default(request)
+        expected = {"body": {"key": "value"}}
+        self.assertEqual(actual, expected)