diff --git a/application/database/db.py b/application/database/db.py index 522612161..2ba28d271 100644 --- a/application/database/db.py +++ b/application/database/db.py @@ -2747,10 +2747,10 @@ def _expand_text_search_aliases(self, text: str) -> List[str]: deduped.append(entry) return deduped - def get_root_cres(self): - """Returns CRES that only have "Contains" links""" + def _root_cres_query(self): + """CREs that no other CRE contains: the tops of the graph.""" # select name from cre where cre.id not in (select cre from cre_links where type="Contains") and cre.id not in (select "group" from cre_links where type="Is Part OF"); - cres = ( + return ( self.session.query(CRE) .filter( ~CRE.id.in_( @@ -2766,10 +2766,34 @@ def get_root_cres(self): ) ) ) - .all() + # Without an explicit order the database is free to return rows in + # any order, so page boundaries could shift between requests and a + # CRE could appear on two pages while another was never returned. + # external_id first because that is the order the catalogue reads + # in; id breaks ties and is the primary key, so the order is total. + .order_by(CRE.external_id, CRE.id) ) + + def get_root_cres(self): + """Returns CRES that only have "Contains" links""" + cres = self._root_cres_query().all() return self._hydrate_cres_batch(list(cres)) + def get_root_cres_with_pagination( + self, page: int = 1, per_page: int = 10 + ) -> Tuple[List[cre_defs.CRE], int, int]: + """One page of root CREs, in the order get_root_cres returns them. + + Mirrors all_cres_with_pagination: a page past the end is empty rather + than an error, and total_pages is always reported. + """ + cres = self._root_cres_query().paginate( + page=int(page), per_page=per_page, error_out=False + ) + total_pages = cres.pages + result = self._hydrate_cres_batch(list(cres.items)) + return result, page, total_pages + def health_check(self) -> Dict[str, Any]: """Lightweight liveness/readiness probe for the serving database. diff --git a/application/tests/db_test.py b/application/tests/db_test.py index c3979753d..38b8c5b50 100644 --- a/application/tests/db_test.py +++ b/application/tests/db_test.py @@ -1324,6 +1324,43 @@ def test_get_root_cres(self): self.maxDiff = None self.assertCountEqual(root_cres, [cres[0], cres[1], cres[7]]) + def test_get_root_cres_with_pagination(self): + """Pages carry only root CREs, in the same order as get_root_cres, + and a page past the end is empty rather than an error.""" + sqla.session.remove() + sqla.drop_all() + sqla.create_all() + collection = db.Node_collection().with_graph() + + dbcres = [] + for i in range(7): + dbcres.append( + collection.add_cre(defs.CRE(name=f"C{i}", id=f"{i}{i}{i}-{i}{i}{i}")) + ) + # C5 is contained by C0 and C6 is contained by C1: neither is a root. + collection.add_internal_link( + higher=dbcres[0], lower=dbcres[5], ltype=defs.LinkTypes.Contains + ) + collection.add_internal_link( + higher=dbcres[1], lower=dbcres[6], ltype=defs.LinkTypes.Contains + ) + collection.session.commit() + + all_roots = collection.get_root_cres() + self.assertEqual(5, len(all_roots)) + + page1, page, total_pages = collection.get_root_cres_with_pagination(1, 2) + self.assertEqual((1, 3), (page, total_pages)) + self.assertEqual(all_roots[0:2], page1) + + page3, page, total_pages = collection.get_root_cres_with_pagination(3, 2) + self.assertEqual((3, 3), (page, total_pages)) + self.assertEqual(all_roots[4:5], page3) + + page4, page, total_pages = collection.get_root_cres_with_pagination(4, 2) + self.assertEqual([], page4) + self.assertEqual(3, total_pages) + @patch.object(db.NEO_DB, "gap_analysis") def test_gap_analysis_disconnected(self, gap_mock): collection = db.Node_collection() diff --git a/application/tests/web_main_test.py b/application/tests/web_main_test.py index 3d183e1b8..3fe17bfdc 100644 --- a/application/tests/web_main_test.py +++ b/application/tests/web_main_test.py @@ -526,6 +526,186 @@ def test_find_root_cres(self) -> None: self.assertEqual(json.loads(response.data.decode()), expected) self.assertEqual(200, response.status_code) + @patch.object(db, "Node_collection") + def test_find_root_cres_paginates_only_when_asked(self, db_mock) -> None: + """Without page/per_page the endpoint keeps its original shape: every + root CRE and no pagination keys. The CLI import, the Explorer tree and + the MCP tool all read the whole list, so that path must not change.""" + cres = [defs.CRE(name=f"root{i}", id=f"{i}{i}{i}-{i}{i}{i}") for i in range(3)] + db_mock.return_value.get_root_cres.return_value = cres + + with self.app.test_client() as client: + response = client.get( + "/rest/v1/root_cres", + headers={"Content-Type": "application/json"}, + ) + self.assertEqual(200, response.status_code) + self.assertEqual( + {"data": [c.todict() for c in cres]}, json.loads(response.data) + ) + db_mock.return_value.get_root_cres.assert_called_once_with() + db_mock.return_value.get_root_cres_with_pagination.assert_not_called() + + @patch.object(db, "Node_collection") + def test_find_root_cres_caps_per_page(self, db_mock) -> None: + db_mock.return_value.get_root_cres_with_pagination.return_value = ([], 1, 1) + + with self.app.test_client() as client: + client.get( + "/rest/v1/root_cres?per_page=1000", + headers={"Content-Type": "application/json"}, + ) + + db_mock.return_value.get_root_cres_with_pagination.assert_called_once_with( + 1, 100 + ) + db_mock.return_value.get_root_cres.assert_not_called() + + def test_find_root_cres_pagination_integration(self) -> None: + """root_cres pages through root CREs against a real database, with the + same metadata all_cres returns, and non-root CREs never appear.""" + collection = db.Node_collection().with_graph() + roots = [] + for i in range(25): + roots.append( + collection.add_cre(defs.CRE(name=f"root-{i}", id=f"{i:03d}-{i:03d}")) + ) + # A child of the first root: reachable through a Contains link, so it + # is not a root and must be excluded from every page. + child = collection.add_cre(defs.CRE(name="child", id="999-999")) + collection.add_internal_link( + higher=roots[0], lower=child, ltype=defs.LinkTypes.Contains + ) + collection.session.commit() + + with self.app.test_client() as client: + response = client.get( + "/rest/v1/root_cres?per_page=10&page=1", + headers={"Content-Type": "application/json"}, + ) + self.assertEqual(200, response.status_code) + body = json.loads(response.data) + self.assertEqual(10, len(body["data"])) + self.assertEqual(1, body["page"]) + self.assertEqual(3, body["total_pages"]) + + response = client.get( + "/rest/v1/root_cres?per_page=10&page=3", + headers={"Content-Type": "application/json"}, + ) + self.assertEqual(200, response.status_code) + body = json.loads(response.data) + self.assertEqual(5, len(body["data"])) + self.assertEqual(3, body["page"]) + self.assertEqual(3, body["total_pages"]) + + seen = set() + for page in (1, 2, 3): + response = client.get( + f"/rest/v1/root_cres?per_page=10&page={page}", + headers={"Content-Type": "application/json"}, + ) + seen.update(doc["id"] for doc in json.loads(response.data)["data"]) + self.assertEqual(25, len(seen)) + self.assertNotIn("999-999", seen) + + # Only `page` given: per_page falls back to the default page size. + response = client.get( + "/rest/v1/root_cres?page=1", + headers={"Content-Type": "application/json"}, + ) + body = json.loads(response.data) + self.assertEqual(web_main.ITEMS_PER_PAGE, len(body["data"])) + self.assertEqual(2, body["total_pages"]) + + # A page past the end is an empty page, not everything. + response = client.get( + "/rest/v1/root_cres?per_page=10&page=4", + headers={"Content-Type": "application/json"}, + ) + self.assertEqual(404, response.status_code) + + def test_find_root_cres_orders_duplicate_external_ids_by_primary_key(self) -> None: + """external_id is not unique on its own: the constraint is on + (name, external_id), so two root CREs may carry the same one. Ordering + by external_id alone would leave those ties undefined, which is what the + CRE.id half of _root_cres_query's sort is for. The primary keys are + fixed here rather than generated, so the expected order cannot drift.""" + collection = db.Node_collection().with_graph() + # Inserted in the opposite order to their primary keys. + for pk, name in ( + ("bbbbbbbb-0000-0000-0000-000000000000", "tie-second"), + ("aaaaaaaa-0000-0000-0000-000000000000", "tie-first"), + ): + collection.session.add( + db.CRE(id=pk, external_id="500-500", name=name, description="") + ) + collection.session.commit() + + with self.app.test_client() as client: + response = client.get( + "/rest/v1/root_cres?per_page=10&page=1", + headers={"Content-Type": "application/json"}, + ) + self.assertEqual(200, response.status_code) + names = [doc["name"] for doc in json.loads(response.data)["data"]] + + self.assertEqual(["tie-first", "tie-second"], names) + + def test_find_root_cres_rejects_malformed_pagination(self) -> None: + """A non-integer page or per_page is a client error, not a 500.""" + collection = db.Node_collection().with_graph() + collection.add_cre(defs.CRE(name="root", id="111-111")) + collection.session.commit() + + with self.app.test_client() as client: + for query in ("page=abc", "per_page=abc", "page=1.5"): + response = client.get( + f"/rest/v1/root_cres?{query}", + headers={"Content-Type": "application/json"}, + ) + self.assertEqual(400, response.status_code, query) + + def test_find_root_cres_pages_deterministically(self) -> None: + """Paging twice returns the same rows in the same places. Without an + ORDER BY the database may return rows in any order, which lets a CRE + appear on two pages while another is never returned at all.""" + collection = db.Node_collection().with_graph() + # Insert in an order that is deliberately not the expected one. SQLite + # hands back a small table in insertion order, so a fixture inserted + # already-sorted would pass even with the ORDER BY removed. + external_ids = [f"{i:03d}-{i:03d}" for i in range(30)] + for external_id in reversed(external_ids): + collection.add_cre(defs.CRE(name=f"root-{external_id}", id=external_id)) + collection.session.commit() + + with self.app.test_client() as client: + + def ids(page: int) -> list: + response = client.get( + f"/rest/v1/root_cres?per_page=10&page={page}", + headers={"Content-Type": "application/json"}, + ) + return [doc["id"] for doc in json.loads(response.data)["data"]] + + first = [ids(p) for p in (1, 2, 3)] + second = [ids(p) for p in (1, 2, 3)] + self.assertEqual(first, second) + + flat = [cre_id for page in first for cre_id in page] + self.assertEqual(30, len(set(flat)), "a CRE appeared on two pages") + # Inserted descending, returned ascending: this is what fails if the + # ORDER BY goes away. + self.assertEqual(external_ids, flat) + + # The paginated pages are exactly the unpaginated list, in order. + response = client.get( + "/rest/v1/root_cres", headers={"Content-Type": "application/json"} + ) + self.assertEqual( + [doc["id"] for doc in json.loads(response.data)["data"]], flat + ) + def test_smartlink(self) -> None: self.maxDiff = None collection = db.Node_collection().with_graph() diff --git a/application/web/openapi_registry.py b/application/web/openapi_registry.py index 4d2677e44..708d98491 100644 --- a/application/web/openapi_registry.py +++ b/application/web/openapi_registry.py @@ -281,9 +281,14 @@ def __init__( "/rest/v1/root_cres", "find_root_cres", tags=["CRE"], - summary="Get root CREs", - query_schema=schemas.FormatQuerySchema, - response_schema=schemas.DataListResponseSchema, + summary="Get root CREs (optionally paginated)", + description=( + "Returns every root CRE by default. Pass page and/or per_page to " + "page through them; the response then also carries page and " + "total_pages, as /rest/v1/all_cres does." + ), + query_schema=schemas.RootCREsQuerySchema, + response_schema=schemas.RootCREsResponseSchema, ), PathSpec( "/rest/v1/deeplink/{name}", diff --git a/application/web/openapi_schemas.py b/application/web/openapi_schemas.py index 7a77aa505..6aeeb4d4d 100644 --- a/application/web/openapi_schemas.py +++ b/application/web/openapi_schemas.py @@ -108,6 +108,27 @@ class HealthResponseSchema(Schema): message = fields.Str(required=False) +class RootCREsQuerySchema(FormatQuerySchema): + page = fields.Int( + required=False, + metadata={ + "description": "Page number. Supplying page or per_page switches " + "the response to paginated form; without either, every root CRE " + "is returned." + }, + ) + per_page = fields.Int( + required=False, + metadata={"description": "Items per page (default 20, maximum 100)"}, + ) + + +class RootCREsResponseSchema(Schema): + data = fields.List(fields.Dict(keys=fields.Str(), values=fields.Raw())) + page = fields.Int(required=False) + total_pages = fields.Int(required=False) + + class AllCREsQuerySchema(Schema): page = fields.Int(required=False) per_page = fields.Int(required=False) diff --git a/application/web/web_main.py b/application/web/web_main.py index a40b5fa7d..05e77f085 100644 --- a/application/web/web_main.py +++ b/application/web/web_main.py @@ -10,7 +10,7 @@ import pathlib import re import urllib.parse -from typing import Any +from typing import Any, Dict from rq import job, exceptions from rq import Queue @@ -670,10 +670,47 @@ def find_root_cres() -> Any: database = db.Node_collection() # opt_osib = request.args.get("osib") opt_format = request.args.get("format") - documents = database.get_root_cres() + + # Pagination is opt-in. The CLI import (cre_main.download_cre_from_upstream), + # the Explorer tree and the MCP tool all read this endpoint as the complete + # list of roots, so without page/per_page the response keeps its original + # shape. With either present it pages like /rest/v1/all_cres does. + paginated = ( + request.args.get("page") is not None or request.args.get("per_page") is not None + ) + result: Dict[str, Any] + if paginated: + page = 1 + per_page = ITEMS_PER_PAGE + # Parse once: int() on a non-integer raises ValueError, which has no + # handler and would surface as a 500 for what is a client mistake. + # A value of zero or less falls back to the default, as in all_cres. + try: + if request.args.get("page") is not None: + requested_page = int(request.args.get("page")) + if requested_page > 0: + page = requested_page + + if request.args.get("per_page") is not None: + requested_per_page = int(request.args.get("per_page")) + if requested_per_page > 0: + per_page = requested_per_page + except ValueError: + abort(400, "page and per_page must be integers") + per_page = min(per_page, MAX_ITEMS_PER_PAGE) + + documents, page, total_pages = database.get_root_cres_with_pagination( + page, per_page + ) + else: + documents = database.get_root_cres() + if documents: res = [doc.todict() for doc in documents] result = {"data": res} + if paginated: + result["page"] = page + result["total_pages"] = total_pages # if opt_osib: # result["osib"] = odefs.cre2osib(documents).todict() if opt_format == SupportedFormats.Markdown.value: diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 3708aa0f4..303c49461 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -313,7 +313,10 @@ paths: get: tags: - CRE - summary: Get root CREs + summary: Get root CREs (optionally paginated) + description: Returns every root CRE by default. Pass page and/or per_page to + page through them; the response then also carries page and total_pages, as + /rest/v1/all_cres does. parameters: - name: format in: query @@ -326,13 +329,26 @@ paths: - csv - oscal description: 'Response format (default: json)' + - name: page + in: query + required: false + schema: + type: integer + description: Page number. Supplying page or per_page switches the response + to paginated form; without either, every root CRE is returned. + - name: per_page + in: query + required: false + schema: + type: integer + description: Items per page (default 20, maximum 100) responses: '200': description: Success content: application/json: schema: - $ref: '#/components/schemas/DataListResponseSchema' + $ref: '#/components/schemas/RootCREsResponseSchema' '404': description: Not found /rest/v1/standard/{name}: @@ -1033,6 +1049,19 @@ components: message: type: string additionalProperties: false + RootCREsResponseSchema: + type: object + properties: + data: + type: array + items: + type: object + additionalProperties: {} + page: + type: integer + total_pages: + type: integer + additionalProperties: false AllCREsResponseSchema: type: object properties: