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

[William Wolf]

Add next links for server lists in OSAPI 1.1. This adds servers_links to the json responses, and an extra atom:link element to the servers node in the xml response.

[Brian Waldon (bcwaldon)]

When making a request with a changes-since query param such as this:

curl -i -H "X-Auth-Token: admin:admin" 'http://localhost:8774/v1.1/admin/servers?limit=2&changes-since=2011-09-01T00:00:00Z'

The values of changes-since and limit are respected, but the servers_links value returned is unusable:

http://localhost:8774/v1.1?marker=6&limit=2&changes-since=2011-09-01T00%3A00%3A00Z

I would expect the changes-since value not to be url-encoded at all.

review: Needs Fixing

[lp:~rackspace-titan/nova/servers-next]

1582. By William Wolf on 2011-09-19

merge with trunk

1583. By William Wolf on 2011-09-19

make our own function instead of using urllib.urlencode since we apparently don't suppor urlencoded strings yet

1584. By William Wolf on 2011-09-19

merge with trunk

[William Wolf (throughnothing)]

I've replaced urllib with a common function that doesn't urlencode the strings from the dict. I think ultimately we should support urlencoded URL's though, and give those in our own links just to ensure everything is correctly set/interpreted.

[Brian Waldon (bcwaldon)]

I wouldn't call it elegant, but it definitely works. However, now I'm getting the following url back. Notice the missing 'admin/servers' after 'v1.1':

http://localhost:8774/v1.1?marker=8&limit=1&changes-since=2011-09-01T00:00:00Z

[William Wolf (throughnothing)]

Wow, good catch waldon. Seems I wasn't even checking that in the tests either. Updated the tests and the code, that should be fixed now.

[lp:~rackspace-titan/nova/servers-next]

1585. By William Wolf on 2011-09-19

remove urllib import

1586. By William Wolf on 2011-09-20

oops, add project_id and 'servers' to next links

1587. By William Wolf on 2011-09-20

merged with trunk

1588. By William Wolf on 2011-09-20

merge from trunk

1589. By William Wolf on 2011-09-20

pep8 fixes

[Brian Waldon (bcwaldon)]

Great stuff.

review: Approve

[Brian Lamar (blamar)]

review: Approve

=== modified file 'nova/api/openstack/common.py'
--- nova/api/openstack/common.py	2011-09-14 15:23:11 +0000
+++ nova/api/openstack/common.py	2011-09-20 18:47:17 +0000
@@ -259,6 +259,16 @@
                                                 headers={'Retry-After': 0})
 
 
+def dict_to_query_str(params):
+    # TODO: we should just use urllib.urlencode instead of this
+    # But currently we don't work with urlencoded url's
+    param_str = ""
+    for key, val in params.iteritems():
+        param_str = param_str + '='.join([str(key), str(val)]) + '&'
+
+    return param_str.rstrip('&')
+
+
 class MetadataXMLDeserializer(wsgi.XMLDeserializer):
 
     def extract_metadata(self, metadata_node):

=== modified file 'nova/api/openstack/schemas/v1.1/servers_index.rng'
--- nova/api/openstack/schemas/v1.1/servers_index.rng	2011-08-19 19:55:56 +0000
+++ nova/api/openstack/schemas/v1.1/servers_index.rng	2011-09-20 18:47:17 +0000
@@ -9,4 +9,7 @@
       </zeroOrMore>
     </element>
   </zeroOrMore>
+  <zeroOrMore>
+    <externalRef href="../atom-link.rng"/>
+  </zeroOrMore>
 </element>

=== modified file 'nova/api/openstack/servers.py'
--- nova/api/openstack/servers.py	2011-09-18 21:01:44 +0000
+++ nova/api/openstack/servers.py	2011-09-20 18:47:17 +0000
@@ -75,6 +75,9 @@
     def _build_view(self, req, instance, is_detail=False):
         raise NotImplementedError()
 
+    def _build_list(self, req, instances, is_detail=False):
+        raise NotImplementedError()
+
     def _limit_items(self, items, req):
         raise NotImplementedError()
 
@@ -130,10 +133,7 @@
                                                  search_opts=search_opts)
 
         limited_list = self._limit_items(instance_list, req)
