Skip to content

Commit 3883280

Browse files
committed
Address review nits: named FetchNextPage alias and HTTP call count assertions
- Extract the page-fetch callable type into a named `FetchNextPage` alias in `typedef.py` so future paginated endpoints have a canonical type to reference instead of repeating the inline `Callable[[str], tuple[list[T], str | None]]` signature. - Wrap `catalog._session.get` in both integration tests and assert call count equals the number of items — proves `rest-page-size=1` is actually triggering one HTTP fetch per item rather than silently returning everything in one shot.
1 parent 769728d commit 3883280

2 files changed

Lines changed: 20 additions & 13 deletions

File tree

pyiceberg/typedef.py

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,8 @@ def __hash__(self) -> int:
216216
TableVersion: TypeAlias = Literal[1, 2, 3]
217217
ViewVersion: TypeAlias = Literal[1]
218218

219+
FetchNextPage: TypeAlias = Callable[[str], tuple[list[T], str | None]]
220+
219221

220222
class PaginationList(list[T]):
221223
"""A list that lazily fetches subsequent pages from a paginated API.
@@ -229,15 +231,15 @@ class PaginationList(list[T]):
229231
first_page: Items from the first API response.
230232
next_page_token: Pagination token returned with the first response,
231233
or ``None`` if no further pages exist.
232-
fetch_next_page: Callable that accepts a page token and returns a
233-
tuple of ``(items, next_page_token_or_None)``.
234+
fetch_next_page: Callable matching ``FetchNextPage[T]`` — accepts a
235+
page token and returns ``(items, next_page_token_or_None)``.
234236
"""
235237

236238
def __init__(
237239
self,
238240
first_page: list[T],
239241
next_page_token: str | None,
240-
fetch_next_page: Callable[[str], tuple[list[T], str | None]],
242+
fetch_next_page: FetchNextPage[T],
241243
) -> None:
242244
super().__init__(first_page)
243245
self._next_page_token = next_page_token

tests/integration/test_rest_catalog.py

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
# pylint:disable=redefined-outer-name
1818

1919
import time
20+
from unittest.mock import patch
2021

2122
import pytest
2223
from pytest_lazy_fixtures import lf
@@ -125,12 +126,14 @@ def test_list_tables_returns_pagination_list(table_schema_nested: Schema, databa
125126
catalog.create_table((database_name, table_name), table_schema_nested)
126127

127128
try:
128-
result = catalog.list_tables(database_name)
129-
130-
assert isinstance(result, PaginationList)
131-
assert len(result) == len(table_list)
132-
for table_name in table_list:
133-
assert (database_name, table_name) in result
129+
with patch.object(catalog._session, "get", wraps=catalog._session.get) as mock_get:
130+
result = catalog.list_tables(database_name)
131+
132+
assert isinstance(result, PaginationList)
133+
assert len(result) == len(table_list)
134+
for table_name in table_list:
135+
assert (database_name, table_name) in result
136+
assert mock_get.call_count == len(table_list)
134137
finally:
135138
clean_up(catalog)
136139

@@ -144,10 +147,12 @@ def test_list_namespaces_returns_pagination_list(database_list: list[str]) -> No
144147
catalog.create_namespace(namespace)
145148

146149
try:
147-
result = catalog.list_namespaces()
150+
with patch.object(catalog._session, "get", wraps=catalog._session.get) as mock_get:
151+
result = catalog.list_namespaces()
148152

149-
assert isinstance(result, PaginationList)
150-
for namespace in database_list:
151-
assert (namespace,) in result
153+
assert isinstance(result, PaginationList)
154+
for namespace in database_list:
155+
assert (namespace,) in result
156+
assert mock_get.call_count == len(result)
152157
finally:
153158
clean_up(catalog)

0 commit comments

Comments
 (0)