Merge "Implement pagination for users and groups"

api-ref/source/v3/groups.inc

@@ -40,6 +40,10 @@ Parameters
 
    - name: group_name_query
    - domain_id: domain_id_query
+   - limit: limit_query
+   - marker: marker_query
+   - sort_key: sort_key
+   - sort_dir: sort_dir
 
 Response
 --------

api-ref/source/v3/parameters.yaml

@@ -464,6 +464,19 @@ service_type_query:
   in: query
   required: false
   type: string
+sort_dir:
+  description: |
+    Sort direction. A valid value is asc (ascending) or desc
+    (descending).
+  in: query
+  required: false
+  type: string
+sort_key:
+  description: |
+    Sorts resources by attribute
+  in: query
+  required: false
+  type: string
 subtree_as_ids:
   description: |
     The entire child hierarchy will be included as nested dictionaries

api-ref/source/v3/users.inc

@@ -41,6 +41,10 @@ Parameters
    - password_expires_at: password_expires_at_query
    - protocol_id: protocol_id_query
    - unique_id: unique_id_query
+   - limit: limit_query
+   - marker: marker_query
+   - sort_key: sort_key
+   - sort_dir: sort_dir
 
 Response
 --------

keystone/api/validation/parameter_types.py

@@ -118,3 +118,14 @@ id_string: dict[str, Any] = {
     "maxLength": 64,
     "pattern": r"^[a-zA-Z0-9-]+$",
 }
+
+sort_key: dict[str, Any] = {
+    "type": "string",
+    "description": "Sorts resources by attribute.",
+}
+
+sort_dir: dict[str, Any] = {
+    "type": "string",
+    "description": "Sort direction. A valid value is asc (ascending) or desc (descending).",
+    "enum": ["asc", "desc"],
+}

keystone/identity/backends/sql.py

@@ -15,6 +15,7 @@
 import datetime
 
 from oslo_db import api as oslo_db_api
+from oslo_db.sqlalchemy import utils as sqlalchemyutils
 from oslo_utils import timeutils
 import sqlalchemy
 
@@ -186,8 +187,39 @@ class Identity(base.IdentityDriverBase):
             query, hints = self._create_password_expires_query(
                 session, query, hints
             )
-            user_refs = sql.filter_limit_query(model.User, query, hints)
+            query = sql.filter_query(model.User, query, hints)
-            return [base.filter_user(x.to_dict()) for x in user_refs]
+            marker_row = None
+            if hints.marker is not None:
+                marker_row = (
+                    session.query(model.User)
+                    .filter_by(id=hints.marker)
+                    .first()
+                )
+                if not marker_row:
+                    raise exception.MarkerNotFound(marker=hints.marker)
+
+            user_refs = sqlalchemyutils.paginate_query(
+                query,
+                model.User,
+                hints.get_limit_or_max(),
+                ["id"],
+                marker=marker_row,
+            )
+
+            data = user_refs.all()
+            if hints.limit:
+                # the `common.manager.response_truncated` decorator expects
+                # that when driver truncates results it should also raise
+                # 'truncated' flag to indicate that. Since we do not really
+                # know whether there are more records once we applied filters
+                # we can only "assume" and set the flag when count of records
+                # is equal to what we have limited to.
+                # NOTE(gtema) get rid of that once proper pagination is
+                # enabled for all resources
+                if len(data) >= hints.limit["limit"]:
+                    hints.limit["truncated"] = True
+
+            return [base.filter_user(x.to_dict()) for x in data]
 
     def unset_default_project_id(self, project_id):
         with sql.session_for_write() as session:
@@ -441,8 +473,41 @@ class Identity(base.IdentityDriverBase):
     def list_groups(self, hints):
         with sql.session_for_read() as session:
             query = session.query(model.Group)
