Add support for number-based pagination (#1139)

* Added support for page_number and adjusted the behaviour of conftest in the case of paging so ids of documents that are not returned on this page do not have to be specified.

* Also sort response_ids in check_response, so comparissons also work when the sorting takes place on a field other than id.

* Apply suggestions from code review

* Added check so that page_offset and page_number cannot both be set at the same time.

* Throw a warning instead of an error when multiple pagination parameters are supplied.

* Corrected warning for when both the page_number and the page_offset parameters are set.

* Tweak for single structure testing with `check_response`

- If a single ID is passed as a string, validate the request as a single
entry request
- If a single ID is passed within a list, validate it is a multi-entry
request that only returns one result

Co-authored-by: Matthew Evans <git@ml-evs.science>

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

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

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
 

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,

tests/server/conftest.py

@@ -102,38 +102,49 @@ 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,
         expected_warnings: List[Dict[str, str]] = None,
         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"]

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
+    )

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,

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)