mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-10-05 13:41:36 +02:00
fix(backup): stop Gitea's tree pager failing open on a missing total_count (#2656)
`if not isinstance(total, int) or seen >= total or not entries: return blobs, ""` — the first arm short-circuited the page loop into a **success** holding page 1 only. Gitea clamps `per_page` to `MAX_RESPONSE_ITEMS` (default 50), so that is 50 entries of an arbitrarily large tree returned as a complete listing. The restore then reports genuinely-present categories as "Not present in this backup commit". That silent skip is the exact failure this override exists to prevent, and the same class as E7 and G2 — G2 fixed the arithmetic here and left the shape. GitHub and GitLab both hard-fail in the equivalent spot; only Gitea guessed, and it guessed in the one direction that loses data quietly. Whether Gitea always sends `total_count` on this route is beside the point: the code was defending against a response shape it did not trust, and then trusting it. Now a missing or non-int `total_count` means "page until a short or empty page". A page shorter than the first one is the last one, floored at Gitea's default clamp so a genuinely small tree still costs exactly one request — the reason G2 rejected paging-until-short in the `total_count`-present case, which is unchanged and still stops on the count. The existing `page <= 50` ceiling gives the correct hard failure for a tree that really is over cap, so this cannot truncate. Residual, and deliberately not widened into a `return None` on the first ambiguous response — that would break single-page trees, the common case: an instance whose `MAX_RESPONSE_ITEMS` is set *below* 50 *and* which omits `total_count` would still stop at page 1. Both halves have to be true. Tests: +6 (paged to the end with no count, on both Gitea and Forgejo; a short page ends it; a non-int count is treated as no count; the page ceiling still fails). Fail-pre-fix 5, control that passes either way 1 (a small tree is one request). These don't match `-k github`, so 274 -> 280 across the three restore files but `-k github` is unmoved.
This commit is contained in:
@@ -12,6 +12,11 @@ from backend.app.services.git_providers.github import GitHubBackend
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
# Gitea clamps per_page to MAX_RESPONSE_ITEMS, which defaults to 50. Consulted
|
||||
# only when a tree response carries no usable total_count: a page at least this
|
||||
# long may be a clamped full page and cannot be assumed to be the last one.
|
||||
_ASSUMED_MIN_PAGE_SIZE = 50
|
||||
|
||||
|
||||
class GiteaBackend(GitHubBackend):
|
||||
"""Backend for Gitea instances.
|
||||
@@ -128,6 +133,7 @@ class GiteaBackend(GitHubBackend):
|
||||
blobs: dict[str, str] = {}
|
||||
seen = 0
|
||||
page = 1
|
||||
page_size: int | None = None
|
||||
while page <= 50:
|
||||
response = await client.get(
|
||||
f"{api_base}/repos/{owner}/{repo}/git/trees/{ref}",
|
||||
@@ -170,7 +176,28 @@ class GiteaBackend(GitHubBackend):
|
||||
# silently, the exact failure this override exists to prevent.
|
||||
total = data.get("total_count")
|
||||
seen += len(entries)
|
||||
if not isinstance(total, int) or seen >= total or not entries:
|
||||
if page_size is None:
|
||||
page_size = max(len(entries), _ASSUMED_MIN_PAGE_SIZE)
|
||||
|
||||
if not entries:
|
||||
return blobs, ""
|
||||
if isinstance(total, int):
|
||||
if seen >= total:
|
||||
return blobs, ""
|
||||
elif len(entries) < page_size:
|
||||
# No usable total_count. This used to return here on the *first*
|
||||
# page, i.e. fail open into a success holding whatever one page
|
||||
# happened to be — 50 entries of an arbitrarily large tree under
|
||||
# the default clamp — and the restore then reported every
|
||||
# category beyond it as absent from the commit. Page until a
|
||||
# short or empty page instead; the page-count ceiling below
|
||||
# still gives the correct hard failure for a tree that really is
|
||||
# too large. A page shorter than the first one (or than Gitea's
|
||||
# default clamp, so a genuinely small tree stays one request)
|
||||
# cannot be followed by another. The residual case is an
|
||||
# instance whose MAX_RESPONSE_ITEMS is set *below* 50 and which
|
||||
# also omits total_count; real Gitea and Forgejo always send it
|
||||
# on this route.
|
||||
return blobs, ""
|
||||
page += 1
|
||||
|
||||
|
||||
@@ -461,6 +461,83 @@ class TestGiteaAndForgejoInheritReads:
|
||||
assert len(result["paths"]) == total
|
||||
assert "f119.json" in result["paths"], "the tail of the tree is what a clamped pager loses"
|
||||
|
||||
# --- a response with no usable total_count must not fail open -----------
|
||||
#
|
||||
# The pager used to short-circuit into a *success* holding page 1 whenever
|
||||
# total_count was missing or not an int — 50 entries of an arbitrarily large
|
||||
# tree under Gitea's default clamp. The restore then reported the categories
|
||||
# it could not see as "not present in this backup commit", the same silent
|
||||
# skip this whole override exists to prevent. GitHub and GitLab both
|
||||
# hard-fail in the equivalent spot; only Gitea guessed.
|
||||
|
||||
@staticmethod
|
||||
def _page(start, count, **extra):
|
||||
return _make_mock_response(
|
||||
200,
|
||||
{
|
||||
"tree": [{"type": "blob", "path": f"f{i}.json", "sha": f"s{i}"} for i in range(start, start + count)],
|
||||
**extra,
|
||||
},
|
||||
)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("backend_cls", [GiteaBackend, ForgejoBackend])
|
||||
async def test_a_countless_response_is_paged_to_the_end(self, backend_cls):
|
||||
client = AsyncMock()
|
||||
client.get = AsyncMock(side_effect=[self._page(0, 50), self._page(50, 50), self._page(100, 0)])
|
||||
|
||||
result = await backend_cls().list_tree("https://git.example.com/owner/repo", "tok", "abc1234", client)
|
||||
|
||||
assert result["success"] is True
|
||||
assert client.get.await_count == 3
|
||||
assert len(result["paths"]) == 100
|
||||
assert "f99.json" in result["paths"], "the tail is what a fail-open pager loses"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_countless_short_page_ends_the_paging(self):
|
||||
client = AsyncMock()
|
||||
client.get = AsyncMock(side_effect=[self._page(0, 50), self._page(50, 7)])
|
||||
|
||||
result = await GiteaBackend().list_tree("https://git.example.com/owner/repo", "tok", "abc1234", client)
|
||||
|
||||
assert result["success"] is True
|
||||
assert client.get.await_count == 2
|
||||
assert len(result["paths"]) == 57
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_countless_single_page_tree_still_costs_one_request(self):
|
||||
"""Control: a small tree must not pay for the fix."""
|
||||
client = AsyncMock()
|
||||
client.get = AsyncMock(return_value=self._page(0, 3))
|
||||
|
||||
result = await GiteaBackend().list_tree("https://git.example.com/owner/repo", "tok", "abc1234", client)
|
||||
|
||||
assert result["paths"] == ["f0.json", "f1.json", "f2.json"]
|
||||
assert client.get.await_count == 1
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_non_int_total_count_is_treated_as_no_count(self):
|
||||
"""The arm the code was written to defend against, and then trusted."""
|
||||
client = AsyncMock()
|
||||
client.get = AsyncMock(side_effect=[self._page(0, 50, total_count="120"), self._page(50, 4)])
|
||||
|
||||
result = await GiteaBackend().list_tree("https://git.example.com/owner/repo", "tok", "abc1234", client)
|
||||
|
||||
assert result["success"] is True
|
||||
assert client.get.await_count == 2
|
||||
assert len(result["paths"]) == 54
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_countless_tree_beyond_the_page_cap_still_fails(self):
|
||||
"""The page ceiling is what keeps "page until short" from truncating."""
|
||||
client = AsyncMock()
|
||||
client.get = AsyncMock(return_value=self._page(0, 1000))
|
||||
|
||||
result = await GiteaBackend().list_tree("https://git.example.com/owner/repo", "tok", "abc1234", client)
|
||||
|
||||
assert result["success"] is False
|
||||
assert "listing limit" in result["message"]
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_tree_beyond_the_page_cap_fails_rather_than_truncating(self):
|
||||
page = {"tree": [{"type": "blob", "path": f"f{i}.json", "sha": f"s{i}"} for i in range(1000)]}
|
||||
|
||||
Reference in New Issue
Block a user