-            refs = sql.filter_limit_query(model.Group, query, hints)
+
-            return [ref.to_dict() for ref in refs]
+            query = sql.filter_query(model.Group, query, hints)
+
+            marker_row = None
+            if hints.marker is not None:
+                marker_row = (
+                    session.query(model.Group)
+                    .filter_by(id=hints.marker)
+                    .first()
+                )
+                if not marker_row:
+                    raise exception.MarkerNotFound(marker=hints.marker)
+
+            group_refs = sqlalchemyutils.paginate_query(
+                query,
+                model.Group,
+                hints.get_limit_or_max(),
+                ["id"],
+                marker=marker_row,
+            )
+
+            data = group_refs.all()
+            if hints.limit:
+                # the `common.manager.response_truncated` decorator expects
+                # that when driver truncates results it should also raise
+                # 'truncated' flag to indicate that. Since we do not really
+                # know whether there are more records once we applied filters
+                # we can only "assume" and set the flag when count of records
+                # is equal to what we have limited to.
+                # NOTE(gtema) get rid of that once proper pagination is
+                # enabled for all resources
+                if len(data) >= hints.limit["limit"]:
+                    hints.limit["truncated"] = True
+
+            return [ref.to_dict() for ref in data]
 
     def _get_group(self, session, group_id):
         ref = session.get(model.Group, group_id)

keystone/identity/schema.py

@@ -73,6 +73,13 @@ user_index_request_query: dict[str, Any] = {
             "type": "string",
             "description": "Filters the response by a unique ID.",
         },
+        "marker": {
+            "type": "string",
+            "description": "ID of the last fetched entry",
+        },
+        "limit": {"type": ["integer", "string"]},
+        "sort_key": parameter_types.sort_key,
+        "sort_dir": parameter_types.sort_dir,
     },
     "additionalProperties": True,
 }
@@ -191,6 +198,13 @@ group_index_request_query: dict[str, Any] = {
     "properties": {
         "domain_id": parameter_types.domain_id,
         "name": parameter_types.name,
+        "marker": {
+            "type": "string",
+            "description": "ID of the last fetched entry",
+        },
+        "limit": {"type": ["integer", "string"]},
+        "sort_key": parameter_types.sort_key,
+        "sort_dir": parameter_types.sort_dir,
     },
     "additionalProperties": False,
 }

keystone/tests/unit/test_v3.py

@@ -1666,11 +1666,14 @@ class PaginationTestCaseBase(RestfulTestCase):
     """Base test for the resource pagination."""
 
     resource_name: ty.Optional[str] = None
+    config_group: ty.Optional[str] = None
 
     def setUp(self):
         super().setUp()
         if not self.resource_name:
             self.skipTest("Not testing the base")
+        if not self.config_group:
+            self.skipTest("Not testing the base")
 
     @abc.abstractmethod
     def _create_resources(self, count: int):
@@ -1728,7 +1731,7 @@ class PaginationTestCaseBase(RestfulTestCase):
         res_list = response.json_body[f"{self.resource_name}s"]
         self.assertGreaterEqual(
             len(response.json_body[f"{self.resource_name}s"]),
-            count_resources,
+            2,
             "Requested limit higher then default wins",
         )
 
@@ -1770,7 +1773,7 @@ class PaginationTestCaseBase(RestfulTestCase):
             "Next page link contains corrected limit and marker",
         )
 
-        self.config_fixture.config(group="resource", list_limit=3)
+        self.config_fixture.config(group=self.config_group, list_limit=3)
 
         response = self.get(f'/{self.resource_name}s')
         res_list = response.json_body[f"{self.resource_name}s"]
@@ -1821,7 +1824,9 @@ class PaginationTestCaseBase(RestfulTestCase):
         current_count = len(response.json_body[f"{self.resource_name}s"])
 
         # Set pagination default at 5
-        self.config_fixture.config(group="resource", list_limit=page_size)
+        self.config_fixture.config(
+            group=self.config_group, list_limit=page_size
+        )
 
         (found_resources, pages) = self._consume_paginated_list()
         self.assertGreaterEqual(

keystone/tests/unit/test_v3_filters.py

@@ -33,6 +33,14 @@ CONF = keystone.conf.CONF
 PROVIDERS = provider_api.ProviderAPIs
 
 
+def _get_id_list_from_ref_list(ref_list):
+    """Get entity IDs from the response"""
+    result_list = []
+    for x in ref_list:
+        result_list.append(x['id'])
+    return result_list
+
+
 class IdentityTestFilteredCase(filtering.FilterTests, test_v3.RestfulTestCase):
     """Test filter enforcement on the v3 Identity API."""
 
@@ -96,12 +104,6 @@ class IdentityTestFilteredCase(filtering.FilterTests, test_v3.RestfulTestCase):
             user_id=self.user1['id'], password=self.user1['password']
         )
 
