From 82d71cd8d0aa663fc348f90fb5766f804fb2122f Mon Sep 17 00:00:00 2001
From: Theodoros Tsioutsias <theodoros.tsioutsias@cern.ch>
Date: Thu, 14 Nov 2019 10:14:17 +0000
Subject: [PATCH] nodegroup list with --limit gives wrong next URL

This fixes the next url in the list nodegroups API response.

Change-Id: Ice682bcf77c3ce1c8b1fbd12296eada63219e9e5
story: 2006851
task: 37442
---
 magnum/api/controllers/v1/nodegroup.py        |  8 ++++---
 .../unit/api/controllers/v1/test_nodegroup.py | 24 ++++++++++++++++---
 .../nodegroup-limit-89930d45ee06c621.yaml     |  4 ++++
 3 files changed, 30 insertions(+), 6 deletions(-)
 create mode 100644 releasenotes/notes/nodegroup-limit-89930d45ee06c621.yaml

diff --git a/magnum/api/controllers/v1/nodegroup.py b/magnum/api/controllers/v1/nodegroup.py
index 449a7db4d3..dcd15c809c 100644
--- a/magnum/api/controllers/v1/nodegroup.py
+++ b/magnum/api/controllers/v1/nodegroup.py
@@ -180,12 +180,13 @@ class NodeGroupCollection(collection.Collection):
         self._type = 'nodegroups'
 
     @staticmethod
-    def convert(nodegroups, limit, expand=True, **kwargs):
+    def convert(nodegroups, cluster_id, limit, expand=True, **kwargs):
         collection = NodeGroupCollection()
         collection.nodegroups = [NodeGroup.convert(ng, expand)
                                  for ng in nodegroups]
+        url = "clusters/%s/nodegroups" % cluster_id
         collection.next = collection.get_next(limit,
-                                              marker_attribute='id',
+                                              url=url,
                                               **kwargs)
         return collection
 
@@ -217,13 +218,14 @@ class NodeGroupController(base.Controller):
                                             filters=filters)
 
         return NodeGroupCollection.convert(nodegroups,
+                                           cluster_id,
                                            limit,
                                            expand=expand,
                                            sort_key=sort_key,
                                            sort_dir=sort_dir)
 
     @base.Controller.api_version("1.9")
-    @expose.expose(NodeGroupCollection, types.uuid_or_name, int, int,
+    @expose.expose(NodeGroupCollection, types.uuid_or_name, types.uuid, int,
                    wtypes.text, wtypes.text, wtypes.text)
     def get_all(self, cluster_id, marker=None, limit=None, sort_key='id',
                 sort_dir='asc', role=None):
diff --git a/magnum/tests/unit/api/controllers/v1/test_nodegroup.py b/magnum/tests/unit/api/controllers/v1/test_nodegroup.py
index cd460d6940..78c6e0ee12 100644
--- a/magnum/tests/unit/api/controllers/v1/test_nodegroup.py
+++ b/magnum/tests/unit/api/controllers/v1/test_nodegroup.py
@@ -114,11 +114,29 @@ class TestListNodegroups(NodeGroupControllerTest):
         self._test_list_nodegroups(self.cluster.name, expected=expected)
 
     def test_get_all_with_pagination_marker(self):
-        ng_uuid = self.cluster.default_ng_master.uuid
-        url = '/clusters/%s/nodegroups?limit=1&marker=1' % (self.cluster_uuid)
+        worker_ng_uuid = self.cluster.default_ng_worker.uuid
+        master_ng_uuid = self.cluster.default_ng_master.uuid
+        # First make sure that the api returns 1 ng and since they
+        # are sorted by id, the ng should be the default-worker
+        url = '/clusters/%s/nodegroups?limit=1' % (self.cluster_uuid)
         response = self.get_json(url)
         self.assertEqual(1, len(response['nodegroups']))
-        self.assertEqual(ng_uuid, response['nodegroups'][0]['uuid'])
+        self.assertEqual(worker_ng_uuid, response['nodegroups'][0]['uuid'])
+        marker = "marker=%s" % worker_ng_uuid
+        self.assertIn(marker, response['next'])
+        # Now using the next url make sure that we get the default-master
+        next_url = response['next'].split('v1')[1]
+        response = self.get_json(next_url)
+        self.assertEqual(1, len(response['nodegroups']))
+        self.assertEqual(master_ng_uuid, response['nodegroups'][0]['uuid'])
+        marker = "marker=%s" % master_ng_uuid
+        self.assertIn(marker, response['next'])
+        # Now we should not get any other entry since the cluster only has two
+        # nodegroups and the marker is set at the default-master.
+        next_url = response['next'].split('v1')[1]
+        response = self.get_json(next_url)
+        self.assertEqual(0, len(response['nodegroups']))
+        self.assertNotIn('next', response)
 
     def test_get_all_by_role(self):
         filters = {'role': 'master'}
diff --git a/releasenotes/notes/nodegroup-limit-89930d45ee06c621.yaml b/releasenotes/notes/nodegroup-limit-89930d45ee06c621.yaml
new file mode 100644
index 0000000000..775d8a30b3
--- /dev/null
+++ b/releasenotes/notes/nodegroup-limit-89930d45ee06c621.yaml
@@ -0,0 +1,4 @@
+---
+fixes:
+  - |
+    Fixes the next url in the list nodegroups API response.