Merge lp:~jaypipes/glance/clients into lp:~hudson-openstack/glance/trunk

[Jay Pipes]

Adds client classes for Parallax and Teller and fixes some issues where our controller was not returning proper HTTP response codes on errors...

Adds client classes for Parallax and Teller and fixes some issues where our controller was not returning proper HTTP response codes on errors...

[Rick Harris (rconradharris)]

Overall lgtm. Have a few points I'd like to open up for discussion:

1. The catching of KeyError on line 77 seems too broad; it should just wrap the dictionary lookup, not the lookup and the ping. If the connection object (or more likely a mocked version of the connection object) erroneously raised KeyError, the re-raised exception would be misleading.

2. Pinging the endpoint from within the __init__ feels messy: the usual errors associated with network traffic are unexpected when all you're trying to do is construct and initialize an object. I would feel better if this test were in a separate connect() method. Another option would be to provide a kwarg test_connection=False on __init__ which requires the caller to acknowledge that their __init__ invocation is causing network traffic. A third option would be to just omit it and require the consumer to handle the destination being down just as they would for any other network library.

3. Very minor but the pattern
137 + c = self.connection_type(self.netloc, self.port)
138 + c.request("GET", "images")
139 + res = c.getresponse()
is repeated in a bunch of places.

Perhaps a do_request method would DRY up the code a bit.

def do_request(method, path, headers=None, data=None):
    """Something like this"""
    conn = self.connection_type(self.netloc, self.port)
    conn.request(method, path, headers=header or {}, data=data or None)
    # could check for non-200 status code and raise exceptions here as well.
    return conn.getresponse()

4. At some point we should clarify terminology: what exactly is meant by "image metadata"? I think in difference parts of the code and design docs we're using it to mean different things. In one case (I think how it's being used here), it's used to refer to the image registry information (name, backend_uri, created_at, updated_at) as opposed to "image data" which is used to refer to the actual image files.

The other use of "image metadata" is within the parallax db.model where I added the table "image_metadatum". This is a table used to store arbitrary key/value pairs associated with an image. This makes it image metadata metadata, I guess.

Perhaps we should rename the latter "image_properties", or maybe the former should be called "image_descriptor". Whatever the names, we should just clarify this soon.

Anyway these are all minor points, interested to hear your thoughts.

[Jay Pipes (jaypipes)]

On Mon, Nov 22, 2010 at 4:04 PM, Rick Harris <email address hidden> wrote:
> Overall lgtm. Have a few points I'd like to open up for discussion:
>
> 1. The catching of KeyError on line 77 seems too broad; it should just wrap the dictionary lookup, not the lookup and the ping. If the connection object (or more likely a mocked version of the connection object) erroneously raised KeyError, the re-raised exception would be misleading.

Yep, I'll fix that up.

> 2. Pinging the endpoint from within the __init__ feels messy: the usual errors associated with network traffic are unexpected when all you're trying to do is construct and initialize an object. I would feel better if this test were in a separate connect() method. Another option would be to provide a kwarg test_connection=False on __init__ which requires the caller to acknowledge that their __init__  invocation is causing network traffic. A third option would be to just omit it and require the consumer to handle the destination being down just as they would for any other network library.

Hmm, yeah, so I toyed around with all three of those options while
developing this, actually :)

I'll rework and propose something along the lines of option #3 since
that indeed is how most network libraries behave.

> 3. Very minor but the pattern
> 137     +            c = self.connection_type(self.netloc, self.port)
> 138     +            c.request("GET", "images")
> 139     +            res = c.getresponse()
> is repeated in a bunch of places.
>
> Perhaps a do_request method would DRY up the code a bit.
>
> def do_request(method, path, headers=None, data=None):
>    """Something like this"""
>    conn = self.connection_type(self.netloc, self.port)
>    conn.request(method, path, headers=header or {}, data=data or None)
>    # could check for non-200 status code and raise exceptions here as well.
>    return conn.getresponse()

Yup. Will do.