-    def _get_id_list_from_ref_list(self, ref_list):
-        result_list = []
-        for x in ref_list:
-            result_list.append(x['id'])
-        return result_list
-
     def _set_policy(self, new_policy):
         with open(self.tmpfilename, "w") as policyfile:
             policyfile.write(jsonutils.dumps(new_policy))
@@ -120,7 +122,7 @@ class IdentityTestFilteredCase(filtering.FilterTests, test_v3.RestfulTestCase):
         url_by_name = '/users?domain_id={}'.format(self.domainB['id'])
         r = self.get(url_by_name, auth=self.auth)
         # We should  get back two users, those in DomainB
-        id_list = self._get_id_list_from_ref_list(r.result.get('users'))
+        id_list = _get_id_list_from_ref_list(r.result.get('users'))
         self.assertIn(self.user2['id'], id_list)
         self.assertIn(self.user3['id'], id_list)
 
@@ -140,28 +142,28 @@ class IdentityTestFilteredCase(filtering.FilterTests, test_v3.RestfulTestCase):
         new_policy = {"identity:list_domains": []}
         self._set_policy(new_policy)
         r = self.get('/domains?enabled=0', auth=self.auth)
-        id_list = self._get_id_list_from_ref_list(r.result.get('domains'))
+        id_list = _get_id_list_from_ref_list(r.result.get('domains'))
         self.assertEqual(1, len(id_list))
         self.assertIn(self.domainC['id'], id_list)
 
         # Try a few ways of specifying 'false'
         for val in ('0', 'false', 'False', 'FALSE', 'n', 'no', 'off'):
             r = self.get(f'/domains?enabled={val}', auth=self.auth)
-            id_list = self._get_id_list_from_ref_list(r.result.get('domains'))
+            id_list = _get_id_list_from_ref_list(r.result.get('domains'))
             self.assertEqual([self.domainC['id']], id_list)
 
         # Now try a few ways of specifying 'true' when we should get back
         # the other two domains, plus the default domain
         for val in ('1', 'true', 'True', 'TRUE', 'y', 'yes', 'on'):
             r = self.get(f'/domains?enabled={val}', auth=self.auth)
-            id_list = self._get_id_list_from_ref_list(r.result.get('domains'))
+            id_list = _get_id_list_from_ref_list(r.result.get('domains'))
             self.assertEqual(3, len(id_list))
             self.assertIn(self.domainA['id'], id_list)
             self.assertIn(self.domainB['id'], id_list)
             self.assertIn(CONF.identity.default_domain_id, id_list)
 
         r = self.get('/domains?enabled', auth=self.auth)
-        id_list = self._get_id_list_from_ref_list(r.result.get('domains'))
+        id_list = _get_id_list_from_ref_list(r.result.get('domains'))
         self.assertEqual(3, len(id_list))
         self.assertIn(self.domainA['id'], id_list)
         self.assertIn(self.domainB['id'], id_list)
@@ -182,7 +184,7 @@ class IdentityTestFilteredCase(filtering.FilterTests, test_v3.RestfulTestCase):
 
         my_url = '/domains?enabled&name={}'.format(self.domainA['name'])
         r = self.get(my_url, auth=self.auth)
-        id_list = self._get_id_list_from_ref_list(r.result.get('domains'))
+        id_list = _get_id_list_from_ref_list(r.result.get('domains'))
         self.assertEqual(1, len(id_list))
         self.assertIn(self.domainA['id'], id_list)
         self.assertIs(True, r.result.get('domains')[0]['enabled'])
@@ -202,7 +204,7 @@ class IdentityTestFilteredCase(filtering.FilterTests, test_v3.RestfulTestCase):
 
         my_url = '/domains?enableds=0&name={}'.format(self.domainA['name'])
         r = self.get(my_url, auth=self.auth)
