♻️ Refactor Explorer codebase for improved quality and maintainability

Author: jasodeep

examples/explorer.py

@@ -428,11 +428,9 @@ def main() -> None:
             updated = client.explorer.update_saved_view(org, created.id, update_opts)
             print(f"  update_saved_view: name is now {updated.name!r}")
 
-            # client.explorer.delete_saved_view removes the saved view; some API
-            # responses omit JSON, in which case the client still returns a minimal
-            # ExplorerSavedView carrying the deleted id.
-            deleted = client.explorer.delete_saved_view(org, created.id)
-            print(f"  delete_saved_view: completed for id {deleted.id}")
+            # client.explorer.delete_saved_view removes the saved view and returns None.
+            client.explorer.delete_saved_view(org, created.id)
+            print(f"  delete_saved_view: completed for id {created.id}")
             print("Summary: mutation sequence finished.")
         except TFEError as e:
             print(f"  API error: {e}")

src/pytfe/resources/_base.py

@@ -9,27 +9,79 @@
 from .._http import HTTPTransport
 
 
+def _to_int(value: Any) -> int | None:
+    """Best-effort integer coercion for pagination metadata values."""
+    if isinstance(value, int):
+        return value
+    if isinstance(value, str):
+        try:
+            return int(value)
+        except ValueError:
+            return None
+    return None
+
+
 class _Service:
     def __init__(self, t: HTTPTransport) -> None:
         self.t = t
 
     def _list(
         self, path: str, *, params: dict | None = None
     ) -> Iterator[dict[str, Any]]:
-        page = 1
+        base_params = dict(params or {})
+        page = int(base_params.get("page[number]", 1))
         while True:
-            p = dict(params or {})
+            p = dict(base_params)
             p["page[number]"] = page
             p.setdefault("page[size]", 100)
             r = self.t.request("GET", path, params=p)
 
             # Handle cases where r.json() returns None or is not a dict
             json_response = r.json()
-            if json_response is None:
+            if json_response is None or not isinstance(json_response, dict):
                 json_response = {}
 
             data = json_response.get("data", [])
+            if not isinstance(data, list):
+                data = []
             yield from data
+            if not data:
+                # Defensive stop: some endpoints can return inconsistent pagination
+                # metadata while yielding no rows; avoid unbounded follow-up requests.
+                break
+
+            # Prefer server pagination metadata when available. This avoids
+            # prematurely terminating when servers clamp requested page sizes.
+            meta = json_response.get("meta")
+            pagination = meta.get("pagination", {}) if isinstance(meta, dict) else {}
+            if isinstance(pagination, dict) and pagination:
+                next_page = _to_int(
+                    pagination.get("next-page", pagination.get("next_page"))
+                )
+                if next_page is not None and next_page > page:
+                    page = next_page
+                    continue
+
+                current_page = _to_int(
+                    pagination.get("current-page", pagination.get("current_page"))
+                )
+                total_pages = _to_int(
+                    pagination.get("total-pages", pagination.get("total_pages"))
+                )
+                if (
+                    current_page is not None
+                    and total_pages is not None
+                    and current_page < total_pages
+                ):
+                    candidate_page = current_page + 1
+                    if candidate_page > page:
+                        page = candidate_page
+                        continue
+
+                # Metadata present and indicates no next page.
+                break
+
+            # Fallback for endpoints that do not return pagination metadata.
             page_size = int(p["page[size]"])
             if len(data) < page_size:
                 break

src/pytfe/resources/explorer.py

@@ -121,6 +121,11 @@ def _parse_row(item: dict[str, Any]) -> ExplorerRow:
     return ExplorerRow.model_validate(item)
 
 
+def _normalize_filter_field_name(raw_field: Any) -> str:
+    """Normalize filter field names to SDK model style."""
+    return str(raw_field).replace("-", "_")
+
+
 def _saved_query_to_api_shape(raw_query: dict[str, Any]) -> dict[str, Any]:
     """Map {field, operator, value} filter rows to nested {field: {operator: [...]}} JSON."""
     query = dict(raw_query)
@@ -134,7 +139,7 @@ def _saved_query_to_api_shape(raw_query: dict[str, Any]) -> dict[str, Any]:
             if "field" not in entry or "operator" not in entry:
                 mapped_filters.append(entry)
                 continue
-            field = str(entry.get("field", "")).replace("-", "_")
+            field = _normalize_filter_field_name(entry.get("field", ""))
             operator = str(entry.get("operator", ""))
             values = entry.get("value", [])
             if not isinstance(values, list):
