Merge lp:~rconradharris/glance/use_datetime_obj into lp:~hudson-openstack/glance/trunk

[Rick Harris]

Converts timestamp attributes to datetime objects before persisting.

Refactors image_update and image_create to use the same basic code.

[Jay Pipes (jaypipes)]

Hi!

Good stuff, Rick :) Couple quick comments:

* Please add a test case for this... easy enough to change the stub_out_db_api stuff to have (one or more) of the FIXTURES have a string datetime instead of datetime.datetime.utcnow()...
* Please pep8 check:
** No spaces before docstring comments so """ Used internally by image_create and image_update """
should be """Used internally by image_create and image_update""" or
"""
Used internally by image_create and image_update
"""
** No newlines at end of file
** Two newlines between each module-level object and also after import statements

Cheers!
jay

review: Needs Fixing

[lp:~rconradharris/glance/use_datetime_obj]

29. By Rick Harris on 2010-12-23

Adding __protected_attributes__, some PEP8 cleanups

[Jay Pipes (jaypipes)]

very nice.

review: Approve

=== modified file 'glance/client.py'
--- glance/client.py	2010-12-21 05:30:04 +0000
+++ glance/client.py	2010-12-23 19:24:08 +0000
@@ -255,7 +255,7 @@
             headers['content-type'] = 'application/octet-stream'
         else:
             body = None
-        
+
         res = self.do_request("POST", "/images", body, headers)
         data = json.loads(res.read())
         return data['image']['id']

=== modified file 'glance/common/db/__init__.py'
--- glance/common/db/__init__.py	2010-10-01 19:51:29 +0000
+++ glance/common/db/__init__.py	2010-12-23 19:24:08 +0000
@@ -19,4 +19,3 @@
 """
 DB abstraction for Nova and Glance
 """
-

=== modified file 'glance/common/wsgi.py'
--- glance/common/wsgi.py	2010-12-18 18:00:21 +0000
+++ glance/common/wsgi.py	2010-12-23 19:24:08 +0000
@@ -21,6 +21,7 @@
 Utility methods for working with WSGI servers
 """
 
+import json
 import logging
 import sys
 import datetime
@@ -115,11 +116,11 @@
     behavior.
     """
 
-    def __init__(self, application): # pylint: disable-msg=W0231
+    def __init__(self, application):  # pylint: disable-msg=W0231
         self.application = application
 
     @webob.dec.wsgify
-    def __call__(self, req): # pylint: disable-msg=W0221
+    def __call__(self, req):  # pylint: disable-msg=W0221
         """Override to implement middleware behavior."""
         return self.application
 
@@ -237,7 +238,7 @@
         arg_dict['req'] = req
         result = method(**arg_dict)
         if type(result) is dict:
-            return self._serialize(result, req) 
+            return self._serialize(result, req)
         else:
             return result
 
@@ -282,7 +283,6 @@
         return self._methods.get(mimetype, repr)(data)
 
     def _to_json(self, data):
-        import json
         def sanitizer(obj):
             if isinstance(obj, datetime.datetime):
                 return obj.isoformat()
@@ -320,7 +320,7 @@
                 else:
                     node = self._to_xml_node(doc, metadata, k, v)
                     result.appendChild(node)
-        else: # atom
+        else:  # atom
             node = doc.createTextNode(str(data))
             result.appendChild(node)
         return result

=== modified file 'glance/registry/__init__.py'
--- glance/registry/__init__.py	2010-12-14 15:10:54 +0000
+++ glance/registry/__init__.py	2010-12-23 19:24:08 +0000
@@ -17,7 +17,7 @@
 #    under the License.
 
 """
-Registry API 
+Registry API
 """
 
 from glance.registry import client

=== modified file 'glance/registry/db/sqlalchemy/api.py'
--- glance/registry/db/sqlalchemy/api.py	2010-12-21 15:07:16 +0000
+++ glance/registry/db/sqlalchemy/api.py	2010-12-23 19:24:08 +0000
@@ -52,19 +52,7 @@
 
 
 def image_create(_context, values):
-    values['size'] = int(values['size'])
-    values['is_public'] = bool(values.get('is_public', False))
-    properties = values.pop('properties', {})
-
-    image_ref = models.Image()
-    image_ref.update(values)
-    image_ref.save()
- 
-    for key, value in properties.iteritems():
-        prop_values = {'image_id': image_ref.id, 'key': key, 'value': value}
-        image_property_create(_context, prop_values)
- 
-    return image_get(_context, image_ref.id)
+    return _image_update(_context, values, None)
 
 
 def image_destroy(_context, image_id):