-        id_list = self._get_id_list_from_ref_list(r.result.get('domains'))
+        id_list = _get_id_list_from_ref_list(r.result.get('domains'))
 
         # domainA is returned and it is enabled, since enableds=0 is not the
         # same as enabled=0
@@ -238,7 +240,7 @@ class IdentityTestFilteredCase(filtering.FilterTests, test_v3.RestfulTestCase):
             r = self.get(url_by_name, auth=self.auth)
 
             self.assertEqual(1, len(r.result.get('users')))
-            self.assertEqual(user['id'], r.result.get('users')[0]['id'])
+            self.assertIn(user['id'], [x["id"] for x in r.result.get('users')])
 
     def test_inexact_filters(self):
         # Create 20 users
@@ -297,13 +299,19 @@ class IdentityTestFilteredCase(filtering.FilterTests, test_v3.RestfulTestCase):
         url_by_name = '/users?name__endswith=of'
         r = self.get(url_by_name, auth=self.auth)
         self.assertEqual(1, len(r.result.get('users')))
-        self.assertEqual(user_list[7]['id'], r.result.get('users')[0]['id'])
+        self.assertIn(
+            user_list[7]['id'], [x["id"] for x in r.result.get('users')]
+        )
 
         url_by_name = '/users?name__iendswith=OF'
         r = self.get(url_by_name, auth=self.auth)
         self.assertEqual(2, len(r.result.get('users')))
-        self.assertEqual(user_list[7]['id'], r.result.get('users')[0]['id'])
+        self.assertIn(
-        self.assertEqual(user_list[10]['id'], r.result.get('users')[1]['id'])
+            user_list[7]['id'], [x["id"] for x in r.result.get('users')]
+        )
+        self.assertIn(
+            user_list[10]['id'], [x["id"] for x in r.result.get('users')]
+        )
 
         self._delete_test_data('user', user_list)
 
@@ -476,7 +484,7 @@ class IdentityPasswordExpiryFilteredTestCase(
             'eq',
         )
         resp_users = self.get(expire_at_url).result.get('users')
