Add pagination to /bookmarks (#602)

* Fix issues with version marker

* Add pagination to listing `bookmarks`

Author: PGijsbers

.pre-commit-config.yaml

@@ -40,7 +40,7 @@ repos:
     hooks:
       - id: pytest-check
         name: pytest-check
-        entry: pytest src/tests --versions ''
+        entry: pytest src/tests --versions 'latest'
         language: system
         pass_filenames: false
         exclude: ".*.md"

pyproject.toml

@@ -84,3 +84,4 @@ env_files = [
     ".env",  # Only used if running from docker container
     "tests/.env"  # Only used if running from docker container
 ]
+markers=["versions"]

src/routers/bookmark_router.py

@@ -8,6 +8,8 @@
 from database.model.bookmark.bookmark import Bookmark
 from http import HTTPStatus
 from datetime import datetime
+
+from dependencies.pagination import PaginationParams
 from routers.helper_functions import get_asset_type_by_abbreviation
 from versioning import Version
 
@@ -33,11 +35,17 @@ def create(url_prefix: str = "", version: Version = Version.LATEST) -> APIRouter
         response_model=List[BookmarkRead],
     )
     def list_bookmarks(
-        user: KeycloakUser = Depends(get_user_or_raise), session: Session = Depends(get_session)
+        pagination: PaginationParams,
+        user: KeycloakUser = Depends(get_user_or_raise),
+        session: Session = Depends(get_session),
     ) -> List[BookmarkRead]:
-        return session.exec(
-            select(Bookmark).where(Bookmark.user_identifier == user._subject_identifier)
-        ).all()
+        stmt = (
+            select(Bookmark)
+            .where(Bookmark.user_identifier == user._subject_identifier)
+            .offset(pagination.offset)
+            .limit(pagination.limit)
+        )
+        return session.exec(stmt).all()
 
     @router.post(
         path,

src/tests/conftest.py

@@ -15,12 +15,10 @@ def pytest_generate_tests(metafunc):
         selected_versions = version_marker.args if version_marker else default_versions
 
         # If version is set through the command line, only use that for testing.
-        all_versions = list(Version)
-        versions_to_include = metafunc.config.getoption("versions") or all_versions
+        versions_to_include = list(Version)
+        if cmd_line_versions := metafunc.config.getoption("versions"):
+            versions_to_include = [Version(v) for v in cmd_line_versions]
         versions = set(selected_versions).intersection(set(versions_to_include))
-        if not versions:
-            pytest.skip(f"Test not defined for selection: {versions_to_include}")
-
         metafunc.parametrize(
             "client", versions, indirect=True
         )

src/tests/test_bookmark_endpoints.py

@@ -1,5 +1,6 @@
 from http import HTTPStatus
 
+import pytest
 from starlette.testclient import TestClient
 from database.session import DbSession
 
@@ -10,6 +11,7 @@
 from datetime import datetime
 from database.model.agent.person import Person
 from database.model.agent.contact import Contact
+from versioning import Version
 
 
 def test_create_bookmark(
@@ -140,3 +142,45 @@ def test_delete_bookmark(
         )
     assert response.status_code == HTTPStatus.OK
     assert all(b["resource_identifier"] != identifier for b in response.json())
+
+
+@pytest.mark.versions(Version.V2, Version.LATEST)
+def test_get_bookmark_pagination(client: TestClient, publication_factory) -> None:
+    PAGINATION_DEFAULT_LIMIT = 10
+    publications = [publication_factory() for _ in range(PAGINATION_DEFAULT_LIMIT + 1)]
+    with logged_in_user(ALICE):
+        for pub in publications:
+            register_asset(pub, owner=ALICE)
+            response = client.post(
+                f"/bookmarks?resource_identifier={pub.identifier}",
+                headers={"Authorization": "fake token"},
+            )
+            assert response.status_code == HTTPStatus.OK
+
+        response = client.get(
+            "/bookmarks",
+            headers={"Authorization": "fake token"},
+        )
+        assert response.status_code == HTTPStatus.OK
+        assert len(response.json()) == PAGINATION_DEFAULT_LIMIT
+
+        response = client.get(
+            "/bookmarks?limit=100",
+            headers={"Authorization": "fake token"},
+        )
+        assert response.status_code == HTTPStatus.OK
+        assert len(response.json()) == PAGINATION_DEFAULT_LIMIT + 1
+
+        response = client.get(
+            "/bookmarks?offset=10",
+            headers={"Authorization": "fake token"},
+        )
+        assert response.status_code == HTTPStatus.OK
+        assert len(response.json()) == 1
+
+        response = client.get(
+            "/bookmarks?offset=8&limit=2",
+            headers={"Authorization": "fake token"},
+        )
+        assert response.status_code == HTTPStatus.OK
+        assert len(response.json()) == 2

src/tests/testutils/default_sqlalchemy.py

@@ -152,7 +152,7 @@ def client(request, engine: Engine) -> TestClient:
     app = build_app(version="unittest")
     client = TestClient(
         app,
-        base_url=f"http://localhost/{request.param.prefix}",  # param is Version
+        base_url=f"http://localhost{request.param.prefix}/",  # param is Version
     )
     client.version = request.param
     yield client