Merge pull request #367 from ipnet-mesh/fix/map-data-pagination

fix(web): paginate /map/data node and profile fetches

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)

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:

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"}