-        self.assertEqual(self.user2['id'], resp_users[0]['id'])
+        self.assertIn(self.user2['id'], [x["id"] for x in resp_users])
 
         expire_at_url = self._list_users_by_password_expires_at(
             self._format_timestamp(
@@ -484,9 +492,10 @@ class IdentityPasswordExpiryFilteredTestCase(
             ),
             'neq',
         )
-        resp_users = self.get(expire_at_url).result.get('users')
+        resp_users = self.get(expire_at_url).result.get("users")
-        self.assertEqual(self.user['id'], resp_users[0]['id'])
+        id_list = _get_id_list_from_ref_list(resp_users)
-        self.assertEqual(self.user3['id'], resp_users[1]['id'])
+        self.assertIn(self.user['id'], id_list)
+        self.assertIn(self.user3['id'], id_list)
 
     def test_list_users_by_password_expires_before(self):
         """Ensure users can be filtered on lt and lte.
@@ -502,8 +511,9 @@ class IdentityPasswordExpiryFilteredTestCase(
             'lt',
         )
         resp_users = self.get(expire_before_url).result.get('users')
-        self.assertEqual(self.user['id'], resp_users[0]['id'])
+        id_list = _get_id_list_from_ref_list(resp_users)
-        self.assertEqual(self.user2['id'], resp_users[1]['id'])
+        self.assertIn(self.user['id'], id_list)
+        self.assertIn(self.user2['id'], id_list)
 
         expire_before_url = self._list_users_by_password_expires_at(
             self._format_timestamp(
@@ -512,8 +522,9 @@ class IdentityPasswordExpiryFilteredTestCase(
             'lte',
         )
         resp_users = self.get(expire_before_url).result.get('users')
-        self.assertEqual(self.user['id'], resp_users[0]['id'])
+        id_list = _get_id_list_from_ref_list(resp_users)
-        self.assertEqual(self.user2['id'], resp_users[1]['id'])
+        self.assertIn(self.user['id'], id_list)
+        self.assertIn(self.user2['id'], id_list)
 
     def test_list_users_by_password_expires_after(self):
         """Ensure users can be filtered on gt and gte.
@@ -529,7 +540,7 @@ class IdentityPasswordExpiryFilteredTestCase(
             'gt',
         )
         resp_users = self.get(expire_after_url).result.get('users')
-        self.assertEqual(self.user3['id'], resp_users[0]['id'])
+        self.assertIn(self.user3['id'], [x["id"] for x in resp_users])
 
         expire_after_url = self._list_users_by_password_expires_at(
             self._format_timestamp(
@@ -538,8 +549,9 @@ class IdentityPasswordExpiryFilteredTestCase(
             'gte',
         )
         resp_users = self.get(expire_after_url).result.get('users')
-        self.assertEqual(self.user2['id'], resp_users[0]['id'])
+        id_list = _get_id_list_from_ref_list(resp_users)
-        self.assertEqual(self.user3['id'], resp_users[1]['id'])
+        self.assertIn(self.user2['id'], id_list)
+        self.assertIn(self.user3['id'], id_list)
 
     def test_list_users_by_password_expires_interval(self):
         """Ensure users can be filtered on time intervals.
@@ -562,7 +574,7 @@ class IdentityPasswordExpiryFilteredTestCase(
             'gt',
         )
         resp_users = self.get(expire_interval_url).result.get('users')
-        self.assertEqual(self.user2['id'], resp_users[0]['id'])
+        self.assertIn(self.user2['id'], [x["id"] for x in resp_users])
 
         expire_interval_url = self._list_users_by_multiple_password_expires_at(
             self._format_timestamp(
@@ -575,7 +587,7 @@ class IdentityPasswordExpiryFilteredTestCase(
             'lte',
         )
         resp_users = self.get(expire_interval_url).result.get('users')
-        self.assertEqual(self.user2['id'], resp_users[0]['id'])
+        self.assertIn(self.user2['id'], [x["id"] for x in resp_users])
 
     def test_list_users_by_password_expires_with_bad_operator_fails(self):
         """Ensure an invalid operator returns a Bad Request.
@@ -675,7 +687,7 @@ class IdentityPasswordExpiryFilteredTestCase(
             'eq',
         )
         resp_users = self.get(expire_at_url).result.get('users')
-        self.assertEqual(self.user2['id'], resp_users[0]['id'])
+        self.assertIn(self.user2['id'], [x["id"] for x in resp_users])
 
         expire_at_url = self._list_users_in_group_by_password_expires_at(
             self._format_timestamp(
@@ -684,7 +696,7 @@ class IdentityPasswordExpiryFilteredTestCase(
             'neq',
         )
         resp_users = self.get(expire_at_url).result.get('users')
-        self.assertEqual(self.user3['id'], resp_users[0]['id'])
+        self.assertIn(self.user3['id'], [x["id"] for x in resp_users])
 
     def test_list_users_in_group_by_password_expires_before(self):
         """Ensure users in a group can be filtered on with lt and lte.
@@ -700,7 +712,7 @@ class IdentityPasswordExpiryFilteredTestCase(
             'lt',
         )
         resp_users = self.get(expire_before_url).result.get('users')
-        self.assertEqual(self.user2['id'], resp_users[0]['id'])
+        self.assertIn(self.user2['id'], [x["id"] for x in resp_users])
 
         expire_before_url = self._list_users_in_group_by_password_expires_at(
             self._format_timestamp(
@@ -709,7 +721,7 @@ class IdentityPasswordExpiryFilteredTestCase(
             'lte',
         )
         resp_users = self.get(expire_before_url).result.get('users')
-        self.assertEqual(self.user2['id'], resp_users[0]['id'])
+        self.assertIn(self.user2['id'], [x["id"] for x in resp_users])
 
     def test_list_users_in_group_by_password_expires_after(self):
         """Ensure users in a group can be filtered on with gt and gte.
@@ -725,7 +737,7 @@ class IdentityPasswordExpiryFilteredTestCase(
             'gt',
         )
         resp_users = self.get(expire_after_url).result.get('users')
-        self.assertEqual(self.user3['id'], resp_users[0]['id'])
+        self.assertIn(self.user3['id'], [x["id"] for x in resp_users])
 
         expire_after_url = self._list_users_in_group_by_password_expires_at(
             self._format_timestamp(
@@ -734,8 +746,9 @@ class IdentityPasswordExpiryFilteredTestCase(
             'gte',
         )
         resp_users = self.get(expire_after_url).result.get('users')
-        self.assertEqual(self.user2['id'], resp_users[0]['id'])
+        id_list = _get_id_list_from_ref_list(resp_users)
-        self.assertEqual(self.user3['id'], resp_users[1]['id'])
+        self.assertIn(self.user2['id'], id_list)
+        self.assertIn(self.user3['id'], id_list)
 
     def test_list_users_in_group_by_password_expires_interval(self):
         """Ensure users in a group can be filtered on time intervals.
@@ -760,8 +773,9 @@ class IdentityPasswordExpiryFilteredTestCase(
             )
         )
         resp_users = self.get(expire_interval_url).result.get('users')
-        self.assertEqual(self.user2['id'], resp_users[0]['id'])
+        id_list = _get_id_list_from_ref_list(resp_users)
-        self.assertEqual(self.user3['id'], resp_users[1]['id'])
+        self.assertIn(self.user2['id'], id_list)
+        self.assertIn(self.user3['id'], id_list)
 
         expire_interval_url = (
             self._list_users_in_group_by_multiple_password_expires_at(
@@ -776,8 +790,9 @@ class IdentityPasswordExpiryFilteredTestCase(
             )
         )
         resp_users = self.get(expire_interval_url).result.get('users')
-        self.assertEqual(self.user2['id'], resp_users[0]['id'])
+        id_list = _get_id_list_from_ref_list(resp_users)
-        self.assertEqual(self.user3['id'], resp_users[1]['id'])
+        self.assertIn(self.user2['id'], id_list)
+        self.assertIn(self.user3['id'], id_list)
 
     def test_list_users_in_group_by_password_expires_bad_operator_fails(self):
         """Ensure an invalid operator returns a Bad Request.

keystone/tests/unit/test_v3_resource.py

@@ -2054,6 +2054,7 @@ class DomainPaginationTestCase(test_v3.PaginationTestCaseBase):
     """Test domain list pagination."""
 
     resource_name: str = "domain"
+    config_group: str = "resource"
 
     def _create_resources(self, count: int):
         for x in range(count):
@@ -2065,8 +2066,41 @@ class ProjectPaginationTestCase(test_v3.PaginationTestCaseBase):
     """Test project list pagination."""
 
     resource_name: str = "project"
+    config_group: str = "resource"
 
     def _create_resources(self, count: int):
         for x in range(count):
             res = {"project": unit.new_project_ref()}
             response = self.post("/projects", body=res)
+
+
+class UserPaginationTestCase(test_v3.PaginationTestCaseBase):
+    """Test user list pagination."""
+
+    resource_name: str = "user"
+    config_group: str = "identity"
+
+    def _create_resources(self, count: int):
+        for x in range(count):
+            res = {
+                "user": unit.new_user_ref(
+                    domain_id=CONF.identity.default_domain_id
+                )
+            }
+            response = self.post("/users", body=res)
+
+
+class GroupPaginationTestCase(test_v3.PaginationTestCaseBase):
+    """Test group list pagination."""
+
+    resource_name: str = "group"
+    config_group: str = "identity"
+
+    def _create_resources(self, count: int):
+        for x in range(count):
+            res = {
+                "group": unit.new_group_ref(
+                    domain_id=CONF.identity.default_domain_id
+                )
+            }
+            response = self.post("/groups", body=res)

releasenotes/notes/pagination-identity-b127b6d3042d3d3a.yaml

@@ -0,0 +1,6 @@
+---
+features:
+   - |
+    User and group listing supports pagination. Query parameters `limit`
+    and `marker` are added and work as described in `API-SIG doc
+    <https://specs.openstack.org/openstack/api-sig/guidelines/pagination_filter_sort.html>`_