> 4. At some point we should clarify terminology: what exactly is meant by "image metadata"? I think in difference parts of the code and design docs we're using it to mean different things. In one case (I think how it's being used here), it's used to refer to the image registry information (name, backend_uri, created_at, updated_at) as opposed to "image data" which is used to refer to the actual image files.

> The other use of "image metadata" is within the parallax db.model where I added the table "image_metadatum". This is a table used to store arbitrary key/value pairs associated with an image. This makes it image metadata metadata, I guess.
>
> Perhaps we should rename the latter "image_properties", or maybe the former should be called "image_descriptor". Whatever the names, we should just clarify this soon.

I will clarify in the docs. Metadata is ALL the information *about*
the image. I will rename the ImageMetadatum table to ImageTag, and
change the attributes to "tags" instead of "metadata" which will match
the EC2 API as well...

Come to think of it, the ImageMetadatum table is not even used or
tested in the code... I'll fix that :)

[Jay Pipes (jaypipes)]

I like "properties" better than "tags"...

[Rick Harris (rconradharris)]

Looks great!

review: Approve

[Christopher MacGown (0x44)]

lgtm

review: Approve

=== added file 'glance/client.py'
--- glance/client.py	1970-01-01 00:00:00 +0000
+++ glance/client.py	2010-11-23 18:13:46 +0000
@@ -0,0 +1,223 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2010 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.
+
+"""
+Client classes for callers of a Glance system
+"""
+
+import httplib
+import json
+import logging
+import urlparse
+import socket
+import sys
+
+from glance.common import exception
+
+#TODO(jaypipes) Allow a logger param for client classes
+#TODO(jaypipes) Raise proper errors or OpenStack API faults
+
+
+class UnsupportedProtocolError(Exception):
+    """
+    Error resulting from a client connecting to a server
+    on an unsupported protocol
+    """
+    pass
+
+
+class ClientConnectionError(Exception):
+    """Error resulting from a client connecting to a server"""
+    pass
+
+
+class BadInputError(Exception):
+    """Error resulting from a client sending bad input to a server"""
+    pass
+
+
+class BaseClient(object):
+
+    """A base client class"""
+
+    DEFAULT_ADDRESS = 'http://127.0.0.1'
+    DEFAULT_PORT = 9090
+
+    def __init__(self, **kwargs):
+        """
+        Creates a new client to some service.  All args are keyword
+        arguments.
+
+        :param address: The address where service resides (defaults to
+                        http://127.0.0.1)
+        :param port: The port where service resides (defaults to 9090)
+        """
+        self.address = kwargs.get('address', self.DEFAULT_ADDRESS)
+        self.port = kwargs.get('port', self.DEFAULT_PORT)
+        url = urlparse.urlparse(self.address)
+        self.netloc = url.netloc
+        self.protocol = url.scheme
+        self.connection = None
+
+    def do_request(self, method, action, body=None):
+        """
+        Connects to the server and issues a request.  Handles converting
+        any returned HTTP error status codes to OpenStack/Glance exceptions
+        and closing the server connection. Returns the result data, or
+        raises an appropriate exception.
+
+        :param method: HTTP method ("GET", "POST", "PUT", etc...)
+        :param action: part of URL after root netloc
+        :param headers: mapping of headers to send
+        :param data: string of data to send, or None (default)
+        """
+        try:
+            connection_type = {'http': httplib.HTTPConnection,
+                               'https': httplib.HTTPSConnection}\
+                               [self.protocol]
+        except KeyError:
+            raise UnsupportedProtocolError("Unsupported protocol %s. Unable "
+                                           " to connect to server."
+                                           % self.protocol)
+        try:
+            c = connection_type(self.netloc, self.port)
+            c.request(method, action, body)
+            res = c.getresponse()
+            status_code = self.get_status_code(res)
+            if status_code == httplib.OK:
+                return res
+            elif status_code == httplib.UNAUTHORIZED:
+                raise exception.NotAuthorized
+            elif status_code == httplib.FORBIDDEN:
+                raise exception.NotAuthorized
+            elif status_code == httplib.NOT_FOUND:
+                raise exception.NotFound
+            elif status_code == httplib.CONFLICT:
+                raise exception.Duplicate
+            elif status_code == httplib.BAD_REQUEST:
+                raise BadInputError
+            else:
+                raise Exception("Unknown error occurred! %d" % status_code)
+
+        except (socket.error, IOError), e:
+            raise ClientConnectionError("Unable to connect to "
+                                        "server. Got error: %s" % e)
+        finally:
+            c.close()
+
+    def get_status_code(self, response):
+        """
+        Returns the integer status code from the response, which
+        can be either a Webob.Response (used in testing) or httplib.Response
+        """
+        if hasattr(response, 'status_int'):
+            return response.status_int
+        else:
+            return response.status
+
+
+class TellerClient(BaseClient):
+
+    """A client for the Teller image caching service"""
+
+    DEFAULT_ADDRESS = 'http://127.0.0.1'
+    DEFAULT_PORT = 9191
+
+    def __init__(self, **kwargs):
+        """
+        Creates a new client to a Teller service.  All args are keyword
+        arguments.
+
+        :param address: The address where Teller resides (defaults to
+                        http://127.0.0.1)
+        :param port: The port where Teller resides (defaults to 9191)
+        """
+        super(TellerClient, self).__init__(**kwargs)
+
+
+class ParallaxClient(BaseClient):
+
+    """A client for the Parallax image metadata service"""
+
+    DEFAULT_ADDRESS = 'http://127.0.0.1'
+    DEFAULT_PORT = 9292
+
+    def __init__(self, **kwargs):
+        """
+        Creates a new client to a Parallax service.  All args are keyword
+        arguments.
+
+        :param address: The address where Parallax resides (defaults to
+                        http://127.0.0.1)
+        :param port: The port where Parallax resides (defaults to 9292)
+        """
+        super(ParallaxClient, self).__init__(**kwargs)
+
+    def get_images(self):
+        """
+        Returns a list of image id/name mappings from Parallax
+        """
+        res = self.do_request("GET", "images")
+        data = json.loads(res.read())['images']
+        return data
+
+    def get_images_detailed(self):
+        """
+        Returns a list of detailed image data mappings from Parallax
+        """
+        res = self.do_request("GET", "images/detail")
+        data = json.loads(res.read())['images']
+        return data
+
+    def get_image(self, image_id):
+        """
+        Returns a mapping of image metadata from Parallax
+
+        :raises exception.NotFound if image is not in registry
+        """
+        res = self.do_request("GET", "images/%s" % image_id)
+        data = json.loads(res.read())['image']
+        return data
+
+    def add_image(self, image_metadata):
+        """
+        Tells parallax about an image's metadata
+        """
+        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)
+        # Parallax returns a JSONified dict(image=image_info)
+        data = json.loads(res.read())
+        return data['image']['id']
+
+    def update_image(self, image_id, image_metadata):
+        """
+        Updates Parallax's information about an image
+        """
+        if 'image' not in image_metadata.keys():
+            image_metadata = dict(image=image_metadata)
+        body = json.dumps(image_metadata)
+        self.do_request("PUT", "images/%s" % image_id, body)
+        return True
+
+    def delete_image(self, image_id):
+        """
+        Deletes Parallax's information about an image
+        """
+        self.do_request("DELETE", "images/%s" % image_id)
+        return True

