From 462e83b7cf1e7f605f78559665ca71aca450e577 Mon Sep 17 00:00:00 2001 From: Kunal Kumar Date: Tue, 8 Sep 2026 06:43:47 +0530 Subject: [PATCH 1/4] feat(api): paginate /rest/v1/root_cres on request (#847) /rest/v1/root_cres returned every root CRE in one unbounded response and had no way to page through them. Pagination is opt-in. The CLI import (cre_main.download_cre_from_upstream), the Explorer tree (DataProvider.rebuildDataTree), the Browse page and the list_root_cres MCP tool all read this endpoint as the complete list of roots, so a response that paged by default would silently truncate an upstream import to the first page. Without page/per_page the response is unchanged; with either present it pages exactly as /rest/v1/all_cres does, including the same MAX_ITEMS_PER_PAGE cap and the page/total_pages keys. The root-CRE query is shared between get_root_cres and the new get_root_cres_with_pagination, so both return the same set in the same order. The other half of #847, capping per_page on /rest/v1/all_cres, was fixed in 694f61e5. The OpenAPI spec is regenerated, which also gives the list_root_cres MCP tool page and per_page automatically. --- application/database/db.py | 26 ++++++-- application/tests/db_test.py | 37 +++++++++++ application/tests/web_main_test.py | 99 +++++++++++++++++++++++++++++ application/web/openapi_registry.py | 11 +++- application/web/openapi_schemas.py | 21 ++++++ application/web/web_main.py | 34 +++++++++- docs/api/openapi.yaml | 33 +++++++++- 7 files changed, 250 insertions(+), 11 deletions(-) diff --git a/application/database/db.py b/application/database/db.py index 522612161..36c5ae164 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,28 @@ def get_root_cres(self): ) ) ) - .all() ) + + 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..a0aad26b2 100644 --- a/application/tests/web_main_test.py +++ b/application/tests/web_main_test.py @@ -526,6 +526,105 @@ 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_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..43ed6c628 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,40 @@ 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 + if request.args.get("page") is not None and int(request.args.get("page")) > 0: + page = int(request.args.get("page")) + + if ( + request.args.get("per_page") is not None + and int(request.args.get("per_page")) > 0 + ): + per_page = int(request.args.get("per_page")) + 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: From efbb684f289736d7fdf84ce743c178ac3706fbfc Mon Sep 17 00:00:00 2001 From: Kunal Kumar Date: Tue, 8 Sep 2026 10:10:01 +0530 Subject: [PATCH 2/4] fix(api): order root CREs before paging, reject non-integer params Two problems with the pagination added in the previous commit, both raised in review. Ordering: _root_cres_query paginated a query with no ORDER BY. The database is then free to return rows in any order, so page boundaries can move between requests and a CRE can appear on two pages while another is never returned. Order by external_id, with the primary key breaking ties so the order is total. get_root_cres shares the query, so the paginated pages remain exactly the unpaginated list. This does not reproduce on SQLite, which returns a small table in insertion order, so the new test is a regression guard rather than a reproduction. It matters on Postgres, which is what production runs. Parameters: int() on a non-integer page or per_page raised ValueError with nothing to handle it, so ?page=abc was a 500 for what is a client mistake. Parse each value once and return 400. Values of zero or less still fall back to the defaults, as in all_cres. --- application/database/db.py | 6 ++++ application/tests/web_main_test.py | 47 ++++++++++++++++++++++++++++++ application/web/web_main.py | 23 ++++++++++----- 3 files changed, 68 insertions(+), 8 deletions(-) diff --git a/application/database/db.py b/application/database/db.py index 36c5ae164..2ba28d271 100644 --- a/application/database/db.py +++ b/application/database/db.py @@ -2766,6 +2766,12 @@ def _root_cres_query(self): ) ) ) + # 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): diff --git a/application/tests/web_main_test.py b/application/tests/web_main_test.py index a0aad26b2..554d469a3 100644 --- a/application/tests/web_main_test.py +++ b/application/tests/web_main_test.py @@ -625,6 +625,53 @@ def test_find_root_cres_pagination_integration(self) -> None: ) self.assertEqual(404, response.status_code) + 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() + for i in range(30): + collection.add_cre(defs.CRE(name=f"root-{i}", id=f"{i:03d}-{i:03d}")) + 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") + + # 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/web_main.py b/application/web/web_main.py index 43ed6c628..05e77f085 100644 --- a/application/web/web_main.py +++ b/application/web/web_main.py @@ -682,14 +682,21 @@ def find_root_cres() -> Any: if paginated: page = 1 per_page = ITEMS_PER_PAGE - if request.args.get("page") is not None and int(request.args.get("page")) > 0: - page = int(request.args.get("page")) - - if ( - request.args.get("per_page") is not None - and int(request.args.get("per_page")) > 0 - ): - per_page = int(request.args.get("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( From 9e2582ae11ab09f73f1901016e29bab61ab8c136 Mon Sep 17 00:00:00 2001 From: Kunal Kumar Date: Tue, 8 Sep 2026 12:17:31 +0530 Subject: [PATCH 3/4] test(api): make the ordering test fail when the ORDER BY is removed The fixture inserted root CREs already in external_id order, and SQLite hands back a small table in insertion order, so the test passed with or without the ordering it was meant to protect. Insert them descending and assert the ascending sequence: removing the ORDER BY now fails the test with the rows in insertion order. Same weakness as the all_cres test on #1060, raised in review there. --- application/tests/web_main_test.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/application/tests/web_main_test.py b/application/tests/web_main_test.py index 554d469a3..cb479eb03 100644 --- a/application/tests/web_main_test.py +++ b/application/tests/web_main_test.py @@ -644,8 +644,12 @@ def test_find_root_cres_pages_deterministically(self) -> None: 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() - for i in range(30): - collection.add_cre(defs.CRE(name=f"root-{i}", id=f"{i:03d}-{i:03d}")) + # 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: @@ -663,6 +667,9 @@ def ids(page: int) -> list: 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( From 4ecede5e34e234b13f9f5ecccaea198a320e2196 Mon Sep 17 00:00:00 2001 From: Kunal Kumar Date: Tue, 8 Sep 2026 13:04:34 +0530 Subject: [PATCH 4/4] test(api): cover the CRE.id tie-break in _root_cres_query's ordering Same gap as the all_cres test on #1060: every ordering fixture used a distinct external_id, so the test passed whether or not the sort carried its CRE.id half. external_id is not unique on its own -- the constraint is on (name, external_id) -- so ties are possible and the tie-break is what makes the order total. Two root CREs share an external_id here and are inserted in the opposite order to their primary keys, which are fixed rather than generated. Dropping CRE.id from the sort now fails the test. --- application/tests/web_main_test.py | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/application/tests/web_main_test.py b/application/tests/web_main_test.py index cb479eb03..3fe17bfdc 100644 --- a/application/tests/web_main_test.py +++ b/application/tests/web_main_test.py @@ -625,6 +625,33 @@ def test_find_root_cres_pagination_integration(self) -> None: ) 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()