Merge lp:~rackspace-titan/nova/fix_list_v11_snapshot_images into lp:~hudson-openstack/nova/trunk

[Dan Prince]

Fixes an issue where 'invalid literal for int' would occur when listing images after making a v1.1 server snapshot (with a UUID).

v1.1 image id's are now treated as strings (not integer ID's). The v1.0 API still tries to treat image id's as integers but doesn't fail miserably if they are uuid's either.

This should pave the way for image ID's as uuids and more closely matches the v1.1 spec with regards to images and the server refs they contain.

[Brian Lamar (blamar)]

36 + return urlparse.urlsplit("%s" % href).path.split('/')[-1]

Good stuff, I actually like this strategy much better than the one I implemented/advocated for which first attempts to cast to an integer and then tries to parse a URL.

Thanks!

review: Approve

[Brian Waldon (bcwaldon)]

60: Does this mean that if we switch to uuids for images we will not be able to communicate that through the v1.0 api?

review: Needs Information

[Dan Prince (dan-prince)]

The v1.0 SPEC doesn't "officially" allow for UUIDs. We have taken the middle ground on UUID's for zones support etc. however in nova. So v1.0 as far as this code goes v1.0 tries to give you an integer (because that is what the SPEC says). If you are using v1.0 and there is a UUID present it will give you a JSON string UUID.

[Brian Waldon (bcwaldon)]

Okay, I think I'm okay with this behavior. You do have one merge conflict.

[Dan Prince (dan-prince)]

Okay after 2 rounds of conflicts related to the image tests this should be ready again. Incidentally this branch also fixes an issue that crept into nova trunk recently where v1.0 image ID's are getting incorrectly getting returned as strings again. I've updated the v1.0 tests to use integer IDs again.

[Brian Waldon (bcwaldon)]

Looks good, Dan.

review: Approve

=== modified file 'nova/api/openstack/common.py'
--- nova/api/openstack/common.py	2011-09-02 05:39:31 +0000
+++ nova/api/openstack/common.py	2011-09-14 15:37:50 +0000
@@ -187,30 +187,16 @@
 
 
 def get_id_from_href(href):
-    """Return the id portion of a url as an int.
+    """Return the id or uuid portion of a url.
 
     Given: 'http://www.foo.com/bar/123?q=4'
-    Returns: 123
+    Returns: '123'
 
