You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@magpie.apache.org by po...@apache.org on 2026/08/25 15:30:26 UTC
(magpie) branch main updated: refactor(bitbucket): centralize Data Center pagination guard (#1105)
This is an automated email from the ASF dual-hosted git repository.
potiuk pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/magpie.git
The following commit(s) were added to refs/heads/main by this push:
new dee7da49 refactor(bitbucket): centralize Data Center pagination guard (#1105)
dee7da49 is described below
commit dee7da491e91f0c8045e38a4715ba369b3950014
Author: Davide Polato <dp...@apache.org>
AuthorDate: Tue Aug 25 17:30:19 2026 +0200
refactor(bitbucket): centralize Data Center pagination guard (#1105)
* refactor(bitbucket): centralize Data Center pagination guard
Data Center repeated the same nextPageStart guard in six paginators,
while regression coverage was uneven: removing the repository-
restrictions copy still left the suite green. Hoist the guard into
_next_page_start(), mirroring cloud.py's single-helper pattern.
Add focused unit coverage for the helper's stop conditions and a
parameterized regression test that drives all six paginators against
a non-advancing response.
* refactor(bitbucket): drop redundant lambda wrappers in pagination test
CodeQL flagged two parametrize entries that wrapped a callable in a
lambda without binding any extra arguments. Pass the functions
directly; the remaining lambdas bind extra arguments and stay.
---------
Co-authored-by: Arnav <a....@gmail.com>
---
tools/bitbucket/src/magpie_bitbucket/datacenter.py | 80 +++++++++-------------
tools/bitbucket/tests/test_bitbucket.py | 72 +++++++++++++++++++
2 files changed, 104 insertions(+), 48 deletions(-)
diff --git a/tools/bitbucket/src/magpie_bitbucket/datacenter.py b/tools/bitbucket/src/magpie_bitbucket/datacenter.py
index 35f04d51..2d253796 100644
--- a/tools/bitbucket/src/magpie_bitbucket/datacenter.py
+++ b/tools/bitbucket/src/magpie_bitbucket/datacenter.py
@@ -30,6 +30,26 @@ def _api_base(config: BitbucketConfig) -> str:
return f"{base_url}/rest/api/1.0"
+def _next_page_start(page: dict[str, Any], start: int) -> int | None:
+ """Return the validated next page offset, or None when pagination must stop.
+
+ A server that reports a missing, malformed, or non-advancing
+ ``nextPageStart`` would otherwise keep the caller requesting the same
+ page forever; every Data Center paginator routes through this guard.
+ """
+ if page.get("isLastPage") is True:
+ return None
+
+ next_start = page.get("nextPageStart")
+ if not isinstance(next_start, int):
+ return None
+
+ if next_start <= start:
+ return None
+
+ return next_start
+
+
def get_repository(config: BitbucketConfig) -> dict[str, Any]:
"""Fetch repository metadata from Bitbucket Data Center."""
project_key = quote_path(require(config.project_key, "BITBUCKET_PROJECT_KEY"))
@@ -64,14 +84,8 @@ def get_repository_restrictions(config: BitbucketConfig) -> dict[str, Any]:
if isinstance(values, list):
combined["values"].extend(item for item in values if isinstance(item, dict))
- if page.get("isLastPage") is True:
- break
-
- next_start = page.get("nextPageStart")
- if not isinstance(next_start, int):
- break
-
- if next_start <= start:
+ next_start = _next_page_start(page, start)
+ if next_start is None:
break
start = next_start
@@ -144,14 +158,8 @@ def list_open_pull_requests(config: BitbucketConfig) -> dict[str, Any]:
if isinstance(values, list):
combined["values"].extend(item for item in values if isinstance(item, dict))
- if page.get("isLastPage") is True:
- break
-
- next_start = page.get("nextPageStart")
- if not isinstance(next_start, int):
- break
-
- if next_start <= start:
+ next_start = _next_page_start(page, start)
+ if next_start is None:
break
start = next_start
@@ -191,14 +199,8 @@ def get_pull_request_commits(config: BitbucketConfig, pull_request_id: str) -> d
if isinstance(values, list):
combined["values"].extend(item for item in values if isinstance(item, dict))
- if page.get("isLastPage") is True:
- break
-
- next_start = page.get("nextPageStart")
- if not isinstance(next_start, int):
- break
-
- if next_start <= start:
+ next_start = _next_page_start(page, start)
+ if next_start is None:
break
start = next_start
@@ -298,14 +300,8 @@ def get_pull_request_status(config: BitbucketConfig, pull_request_id: str) -> di
if isinstance(values, list):
combined["values"].extend(item for item in values if isinstance(item, dict))
- if page.get("isLastPage") is True:
- break
-
- next_start = page.get("nextPageStart")
- if not isinstance(next_start, int):
- break
-
- if next_start <= start:
+ next_start = _next_page_start(page, start)
+ if next_start is None:
break
start = next_start
@@ -341,14 +337,8 @@ def get_pull_request_reviews(config: BitbucketConfig, pull_request_id: str) -> d
if isinstance(values, list):
combined["values"].extend(item for item in values if isinstance(item, dict))
- if page.get("isLastPage") is True:
- break
-
- next_start = page.get("nextPageStart")
- if not isinstance(next_start, int):
- break
-
- if next_start <= start:
+ next_start = _next_page_start(page, start)
+ if next_start is None:
break
start = next_start
@@ -397,14 +387,8 @@ def get_pull_request_discussion(config: BitbucketConfig, pull_request_id: str) -
if isinstance(values, list):
combined["values"].extend(item for item in values if isinstance(item, dict))
- if page.get("isLastPage") is True:
- break
-
- next_start = page.get("nextPageStart")
- if not isinstance(next_start, int):
- break
-
- if next_start <= start:
+ next_start = _next_page_start(page, start)
+ if next_start is None:
break
start = next_start
diff --git a/tools/bitbucket/tests/test_bitbucket.py b/tools/bitbucket/tests/test_bitbucket.py
index a12683aa..3d5e8240 100644
--- a/tools/bitbucket/tests/test_bitbucket.py
+++ b/tools/bitbucket/tests/test_bitbucket.py
@@ -1019,6 +1019,78 @@ def test_datacenter_get_pull_request_commits_stops_on_non_advancing_next_page_st
assert [item["id"] for item in result["values"]] == ["abc123"]
+_DATACENTER_PR_PAGE = {"id": 9, "fromRef": {"latestCommit": "def456"}}
+
+
+@patch("urllib.request.build_opener")
+@pytest.mark.parametrize(
+ ("paginate", "leading_pages"),
+ [
+ pytest.param(
+ datacenter.get_repository_restrictions,
+ (),
+ id="repository_restrictions",
+ ),
+ pytest.param(
+ datacenter.list_open_pull_requests,
+ (),
+ id="open_pull_requests",
+ ),
+ pytest.param(
+ lambda config: datacenter.get_pull_request_commits(config, "9"),
+ (),
+ id="pull_request_commits",
+ ),
+ pytest.param(
+ lambda config: datacenter.get_pull_request_status(config, "9"),
+ (_DATACENTER_PR_PAGE,),
+ id="pull_request_status",
+ ),
+ pytest.param(
+ lambda config: datacenter.get_pull_request_reviews(config, "9"),
+ (_DATACENTER_PR_PAGE,),
+ id="pull_request_reviews",
+ ),
+ pytest.param(
+ lambda config: datacenter.get_pull_request_discussion(config, "9"),
+ (),
+ id="pull_request_discussion",
+ ),
+ ],
+)
+def test_datacenter_paginators_stop_on_non_advancing_next_page_start(
+ mock_build_opener: MagicMock,
+ paginate: Any,
+ leading_pages: tuple[dict[str, Any], ...],
+ datacenter_env: None,
+) -> None:
+ opener = mock_opener(
+ mock_build_opener,
+ *leading_pages,
+ {"values": [{"id": "abc123"}], "isLastPage": False, "nextPageStart": 0},
+ )
+
+ result = paginate(load_config())
+
+ assert len(opener.open.call_args_list) == len(leading_pages) + 1
+ assert [item["id"] for item in result["values"]] == ["abc123"]
+
+
+@pytest.mark.parametrize(
+ ("page", "start", "expected"),
+ [
+ pytest.param({"isLastPage": True, "nextPageStart": 25}, 0, None, id="last_page"),
+ pytest.param({"isLastPage": False}, 0, None, id="missing_next_page_start"),
+ pytest.param({"isLastPage": False, "nextPageStart": "25"}, 0, None, id="non_int_next_page_start"),
+ pytest.param({"isLastPage": False, "nextPageStart": 0}, 0, None, id="repeated_next_page_start"),
+ pytest.param({"isLastPage": False, "nextPageStart": 5}, 10, None, id="backwards_next_page_start"),
+ pytest.param({"isLastPage": False, "nextPageStart": 25}, 0, 25, id="advancing_next_page_start"),
+ ],
+)
+def test_datacenter_next_page_start(page: dict[str, Any], start: int, expected: int | None) -> None:
+ assert datacenter._next_page_start(page, start) == expected
+
+
def test_normalize_cloud_pull_request_commits() -> None:
raw = {
"pull_request_id": "7",