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.results # dict(image=image_meta) response.body = self.to_json(meta) return response def create(self, response): return self._serialize(response) def update(self, response): return self._serialize(response) And leave all the other logic and header-setting in the Controller class... Note that above, I use: meta = response.results In the Controller, you would change this: return {'image_meta': blah} to be like what it was before, with a slight change: response = webob.Response(request=req) # Add response headers... response.results = image_meta return response This allows logic to be centralized in the Controller objects and for the Serializer objects to focus their responsibility only on serializing the contents of the Response objects returned from the Controller methods. 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 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. 3) few little typos 67 + :retval similiar to 'show' method but without image_data s/similiar/similar 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'): 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: 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) 6) the wsgi.Resource constructor 665 + def __init__(self, deserializer, controller, serializer): Could you change the param order to (controller, serializer, deserializer)? You list the params in the docstring that way, and I think it makes more sense that way... 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?