-        servers = [self._build_view(req, inst, is_detail)['server']
-                    for inst in limited_list]
-
-        return dict(servers=servers)
+        return self._build_list(req, limited_list, is_detail=is_detail)
 
     @scheduler_api.redirect_handler
     def show(self, req, id):
@@ -595,6 +595,11 @@
         builder = nova.api.openstack.views.servers.ViewBuilderV10(addresses)
         return builder.build(instance, is_detail=is_detail)
 
+    def _build_list(self, req, instances, is_detail=False):
+        addresses = nova.api.openstack.views.addresses.ViewBuilderV10()
+        builder = nova.api.openstack.views.servers.ViewBuilderV10(addresses)
+        return builder.build_list(instances, is_detail=is_detail)
+
     def _limit_items(self, items, req):
         return common.limited(items, req)
 
@@ -692,6 +697,25 @@
 
         return builder.build(instance, is_detail=is_detail)
 
+    def _build_list(self, req, instances, is_detail=False):
+        params = req.GET.copy()
+        pagination_params = common.get_pagination_params(req)
+        # Update params with int() values from pagination params
+        for key, val in pagination_params.iteritems():
+            params[key] = val
+
+        project_id = getattr(req.environ['nova.context'], 'project_id', '')
+        base_url = req.application_url
+        flavor_builder = nova.api.openstack.views.flavors.ViewBuilderV11(
+            base_url, project_id)
+        image_builder = nova.api.openstack.views.images.ViewBuilderV11(
+            base_url, project_id)
+        addresses_builder = nova.api.openstack.views.addresses.ViewBuilderV11()
+        builder = nova.api.openstack.views.servers.ViewBuilderV11(
+            addresses_builder, flavor_builder, image_builder,
+            base_url, project_id)
+        return builder.build_list(instances, is_detail=is_detail, **params)
+
     def _action_change_password(self, input_dict, req, id):
         context = req.environ['nova.context']
         if (not 'changePassword' in input_dict
@@ -939,18 +963,22 @@
                                                   'security_group')
                     group_elem.set('name', group['name'])
 
