Code review comment for lp:~rackspace-titan/nova/osapi-serialization

Revision history for this message
Mark Washenberger (markwash) wrote :

Overall, I'm quite pleased. This is very much the direction I was hoping to go with the serialization blueprint and you've done a good job of handling some of the trickier details. This approach is going to be a big help as we go move towards adding better support for xml requests in particular.

- Thanks for giving my nasty one-off deserializer for server create requests a home.

- I look forward to seeing how we can build on this with request validation.

Nits, Information, and Issues:

- I like the convenience functions for creating Resources for each entry in the mapper. But a factory is usually an object, not a function. Maybe the controller module functions should be called 'get_resource()' or 'create_resource()' ?

- I see that the approach we take with extensions leans on the serialization components you are changing. However, I think it might be better if we don't reuse Resources for extensions. Rather, we might want to reuse the serializer and deserializer directly. Does this sound feasible?

- It seems like an unnecessary duplication to pass in both the request object and the deserialized body of that request to the controller. Mostly it seems like controllers use the request object to get access to the nova.context object. Perhaps we could extract context from the request and just pass in the context and the body? If that is the case, we could also rename the body variable to "request" which would be more intuitive to me. This change might cause problems with the extensions, which seem to need access to the full request--which is partly why I propose that we pull Resource out of the extensions module.

- I think you have some conflicts to resolve from the request extensions branch that just merged into trunk.

review: Needs Fixing

« Back to merge proposal