-    In order to support local hrefs, the href argument can be just an id:
-    Given: '123'
-    Returns: 123
+    Given: 'http://www.foo.com/bar/abc123?q=4'
+    Returns: 'abc123'
 
     """
-    LOG.debug(_("Attempting to treat %(href)s as an integer ID.") % locals())
-
-    try:
-        return int(href)
-    except ValueError:
-        pass
-
-    LOG.debug(_("Attempting to treat %(href)s as a URL.") % locals())
-
-    try:
-        return int(urlparse.urlsplit(href).path.split('/')[-1])
-    except ValueError as error:
-        LOG.debug(_("Failed to parse ID from %(href)s: %(error)s") % locals())
-        raise
+    return urlparse.urlsplit("%s" % href).path.split('/')[-1]
 
 
 def remove_version_from_href(href):

=== modified file 'nova/api/openstack/views/images.py'
--- nova/api/openstack/views/images.py	2011-09-09 21:25:45 +0000
+++ nova/api/openstack/views/images.py	2011-09-14 15:37:50 +0000
@@ -71,6 +71,7 @@
         }
 
         self._build_server(image, image_obj)
+        self._build_image_id(image, image_obj)
 
         if detail:
             image.update({
@@ -96,6 +97,12 @@
         except (KeyError, ValueError):
             pass
 
+    def _build_image_id(self, image, image_obj):
+        try:
+            image['id'] = int(image_obj['id'])
+        except ValueError:
+            pass
+
 
 class ViewBuilderV11(ViewBuilder):
     """OpenStack API v1.1 Image Builder"""
@@ -119,6 +126,9 @@
         except KeyError:
             return
 
+    def _build_image_id(self, image, image_obj):
+        image['id'] = "%s" % image_obj['id']
+
     def generate_href(self, image_id):
         """Return an href string pointing to this object."""
         return os.path.join(self.base_url, self.project_id,

=== modified file 'nova/tests/api/openstack/test_common.py'
--- nova/tests/api/openstack/test_common.py	2011-08-25 20:35:04 +0000
+++ nova/tests/api/openstack/test_common.py	2011-09-14 15:37:50 +0000
@@ -243,21 +243,41 @@
                           common.remove_version_from_href,
                           fixture)
 
-    def test_get_id_from_href(self):
+    def test_get_id_from_href_with_int_url(self):
         fixture = 'http://www.testsite.com/dir/45'
         actual = common.get_id_from_href(fixture)
-        expected = 45
-        self.assertEqual(actual, expected)
-
-    def test_get_id_from_href_bad_request(self):
-        fixture = 'http://45'
-        self.assertRaises(ValueError,
-                          common.get_id_from_href,
-                          fixture)
-
-    def test_get_id_from_href_int(self):
-        fixture = 1
-        self.assertEqual(fixture, common.get_id_from_href(fixture))
+        expected = '45'
+        self.assertEqual(actual, expected)
+
+    def test_get_id_from_href_with_int(self):
+        fixture = '45'
+        actual = common.get_id_from_href(fixture)
+        expected = '45'
+        self.assertEqual(actual, expected)
+
+    def test_get_id_from_href_with_int_url_query(self):
+        fixture = 'http://www.testsite.com/dir/45?asdf=jkl'
+        actual = common.get_id_from_href(fixture)
+        expected = '45'
+        self.assertEqual(actual, expected)
+
+    def test_get_id_from_href_with_uuid_url(self):
+        fixture = 'http://www.testsite.com/dir/abc123'
+        actual = common.get_id_from_href(fixture)
+        expected = "abc123"
+        self.assertEqual(actual, expected)
+
+    def test_get_id_from_href_with_uuid_url_query(self):
+        fixture = 'http://www.testsite.com/dir/abc123?asdf=jkl'
+        actual = common.get_id_from_href(fixture)
+        expected = "abc123"
+        self.assertEqual(actual, expected)
+
+    def test_get_id_from_href_with_uuid(self):
+        fixture = 'abc123'
+        actual = common.get_id_from_href(fixture)
+        expected = 'abc123'
+        self.assertEqual(actual, expected)
 
     def test_get_version_from_href(self):
         fixture = 'http://www.testsite.com/v1.1/images'

=== modified file 'nova/tests/api/openstack/test_images.py'
--- nova/tests/api/openstack/test_images.py	2011-09-13 18:00:20 +0000
+++ nova/tests/api/openstack/test_images.py	2011-09-14 15:37:50 +0000
@@ -77,14 +77,14 @@
         response_dict = json.loads(response.body)
         response_list = response_dict["images"]
 
-        expected = [{'id': '123', 'name': 'public image'},
-                    {'id': '124', 'name': 'queued snapshot'},
-                    {'id': '125', 'name': 'saving snapshot'},
-                    {'id': '126', 'name': 'active snapshot'},
-                    {'id': '127', 'name': 'killed snapshot'},
-                    {'id': '128', 'name': 'deleted snapshot'},
-                    {'id': '129', 'name': 'pending_delete snapshot'},
-                    {'id': '130', 'name': None}]
+        expected = [{'id': 123, 'name': 'public image'},
+                    {'id': 124, 'name': 'queued snapshot'},
+                    {'id': 125, 'name': 'saving snapshot'},
+                    {'id': 126, 'name': 'active snapshot'},
+                    {'id': 127, 'name': 'killed snapshot'},
+                    {'id': 128, 'name': 'deleted snapshot'},
+                    {'id': 129, 'name': 'pending_delete snapshot'},
+                    {'id': 130, 'name': None}]
 
         self.assertDictListMatch(response_list, expected)
 
@@ -99,7 +99,7 @@
 
         expected_image = {
             "image": {
-                "id": "123",
+                "id": 123,
                 "name": "public image",
                 "updated": NOW_API_FORMAT,
                 "created": NOW_API_FORMAT,
@@ -131,7 +131,7 @@
                 "status": "SAVING",
                 "progress": 0,
                 'server': {
-                    'id': 42,
+                    'id': '42',
                     "links": [{
                         "rel": "self",
                         "href": server_href,
@@ -406,7 +406,7 @@
         response_list = response_dict["images"]
 
         expected = [{
-            'id': '123',
+            'id': 123,
             'name': 'public image',
             'updated': NOW_API_FORMAT,
             'created': NOW_API_FORMAT,
@@ -414,7 +414,7 @@
             'progress': 100,
         },
         {
-            'id': '124',
+            'id': 124,
             'name': 'queued snapshot',
             'updated': NOW_API_FORMAT,
             'created': NOW_API_FORMAT,
@@ -422,7 +422,7 @@
             'progress': 0,
         },
         {
-            'id': '125',
+            'id': 125,
             'name': 'saving snapshot',
             'updated': NOW_API_FORMAT,
             'created': NOW_API_FORMAT,
@@ -430,7 +430,7 @@
             'progress': 0,
         },
         {
-            'id': '126',
+            'id': 126,
             'name': 'active snapshot',
             'updated': NOW_API_FORMAT,
             'created': NOW_API_FORMAT,
@@ -438,7 +438,7 @@
             'progress': 100,
         },
         {
-            'id': '127',
+            'id': 127,
             'name': 'killed snapshot',
             'updated': NOW_API_FORMAT,
             'created': NOW_API_FORMAT,
@@ -446,7 +446,7 @@
             'progress': 0,
         },
         {
-            'id': '128',
+            'id': 128,
             'name': 'deleted snapshot',
             'updated': NOW_API_FORMAT,
             'created': NOW_API_FORMAT,
@@ -454,7 +454,7 @@
             'progress': 0,
         },
         {
-            'id': '129',
+            'id': 129,
             'name': 'pending_delete snapshot',
             'updated': NOW_API_FORMAT,
             'created': NOW_API_FORMAT,
@@ -462,7 +462,7 @@
             'progress': 0,
         },
         {
-            'id': '130',
+            'id': 130,
             'name': None,
             'updated': NOW_API_FORMAT,
             'created': NOW_API_FORMAT,
@@ -511,7 +511,7 @@
             'status': 'SAVING',
             'progress': 0,
             'server': {
-                'id': 42,
+                'id': '42',
                 "links": [{
                     "rel": "self",
                     "href": server_href,
@@ -542,7 +542,7 @@
             'status': 'SAVING',
             'progress': 0,
             'server': {
-                'id': 42,
+                'id': '42',
                 "links": [{
                     "rel": "self",
                     "href": server_href,
@@ -573,7 +573,7 @@
             'status': 'ACTIVE',
             'progress': 100,
             'server': {
-                'id': 42,
+                'id': '42',
                 "links": [{
                     "rel": "self",
                     "href": server_href,
@@ -604,7 +604,7 @@
             'status': 'ERROR',
             'progress': 0,
             'server': {
-                'id': 42,
+                'id': '42',
                 "links": [{
                     "rel": "self",
                     "href": server_href,
@@ -635,7 +635,7 @@
             'status': 'DELETED',
             'progress': 0,
             'server': {
-                'id': 42,
+                'id': '42',
                 "links": [{
                     "rel": "self",
                     "href": server_href,
@@ -666,7 +666,7 @@
             'status': 'DELETED',
             'progress': 0,
             'server': {
-                'id': 42,
+                'id': '42',
                 "links": [{
                     "rel": "self",
                     "href": server_href,
@@ -914,7 +914,7 @@
         app = fakes.wsgi_app(fake_auth_context=self._get_fake_context())
         res = req.get_response(app)
         image_meta = json.loads(res.body)['image']
-        expected = {'id': '123', 'name': 'public image',
+        expected = {'id': 123, 'name': 'public image',
                     'updated': NOW_API_FORMAT,
                     'created': NOW_API_FORMAT, 'status': 'ACTIVE',
                     'progress': 100}