mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-10-22 03:49:18 +02:00
fix(backup): halve the provider round-trips, and stop losing commit metadata (#2656)
The three remaining review items, all in the read path. E1 — four provider calls where two would do. preview() called list_commits twice: once inside _resolve_ref to turn HEAD into a SHA, once more at limit=20 purely to find the entry describing that same SHA. And list_tree's recursive tree GET was thrown away, so fetch_files immediately fetched the identical tree again to map path -> blob SHA. _resolve_ref now returns the entry it already has, and list_tree returns its blob_shas map for fetch_files to take as an optional argument. GitLab reads files by path and ignores it. E2 — `commit: null` for a ref outside the 20 most recent. Two causes, and the second is the one that actually bit: REF_PATTERN accepts a 7-character ref while providers return the full 40, so the exact `==` in the scan never matched an abbreviated SHA *even when the commit was in the window*. Fixed by prefix comparison, plus a get_commit(ref) on the GitHub and GitLab backends for the genuinely-outside-the-window case. Gitea and Forgejo inherit GitHub's. Still best-effort: it is a subject line and a date, so a failed lookup renders the preview without them rather than failing it. E7 — the two tree readers disagreed, and each was wrong in the other's direction. GitHub's recursive trees endpoint is not paginated and signals overflow with truncated=true, which _blob_shas_at hard-fails on. Gitea and Forgejo *do* page that endpoint, and inherited that single GET unchanged — so a large backup repo returned only the first page and every category beyond it looked absent from the commit. GiteaBackend now has its own paging _blob_shas_at. GitLab had the mirror-image bug the review did not name: at its 50-page cap it exited through the while condition and returned success: True with a silently partial path list. Both now fail loudly, which is what the GitHub version was always doing. Both halves of E7 are the same failure the module already refuses to allow: a restore that skips categories and calls it "not present in this backup commit". 24 new or changed tests, all failing against this commit's parent.
This commit is contained in:
jmoore-skild
@@ -115,6 +115,67 @@ class TestGitHubListCommits:
|
||||
assert "Unexpected shape" in result["message"]
|
||||
|
||||
|
||||
class TestGetCommit:
|
||||
"""A ref older than the list window still needs a subject line and a date."""
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_github_reads_one_commit_by_sha(self):
|
||||
client = AsyncMock()
|
||||
client.get = AsyncMock(return_value=_make_mock_response(200, _github_commit("abc1234567")))
|
||||
|
||||
result = await GitHubBackend().get_commit("https://github.com/owner/repo", "tok", "abc1234567", client)
|
||||
|
||||
assert result["success"] is True
|
||||
assert result["commit"] == {
|
||||
"sha": "abc1234567",
|
||||
"message": "Bambuddy backup",
|
||||
"author": "Bambuddy",
|
||||
"date": "2026-07-01T10:00:00Z",
|
||||
}
|
||||
assert "repos/owner/repo/commits/abc1234567" in client.get.await_args.args[0]
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_github_404_names_the_ref(self):
|
||||
client = AsyncMock()
|
||||
client.get = AsyncMock(return_value=_make_mock_response(404, {}))
|
||||
|
||||
result = await GitHubBackend().get_commit("https://github.com/owner/repo", "tok", "deadbee", client)
|
||||
|
||||
assert result["success"] is False
|
||||
assert result["commit"] is None
|
||||
assert "deadbee" in result["message"]
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_gitlab_reads_its_flattened_shape(self):
|
||||
client = AsyncMock()
|
||||
client.get = AsyncMock(
|
||||
return_value=_make_mock_response(
|
||||
200,
|
||||
{
|
||||
"id": "abc1234567",
|
||||
"message": "Bambuddy backup",
|
||||
"author_name": "Bambuddy",
|
||||
"committed_date": "2026-07-02T10:00:00Z",
|
||||
},
|
||||
)
|
||||
)
|
||||
|
||||
result = await GitLabBackend().get_commit("https://gitlab.com/owner/repo", "tok", "abc1234567", client)
|
||||
|
||||
assert result["commit"]["author"] == "Bambuddy"
|
||||
assert result["commit"]["date"] == "2026-07-02T10:00:00Z"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_gitlab_404_names_the_ref(self):
|
||||
client = AsyncMock()
|
||||
client.get = AsyncMock(return_value=_make_mock_response(404, {}))
|
||||
|
||||
result = await GitLabBackend().get_commit("https://gitlab.com/owner/repo", "tok", "deadbee", client)
|
||||
|
||||
assert result["success"] is False
|
||||
assert "deadbee" in result["message"]
|
||||
|
||||
|
||||
class TestGitHubListTree:
|
||||
def setup_method(self):
|
||||
self.backend = GitHubBackend()
|
||||
@@ -221,6 +282,40 @@ class TestGitHubFetchFiles:
|
||||
assert result["files"] == {"a.json": "1", "b.json": "2"}
|
||||
assert client.get.await_count == 3
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_supplied_blob_map_skips_the_second_tree_read(self):
|
||||
"""list_tree already fetched this; fetching it again was a wasted GET."""
|
||||
client = AsyncMock()
|
||||
client.get = AsyncMock(return_value=_make_mock_response(200, {"content": _b64("1"), "encoding": "base64"}))
|
||||
|
||||
result = await self.backend.fetch_files(
|
||||
self.repo_url, self.token, "abc1234", ["a.json"], client, blob_shas={"a.json": "sha-a"}
|
||||
)
|
||||
|
||||
assert result["files"] == {"a.json": "1"}
|
||||
# The blob read and nothing else.
|
||||
assert client.get.await_count == 1
|
||||
assert "git/blobs/sha-a" in client.get.await_args.args[0]
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_list_tree_hands_back_the_map_it_built(self):
|
||||
client = AsyncMock()
|
||||
client.get = AsyncMock(
|
||||
return_value=_make_mock_response(
|
||||
200,
|
||||
{
|
||||
"tree": [
|
||||
{"type": "blob", "path": "a.json", "sha": "sha-a"},
|
||||
{"type": "tree", "path": "dir", "sha": "sha-d"},
|
||||
]
|
||||
},
|
||||
)
|
||||
)
|
||||
|
||||
result = await self.backend.list_tree(self.repo_url, self.token, "abc1234", client)
|
||||
|
||||
assert result["blob_shas"] == {"a.json": "sha-a"}
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_missing_path_is_skipped_not_an_error(self):
|
||||
"""Which categories a backup contains varies by config, so an absent
|
||||
@@ -273,13 +368,85 @@ class TestGitHubFetchFiles:
|
||||
|
||||
|
||||
class TestGiteaAndForgejoInheritReads:
|
||||
"""Gitea overrides the *write* path only; reads come from GitHubBackend."""
|
||||
"""Gitea overrides the *write* path, plus the one read that genuinely differs."""
|
||||
|
||||
@pytest.mark.parametrize("backend_cls", [GiteaBackend, ForgejoBackend])
|
||||
def test_read_methods_are_not_overridden(self, backend_cls):
|
||||
for method in ("list_commits", "list_tree", "fetch_files"):
|
||||
for method in ("list_commits", "list_tree", "fetch_files", "get_commit"):
|
||||
assert getattr(backend_cls, method) is getattr(GitHubBackend, method)
|
||||
|
||||
@pytest.mark.parametrize("backend_cls", [GiteaBackend, ForgejoBackend])
|
||||
def test_the_tree_read_is_paged_rather_than_inherited(self, backend_cls):
|
||||
"""GitHub's trees endpoint is not paginated; Gitea's is (#2656)."""
|
||||
assert backend_cls._blob_shas_at is not GitHubBackend._blob_shas_at
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("backend_cls", [GiteaBackend, ForgejoBackend])
|
||||
async def test_a_paged_tree_is_read_to_the_end(self, backend_cls):
|
||||
"""Inheriting GitHub's single GET read only the first page.
|
||||
|
||||
The rest of the backup then looked absent from the commit, and the
|
||||
preview reported those categories as "not present" — a restore silently
|
||||
skipping data, which is exactly what GitHub's truncated=true check
|
||||
exists to prevent.
|
||||
"""
|
||||
page1 = {
|
||||
"tree": [{"type": "blob", "path": f"f{i}.json", "sha": f"s{i}"} for i in range(1000)],
|
||||
"total_count": 1002,
|
||||
}
|
||||
page2 = {
|
||||
"tree": [
|
||||
{"type": "blob", "path": "settings/app_settings.json", "sha": "sx"},
|
||||
{"type": "tree", "path": "settings", "sha": "dx"},
|
||||
],
|
||||
"total_count": 1002,
|
||||
}
|
||||
client = AsyncMock()
|
||||
client.get = AsyncMock(side_effect=[_make_mock_response(200, page1), _make_mock_response(200, page2)])
|
||||
|
||||
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 == 2
|
||||
assert "settings/app_settings.json" in result["paths"]
|
||||
assert len(result["paths"]) == 1001
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_single_page_tree_costs_one_request(self):
|
||||
client = AsyncMock()
|
||||
client.get = AsyncMock(
|
||||
return_value=_make_mock_response(
|
||||
200, {"tree": [{"type": "blob", "path": "a.json", "sha": "s1"}], "total_count": 1}
|
||||
)
|
||||
)
|
||||
|
||||
result = await GiteaBackend().list_tree("https://git.example.com/owner/repo", "tok", "abc1234", client)
|
||||
|
||||
assert result["paths"] == ["a.json"]
|
||||
assert client.get.await_count == 1
|
||||
|
||||
@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)]}
|
||||
page["total_count"] = 10_000_000
|
||||
client = AsyncMock()
|
||||
client.get = AsyncMock(return_value=_make_mock_response(200, page))
|
||||
|
||||
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_missing_ref_is_still_named(self):
|
||||
client = AsyncMock()
|
||||
client.get = AsyncMock(return_value=_make_mock_response(404, {}))
|
||||
|
||||
result = await GiteaBackend().list_tree("https://git.example.com/owner/repo", "tok", "deadbee", client)
|
||||
|
||||
assert result["success"] is False
|
||||
assert "deadbee" in result["message"]
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_gitea_list_commits_uses_its_own_api_base(self):
|
||||
backend = GiteaBackend()
|
||||
@@ -299,6 +466,7 @@ class TestGiteaAndForgejoInheritReads:
|
||||
client = AsyncMock()
|
||||
client.get = AsyncMock(return_value=_make_mock_response(200, {"tree": []}))
|
||||
|
||||
client.get = AsyncMock(return_value=_make_mock_response(200, {"tree": [], "total_count": 0}))
|
||||
await backend.list_tree("https://example.com/git/owner/repo", "tok", "abc1234", client)
|
||||
|
||||
url = client.get.await_args.args[0]
|
||||
@@ -398,6 +566,47 @@ class TestGitLabReads:
|
||||
assert len(result["paths"]) == 101
|
||||
assert "last.json" in result["paths"]
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_hitting_the_page_cap_is_a_failure_not_a_partial_list(self):
|
||||
"""The mirror image of GitHub's truncated=true check.
|
||||
|
||||
Falling out of the `while page <= 50` condition used to return
|
||||
success: True with a silently partial path list, which the restore then
|
||||
reported as "those categories are not present in this commit" — data
|
||||
skipped without anyone being told.
|
||||
"""
|
||||
full_page = [{"type": "blob", "path": f"f{i}.json"} for i in range(100)]
|
||||
client = AsyncMock()
|
||||
client.get = AsyncMock(return_value=_make_mock_response(200, full_page))
|
||||
|
||||
result = await self.backend.list_tree(self.repo_url, self.token, "abc1234", client)
|
||||
|
||||
assert result["success"] is False
|
||||
assert result["paths"] == []
|
||||
assert "cannot be enumerated reliably" in result["message"]
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_list_tree_returns_no_blob_map(self):
|
||||
"""GitLab reads files by path, so there is nothing to share."""
|
||||
client = AsyncMock()
|
||||
client.get = AsyncMock(return_value=_make_mock_response(200, [{"type": "blob", "path": "a.json"}]))
|
||||
|
||||
result = await self.backend.list_tree(self.repo_url, self.token, "abc1234", client)
|
||||
|
||||
assert result["blob_shas"] == {}
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_fetch_files_ignores_a_blob_map(self):
|
||||
client = AsyncMock()
|
||||
client.get = AsyncMock(return_value=_make_mock_response(200, {"content": _b64("1"), "encoding": "base64"}))
|
||||
|
||||
result = await self.backend.fetch_files(
|
||||
self.repo_url, self.token, "abc1234", ["a.json"], client, blob_shas={"a.json": "irrelevant"}
|
||||
)
|
||||
|
||||
assert result["files"] == {"a.json": "1"}
|
||||
assert "repository/files/a.json" in client.get.await_args.args[0]
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_fetch_files_decodes_base64(self):
|
||||
client = AsyncMock()
|
||||
|
||||
@@ -1837,10 +1837,12 @@ class TestResolveRef:
|
||||
service.list_commits = AsyncMock()
|
||||
config = MagicMock(branch="main")
|
||||
|
||||
resolved, error = await service._resolve_ref(config, "abc1234")
|
||||
resolved, error, commit = await service._resolve_ref(config, "abc1234")
|
||||
|
||||
assert resolved == "abc1234"
|
||||
assert error == ""
|
||||
# Nothing was fetched, so there is no entry to describe it with.
|
||||
assert commit is None
|
||||
service.list_commits.assert_not_awaited()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@@ -1851,10 +1853,13 @@ class TestResolveRef:
|
||||
)
|
||||
config = MagicMock(branch="main")
|
||||
|
||||
resolved, error = await service._resolve_ref(config, "HEAD")
|
||||
resolved, error, commit = await service._resolve_ref(config, "HEAD")
|
||||
|
||||
assert resolved == "tipsha1"
|
||||
assert error == ""
|
||||
# Handed back so preview does not list commits a second time just to
|
||||
# describe the one it already fetched.
|
||||
assert commit == {"sha": "tipsha1"}
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_empty_history_is_an_error(self):
|
||||
@@ -1862,7 +1867,80 @@ class TestResolveRef:
|
||||
service.list_commits = AsyncMock(return_value={"success": True, "commits": []})
|
||||
config = MagicMock(branch="main")
|
||||
|
||||
resolved, error = await service._resolve_ref(config, "HEAD")
|
||||
resolved, error, commit = await service._resolve_ref(config, "HEAD")
|
||||
|
||||
assert resolved is None
|
||||
assert "no commits" in error
|
||||
assert commit is None
|
||||
|
||||
|
||||
class TestDescribeCommit:
|
||||
"""A preview that says `commit: null` gives the user no idea what they picked."""
|
||||
|
||||
def _config(self):
|
||||
return MagicMock(branch="main", provider="github", repository_url="https://github.com/o/r", access_token="t")
|
||||
|
||||
def _entry(self, sha: str):
|
||||
return {"sha": sha, "message": "Bambuddy backup", "author": "Bambuddy", "date": "2026-07-01T10:00:00Z"}
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_an_abbreviated_ref_matches_a_full_sha_in_the_window(self):
|
||||
"""REF_PATTERN accepts 7 characters; providers return 40.
|
||||
|
||||
The old exact `==` therefore never matched an abbreviated ref, even when
|
||||
the commit was right there in the top 20.
|
||||
"""
|
||||
service = _service()
|
||||
full = "abc1234" + "0" * 33
|
||||
service.list_commits = AsyncMock(return_value={"success": True, "commits": [self._entry(full)]})
|
||||
|
||||
found = await service._describe_commit(self._config(), "abc1234")
|
||||
|
||||
assert found is not None
|
||||
assert found["sha"] == full
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_full_sha_matches_an_abbreviated_entry(self):
|
||||
service = _service()
|
||||
service.list_commits = AsyncMock(return_value={"success": True, "commits": [self._entry("abc1234")]})
|
||||
|
||||
found = await service._describe_commit(self._config(), "abc1234" + "0" * 33)
|
||||
|
||||
assert found is not None
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_commit_outside_the_window_is_fetched_directly(self):
|
||||
service = _service()
|
||||
service.list_commits = AsyncMock(return_value={"success": True, "commits": [self._entry("f" * 40)]})
|
||||
backend = MagicMock()
|
||||
backend.get_commit = AsyncMock(return_value={"success": True, "commit": self._entry("old" + "0" * 37)})
|
||||
|
||||
with patch("backend.app.services.github_restore.get_provider_backend", return_value=backend):
|
||||
found = await service._describe_commit(self._config(), "old" + "0" * 37)
|
||||
|
||||
assert found["sha"] == "old" + "0" * 37
|
||||
backend.get_commit.assert_awaited_once()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_direct_lookup_failure_is_not_fatal(self):
|
||||
"""It is a subject line: render the preview without it."""
|
||||
service = _service()
|
||||
service.list_commits = AsyncMock(return_value={"success": True, "commits": []})
|
||||
backend = MagicMock()
|
||||
backend.get_commit = AsyncMock(return_value={"success": False, "message": "boom", "commit": None})
|
||||
|
||||
with patch("backend.app.services.github_restore.get_provider_backend", return_value=backend):
|
||||
assert await service._describe_commit(self._config(), "a" * 40) is None
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_the_window_scan_is_not_run_twice(self):
|
||||
"""_resolve_ref already listed commits for HEAD; preview reuses that."""
|
||||
service = _service()
|
||||
tip = self._entry("t" * 40)
|
||||
service.list_commits = AsyncMock(return_value={"success": True, "commits": [tip]})
|
||||
|
||||
resolved, _, commit = await service._resolve_ref(self._config(), "HEAD")
|
||||
|
||||
assert resolved == "t" * 40
|
||||
assert commit == tip
|
||||
assert service.list_commits.await_count == 1
|
||||
|
||||
Reference in New Issue
Block a user