From 9b6a16d86288d3f30ff2d75dbb0b5b4c1ddb23b6 Mon Sep 17 00:00:00 2001 From: Louis King Date: Sat, 29 Aug 2026 13:01:16 +0100 Subject: [PATCH] fix(web): paginate /map/data node and profile fetches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The map endpoint fetched a single limit=500 page (the API's hard max, sorted last_seen desc), so networks with more than 500 nodes silently hid stale/offline nodes from the unfiltered map — including adopted infrastructure, which still appeared under the adopted_by filter because that filter narrows the query before the limit applies. Follow offset/total pagination via a new _fetch_all_pages helper (40 pages / 20k nodes safety cap, partial results on later-page failure) for both /api/v1/nodes and /api/v1/user/profiles, and report the API-reported total in debug.total_nodes. --- src/meshcore_hub/web/app.py | 121 +++++++++++++++++---- tests/test_web/conftest.py | 43 +++++--- tests/test_web/test_map.py | 208 ++++++++++++++++++++++++++++++++++++ 3 files changed, 339 insertions(+), 33 deletions(-) diff --git a/src/meshcore_hub/web/app.py b/src/meshcore_hub/web/app.py index c3bd464..b7e48c2 100644 --- a/src/meshcore_hub/web/app.py +++ b/src/meshcore_hub/web/app.py @@ -83,6 +83,83 @@ def _load_asset_manifest() -> dict[str, Any]: return {} +# Pagination bounds for web-tier aggregation fetches (e.g. /map/data). +# List endpoints cap `limit` at 500, so fetching "everything" requires +# following offset pages; MAP_MAX_PAGES is a safety valve against a +# misbehaving API that keeps reporting more data. +MAP_PAGE_SIZE = 500 +MAP_MAX_PAGES = 40 + + +async def _fetch_all_pages( + client: httpx.AsyncClient, + path: str, + extra_params: dict[str, str | int] | None = None, + page_size: int = MAP_PAGE_SIZE, + max_pages: int = MAP_MAX_PAGES, +) -> tuple[list[dict[str, Any]], int | None, str | None]: + """Fetch every item from a paginated list endpoint via offset paging. + + List endpoints cap ``limit`` at 500, so a single request only covers the + first page (e.g. the 500 most recently seen nodes) and silently drops the + rest. This helper follows ``offset``/``total`` until the collection is + exhausted. + + Returns: + ``(items, total, error)``. ``error`` is set only when the FIRST page + fails; a failure on a later page logs a warning and returns the items + collected so far. ``total`` is the API-reported total, or None when + the responses omit it. + """ + items: list[dict[str, Any]] = [] + total: int | None = None + params: dict[str, str | int] = dict(extra_params or {}) + params["limit"] = page_size + offset = 0 + + for page in range(max_pages): + params["offset"] = offset + response = await client.get(path, params=params) + if response.status_code != 200: + if page == 0: + return [], None, f"API returned status {response.status_code}" + logger.warning( + "Pagination of %s aborted at offset %d: API returned status %d " + "(returning %d items collected so far)", + path, + offset, + response.status_code, + len(items), + ) + break + data = response.json() + page_items = data.get("items", []) + if not isinstance(page_items, list): + break + items.extend(page_items) + reported_total = data.get("total") + if isinstance(reported_total, int): + total = reported_total + if not page_items: + break + offset += len(page_items) + if total is not None: + if offset >= total: + break + elif len(page_items) < page_size: + break + else: + logger.warning( + "Pagination of %s capped at %d pages (%d items); result may be " + "truncated", + path, + max_pages, + len(items), + ) + + return items, total, None + + # Per-endpoint, per-method role access mapping for the API proxy. # Key: URL path prefix (after /api/), Value: {method -> allowed roles}. # _OPEN = unconditional access (OIDC on or off, anonymous OK). @@ -851,30 +928,34 @@ def create_app( nodes_with_coords = 0 try: - profiles_response = await request.app.state.http_client.get( - "/api/v1/user/profiles", params={"limit": 500} + profile_items, _, _ = await _fetch_all_pages( + request.app.state.http_client, "/api/v1/user/profiles" ) - if profiles_response.status_code == 200: - profiles_data = profiles_response.json() - for profile in profiles_data.get("items", []): - profiles_by_id[profile["id"]] = { - "id": profile.get("id"), - "name": profile.get("name"), - "callsign": profile.get("callsign"), - "roles": profile.get("roles", []), - } + for profile in profile_items: + profiles_by_id[profile["id"]] = { + "id": profile.get("id"), + "name": profile.get("name"), + "callsign": profile.get("callsign"), + "roles": profile.get("roles", []), + } - # Fetch all nodes from API - nodes_params: dict[str, str | int] = {"limit": 500} + # Fetch all nodes from the API, following pagination. The API caps + # limit at 500 and sorts last_seen desc, so a single request only + # covered the 500 most recently seen nodes and silently hid + # stale/offline nodes — including adopted infrastructure — from + # the unfiltered map. + nodes_params: dict[str, str | int] = {} if adopted_by: nodes_params["adopted_by"] = adopted_by - response = await request.app.state.http_client.get( - "/api/v1/nodes", params=nodes_params + nodes, nodes_total, fetch_error = await _fetch_all_pages( + request.app.state.http_client, "/api/v1/nodes", nodes_params ) - if response.status_code == 200: - data = response.json() - nodes = data.get("items", []) - total_nodes = len(nodes) + if fetch_error is not None: + error = fetch_error + else: + total_nodes = ( + nodes_total if isinstance(nodes_total, int) else len(nodes) + ) for node in nodes: tags = node.get("tags", []) @@ -937,8 +1018,6 @@ def create_app( "owner": owner, } ) - else: - error = f"API returned status {response.status_code}" except Exception as e: error = str(e) diff --git a/tests/test_web/conftest.py b/tests/test_web/conftest.py index b091806..a43b3c7 100644 --- a/tests/test_web/conftest.py +++ b/tests/test_web/conftest.py @@ -45,6 +45,10 @@ class MockHttpClient: def __init__(self) -> None: """Initialize mock client with default responses.""" self._responses: dict[str, dict[str, Any]] = {} + # Param-aware handlers for paginated endpoints. Key: "METHOD:path", + # value: callable(params) -> {"status_code": int, "json": Any}. + # Checked before _responses so pagination tests can override defaults. + self._paged_handlers: dict[str, Any] = {} # Records the params forwarded by the most recent request() call so # tests can assert how the proxy forwards query parameters. self.last_request_params: Any = None @@ -259,19 +263,19 @@ class MockHttpClient: "json": json_data, } - def _create_response(self, key: str) -> Response: - """Create a mock response for a given key.""" - import json as _json + def set_paged_response(self, method: str, path: str, handler: Any) -> None: + """Register a param-aware handler for a paginated endpoint. - response_data = self._responses.get(key) - if response_data is None: - # Return 404 for unknown endpoints - response = MagicMock(spec=Response) - response.status_code = 404 - response.json.return_value = {"detail": "Not found"} - response.content = b'{"detail": "Not found"}' - response.headers = {"content-type": "application/json"} - return response + The handler receives the request params dict (including ``limit`` and + ``offset``) and returns the response payload dict + (``{"status_code": int, "json": Any}``), enabling different pages per + offset. Takes precedence over seeded/static responses for that path. + """ + self._paged_handlers[f"{method}:{path}"] = handler + + def _response_from_data(self, response_data: dict[str, Any]) -> Response: + """Create a mock response from a {"status_code", "json"} payload.""" + import json as _json response = MagicMock(spec=Response) response.status_code = response_data["status_code"] @@ -280,6 +284,16 @@ class MockHttpClient: response.headers = {"content-type": "application/json"} return response + def _create_response(self, key: str) -> Response: + """Create a mock response for a given key.""" + response_data = self._responses.get(key) + if response_data is None: + # Return 404 for unknown endpoints + return self._response_from_data( + {"status_code": 404, "json": {"detail": "Not found"}} + ) + return self._response_from_data(response_data) + async def request( self, method: str, @@ -307,6 +321,11 @@ class MockHttpClient: ) -> Response: """Mock GET request.""" self.last_get_headers = headers + # Param-aware paged handlers take precedence over static responses + # (including seeded defaults) so pagination tests control every page. + handler = self._paged_handlers.get(f"GET:{path}") + if handler is not None: + return self._response_from_data(handler(params)) # Try exact match first key = f"GET:{path}" if key in self._responses: diff --git a/tests/test_web/test_map.py b/tests/test_web/test_map.py index ef828fc..a54c276 100644 --- a/tests/test_web/test_map.py +++ b/tests/test_web/test_map.py @@ -462,3 +462,211 @@ class TestMapDataAdoptedByFilter: # Default mock has 2 nodes, 1 with coordinates assert len(data["nodes"]) == 1 assert data["nodes"][0]["name"] == "Node Two" + + +class TestMapDataPagination: + """Tests that /map/data follows API pagination instead of one 500-item page. + + Regression tests: the endpoint used to fetch a single ``limit=500`` page + (the API's hard max), so networks with more than 500 nodes silently hid + stale/offline nodes — including adopted infrastructure — from the + unfiltered map, while the ``adopted_by`` filter (applied before the limit) + still showed them. + """ + + @staticmethod + def _node( + idx: int, + name: str, + lat: float | None = None, + lon: float | None = None, + last_seen: str | None = "2024-01-01T12:00:00Z", + adopted_by: dict[str, Any] | None = None, + ) -> dict[str, Any]: + return { + "id": f"node-{idx}", + "public_key": f"{idx:064x}", + "name": name, + "adv_type": "REPEATER", + "lat": lat, + "lon": lon, + "last_seen": last_seen, + "tags": [], + "adopted_by": adopted_by, + } + + def test_map_data_includes_nodes_beyond_first_page( + self, web_app: Any, mock_http_client: MockHttpClient + ) -> None: + """A stale adopted node on page 2 must appear in the unfiltered map.""" + calls: list[dict | None] = [] + + def handler(params: dict | None) -> dict[str, Any]: + calls.append(params) + offset = (params or {}).get("offset", 0) + if offset == 0: + items = [self._node(1, "Recent Node", lat=41.0, lon=-75.0)] + else: + items = [ + self._node( + 2, + "Stale Repeater", + lat=40.0, + lon=-74.0, + last_seen="2023-06-01T00:00:00Z", + adopted_by={ + "user_id": "user-1", + "name": "Operator", + "callsign": "W1ABC", + "profile_id": "profile-1", + }, + ) + ] + return {"status_code": 200, "json": {"items": items, "total": 2}} + + mock_http_client.set_paged_response("GET", "/api/v1/nodes", handler) + + client = TestClient(web_app, raise_server_exceptions=True) + response = client.get("/map/data") + assert response.status_code == 200 + data = response.json() + + # Both pages were fetched + assert len(calls) == 2 + nodes_by_name = {n["name"]: n for n in data["nodes"]} + assert "Recent Node" in nodes_by_name + assert "Stale Repeater" in nodes_by_name + assert nodes_by_name["Stale Repeater"]["is_adopted"] is True + assert data["adopted_center"] == {"lat": 40.0, "lon": -74.0} + # debug.total_nodes reports the API total, not the page length + assert data["debug"]["total_nodes"] == 2 + + def test_map_data_forwards_adopted_by_on_every_page( + self, web_app: Any, mock_http_client: MockHttpClient + ) -> None: + """The adopted_by filter must be forwarded to every page request.""" + calls: list[dict | None] = [] + + def handler(params: dict | None) -> dict[str, Any]: + # Snapshot: the caller mutates the same params dict between pages + calls.append(dict(params) if params else None) + offset = (params or {}).get("offset", 0) + items = [self._node(offset + 1, f"Node {offset}", lat=40.0, lon=-74.0)] + return {"status_code": 200, "json": {"items": items, "total": 2}} + + mock_http_client.set_paged_response("GET", "/api/v1/nodes", handler) + + client = TestClient(web_app, raise_server_exceptions=True) + response = client.get("/map/data?adopted_by=some-profile-uuid") + assert response.status_code == 200 + + assert [c.get("offset") if c else None for c in calls] == [0, 1] + for params in calls: + assert params is not None + assert params.get("adopted_by") == "some-profile-uuid" + assert params.get("limit") == 500 + + def test_map_data_terminates_on_short_page_without_total( + self, web_app: Any, mock_http_client: MockHttpClient + ) -> None: + """A short page with no total field means "last page" — no extra calls.""" + calls: list[dict | None] = [] + + def handler(params: dict | None) -> dict[str, Any]: + calls.append(params) + return { + "status_code": 200, + "json": {"items": [self._node(1, "Only Node", lat=40.0, lon=-74.0)]}, + } + + mock_http_client.set_paged_response("GET", "/api/v1/nodes", handler) + + client = TestClient(web_app, raise_server_exceptions=True) + response = client.get("/map/data") + assert response.status_code == 200 + data = response.json() + + assert len(calls) == 1 + assert [n["name"] for n in data["nodes"]] == ["Only Node"] + + def test_map_data_pagination_capped_at_max_pages( + self, web_app: Any, mock_http_client: MockHttpClient + ) -> None: + """A pathological API reporting a huge total must not loop forever.""" + from meshcore_hub.web.app import MAP_MAX_PAGES + + calls: list[dict | None] = [] + + def handler(params: dict | None) -> dict[str, Any]: + calls.append(params) + offset = (params or {}).get("offset", 0) + items = [self._node(offset, f"Node {offset}", lat=40.0, lon=-74.0)] + return {"status_code": 200, "json": {"items": items, "total": 10**9}} + + mock_http_client.set_paged_response("GET", "/api/v1/nodes", handler) + + client = TestClient(web_app, raise_server_exceptions=True) + response = client.get("/map/data") + assert response.status_code == 200 + data = response.json() + + assert len(calls) == MAP_MAX_PAGES + assert len(data["nodes"]) == MAP_MAX_PAGES + # debug.total_nodes still reports the API-reported total + assert data["debug"]["total_nodes"] == 10**9 + + def test_map_data_keeps_partial_results_when_later_page_fails( + self, web_app: Any, mock_http_client: MockHttpClient + ) -> None: + """A failure on page 2 keeps the nodes collected from page 1.""" + calls: list[dict | None] = [] + + def handler(params: dict | None) -> dict[str, Any]: + calls.append(params) + offset = (params or {}).get("offset", 0) + if offset == 0: + return { + "status_code": 200, + "json": { + "items": [self._node(1, "First Page Node", 40.0, -74.0)], + "total": 3, + }, + } + return {"status_code": 500, "json": None} + + mock_http_client.set_paged_response("GET", "/api/v1/nodes", handler) + + client = TestClient(web_app, raise_server_exceptions=True) + response = client.get("/map/data") + assert response.status_code == 200 + data = response.json() + + assert len(calls) == 2 + assert [n["name"] for n in data["nodes"]] == ["First Page Node"] + + def test_map_data_paginates_profiles( + self, web_app: Any, mock_http_client: MockHttpClient + ) -> None: + """The operator dropdown source (profiles) is paginated too.""" + calls: list[dict | None] = [] + + def handler(params: dict | None) -> dict[str, Any]: + # Snapshot: the caller mutates the same params dict between pages + calls.append(dict(params) if params else None) + offset = (params or {}).get("offset", 0) + items: list[dict[str, Any]] + if offset == 0: + items = [{"id": "p1", "name": "Op One", "callsign": None, "roles": []}] + else: + items = [{"id": "p2", "name": "Op Two", "callsign": None, "roles": []}] + return {"status_code": 200, "json": {"items": items, "total": 2}} + + mock_http_client.set_paged_response("GET", "/api/v1/user/profiles", handler) + + client = TestClient(web_app, raise_server_exceptions=True) + response = client.get("/map/data") + assert response.status_code == 200 + data = response.json() + + assert len(calls) == 2 + assert {p["id"] for p in data["profiles"]} == {"p1", "p2"}