From 2bce993d0ecc6a3d5a6bfdcfb477e3a545ca5e93 Mon Sep 17 00:00:00 2001 From: Davide Polato Date: Tue, 18 Aug 2026 15:44:28 +0200 Subject: [PATCH 1/2] 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. --- .../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 8d518a7d..0baa1191 100644 --- a/tools/bitbucket/tests/test_bitbucket.py +++ b/tools/bitbucket/tests/test_bitbucket.py @@ -992,6 +992,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( + lambda config: datacenter.get_repository_restrictions(config), + (), + id="repository_restrictions", + ), + pytest.param( + lambda config: datacenter.list_open_pull_requests(config), + (), + 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", From 3e47136214a55a6c4180a961cb1dc8be145dac43 Mon Sep 17 00:00:00 2001 From: Davide Polato Date: Wed, 19 Aug 2026 15:34:25 +0200 Subject: [PATCH 2/2] 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. --- tools/bitbucket/tests/test_bitbucket.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/bitbucket/tests/test_bitbucket.py b/tools/bitbucket/tests/test_bitbucket.py index 0baa1191..46d857f5 100644 --- a/tools/bitbucket/tests/test_bitbucket.py +++ b/tools/bitbucket/tests/test_bitbucket.py @@ -1000,12 +1000,12 @@ def test_datacenter_get_pull_request_commits_stops_on_non_advancing_next_page_st ("paginate", "leading_pages"), [ pytest.param( - lambda config: datacenter.get_repository_restrictions(config), + datacenter.get_repository_restrictions, (), id="repository_restrictions", ), pytest.param( - lambda config: datacenter.list_open_pull_requests(config), + datacenter.list_open_pull_requests, (), id="open_pull_requests", ),