=== modified file 'glance/common/exception.py'
--- glance/common/exception.py	2010-09-28 16:59:01 +0000
+++ glance/common/exception.py	2010-11-23 18:13:46 +0000
@@ -83,5 +83,3 @@
             raise
     _wrap.func_name = f.func_name
     return _wrap
-
-

=== modified file 'glance/parallax/controllers.py'
--- glance/parallax/controllers.py	2010-10-17 16:43:19 +0000
+++ glance/parallax/controllers.py	2010-11-23 18:13:46 +0000
@@ -83,7 +83,10 @@
 
         """
         context = None
-        updated_image = db.image_destroy(context, id)
+        try:
+            db.image_destroy(context, id)
+        except exception.NotFound:
+            return exc.HTTPNotFound()
 
     def create(self, req):
         """Registers a new image with the registry.
@@ -102,8 +105,13 @@
         image_data.setdefault('status', 'available')
 
         context = None
-        new_image = db.image_create(context, image_data)
-        return dict(image=new_image)
+        try:
+            new_image = db.image_create(context, image_data)
+            return dict(image=new_image)
+        except exception.Duplicate:
+            return exc.HTTPConflict()
+        except exception.Invalid:
+            return exc.HTTPBadRequest()
 
     def update(self, req, id):
         """Updates an existing image with the registry.
@@ -119,8 +127,11 @@
         image_data = json.loads(req.body)['image']
 
         context = None
-        updated_image = db.image_update(context, id, image_data)
-        return dict(image=updated_image)
+        try:
+            updated_image = db.image_update(context, id, image_data)
+            return dict(image=updated_image)
+        except exception.NotFound:
+            return exc.HTTPNotFound()
 
     @staticmethod
     def _make_image_dict(image):
@@ -137,13 +148,14 @@
         # TODO(sirp): should this be a dict, or a list of dicts?
         # A plain dict is more convenient, but list of dicts would provide
         # access to created_at, etc
-        metadata = dict((m['key'], m['value']) for m in image['metadata'] 
-                        if not m['deleted'])
+        properties = dict((p['key'], p['value'])
+                          for p in image['properties']
+                          if not p['deleted'])
 
         image_dict = _fetch_attrs(image, db.IMAGE_ATTRS)
 
         image_dict['files'] = files
-        image_dict['metadata'] = metadata
+        image_dict['properties'] = properties
         return image_dict
 
 

=== modified file 'glance/parallax/db/api.py'
--- glance/parallax/db/api.py	2010-10-01 20:27:48 +0000
+++ glance/parallax/db/api.py	2010-11-23 18:13:46 +0000
@@ -86,7 +86,6 @@
 ###################
 
 
-def image_metadatum_create(context, values):
-    """Create an image metadatum from the values dictionary."""
-    return IMPL.image_metadatum_create(context, values)
-
+def image_property_create(context, values):
+    """Create an image property from the values dictionary."""
+    return IMPL.image_property_create(context, values)

=== modified file 'glance/parallax/db/sqlalchemy/api.py'
--- glance/parallax/db/sqlalchemy/api.py	2010-10-14 17:16:09 +0000
+++ glance/parallax/db/sqlalchemy/api.py	2010-11-23 18:13:46 +0000
@@ -70,7 +70,7 @@
     try:
         return session.query(models.Image
                      ).options(joinedload(models.Image.files)
-                     ).options(joinedload(models.Image.metadata)
+                     ).options(joinedload(models.Image.properties)
                      ).filter_by(deleted=_deleted(context)
                      ).filter_by(id=image_id
                      ).one()
@@ -83,7 +83,7 @@
     session = get_session()
     return session.query(models.Image
                  ).options(joinedload(models.Image.files)
-                 ).options(joinedload(models.Image.metadata)
+                 ).options(joinedload(models.Image.properties)
                  ).filter_by(deleted=_deleted(context)
                  ).all()
 
@@ -92,7 +92,7 @@
     session = get_session()
     return session.query(models.Image
                  ).options(joinedload(models.Image.files)
-                 ).options(joinedload(models.Image.metadata)
+                 ).options(joinedload(models.Image.properties)
                  ).filter_by(deleted=_deleted(context)
                  ).filter_by(is_public=public
                  ).all()
@@ -124,10 +124,9 @@
 ###################
 
 
-def image_metadatum_create(_context, values):
-    image_metadatum_ref = models.ImageMetadatum()
+def image_property_create(_context, values):
+    image_properties_ref = models.ImageProperty()
     for (key, value) in values.iteritems():
-        image_metadatum_ref[key] = value
-    image_metadatum_ref.save()
-    return image_metadatum_ref
-
+        image_properties_ref[key] = value
+    image_properties_ref.save()
+    return image_properties_ref

=== modified file 'glance/parallax/db/sqlalchemy/models.py'
--- glance/parallax/db/sqlalchemy/models.py	2010-10-15 20:03:13 +0000
+++ glance/parallax/db/sqlalchemy/models.py	2010-11-23 18:13:46 +0000
@@ -149,7 +149,7 @@
             raise exception.Invalid("Invalid status '%s' for image." % status)
         return status
     
-    # TODO(sirp): should these be stored as metadata?
+    # TODO(sirp): should these be stored as properties?
     #user_id = Column(String(255))
     #project_id = Column(String(255))
     #arch = Column(String(255))
@@ -179,22 +179,22 @@
     size = Column(Integer)
 
 
-class ImageMetadatum(BASE, ModelBase):
-    """Represents an image metadata in the datastore"""
-    __tablename__ = 'image_metadata'
-    __prefix__ = 'img-meta'
+class ImageProperty(BASE, ModelBase):
+    """Represents an image properties in the datastore"""
+    __tablename__ = 'image_properties'
+    __prefix__ = 'img-prop'
     __table_args__ = (UniqueConstraint('image_id', 'key'), {})
 
     id = Column(Integer, primary_key=True)
     image_id = Column(Integer, ForeignKey('images.id'), nullable=False)
-    image = relationship(Image, backref=backref('metadata'))
+    image = relationship(Image, backref=backref('properties'))
     
     key = Column(String(255), index=True)
     value = Column(Text)
 
 
 def register_models():
-    """Register Models and create metadata"""
+    """Register Models and create properties"""
     engine = get_engine()
     BASE.metadata.create_all(engine)
 
@@ -203,4 +203,3 @@
     """Unregister Models, useful clearing out data before testing"""
     engine = get_engine()
     BASE.metadata.drop_all(engine)
-

=== modified file 'tests/stubs.py'
--- tests/stubs.py	2010-10-15 20:00:47 +0000
+++ tests/stubs.py	2010-11-23 18:13:46 +0000
@@ -23,11 +23,14 @@
 import sys
 
 import stubout
+import webob
 
 from glance.common import exception
+from glance.parallax import controllers as parallax_controllers
 import glance.teller.backends.swift
 import glance.parallax.db.sqlalchemy.api
 
+
 def stub_out_http_backend(stubs):
     """Stubs out the httplib.HTTPRequest.getresponse to return
     faked-out data instead of grabbing actual contents of a resource