@@ -109,13 +97,47 @@
 
 
 def image_update(_context, image_id, values):
+    return _image_update(_context, values, image_id)
+
+
+###################
+
+
+def image_property_create(_context, values):
+    _drop_protected_attrs(models.Image, values)
+    image_property_ref = models.ImageProperty()
+    image_property_ref.update(values)
+    image_property_ref.save()
+    return image_property_ref
+
+
+def _drop_protected_attrs(model_class, values):
+    """Removed protected attributes from values dictionary using the models
+    __protected_attributes__ field.
+    """
+    for attr in model_class.__protected_attributes__:
+        if attr in values:
+            del values[attr]
+
+
+def _image_update(_context, values, image_id):
+    """Used internally by image_create and image_update
+
+    :param image_id: If None, create the image, otherwise, find and update it
+    """
     session = get_session()
     with session.begin():
+        _drop_protected_attrs(models.Image, values)
+
         values['size'] = int(values['size'])
         values['is_public'] = bool(values.get('is_public', False))
         properties = values.pop('properties', {})
 
-        image_ref = models.Image.find(image_id, session=session)
+        if image_id:
+            image_ref = models.Image.find(image_id, session=session)
+        else:
+            image_ref = models.Image()
+
         image_ref.update(values)
         image_ref.save(session=session)
 
@@ -123,22 +145,4 @@
             prop_values = {'image_id': image_ref.id, 'key': key, 'value': value}
             image_property_create(_context, prop_values)
 
-
-###################
-
-
-def image_file_create(_context, values):
-    image_file_ref = models.ImageFile()
-    image_file_ref.update(values)
-    image_file_ref.save()
-    return image_file_ref
-
-
-###################
-
-
-def image_property_create(_context, values):
-    image_property_ref = models.ImageProperty()
-    image_property_ref.update(values)
-    image_property_ref.save()
-    return image_property_ref
+    return image_get(_context, image_ref.id)

=== modified file 'glance/registry/db/sqlalchemy/models.py'
--- glance/registry/db/sqlalchemy/models.py	2010-12-16 22:04:58 +0000
+++ glance/registry/db/sqlalchemy/models.py	2010-12-23 19:24:08 +0000
@@ -45,6 +45,9 @@
     __table_args__ = {'mysql_engine': 'InnoDB'}
     __table_initialized__ = False
     __prefix__ = 'none'
+    __protected_attributes__ = set([
+        "created_at", "updated_at", "deleted_at", "deleted"])
+
     created_at = Column(DateTime, default=datetime.datetime.utcnow)
     updated_at = Column(DateTime, onupdate=datetime.datetime.utcnow)
     deleted_at = Column(DateTime)
@@ -160,23 +163,6 @@
             raise exception.Invalid("Invalid status '%s' for image." % status)
         return status
     
-    # TODO(sirp): should these be stored as properties?
-    #user_id = Column(String(255))
-    #project_id = Column(String(255))
-    #arch = Column(String(255))
-    #default_kernel_id = Column(String(255))
-    #default_ramdisk_id = Column(String(255))
-    #
-    #@validates('default_kernel_id')
-    #def validate_kernel_id(self, key, val):
-    #    if val != 'machine':
-    #        assert(val is None)
-    # 
-    #@validates('default_ramdisk_id')
-    #def validate_ramdisk_id(self, key, val):
-    #    if val != 'machine':
-    #        assert(val is None)
-
 
 class ImageProperty(BASE, ModelBase):
     """Represents an image properties in the datastore"""

=== modified file 'glance/registry/server.py'
--- glance/registry/server.py	2010-12-18 18:00:21 +0000
+++ glance/registry/server.py	2010-12-23 19:24:08 +0000
@@ -28,12 +28,11 @@
 
 
 class ImageController(wsgi.Controller):
-
     """Image Controller """
-    
+
     def index(self, req):
         """Return basic information for all public, non-deleted images
-        
+
         :param req: the Request object coming from the wsgi layer
         :retval a mapping of the following form::
 
@@ -42,7 +41,7 @@
         Where image_list is a sequence of mappings::
 
             {'id': image_id, 'name': image_name}
-        
+
         """
         images = db.image_get_all_public(None)
         image_dicts = [dict(id=i['id'], name=i['name']) for i in images]
