Code review comment for lp:~rackspace-titan/glance/wsgi-refactor

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?