@@ -153,6 +156,53 @@
               fake_parallax_registry.lookup)
 
 
+def stub_out_parallax_server(stubs):
+    """
+    Mocks httplib calls to 127.0.0.1:9292 for testing so
+    that a real Parallax server does not need to be up and
+    running
+    """
+
+    def fake_http_connection_constructor(address, port):
+        """
+        Returns either a faked connection or a real
+        one depending on if the connection is to a parallax
+        server or not...
+        """
+        return FakeParallaxConnection()
+
+
+    class FakeParallaxConnection(object):
+
+        def __init__(self):
+            pass
+
+        def connect(self):
+            return True
+
+        def close(self):
+            return True
+
+        def request(self, method, url, body=None):
+            self.req = webob.Request.blank("/" + url.lstrip("/"))
+            self.req.method = method
+            if body:
+                self.req.body = body
+
+        def getresponse(self):
+            res = self.req.get_response(parallax_controllers.API())
+
+            # httplib.Response has a read() method...fake it out
+            def fake_reader():
+                return res.body
+
+            setattr(res, 'read', fake_reader)
+            return res
+
+    stubs.Set(httplib, 'HTTPConnection',
+              fake_http_connection_constructor)
+
+
 def stub_out_parallax_db_image_api(stubs):
     """Stubs out the database set/fetch API calls for Parallax
     so the calls are routed to an in-memory dict. This helps us
@@ -176,7 +226,7 @@
                 'deleted_at': None,
                 'deleted': False,
                 'files': [],
-                'metadata': []},
+                'properties': []},
             {'id': 2,
                 'name': 'fake image #2',
                 'status': 'available',
@@ -187,7 +237,7 @@
                 'deleted_at': None,
                 'deleted': False,
                 'files': [],
-                'metadata': []}]
+                'properties': []}]
 
         VALID_STATUSES = ('available', 'disabled', 'pending')
 
@@ -196,7 +246,12 @@
             self.next_id = 3
 
         def image_create(self, _context, values):
-            values['id'] = self.next_id
+
+            values['id'] = values.get('id', self.next_id)
+
+            if values['id'] in [image['id'] for image in self.images]:
+                raise exception.Duplicate("Duplicate image id: %s" %
+                                          values['id'])
 
             if 'status' not in values.keys():
                 values['status'] = 'available'
@@ -204,6 +259,19 @@
                 if not values['status'] in self.VALID_STATUSES:
                     raise exception.Invalid("Invalid status '%s' for image" %
                                             values['status'])
+
+            values['deleted'] = False
+            values['files'] = values.get('files', [])
+            values['properties'] = values.get('properties', [])
+            values['created_at'] = datetime.datetime.utcnow() 
+            values['updated_at'] = datetime.datetime.utcnow()
+            values['deleted_at'] = None
+
+            for p in values['properties']:
+                p['deleted'] = False
+                p['created_at'] = datetime.datetime.utcnow() 
+                p['updated_at'] = datetime.datetime.utcnow()
+                p['deleted_at'] = None
             
             self.next_id += 1
             self.images.append(values)

=== added file 'tests/unit/test_clients.py'
--- tests/unit/test_clients.py	1970-01-01 00:00:00 +0000
+++ tests/unit/test_clients.py	2010-11-23 18:13:46 +0000
@@ -0,0 +1,233 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2010 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 json
+import stubout
+import unittest
+
+import webob
+
+from glance import client
+from glance.common import exception
+from tests import stubs
+
+
+class TestBadClients(unittest.TestCase):
+    
+    """Test exceptions raised for bad clients"""
+
+    def test_bad_protocol(self):
+        """Test unsupported protocol raised"""
+        c = client.ParallaxClient(address="hdsa://127.012..1./")
+        self.assertRaises(client.UnsupportedProtocolError,
+                          c.get_image,
+                          1)
+
+    def test_bad_address(self):
+        """Test unsupported protocol raised"""
+        c = client.ParallaxClient(address="http://127.999.1.1/")
+        self.assertRaises(client.ClientConnectionError,
+                          c.get_image,
+                          1)
+
+
+class TestParallaxClient(unittest.TestCase):
+
+    """Test proper actions made for both valid and invalid requests"""
+
+    def setUp(self):
+        """Establish a clean test environment"""
+        self.stubs = stubout.StubOutForTesting()
+        stubs.stub_out_parallax_db_image_api(self.stubs)
+        stubs.stub_out_parallax_server(self.stubs)
+        self.client = client.ParallaxClient()
+
+    def tearDown(self):
+        """Clear the test environment"""
+        self.stubs.UnsetAll()
+
+    def test_get_image_index(self):
+        """Test correct set of public image returned"""
+        fixture = {'id': 2,
+                   'name': 'fake image #2'}
+        images = self.client.get_images()
+        self.assertEquals(len(images), 1)
+
+        for k,v in fixture.iteritems():
+            self.assertEquals(v, images[0][k])
+
+    def test_get_image_details(self):
+        """Tests that the detailed info about public images returned"""
+        fixture = {'id': 2,
+                   'name': 'fake image #2',
+                   'is_public': True,
+                   'image_type': 'kernel',
+                   'status': 'available'
+                  }
+
+        images = self.client.get_images_detailed()
+        self.assertEquals(len(images), 1)
+
+        for k,v in fixture.iteritems():
+            self.assertEquals(v, images[0][k])
+
+    def test_get_image_metadata(self):
+        """Tests that the detailed info about an image returned"""
+        fixture = {'id': 2,
+                   'name': 'fake image #2',
+                   'is_public': True,
+                   'image_type': 'kernel',
+                   'status': 'available'
+                  }
+
+        data = self.client.get_image(2)
+
+        for k,v in fixture.iteritems():
+            self.assertEquals(v, data[k])
+
+    def test_get_image_non_existing(self):
+        """Tests that NotFound is raised when getting a non-existing image"""
+
+        self.assertRaises(exception.NotFound,
+                          self.client.get_image,
+                          42)
+
+    def test_add_image_metadata_basic(self):
+        """Tests that we can add image metadata and returns the new id"""
+        fixture = {'name': 'fake public image',
+                   'is_public': True,
+                   'image_type': 'kernel'
+                  }
+        
+        new_id = self.client.add_image(fixture)
+
+        # Test ID auto-assigned properly
+        self.assertEquals(3, new_id)
+
+        # Test all other attributes set
+        data = self.client.get_image(3)
+
+        for k,v in fixture.iteritems():
+            self.assertEquals(v, data[k])
+
+        # Test status was updated properly
+        self.assertTrue('status' in data.keys())
+        self.assertEquals('available', data['status'])
+
+    def test_add_image_metadata_with_properties(self):
+        """Tests that we can add image metadata with properties"""
+        fixture = {'name': 'fake public image',
+                   'is_public': True,
+                   'image_type': 'kernel',
+                   'properties': [{'key':'disco',
+                                   'value': 'baby'}]
+                  }
+        expected = {'name': 'fake public image',
+                    'is_public': True,
+                    'image_type': 'kernel',
+                    'properties': {'disco': 'baby'}
+                  }
+        
+        new_id = self.client.add_image(fixture)
+
+        # Test ID auto-assigned properly
+        self.assertEquals(3, new_id)
+
+        # Test all other attributes set
+        data = self.client.get_image(3)
+
+        for k,v in expected.iteritems():
+            self.assertEquals(v, data[k])
+
+        # Test status was updated properly
+        self.assertTrue('status' in data.keys())
+        self.assertEquals('available', data['status'])
+
+    def test_add_image_already_exists(self):
+        """Tests proper exception is raised if image with ID already exists"""
+        fixture = {'id': 2,
+                   'name': 'fake public image',
+                   'is_public': True,
+                   'image_type': 'kernel',
+                   'status': 'bad status'
+                  }
+
+        self.assertRaises(exception.Duplicate,
+                          self.client.add_image,
+                          fixture)
+
+    def test_add_image_with_bad_status(self):
+        """Tests proper exception is raised if a bad status is set"""
+        fixture = {'id': 3,
+                   'name': 'fake public image',
+                   'is_public': True,
+                   'image_type': 'kernel',
+                   'status': 'bad status'
+                  }
+
+        self.assertRaises(client.BadInputError,
+                          self.client.add_image,
+                          fixture)
+
+    def test_update_image(self):
+        """Tests that the /images PUT parallax API updates the image"""
+        fixture = {'name': 'fake public image #2',
+                   'image_type': 'ramdisk'
+                  }
+
+        self.assertTrue(self.client.update_image(2, fixture))
+
+        # Test all other attributes set
+        data = self.client.get_image(2)
+
+        for k,v in fixture.iteritems():
+            self.assertEquals(v, data[k])
+
+    def test_update_image_not_existing(self):
+        """Tests non existing image update doesn't work"""
+        fixture = {'id': 3,
+                   'name': 'fake public image',
+                   'is_public': True,
+                   'image_type': 'kernel',
+                   'status': 'bad status'
+                  }
+
+        self.assertRaises(exception.NotFound,
+                          self.client.update_image,
+                          3,
+                          fixture)
+
+    def test_delete_image(self):
+        """Tests that image metadata is deleted properly"""
+
+        # Grab the original number of images
+        orig_num_images = len(self.client.get_images())
+
+        # Delete image #2
+        self.assertTrue(self.client.delete_image(2))
+
+        # Verify one less image
+        new_num_images = len(self.client.get_images())
+
+        self.assertEquals(new_num_images, orig_num_images - 1)
+
+    def test_delete_image_not_existing(self):
+        """Tests cannot delete non-existing image"""
+
+        self.assertRaises(exception.NotFound,
+                          self.client.delete_image,
+                          3)