@@ -50,7 +49,7 @@
 
     def detail(self, req):
         """Return detailed information for all public, non-deleted images
-        
+
         :param req: the Request object coming from the wsgi layer
         :retval a mapping of the following form::
 
@@ -58,7 +57,7 @@
 
         Where image_list is a sequence of mappings containing
         all image model fields.
-        
+
         """
         images = db.image_get_all_public(None)
         image_dicts = [make_image_dict(i) for i in images]
@@ -70,7 +69,7 @@
             image = db.image_get(None, id)
         except exception.NotFound:
             raise exc.HTTPNotFound()
-        
+
         return dict(image=make_image_dict(image))
 
     def delete(self, req, id):
@@ -153,7 +152,7 @@
     Create a dict representation of an image which we can use to
     serialize the image.
     """
-    
+
     def _fetch_attrs(d, attrs):
         return dict([(a, d[a]) for a in attrs
                     if a in d.keys()])

=== modified file 'glance/server.py'
--- glance/server.py	2010-12-21 05:30:04 +0000
+++ glance/server.py	2010-12-23 19:24:08 +0000
@@ -57,10 +57,10 @@
 
     """
     Main WSGI application controller for Glance.
-    
+
     The Glance API is a RESTful web service for image data. The API
     is as follows::
-        
+
         GET /images -- Returns a set of brief metadata about images
         GET /images/detail -- Returns a set of detailed metadata about
                               images
@@ -72,7 +72,7 @@
                             image data is immutable once stored)
         DELETE /images/<ID> -- Delete the image with id <ID>
     """
-    
+
     def index(self, req):
         """
         Returns the following information for all public, available images:
@@ -81,7 +81,7 @@
             * name -- The name of the image
             * size -- Size of image data in bytes
             * type -- One of 'kernel', 'ramdisk', 'raw', or 'machine'
-        
+
         :param request: The WSGI/Webob Request object
         :retval The response body is a mapping of the following form::
 
@@ -98,7 +98,7 @@
     def detail(self, req):
         """
         Returns detailed information for all public, available images
-        
+
         :param request: The WSGI/Webob Request object
         :retval The response body is a mapping of the following form::
 
@@ -225,6 +225,7 @@
                 try:
                     location = store.add(image_meta['id'], req.body)
                 except exception.Duplicate, e:
+                    logging.error("Error adding image to store: %s", str(e))
                     return HTTPConflict(str(e), request=req)
                 image_meta['status'] = 'available'
                 image_meta['location'] = location
@@ -233,8 +234,10 @@
             return dict(image=image_meta)
 
         except exception.Duplicate:
-            return HTTPConflict("An image with identifier %s already exists"
-                                % image_meta['id'], request=req)
+            msg = "An image with identifier %s already exists"\
+                  % image_meta['id']
+            logging.error(msg)
+            return HTTPConflict(msg, request=req)
         except exception.Invalid:
             return HTTPBadRequest()
 

=== modified file 'glance/store/filesystem.py'
--- glance/store/filesystem.py	2010-12-21 05:30:04 +0000
+++ glance/store/filesystem.py	2010-12-23 19:24:08 +0000
@@ -66,10 +66,10 @@
             self.fp = None
 
 
-
 class FilesystemBackend(glance.store.Backend):
     @classmethod
-    def get(cls, parsed_uri, opener=lambda p: open(p, "rb"), expected_size=None):
+    def get(cls, parsed_uri, opener=lambda p: open(p, "rb"),
+            expected_size=None):
         """ Filesystem-based backend
 
         file:///path/to/file.tar.gz.0

=== modified file 'glance/store/http.py'
--- glance/store/http.py	2010-12-14 01:56:11 +0000
+++ glance/store/http.py	2010-12-23 19:24:08 +0000
@@ -29,7 +29,7 @@
         """
 
         if conn_class:
-            pass # use the conn_class passed in
+            pass  # use the conn_class passed in
         elif parsed_uri.scheme == "http":
             conn_class = httplib.HTTPConnection
         elif parsed_uri.scheme == "https":

=== modified file 'glance/store/swift.py'
--- glance/store/swift.py	2010-12-16 22:04:58 +0000
+++ glance/store/swift.py	2010-12-23 19:24:08 +0000
@@ -114,7 +114,7 @@
 
 def get_connection_class(conn_class):
     if conn_class:
-        pass # Use the provided conn_class
+        pass  # Use the provided conn_class
     else:
         # NOTE(sirp): A standard import statement won't work here because
         # this file ('swift.py') is shadowing the swift module, and since