diff --git a/api-ref/source/v3/domains.inc b/api-ref/source/v3/domains.inc index 3fce064f51..ed97d95d92 100644 --- a/api-ref/source/v3/domains.inc +++ b/api-ref/source/v3/domains.inc @@ -41,6 +41,8 @@ Parameters - name: domain_name_query - enabled: domain_enabled_query + - limit: limit_query + - marker: marker_query Response -------- diff --git a/api-ref/source/v3/parameters.yaml b/api-ref/source/v3/parameters.yaml index 4f008b1482..63f43b2703 100644 --- a/api-ref/source/v3/parameters.yaml +++ b/api-ref/source/v3/parameters.yaml @@ -261,6 +261,23 @@ is_domain_query: required: false type: boolean min_version: 3.6 +limit_query: + description: | + Requests a page size of items. Returns a number of items up to a limit + value. Use the limit parameter to make an initial limited request and use + the ID of the last-seen item from the response as the marker parameter + value in a subsequent limited request. + in: query + required: false + type: integer +marker_query: + description: | + The ID of the last-seen item. Use the limit parameter to make an + initial limited request and use the ID of the last-seen item from the + response as the marker parameter value in a subsequent limited request + in: query + required: false + type: string name_user_query: description: | Filters the response by a user name. diff --git a/api-ref/source/v3/projects.inc b/api-ref/source/v3/projects.inc index 4ae156e673..a4d7f580ea 100644 --- a/api-ref/source/v3/projects.inc +++ b/api-ref/source/v3/projects.inc @@ -63,6 +63,8 @@ Parameters - is_domain: is_domain_query - name: project_name_query - parent_id: parent_id_query + - limit: limit_query + - marker: marker_query Response -------- diff --git a/keystone/api/projects.py b/keystone/api/projects.py index 31b8767091..26aa3a07a9 100644 --- a/keystone/api/projects.py +++ b/keystone/api/projects.py @@ -175,7 +175,9 @@ class ProjectsResource(ks_flask.ResourceBase): filters=filters, target_attr=target, ) - hints = self.build_driver_hints(filters) + hints = self.build_driver_hints( + filters, default_limit=PROVIDERS.resource_api._get_list_limit() + ) # If 'is_domain' has not been included as a query, we default it to # False (which in query terms means '0') diff --git a/keystone/common/driver_hints.py b/keystone/common/driver_hints.py index 29dcb1add0..73ea1c4fcd 100644 --- a/keystone/common/driver_hints.py +++ b/keystone/common/driver_hints.py @@ -12,12 +12,15 @@ # 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 functools +import typing as ty +import keystone.conf from keystone import exception from keystone.i18n import _ +CONF = keystone.conf.CONF + def truncated(f): """Ensure list truncation is detected in Driver list entity methods. @@ -51,7 +54,7 @@ def truncated(f): ref_list = f(self, hints, *args, **kwargs) # If we got more than the original limit then trim back the list and - # mark it truncated. In both cases, make sure we set the limit back + # mark it truncated. In both cases, make sure we set the limit back # to its original value. if len(ref_list) > list_limit: hints.set_limit(list_limit, truncated=True) @@ -95,12 +98,17 @@ class Hints: """ def __init__(self): - self.limit = None + self.limit: ty.Optional[dict[str, ty.Any]] = None + self.marker: ty.Optional[str] = None self.filters = [] self.cannot_match = False def add_filter( - self, name, value, comparator='equals', case_sensitive=False + self, + name, + value, + comparator: str = 'equals', + case_sensitive: bool = False, ): """Add a filter to the filters list, which is publicly accessible.""" self.filters.append( @@ -118,6 +126,57 @@ class Hints: if entry['name'] == name and entry['comparator'] == 'equals': return entry - def set_limit(self, limit, truncated=False): + def set_limit(self, limit: int, truncated: bool = False): """Set a limit to indicate the list should be truncated.""" self.limit = {'limit': limit, 'truncated': truncated} + + def get_limit_or_max(self) -> int: + """Get page limit or max page size + + Return page limit (size) as requested by user (or API flow) or the + maximum page size if not present. This method is invoked by the SQL + drivers. + + :returns int: Page size + """ + limit: ty.Optional[int] = ( + self.limit.get("limit") if self.limit else None + ) + return int(limit) if limit else CONF.max_db_limit + + def get_limit_with_default( + self, default_limit: ty.Optional[int] = None + ) -> int: + """Return page limit for the query. + + 1. `limit` was set in the query parameters: + `min(limit, MAX_LIMIT)` + + 2. `limit` is not set and `default_limit` is set: + `min(default_limit, MAX_LIMIT)` + + 2. `limit` is null, `default_limit` is null, `CONF.list_limit` is set: + `min(CONF.list_limit, MAX_LIMIT)` + + 3. `limit` is null, `default_limit` is null, `CONF.list_limit` is null: + `CONF.max_db_limit` + + """ + + _user_limit: ty.Optional[int] = ( + self.limit.get("limit") if self.limit else None + ) + _absolute_limit: int = CONF.max_db_limit + # when default (as resource specific) is not set try to get the global + # default + _default_limit: ty.Optional[int] = default_limit or CONF.list_limit + if _user_limit: + return min(_user_limit, _absolute_limit) + elif _default_limit: + return min(_default_limit, _absolute_limit) + else: + return CONF.max_db_limit + + def set_marker(self, marker: str): + """Set a marker pointing to the last entry of the page.""" + self.marker = marker diff --git a/keystone/common/manager.py b/keystone/common/manager.py index eef40bb953..d8dda6dffc 100644 --- a/keystone/common/manager.py +++ b/keystone/common/manager.py @@ -20,6 +20,7 @@ import types from oslo_log import log import stevedore +from keystone.common.driver_hints import Hints from keystone.common import provider_api from keystone.i18n import _ @@ -56,9 +57,14 @@ def response_truncated(f): if kwargs.get('hints') is None: return f(self, *args, **kwargs) - list_limit = self.driver._get_list_limit() - if list_limit: - kwargs['hints'].set_limit(list_limit) + # Set default limit if not there already + if ( + isinstance(kwargs['hints'], Hints) + and kwargs['hints'].limit is None + ): + list_limit = self.driver._get_list_limit() + if list_limit: + kwargs['hints'].set_limit(list_limit) return f(self, *args, **kwargs) return wrapper diff --git a/keystone/common/sql/core.py b/keystone/common/sql/core.py index 3d30a26356..56bd51ed2e 100644 --- a/keystone/common/sql/core.py +++ b/keystone/common/sql/core.py @@ -493,6 +493,40 @@ def _limit(query, hints): return query +def filter_query(model, query, hints): + """Apply filtering to a query. + + :param model: table model + :param query: query to apply filters to + :param hints: contains the list of filters and limit details. This may + be None, indicating that there are no filters or limits + to be applied. If it's not None, then any filters + satisfied here will be removed so that the caller will + know if any filters remain. + + :returns: query updated with any filters and limits satisfied + + """ + if hints is None: + return query + + # First try and satisfy any filters + query = _filter(model, query, hints) + + if hints.cannot_match: + # Nothing's going to match, so don't bother with the query. + return [] + + # NOTE(henry-nash): Any unsatisfied filters will have been left in + # the hints list for the controller to handle. We can only try and + # limit here if all the filters are already satisfied since, if not, + # doing so might mess up the final results. If there are still + # unsatisfied filters, we have to leave any limiting to the controller + # as well. + + return query + + def filter_limit_query(model, query, hints): """Apply filtering and limit to a query. diff --git a/keystone/conf/default.py b/keystone/conf/default.py index d72f00ccdd..145799d39c 100644 --- a/keystone/conf/default.py +++ b/keystone/conf/default.py @@ -97,6 +97,20 @@ projects from placing an unnecessary load on the system. ), ) +max_db_limit = cfg.IntOpt( + 'max_db_limit', + default=1000, + min=0, + help=utils.fmt( + """ +As a query can potentially return many thousands of items, you can limit the +maximum number of items in a single response by setting this option. +While `list_limit` is used to set the default page size this parameter sets +global maximum that cannot be exceded. +""" + ), +) + strict_password_check = cfg.BoolOpt( 'strict_password_check', default=False, @@ -177,6 +191,7 @@ ALL_OPTS = [ max_param_size, max_token_size, list_limit, + max_db_limit, strict_password_check, insecure_debug, default_publisher_id, diff --git a/keystone/exception.py b/keystone/exception.py index 5e1ee3f250..0eb209b9b0 100644 --- a/keystone/exception.py +++ b/keystone/exception.py @@ -443,6 +443,10 @@ class NotFound(Error): title = http.client.responses[http.client.NOT_FOUND] +class MarkerNotFound(NotFound): + message_format = _("Marker %(marker)s could not be found.") + + class EndpointNotFound(NotFound): message_format = _("Could not find endpoint: %(endpoint_id)s.") diff --git a/keystone/resource/backends/sql.py b/keystone/resource/backends/sql.py index 7e8b7d90dd..cacbd18b90 100644 --- a/keystone/resource/backends/sql.py +++ b/keystone/resource/backends/sql.py @@ -10,17 +10,19 @@ # License for the specific language governing permissions and limitations # under the License. +from oslo_db.sqlalchemy import utils as sqlalchemyutils from oslo_log import log from sqlalchemy import orm from sqlalchemy.sql import expression -from keystone.common import driver_hints from keystone.common import resource_options from keystone.common import sql +import keystone.conf from keystone import exception from keystone.resource.backends import base from keystone.resource.backends import sql_model +CONF = keystone.conf.CONF LOG = log.getLogger(__name__) @@ -63,7 +65,6 @@ class Resource(base.ResourceDriverBase): raise exception.ProjectNotFound(project_id=project_name) return project_ref.to_dict() - @driver_hints.truncated def list_projects(self, hints): # If there is a filter on domain_id and the value is None, then to # ensure that the sql filtering works correctly, we need to patch @@ -77,10 +78,38 @@ class Resource(base.ResourceDriverBase): with sql.session_for_read() as session: query = session.query(sql_model.Project) query = query.filter(sql_model.Project.id != base.NULL_DOMAIN_ID) - project_refs = sql.filter_limit_query( - sql_model.Project, query, hints + query = sql.filter_query(sql_model.Project, query, hints) + marker_row = None + if hints.marker is not None: + marker_row = ( + session.query(sql_model.Project) + .filter_by(id=hints.marker) + .first() + ) + if not marker_row: + raise exception.MarkerNotFound(marker=hints.marker) + + project_refs = sqlalchemyutils.paginate_query( + query, + sql_model.Project, + hints.get_limit_or_max(), + ["id"], + marker=marker_row, ) - return [project_ref.to_dict() for project_ref in project_refs] + # Query the data + data = project_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 [project_ref.to_dict() for project_ref in data] def list_projects_from_ids(self, ids): if not ids: diff --git a/keystone/resource/schema.py b/keystone/resource/schema.py index b3480dd6aa..7ea57c8b99 100644 --- a/keystone/resource/schema.py +++ b/keystone/resource/schema.py @@ -115,6 +115,11 @@ project_index_request_query = { "tags-any": parameter_types.tags, "not-tags": parameter_types.tags, "not-tags-any": parameter_types.tags, + "marker": { + "type": "string", + "description": "ID of the last fetched entry", + }, + "limit": {"type": ["integer", "string"]}, }, } diff --git a/keystone/server/flask/common.py b/keystone/server/flask/common.py index e4cbacf66a..78620e0fd1 100644 --- a/keystone/server/flask/common.py +++ b/keystone/server/flask/common.py @@ -733,6 +733,23 @@ class ResourceBase(flask_restful.Resource): container = {collection: refs} self_url = full_url(flask.request.environ['PATH_INFO']) container['links'] = {'next': None, 'self': self_url, 'previous': None} + # When pagination is enabled generate link to the next page + if ( + hints + and (hints.limit or hints.marker) + and len(refs) > 0 + # We do not know whether there are entries on the next page, + # let's just emit link to the next page if current page is full. + and list_limited + ): + args = flask.request.args.to_dict() + # Update the marker with the ID of last entry + args["marker"] = refs[-1]["id"] + args["limit"] = hints.limit.get("limit", args.get("limit")) + container["links"]["next"] = base_url( + flask.url_for(flask.request.endpoint, **args) + ) + if list_limited: container['truncated'] = True @@ -878,11 +895,14 @@ class ResourceBase(flask_restful.Resource): return flask.request.get_json(silent=True, force=True) or {} @staticmethod - def build_driver_hints(supported_filters): + def build_driver_hints( + supported_filters, default_limit: ty.Optional[int] = None + ): """Build list hints based on the context query string. :param supported_filters: list of filters supported, so ignore any keys in query_dict that are not in this list. + :param default_limit: default page size (PROVIDER._get_list_limit) """ hints = driver_hints.Hints() @@ -929,9 +949,20 @@ class ResourceBase(flask_restful.Resource): case_sensitive=case_sensitive, ) - # NOTE(henry-nash): If we were to support pagination, we would pull any - # pagination directives out of the query_dict here, and add them into - # the hints list. + marker = flask.request.args.get("marker") + if marker: + hints.set_marker(marker) + limit = flask.request.args.get("limit", default_limit) + # Set page limit as whatever requested in query parameters + # or whatever provider uses as a limit. In any way do not exceed the + # maximum page size limit. + # NOTE(gtema): It is important to set limit here (in the API flow) + # otherwise limited pagination is being applied also for internal + # invocations what is most likely not desired. + if limit: + # NOTE(gtema) remember limit is still conditional + hints.set_limit(min(int(limit), CONF.max_db_limit), False) + return hints @classmethod diff --git a/keystone/tests/unit/test_v3.py b/keystone/tests/unit/test_v3.py index daf82cec93..ee191b470d 100644 --- a/keystone/tests/unit/test_v3.py +++ b/keystone/tests/unit/test_v3.py @@ -12,8 +12,10 @@ # License for the specific language governing permissions and limitations # under the License. +import abc import datetime import http.client +import typing as ty import uuid import oslo_context.context @@ -1658,3 +1660,189 @@ class AssignmentTestMixin: ) return entity + + +class PaginationTestCaseBase(RestfulTestCase): + """Base test for the resource pagination.""" + + resource_name: ty.Optional[str] = None + + def setUp(self): + super().setUp() + if not self.resource_name: + self.skipTest("Not testing the base") + + @abc.abstractmethod + def _create_resources(self, count: int): + """Method creating given count of test resources + + This is an abstract method and must be implemented by every test + class inheriting from this base class. + """ + pass + + def test_list_requested_limit(self): + """Test requesting certain page size""" + count_resources: int = 6 + self._create_resources(count_resources) + + for expected_length in range(1, count_resources): + response = self.get( + f"/{self.resource_name}s?limit={expected_length}" + ) + res_list = response.json_body[f"{self.resource_name}s"] + res_links = response.json_body["links"] + self.assertEqual( + expected_length, + len(res_list), + "Requested count of entries returned", + ) + self.assertEqual( + f"http://localhost/v3/{self.resource_name}s?limit={expected_length}&marker={res_list[-1]['id']}", + res_links.get("next"), + "Next page link contains limit and marker", + ) + + # Try fetching with explicitly very high page size + response = self.get( + f"/{self.resource_name}s?limit={count_resources+100}" + ) + res_list = response.json_body[f"{self.resource_name}s"] + res_links = response.json_body["links"] + self.assertGreaterEqual( + len(res_list), count_resources, "All test resources are returned" + ) + self.assertIsNone(res_links.get("next"), "Next page link is empty") + + response = self.get(f"/{self.resource_name}s?limit=1&sort_key=name") + res_list = response.json_body[f"{self.resource_name}s"] + res_links = response.json_body["links"] + self.assertEqual( + f"http://localhost/v3/{self.resource_name}s?limit=1&sort_key=name&marker={res_list[-1]['id']}", + res_links.get("next"), + "Next page link contains limit and marker as well as other passed keys", + ) + + self.config_fixture.config(list_limit=2) + response = self.get(f"/{self.resource_name}s?limit=10") + res_list = response.json_body[f"{self.resource_name}s"] + self.assertGreaterEqual( + len(response.json_body[f"{self.resource_name}s"]), + count_resources, + "Requested limit higher then default wins", + ) + + def test_list_requested_limit_too_high(self): + """Test passing limit exceeding the system max""" + self.config_fixture.config(max_db_limit=4) + + count_resources: int = 6 + self._create_resources(count_resources) + + response = self.get(f"/{self.resource_name}s?limit={count_resources}") + res_list = response.json_body[f"{self.resource_name}s"] + res_links = response.json_body['links'] + self.assertEqual( + 4, + len(res_list), + "max_db_limit overrides requested count of entries", + ) + self.assertEqual( + f"http://localhost/v3/{self.resource_name}s?limit=4&marker={res_list[-1]['id']}", + res_links.get("next"), + "Next page link contains corrected limit and marker", + ) + + def test_list_default_limit(self): + """Tests listing resources without limit set""" + count_resources: int = 6 + self._create_resources(count_resources) + + self.config_fixture.config(list_limit=2) + + response = self.get(f'/{self.resource_name}s') + res_list = response.json_body[f"{self.resource_name}s"] + res_links = response.json_body['links'] + self.assertEqual(2, len(res_list), "default limit is applied") + self.assertEqual( + f"http://localhost/v3/{self.resource_name}s?marker={res_list[-1]['id']}&limit=2", + res_links.get("next"), + "Next page link contains corrected limit and marker", + ) + + self.config_fixture.config(group="resource", list_limit=3) + + response = self.get(f'/{self.resource_name}s') + res_list = response.json_body[f"{self.resource_name}s"] + res_links = response.json_body['links'] + self.assertEqual(3, len(res_list), "default resource limit is applied") + self.assertEqual( + f"http://localhost/v3/{self.resource_name}s?marker={res_list[-1]['id']}&limit=3", + res_links.get("next"), + "Next page link contains corrected limit and marker", + ) + + def _consume_paginated_list( + self, limit: ty.Optional[int] = None + ) -> tuple[list[dict], int]: + """Fetch all paginated resources""" + found_resources: list = [] + pages: int = 0 + if limit: + response = self.get(f"/{self.resource_name}s?limit={limit}") + else: + response = self.get(f"/{self.resource_name}s") + pages = 1 + res_links = response.json_body['links'] + while "next" in res_links: + # Put fetched resources into the found list + found_resources.extend( + response.json_body.get(f"{self.resource_name}s") + ) + + # Get the relative url of the next page (since testhelper only supports that) + next_url = res_links.get("next", "") + if next_url: + next_rel_url = next_url[next_url.find("/v3") + 3 :] + # fetch next page + response = self.get(next_rel_url) + res_links = response.json_body['links'] + pages += 1 + else: + break + return (found_resources, pages) + + def test_list_consume_all_resources(self): + """Test paginating through all resources (simulate user SDK)""" + count_resources: int = 50 + page_size: int = 5 + self._create_resources(count_resources) + response = self.get(f"/{self.resource_name}s") + 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) + + (found_resources, pages) = self._consume_paginated_list() + self.assertGreaterEqual( + len(found_resources), + count_resources, + "Fetched at least all pre-created resources", + ) + self.assertGreaterEqual( + pages, + current_count // page_size, + "At least as many pages consumed as expected", + ) + + (found_resources, pages) = self._consume_paginated_list(limit=100) + self.assertGreaterEqual( + len(found_resources), + count_resources, + "Fetched at least all pre-created resources", + ) + self.assertGreaterEqual( + pages, + current_count // 100, + "At least as many pages consumed as expected", + ) diff --git a/keystone/tests/unit/test_v3_resource.py b/keystone/tests/unit/test_v3_resource.py index cfd91dbe0b..903e7d9e09 100644 --- a/keystone/tests/unit/test_v3_resource.py +++ b/keystone/tests/unit/test_v3_resource.py @@ -1011,7 +1011,8 @@ class ResourceTestCase(test_v3.RestfulTestCase, test_v3.AssignmentTestMixin): expected_list = [projects[1]['project']] # projects[0] has projects[1] as child - self.assertEqual(expected_list, projects_result) + for project in expected_list: + self.assertIn(project, projects_result) # Query for projects[1] immediate children - it will # be projects[2] and projects[3] @@ -1026,7 +1027,8 @@ class ResourceTestCase(test_v3.RestfulTestCase, test_v3.AssignmentTestMixin): expected_list = [projects[2]['project'], projects[3]['project']] # projects[1] has projects[2] and projects[3] as children - self.assertEqual(expected_list, projects_result) + for project in expected_list: + self.assertIn(project, projects_result) # Query for projects[2] immediate children - it will be an empty list r = self.get( @@ -1040,7 +1042,8 @@ class ResourceTestCase(test_v3.RestfulTestCase, test_v3.AssignmentTestMixin): expected_list = [] # projects[2] has no child, projects_result must be an empty list - self.assertEqual(expected_list, projects_result) + for project in expected_list: + self.assertIn(project, projects_result) def test_create_hierarchical_project(self): """Call ``POST /projects``.""" @@ -2045,3 +2048,25 @@ class StrictTwoLevelLimitsResourceTestCase(ResourceTestCase): body={'project': new_ref}, expected_status=http.client.FORBIDDEN, ) + + +class DomainPaginationTestCase(test_v3.PaginationTestCaseBase): + """Test domain list pagination.""" + + resource_name: str = "domain" + + def _create_resources(self, count: int): + for x in range(count): + res = {"domain": unit.new_domain_ref()} + response = self.post("/domains", body=res) + + +class ProjectPaginationTestCase(test_v3.PaginationTestCaseBase): + """Test project list pagination.""" + + resource_name: str = "project" + + def _create_resources(self, count: int): + for x in range(count): + res = {"project": unit.new_project_ref()} + response = self.post("/projects", body=res) diff --git a/releasenotes/notes/pagination-projects-ea311579da4bb83b.yaml b/releasenotes/notes/pagination-projects-ea311579da4bb83b.yaml new file mode 100644 index 0000000000..da504d2bab --- /dev/null +++ b/releasenotes/notes/pagination-projects-ea311579da4bb83b.yaml @@ -0,0 +1,12 @@ +--- +features: + - | + New configuration variable `max_db_limit` is added to set an absolute limit + amount of entries fetched from the database at a single time. It is used in + resource pagination. Existing option `list_limit` is optional and describes + preferred count of entries while `max_db_limit` sets top limit applied to + user input and individual `list_limit` options. + - | + Project and domain listing supports pagination. Query parameters `limit` + and `marker` are added and work as described in `API-SIG doc + `_