@@ -166,7 +171,7 @@ def _normalize_saved_query(
                     value = [str(value)]
                 normalized_filters.append(
                     {
-                        "field": str(entry["field"]).replace("-", "_"),
+                        "field": _normalize_filter_field_name(entry["field"]),
                         "operator": str(entry["operator"]),
                         "value": [str(v) for v in value],
                     }
@@ -182,7 +187,7 @@ def _normalize_saved_query(
                         vals = values if isinstance(values, list) else [values]
                         normalized_filters.append(
                             {
-                                "field": str(field_name).replace("-", "_"),
+                                "field": _normalize_filter_field_name(field_name),
                                 "operator": str(operator),
                                 "value": [str(v) for v in vals],
                             }
@@ -220,18 +225,6 @@ def _parse_saved_view(item: dict[str, Any]) -> ExplorerSavedView:
     )
 
 
-def _deleted_saved_view_fallback(view_id: str) -> ExplorerSavedView:
-    """Build a minimal saved view when delete responses have no body."""
-    return ExplorerSavedView.model_validate(
-        {
-            "id": view_id,
-            "name": "",
-            "query-type": "workspaces",
-            "query": {"type": "workspaces"},
-        }
-    )
-
-
 def _query_options_from_saved_view(
     saved_view: ExplorerSavedView,
 ) -> ExplorerQueryOptions:
@@ -550,38 +543,10 @@ def update_saved_view(
         _log.info("explorer.update_saved_view org=%r id=%r", organization, view.id)
         return view
 
-    def delete_saved_view(self, organization: str, view_id: str) -> ExplorerSavedView:
+    def delete_saved_view(self, organization: str, view_id: str) -> None:
         _require_organization_and_view(organization, view_id)
         path = f"/api/v2/organizations/{organization}/explorer/views/{view_id}"
-        resp = self.t.request("DELETE", path)
-        # DELETE often returns an empty body; callers still receive a minimal ExplorerSavedView.
-        raw_text = (resp.text or "").strip()
-        if not raw_text:
-            _log.debug(
-                "explorer.delete_saved_view: empty body, returning stub org=%r id=%r",
-                organization,
-                view_id,
-            )
-            return _deleted_saved_view_fallback(view_id)
-
-        try:
-            payload = resp.json()
-        except ValueError:
-            _log.debug(
-                "explorer.delete_saved_view: non-JSON body, returning stub org=%r id=%r",
-                organization,
-                view_id,
-            )
-            return _deleted_saved_view_fallback(view_id)
-
-        if isinstance(payload, dict) and isinstance(payload.get("data"), dict):
-            return _parse_saved_view(payload["data"])
-        _log.debug(
-            "explorer.delete_saved_view: no data object, returning stub org=%r id=%r",
-            organization,
-            view_id,
-        )
-        return _deleted_saved_view_fallback(view_id)
+        self.t.request("DELETE", path)
 
     def saved_view_results(
         self, organization: str, view_id: str

tests/units/test_explorer.py

@@ -181,6 +181,138 @@ def test_query_with_filter_and_pagination(self, explorer_service, mock_transport
         mock_transport.request.assert_has_calls(expected_calls)
         assert mock_transport.request.call_count == 4
 
+    def test_query_uses_pagination_meta_when_server_caps_page_size(
+        self, explorer_service, mock_transport
+    ):
+        first = Mock()
+        first.json.return_value = {
+            "data": [_row_payload("ws-1"), _row_payload("ws-2")],
+            "meta": {
+                "pagination": {
+                    "current-page": 1,
+                    "page-size": 2,
+                    "next-page": 2,
+                    "total-pages": 2,
+                }
+            },
+        }
+        second = Mock()
+        second.json.return_value = {
+            "data": [_row_payload("ws-3")],
+            "meta": {
+                "pagination": {
+                    "current-page": 2,
+                    "page-size": 2,
+                    "next-page": None,
+                    "total-pages": 2,
+                }
+            },
+        }
+        mock_transport.request.side_effect = [first, second]
+
+        options = ExplorerQueryOptions(
+            view_type=ExplorerViewType.WORKSPACES,
+            page_size=50,
+        )
+
+        rows = list(explorer_service.query(ORG, options))
+        assert [row.id for row in rows] == ["ws-1", "ws-2", "ws-3"]
+
+        expected_calls = [
+            call(
+                "GET",
+                EXPLORER_PATH,
+                params={"type": "workspaces", "page[size]": 50, "page[number]": 1},
+            ),
+            call(
+                "GET",
+                EXPLORER_PATH,
+                params={"type": "workspaces", "page[size]": 50, "page[number]": 2},
+            ),
+        ]
+        mock_transport.request.assert_has_calls(expected_calls)
+        assert mock_transport.request.call_count == 2
+
+    def test_query_uses_current_and_total_pages_when_next_page_missing(
+        self, explorer_service, mock_transport
+    ):
+        first = Mock()
+        first.json.return_value = {
+            "data": [_row_payload("ws-1")],
+            "meta": {"pagination": {"current-page": 1, "total-pages": 2}},
+        }
+        second = Mock()
+        second.json.return_value = {
+            "data": [_row_payload("ws-2")],
+            "meta": {"pagination": {"current-page": 2, "total-pages": 2}},
+        }
+        mock_transport.request.side_effect = [first, second]
+
+        rows = list(
+            explorer_service.query(
+                ORG, ExplorerQueryOptions(view_type=ExplorerViewType.WORKSPACES)
+            )
+        )
+        assert [row.id for row in rows] == ["ws-1", "ws-2"]
+
+        expected_calls = [
+            call(
+                "GET",
+                EXPLORER_PATH,
+                params={"type": "workspaces", "page[number]": 1, "page[size]": 100},
+            ),
+            call(
+                "GET",
+                EXPLORER_PATH,
+                params={"type": "workspaces", "page[number]": 2, "page[size]": 100},
+            ),
+        ]
+        mock_transport.request.assert_has_calls(expected_calls)
+        assert mock_transport.request.call_count == 2
+
+    def test_query_stops_when_pagination_meta_does_not_advance(
+        self, explorer_service, mock_transport
+    ):
+        first = Mock()
+        first.json.return_value = {
+            "data": [_row_payload("ws-1")],
+            "meta": {"pagination": {"current-page": 1, "total-pages": 2}},
+        }
+        second = Mock()
+        second.json.return_value = {
+            "data": [_row_payload("ws-1")],
+            "meta": {"pagination": {"current-page": 1, "total-pages": 2}},
+        }
+        mock_transport.request.side_effect = [first, second]
+
+        rows = list(
+            explorer_service.query(
+                ORG, ExplorerQueryOptions(view_type=ExplorerViewType.WORKSPACES)
+            )
+        )
+        assert [row.id for row in rows] == ["ws-1", "ws-1"]
+        assert mock_transport.request.call_count == 2
+
+    def test_query_stops_on_empty_page_even_if_next_page_present(
+        self, explorer_service, mock_transport
+    ):
+        first = Mock()
+        first.json.return_value = {
+            "data": [],
+            "meta": {
+                "pagination": {"current-page": 1, "next-page": 2, "total-pages": 5}
+            },
+        }
+        mock_transport.request.return_value = first
+
+        rows = list(
+            explorer_service.query(
+                ORG, ExplorerQueryOptions(view_type=ExplorerViewType.WORKSPACES)
+            )
+        )
+        assert rows == []
+        assert mock_transport.request.call_count == 1
+
     def test_query_invalid_org(self, explorer_service):
         with pytest.raises(InvalidOrgError):
             list(
@@ -379,50 +511,21 @@ def test_update_saved_view_invalid_data_shape_raises(
             explorer_service.update_saved_view(ORG, VIEW_ID, options)
 
     def test_delete_saved_view(self, explorer_service, mock_transport):
-        response = Mock()
-        response.json.return_value = {"data": _saved_view_payload("sq-1")}
-        response.text = '{"data":{"id":"sq-1"}}'
-        mock_transport.request.return_value = response
-
-        view = explorer_service.delete_saved_view(ORG, VIEW_ID)
-        assert view.id == "sq-1"
+        result = explorer_service.delete_saved_view(ORG, VIEW_ID)
+        assert result is None
 
         _assert_single_request_call(mock_transport, "DELETE", f"{VIEWS_PATH}/{VIEW_ID}")
 
-    def test_delete_saved_view_empty_response(self, explorer_service, mock_transport):
-        response = Mock()
-        response.text = ""
-        response.json.side_effect = ValueError("No JSON body")
-        mock_transport.request.return_value = response
-
-        view = explorer_service.delete_saved_view(ORG, VIEW_ID)
-        assert view.id == "sq-1"
-
-    def test_delete_saved_view_non_json_body_returns_stub(
+    def test_delete_saved_view_ignores_response_body(
         self, explorer_service, mock_transport
     ):
         response = Mock()
-        response.text = "deleted"
+        response.text = '{"data":{"id":"unexpected"}}'
         response.json.side_effect = ValueError("No JSON body")
         mock_transport.request.return_value = response
 
-        view = explorer_service.delete_saved_view(ORG, VIEW_ID)
-        assert view.id == "sq-1"
-        assert view.name == ""
-        assert view.query_type == ExplorerViewType.WORKSPACES
-
-    def test_delete_saved_view_invalid_data_shape_returns_stub(
-        self, explorer_service, mock_transport
-    ):
-        response = Mock()
-        response.text = '{"data":[]}'
-        response.json.return_value = {"data": []}
-        mock_transport.request.return_value = response
-
-        view = explorer_service.delete_saved_view(ORG, VIEW_ID)
-        assert view.id == "sq-1"
-        assert view.name == ""
-        assert view.query_type == ExplorerViewType.WORKSPACES
+        result = explorer_service.delete_saved_view(ORG, VIEW_ID)
+        assert result is None
 
     def test_saved_view_results(self, explorer_service, mock_transport):
         first = Mock()