=== modified file 'tests/unit/test_parallax_api.py'
--- tests/unit/test_parallax_api.py	2010-10-17 16:31:04 +0000
+++ tests/unit/test_parallax_api.py	2010-11-23 18:13:46 +0000
@@ -22,7 +22,6 @@
 
 from glance.common import exception
 from glance.parallax import controllers
-from glance.parallax import db
 from tests import stubs
 
 
@@ -30,7 +29,6 @@
     def setUp(self):
         """Establish a clean test environment"""
         self.stubs = stubout.StubOutForTesting()
-        self.image_controller = controllers.ImageController()
         stubs.stub_out_parallax_db_image_api(self.stubs)
 
     def tearDown(self):
@@ -139,7 +137,8 @@
         # TODO(jaypipes): Port Nova's Fault infrastructure
         # over to Glance to support exception catching into
         # standard HTTP errors.
-        self.assertRaises(exception.Invalid, req.get_response, controllers.API())
+        res = req.get_response(controllers.API())
+        self.assertEquals(res.status_int, webob.exc.HTTPBadRequest.code)
 
     def test_update_image(self):
         """Tests that the /images PUT parallax API updates the image"""
@@ -179,7 +178,9 @@
         # TODO(jaypipes): Port Nova's Fault infrastructure
         # over to Glance to support exception catching into
         # standard HTTP errors.