-        for link in server_dict.get('links', []):
-            elem = etree.SubElement(server_elem,
+        self._populate_links(server_elem, server_dict.get('links', []))
+
+    def _populate_links(self, parent, links):
+        for link in links:
+            elem = etree.SubElement(parent,
                                     '{%s}link' % xmlutil.XMLNS_ATOM)
             elem.set('rel', link['rel'])
             elem.set('href', link['href'])
-        return server_elem
 
     def index(self, servers_dict):
         servers = etree.Element('servers', nsmap=self.NSMAP)
         for server_dict in servers_dict['servers']:
             server = etree.SubElement(servers, 'server')
             self._populate_server(server, server_dict, False)
+
+        self._populate_links(servers, servers_dict.get('servers_links', []))
         return self._to_xml(servers)
 
     def detail(self, servers_dict):

=== modified file 'nova/api/openstack/views/servers.py'
--- nova/api/openstack/views/servers.py	2011-09-09 13:48:38 +0000
+++ nova/api/openstack/views/servers.py	2011-09-20 18:47:17 +0000
@@ -40,7 +40,7 @@
     def __init__(self, addresses_builder):
         self.addresses_builder = addresses_builder
 
-    def build(self, inst, is_detail):
+    def build(self, inst, is_detail=False):
         """Return a dict that represenst a server."""
         if inst.get('_is_precooked', False):
             server = dict(server=inst)
@@ -54,6 +54,16 @@
 
         return server
 
+    def build_list(self, server_objs, is_detail=False, **kwargs):
+        limit = kwargs.get('limit', None)
+        servers = []
+        servers_links = []
+
+        for server_obj in server_objs:
+            servers.append(self.build(server_obj, is_detail)['server'])
+
+        return dict(servers=servers)
+
     def _build_simple(self, inst):
         """Return a simple model of a server."""
         return dict(server=dict(id=inst['id'], name=inst['display_name']))
@@ -205,6 +215,31 @@
 
         response["links"] = links
 
+    def build_list(self, server_objs, is_detail=False, **kwargs):
+        limit = kwargs.get('limit', None)
+        servers = []
+        servers_links = []
+
+        for server_obj in server_objs:
+            servers.append(self.build(server_obj, is_detail)['server'])
+
+        if (len(servers) and limit) and (limit == len(servers)):
+            next_link = self.generate_next_link(servers[-1]['id'],
+                                                kwargs, is_detail)
+            servers_links = [dict(rel='next', href=next_link)]
+
+        reval = dict(servers=servers)
+        if len(servers_links) > 0:
+            reval['servers_links'] = servers_links
+        return reval
+
+    def generate_next_link(self, server_id, params, is_detail=False):
+        """ Return an href string with proper limit and marker params"""
+        params['marker'] = server_id
+        return "%s?%s" % (
+            os.path.join(self.base_url, self.project_id, "servers"),
+            common.dict_to_query_str(params))
+
     def generate_href(self, server_id):
         """Create an url that refers to a specific server id."""
         return os.path.join(self.base_url, self.project_id,

=== modified file 'nova/tests/api/openstack/test_servers.py'
--- nova/tests/api/openstack/test_servers.py	2011-09-19 20:16:49 +0000
+++ nova/tests/api/openstack/test_servers.py	2011-09-20 18:47:17 +0000
@@ -20,6 +20,7 @@
 import datetime
 import json
 import unittest
+import urlparse
 from lxml import etree
 from xml.dom import minidom
 
@@ -1152,6 +1153,67 @@
         self.assertEqual(res.status_int, 400)
         self.assertTrue('limit' in res.body)
 
+    def test_get_servers_with_limit_v1_1(self):
+        req = webob.Request.blank('/v1.1/fake/servers?limit=3')
+        res = req.get_response(fakes.wsgi_app())
+        servers = json.loads(res.body)['servers']
+        servers_links = json.loads(res.body)['servers_links']
+        self.assertEqual([s['id'] for s in servers], [0, 1, 2])
+        self.assertEqual(servers_links[0]['rel'], 'next')
+
+        href_parts = urlparse.urlparse(servers_links[0]['href'])
+        self.assertEqual('/v1.1/fake/servers', href_parts.path)
+        params = urlparse.parse_qs(href_parts.query)
+        self.assertDictMatch({'limit': ['3'], 'marker': ['2']}, params)
+
+        req = webob.Request.blank('/v1.1/fake/servers?limit=aaa')
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 400)
+        self.assertTrue('limit' in res.body)
+
+    def test_get_server_details_with_limit_v1_1(self):
+        req = webob.Request.blank('/v1.1/fake/servers/detail?limit=3')
+        res = req.get_response(fakes.wsgi_app())
+        servers = json.loads(res.body)['servers']
+        servers_links = json.loads(res.body)['servers_links']
+        self.assertEqual([s['id'] for s in servers], [0, 1, 2])
+        self.assertEqual(servers_links[0]['rel'], 'next')
+
+        href_parts = urlparse.urlparse(servers_links[0]['href'])
+        self.assertEqual('/v1.1/fake/servers', href_parts.path)
+        params = urlparse.parse_qs(href_parts.query)
+        self.assertDictMatch({'limit': ['3'], 'marker': ['2']}, params)
+
+        req = webob.Request.blank('/v1.1/fake/servers/detail?limit=aaa')
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 400)
+        self.assertTrue('limit' in res.body)
+
+    def test_get_server_details_with_limit_and_other_params_v1_1(self):
+        req = webob.Request.blank('/v1.1/fake/servers/detail?limit=3&blah=2:t')
+        res = req.get_response(fakes.wsgi_app())
+        servers = json.loads(res.body)['servers']
+        servers_links = json.loads(res.body)['servers_links']
+        self.assertEqual([s['id'] for s in servers], [0, 1, 2])
+        self.assertEqual(servers_links[0]['rel'], 'next')
+
+        href_parts = urlparse.urlparse(servers_links[0]['href'])
+        self.assertEqual('/v1.1/fake/servers', href_parts.path)
+        params = urlparse.parse_qs(href_parts.query)
+        self.assertDictMatch({'limit': ['3'], 'blah': ['2:t'],
+                              'marker': ['2']}, params)
+
+        req = webob.Request.blank('/v1.1/fake/servers/detail?limit=aaa')
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 400)
+        self.assertTrue('limit' in res.body)
+
+    def test_get_servers_with_too_big_limit_v1_1(self):
+        req = webob.Request.blank('/v1.1/fake/servers?limit=30')
+        res = req.get_response(fakes.wsgi_app())
+        res_dict = json.loads(res.body)
+        self.assertTrue('servers_links' not in res_dict)
+
     def test_get_servers_with_offset(self):
         req = webob.Request.blank('/v1.0/servers?offset=2')
         res = req.get_response(fakes.wsgi_app())
