diff --git a/openapi/index_openapi.json b/openapi/index_openapi.json index 7927ddb3d..73203b7f7 100644 --- a/openapi/index_openapi.json +++ b/openapi/index_openapi.json @@ -199,7 +199,7 @@ "required": false, "schema": { "title": "Page Number", - "minimum": 0.0, + "minimum": 1.0, "type": "integer", "description": "RECOMMENDED for use with _page-based_ pagination: using `page_number` and `page_limit` is RECOMMENDED.\nIt is RECOMMENDED that the first page has number 1, i.e., that `page_number` is 1-based.\nExample: Fetch page 2 of up to 50 structures per page: `/structures?page_number=2&page_limit=50`.", "default": 0 diff --git a/openapi/openapi.json b/openapi/openapi.json index f6335d970..e3d45ed87 100644 --- a/openapi/openapi.json +++ b/openapi/openapi.json @@ -301,7 +301,7 @@ "required": false, "schema": { "title": "Page Number", - "minimum": 0.0, + "minimum": 1.0, "type": "integer", "description": "RECOMMENDED for use with _page-based_ pagination: using `page_number` and `page_limit` is RECOMMENDED.\nIt is RECOMMENDED that the first page has number 1, i.e., that `page_number` is 1-based.\nExample: Fetch page 2 of up to 50 structures per page: `/structures?page_number=2&page_limit=50`.", "default": 0 @@ -560,7 +560,7 @@ "required": false, "schema": { "title": "Page Number", - "minimum": 0.0, + "minimum": 1.0, "type": "integer", "description": "RECOMMENDED for use with _page-based_ pagination: using `page_number` and `page_limit` is RECOMMENDED.\nIt is RECOMMENDED that the first page has number 1, i.e., that `page_number` is 1-based.\nExample: Fetch page 2 of up to 50 structures per page: `/structures?page_number=2&page_limit=50`.", "default": 0 @@ -984,7 +984,7 @@ "required": false, "schema": { "title": "Page Number", - "minimum": 0.0, + "minimum": 1.0, "type": "integer", "description": "RECOMMENDED for use with _page-based_ pagination: using `page_number` and `page_limit` is RECOMMENDED.\nIt is RECOMMENDED that the first page has number 1, i.e., that `page_number` is 1-based.\nExample: Fetch page 2 of up to 50 structures per page: `/structures?page_number=2&page_limit=50`.", "default": 0 diff --git a/optimade/server/entry_collections/entry_collections.py b/optimade/server/entry_collections/entry_collections.py index 5c89c0116..27e59b1c6 100644 --- a/optimade/server/entry_collections/entry_collections.py +++ b/optimade/server/entry_collections/entry_collections.py @@ -11,7 +11,11 @@ from optimade.server.exceptions import BadRequest, Forbidden, NotFound from optimade.server.mappers import BaseResourceMapper from optimade.server.query_params import EntryListingQueryParams, SingleEntryQueryParams -from optimade.server.warnings import FieldValueNotRecognized, UnknownProviderProperty +from optimade.server.warnings import ( + FieldValueNotRecognized, + UnknownProviderProperty, + QueryParamNotUsed, +) def create_collection( @@ -331,9 +335,20 @@ def handle_query_params( if getattr(params, "sort", False): cursor_kwargs["sort"] = self.parse_sort_params(params.sort) - # page_offset + # page_offset and page_number if getattr(params, "page_offset", False): + if getattr(params, "page_number", False): + warnings.warn( + message="Only one of the query parameters 'page_number' and 'page_offset' should be set - 'page_number' will be ignored.", + category=QueryParamNotUsed, + ) + cursor_kwargs["skip"] = params.page_offset + elif getattr(params, "page_number", False): + if isinstance(params.page_number, int): + cursor_kwargs["skip"] = (params.page_number - 1) * cursor_kwargs[ + "limit" + ] return cursor_kwargs diff --git a/optimade/server/query_params.py b/optimade/server/query_params.py index eb4e5e60a..b238be9b7 100644 --- a/optimade/server/query_params.py +++ b/optimade/server/query_params.py @@ -172,7 +172,6 @@ class EntryListingQueryParams(BaseQueryParams): # The reference server implementation only supports offset-based pagination unsupported_params: List[str] = [ - "page_number", "page_cursor", "page_below", "page_above", @@ -216,7 +215,7 @@ def __init__( page_number: int = Query( 0, description="RECOMMENDED for use with _page-based_ pagination: using `page_number` and `page_limit` is RECOMMENDED.\nIt is RECOMMENDED that the first page has number 1, i.e., that `page_number` is 1-based.\nExample: Fetch page 2 of up to 50 structures per page: `/structures?page_number=2&page_limit=50`.", - ge=0, + ge=1, ), page_cursor: int = Query( 0, diff --git a/tests/server/conftest.py b/tests/server/conftest.py index 2268e4672..04c525c58 100644 --- a/tests/server/conftest.py +++ b/tests/server/conftest.py @@ -102,14 +102,26 @@ def inner( @pytest.fixture def check_response(get_good_response): - """Fixture to check "good" response""" + """Check response matches expectations for a given request. + + Parameters: + request: The request to check. + expected_ids: A list of IDs, or a single ID to check + the response for. + page_limit: The number of results expected per page. + expected_return: The number of results expected to be returned. + expected_as_is: Whether to enforce the order of the IDs. + expected_warnings: A list of expected warning messages. + server: The type of server to test, or the actual test client class. + + """ from typing import List from optimade.server.config import CONFIG from .utils import OptimadeTestClient def inner( request: str, - expected_ids: List[str], + expected_ids: Union[str, List[str]], page_limit: int = CONFIG.page_limit, expected_return: int = None, expected_as_is: bool = False, @@ -117,23 +129,22 @@ def inner( server: Union[str, OptimadeTestClient] = "regular", ): response = get_good_response(request, server) - if isinstance(response["data"], dict): - response_ids = [response["data"]["id"]] - else: - response_ids = [struct["id"] for struct in response["data"]] + if isinstance(expected_ids, str): + expected_ids = [expected_ids] + response["data"] = [response["data"]] - if expected_return is None: - expected_return = len(expected_ids) + response_ids = [struct["id"] for struct in response["data"]] - assert response["meta"]["data_returned"] == expected_return + if expected_return is not None: + assert expected_return == response["meta"]["data_returned"] + + assert len(response["data"]) == len(expected_ids) if not expected_as_is: expected_ids = sorted(expected_ids) + response_ids = sorted(response_ids) - if len(expected_ids) > page_limit: - assert expected_ids[:page_limit] == response_ids - else: - assert expected_ids == response_ids + assert expected_ids == response_ids if expected_warnings: assert "warnings" in response["meta"] diff --git a/tests/server/middleware/test_query_param.py b/tests/server/middleware/test_query_param.py index 8e49011ea..e2f6d4ec5 100644 --- a/tests/server/middleware/test_query_param.py +++ b/tests/server/middleware/test_query_param.py @@ -107,19 +107,19 @@ def test_handling_prefixed_query_param(check_response): def test_unsupported_optimade_query_param(check_response): - request = "/structures?filter=elements LENGTH >= 9&page_number=1" + request = "/structures?filter=elements LENGTH >= 9&page_below=1" expected_ids = ["mpf_3819"] expected_warnings = [ { "title": "QueryParamNotUsed", - "detail": "The query parameter(s) '['page_number']' are not supported by this server and have been ignored.", + "detail": "The query parameter(s) '['page_below']' are not supported by this server and have been ignored.", } ] check_response( request, expected_ids=expected_ids, expected_warnings=expected_warnings ) - request = "/structures?filter=elements LENGTH >= 9&page_number=1&_unknown_filter=elements HAS 'Si'" + request = "/structures?filter=elements LENGTH >= 9&page_cursor=1&_unknown_filter=elements HAS 'Si'" expected_ids = ["mpf_3819"] expected_warnings = [ { @@ -128,9 +128,36 @@ def test_unsupported_optimade_query_param(check_response): }, { "title": "QueryParamNotUsed", - "detail": "The query parameter(s) '['page_number']' are not supported by this server and have been ignored.", + "detail": "The query parameter(s) '['page_cursor']' are not supported by this server and have been ignored.", }, ] check_response( request, expected_ids=expected_ids, expected_warnings=expected_warnings ) + + +def test_page_number_and_offset(check_response): + request = "/structures?sort=id&page_offset=5&page_limit=5" + expected_ids = ["mpf_23", "mpf_259", "mpf_272", "mpf_276", "mpf_281"] + check_response(request, expected_ids=expected_ids) + + request = "/structures?sort=id&page_number=2&page_limit=5" + check_response(request, expected_ids=expected_ids) + + request = "/structures?sort=last_modified&page_number=2&page_limit=5" + expected_ids = ["mpf_30", "mpf_110", "mpf_200", "mpf_220", "mpf_259"] + check_response(request, expected_ids=expected_ids) + + +def test_page_number_and_offset_both_set(check_response): + request = "/structures?sort=last_modified&page_number=2&page_limit=5&page_offset=5" + expected_ids = ["mpf_30", "mpf_110", "mpf_200", "mpf_220", "mpf_259"] + expected_warnings = [ + { + "title": "QueryParamNotUsed", + "detail": "Only one of the query parameters 'page_number' and 'page_offset' should be set - 'page_number' will be ignored.", + } + ] + check_response( + request, expected_ids=expected_ids, expected_warnings=expected_warnings + ) diff --git a/tests/server/query_params/test_sort.py b/tests/server/query_params/test_sort.py index 726ac16fd..920aafa33 100644 --- a/tests/server/query_params/test_sort.py +++ b/tests/server/query_params/test_sort.py @@ -36,7 +36,7 @@ def test_str_asc(check_response, structures): limit = 5 request = f"/structures?sort=id&page_limit={limit}" - expected_ids = sorted([doc["task_id"] for doc in structures]) + expected_ids = sorted([doc["task_id"] for doc in structures])[:limit] check_response( request, expected_ids=expected_ids, @@ -49,7 +49,7 @@ def test_str_desc(check_response, structures): limit = 5 request = f"/structures?sort=-id&page_limit={limit}" - expected_ids = sorted([doc["task_id"] for doc in structures], reverse=True) + expected_ids = sorted([doc["task_id"] for doc in structures], reverse=True)[:limit] check_response( request, expected_ids=expected_ids, diff --git a/tests/server/routers/test_structures.py b/tests/server/routers/test_structures.py index b2d9a368c..c83dd4a20 100644 --- a/tests/server/routers/test_structures.py +++ b/tests/server/routers/test_structures.py @@ -73,7 +73,7 @@ def test_check_response_single_structure(check_response): """Tests whether check_response also handles single endpoint queries correctly.""" test_id = "mpf_1" - expected_ids = ["mpf_1"] + expected_ids = "mpf_1" request = f"/structures/{test_id}?response_fields=chemical_formula_reduced" check_response(request, expected_ids=expected_ids)