-        self.assertRaises(exception.NotFound, req.get_response, controllers.API())
+        res = req.get_response(controllers.API())
+        self.assertEquals(res.status_int,
+                          webob.exc.HTTPNotFound.code)
 
     def test_delete_image(self):
         """Tests that the /images DELETE parallax API deletes the image"""
@@ -221,4 +222,6 @@
         # TODO(jaypipes): Port Nova's Fault infrastructure
         # over to Glance to support exception catching into
         # standard HTTP errors.
-        self.assertRaises(exception.NotFound, req.get_response, controllers.API())
+        res = req.get_response(controllers.API())
+        self.assertEquals(res.status_int,
+                          webob.exc.HTTPNotFound.code)

=== modified file 'tests/unit/test_parallax_models.py'
--- tests/unit/test_parallax_models.py	2010-10-15 05:35:15 +0000
+++ tests/unit/test_parallax_models.py	2010-11-23 18:13:46 +0000
@@ -40,20 +40,20 @@
         same key
 
         """
-        self._make_metadatum(self.image, key="spam", value="eggs")
+        self._make_property(self.image, key="spam", value="eggs")
         
         second_image = self._make_image(id=3, name='fake image #3')
-        self._make_metadatum(second_image, key="spam", value="eggs")
+        self._make_property(second_image, key="spam", value="eggs")
 
     def test_metadata_key_constraint_bad(self):
         """The same image cannot have two distinct pieces of metadata with the
         same key.
 
         """
-        self._make_metadatum(self.image, key="spam", value="eggs")
+        self._make_property(self.image, key="spam", value="eggs")
 
         self.assertRaises(sa_exc.IntegrityError,
-            self._make_metadatum, self.image, key="spam", value="eggs")
+            self._make_property, self.image, key="spam", value="eggs")
 
     def _make_image(self, id, name):
         """Convenience method to create an image with a given name and id"""
@@ -67,11 +67,9 @@
         image = db.api.image_create(context, fixture)
         return image
 
-    def _make_metadatum(self, image, key, value):
+    def _make_property(self, image, key, value):
         """Convenience method to create metadata attached to an image"""
         metadata = {'image_id': image['id'], 'key': key, 'value': value}
         context = None
-        metadatum = db.api.image_metadatum_create(context, metadata)
-        return metadatum
-
-
+        property = db.api.image_property_create(context, metadata)
+        return property