@@ -1952,7 +2014,6 @@
         req.headers["content-type"] = "application/json"
 
         res = req.get_response(fakes.wsgi_app())
-        print res
         self.assertEqual(res.status_int, 202)
         server = json.loads(res.body)['server']
         self.assertEqual(1, server['id'])
@@ -2480,6 +2541,7 @@
         }
         req = webob.Request.blank('/v1.1/fake/servers/detail')
         res = req.get_response(fakes.wsgi_app())
+        print res.body
         res_dict = json.loads(res.body)
 
         for i, s in enumerate(res_dict['servers']):
@@ -4181,6 +4243,7 @@
 
     TIMESTAMP = "2010-10-11T10:30:22Z"
     SERVER_HREF = 'http://localhost/v1.1/servers/123'
+    SERVER_NEXT = 'http://localhost/v1.1/servers?limit=%s&marker=%s'
     SERVER_BOOKMARK = 'http://localhost/servers/123'
     IMAGE_BOOKMARK = 'http://localhost/images/5'
     FLAVOR_BOOKMARK = 'http://localhost/flavors/1'
@@ -4599,6 +4662,74 @@
                 for key, value in link.items():
                     self.assertEqual(link_nodes[i].get(key), value)
 
+    def test_index_with_servers_links(self):
+        serializer = servers.ServerXMLSerializer()
+
+        expected_server_href = 'http://localhost/v1.1/servers/1'
+        expected_server_next = self.SERVER_NEXT % (2, 2)
+        expected_server_bookmark = 'http://localhost/servers/1'
+        expected_server_href_2 = 'http://localhost/v1.1/servers/2'
+        expected_server_bookmark_2 = 'http://localhost/servers/2'
+        fixture = {"servers": [
+            {
+                "id": 1,
+                "name": "test_server",
+                'links': [
+                    {
+                        'href': expected_server_href,
+                        'rel': 'self',
+                    },
+                    {
+                        'href': expected_server_bookmark,
+                        'rel': 'bookmark',
+                    },
+                ],
+            },
+            {
+                "id": 2,
+                "name": "test_server_2",
+                'links': [
+                    {
+                        'href': expected_server_href_2,
+                        'rel': 'self',
+                    },
+                    {
+                        'href': expected_server_bookmark_2,
+                        'rel': 'bookmark',
+                    },
+                ],
+            },
+        ],
+        "servers_links": [
+            {
+                'rel': 'next',
+                'href': expected_server_next,
+            },
+        ]}
+
+        output = serializer.serialize(fixture, 'index')
+        print output
+        root = etree.XML(output)
+        xmlutil.validate_schema(root, 'servers_index')
+        server_elems = root.findall('{0}server'.format(NS))
+        self.assertEqual(len(server_elems), 2)
+        for i, server_elem in enumerate(server_elems):
+            server_dict = fixture['servers'][i]
+            for key in ['name', 'id']:
+                self.assertEqual(server_elem.get(key), str(server_dict[key]))
+
+            link_nodes = server_elem.findall('{0}link'.format(ATOMNS))
+            self.assertEqual(len(link_nodes), 2)
+            for i, link in enumerate(server_dict['links']):
+                for key, value in link.items():
+                    self.assertEqual(link_nodes[i].get(key), value)
+
+        # Check servers_links
+        servers_links = root.findall('{0}link'.format(ATOMNS))
+        for i, link in enumerate(fixture['servers_links']):
+            for key, value in link.items():
+                self.assertEqual(servers_links[i].get(key), value)
+
     def test_detail(self):
         serializer = servers.ServerXMLSerializer()