public_git/bambuddy
1
0
Fork 0
mirror of https://github.com/maziggy/bambuddy.git synced 2026-10-07 06:31:22 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix(backup): don't let an old backup commit blank an archive's owner (#2656)

Browse Source 
`created_by_id` and `deleted_at` both went into the archive `fields` dict
    unconditionally, via `entry.get(...)`. A backup commit taken before the
    collector wrote those keys carries neither, so `.get` yielded None for both
    and the overwrite branch — a blanket `setattr` over every key — wrote NULL
    over a live owner.

    That is exactly the failure carrying `created_by_id` was added to fix, only
    now inflicted on rows that were fine: `_ensure_archive_visible` fails closed
    on a NULL owner, so the archive 404s for the person who owns it. It emitted
    no note either, because `archivesOwnerCleared` only fires for an id that
    isn't in `valid_users`, not for an absent key — and the row still counted as
    restored. `deleted_at` had the mirror problem: an old commit silently
    un-deleted, since `archivesUndeleted` reads the same absent value.

    Absent is not the same as explicitly null. Both keys now only enter `fields`
    when the entry actually carries them, so an old commit leaves the column
    alone on overwrite and a current one can still say "this archive has no
    owner" or "this archive is live". Same shape as the tag-column rule: don't
    clear what the backup doesn't know about.

    Behaviour change to an existing test, called out deliberately:
    `test_overwrite_undeletes_a_locally_deleted_archive_and_says_so` now has to
    put `deleted_at: None` in the entry to mean it.

    Tests: 2 regression (owner and deleted_at both left alone by a key-less
    entry) + 2 controls (an explicit null is still honoured, with its note).
    Both regressions confirmed failing against the pre-fix service.
This commit is contained in:
maziggy
2026-08-15 14:14:04 +02:00
parent be17b62480 be8e8d545f
commit 5015b02760
2 changed files with 148 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/app/services/github_restore.py
+31 -20
View File
@@ -899,12 +899,6 @@ class GitHubRestoreService:
"quantity": entry.get("quantity") or 1,
"energy_kwh": entry.get("energy_kwh"),
"energy_cost": entry.get("energy_cost"),
# A soft-deleted archive is still in the backup (its row is kept
# so stats keep counting it), so carry the flag across or the
# restore turns something the user deleted back into a visible
# archive. Backups written before this key existed have no
# deleted_at, and those rows can only come back live.
"deleted_at": _parse_dt(entry.get("deleted_at")),
}
printer_id = entry.get("printer_id")
@@ -919,21 +913,38 @@ class GitHubRestoreService:
"archivesProjectMissing", "Some archives referenced projects that no longer exist — link cleared"
)
project_id = None
created_by_id = entry.get("created_by_id")
if created_by_id is not None and created_by_id not in valid_users:
# Coerced rather than failing the row: the archive is still worth
# having, and an admin can reassign it. Said out loud because a
# cleared owner is not silent-safe — the archive becomes visible
# only to archives:read_all until someone does.
tally.note(
"archivesOwnerCleared",
"Some archives referenced users that no longer exist — owner cleared, so they are "
"visible only to users with the archives:read_all permission until an admin reassigns them",
)
created_by_id = None
fields["printer_id"] = printer_id
fields["project_id"] = project_id
fields["created_by_id"] = created_by_id
# created_by_id and deleted_at are the two late arrivals — a backup
# commit taken before the collector wrote them carries neither key.
# Absent is NOT the same as null here, because the overwrite branch
# below is a blanket setattr: treating a missing key as None would
# write NULL over a live owner (_ensure_archive_visible then 404s the
# archive for the very user who owns it — the failure carrying the
# column was added to fix) and silently un-delete a row the user
# deleted. So only carry a column the backup actually knows about;
# on insert, an absent key just takes the model default.
if "created_by_id" in entry:
created_by_id = entry.get("created_by_id")
if created_by_id is not None and created_by_id not in valid_users:
# Coerced rather than failing the row: the archive is still
# worth having, and an admin can reassign it. Said out loud
# because a cleared owner is not silent-safe — the archive
# becomes visible only to archives:read_all until someone does.
tally.note(
"archivesOwnerCleared",
"Some archives referenced users that no longer exist — owner cleared, so they are "
"visible only to users with the archives:read_all permission until an admin reassigns them",
)
created_by_id = None
fields["created_by_id"] = created_by_id
if "deleted_at" in entry:
# A soft-deleted archive is still in the backup (its row is kept
# so stats keep counting it), so carry the flag across or the
# restore turns something the user deleted back into a visible
# archive.
fields["deleted_at"] = _parse_dt(entry.get("deleted_at"))
if existing is not None:
if old_id is not None:
@@ -945,7 +956,7 @@ class GitHubRestoreService:
# includes un-deleting one the user deleted after the backup was
# taken. Legitimate, but not obvious from a restored/skipped
# count, so say it.
if existing.deleted_at is not None and fields["deleted_at"] is None:
if existing.deleted_at is not None and "deleted_at" in fields and fields["deleted_at"] is None:
tally.note(
"archivesUndeleted",
"Archive(s) deleted since the backup are visible again — overwrite was on",
backend/tests/unit/test_github_restore.py
+117 -1
View File
@@ -1063,6 +1063,12 @@ class TestRestoreArchives:
@pytest.mark.asyncio
async def test_overwrite_undeletes_a_locally_deleted_archive_and_says_so(self, db_session):
"""The entry has to *say* the archive was live — absent no longer means null.
A commit taken before the collector wrote ``deleted_at`` carries no
opinion about it, and overwrite now leaves the column alone in that
case; see ``TestRestoredArchiveOwnership``.
"""
db_session.add(
PrintArchive(
filename="benchy.3mf",
@@ -1076,7 +1082,9 @@ class TestRestoreArchives:
await db_session.commit()
tally = _CategoryTally()
await _service()._restore_archives(db_session, {"archives": [self._archive_entry()]}, True, tally, {})
await _service()._restore_archives(
db_session, {"archives": [self._archive_entry(deleted_at=None)]}, True, tally, {}
)
await db_session.commit()
row = (await db_session.execute(select(PrintArchive))).scalar_one()
@@ -1571,6 +1579,114 @@ class TestRestoredArchiveOwnership:
row = (await db_session.execute(select(PrintArchive))).scalar_one()
assert row.created_by_id == alice.id
@pytest.mark.asyncio
async def test_overwrite_leaves_the_owner_alone_when_the_backup_predates_the_key(self, db_session):
"""A pre-#2656 commit must not blank the owner of a row that was fine.
The entry carries no ``created_by_id`` at all, so there is nothing to
write. Treating that as an explicit null inflicted the exact bug the
column was added to fix — a 404 for the owner — on rows the restore had
no business touching, silently, while still counting them restored.
"""
bob = await self._user(db_session, "bob")
db_session.add(
PrintArchive(
filename="benchy.3mf",
file_path="/data/benchy.3mf",
file_size=2048,
content_hash="abc123",
started_at=datetime(2026, 3, 1, 10, 0, 0),
created_by_id=bob.id,
)
)
await db_session.commit()
tally = _CategoryTally()
await _service()._restore_archives(db_session, {"archives": [self._entry()]}, True, tally, {})
await db_session.commit()
row = (await db_session.execute(select(PrintArchive))).scalar_one()
assert row.created_by_id == bob.id, "an old backup does not know the owner, so it must not clear one"
assert tally.restored == 1
@pytest.mark.asyncio
async def test_overwrite_leaves_deleted_at_alone_when_the_backup_predates_the_key(self, db_session):
"""The mirror case: an old commit must not un-delete, and must not claim to.
``archivesUndeleted`` reads the same absent value, so the un-delete was
not merely wrong but unannounced.
"""
db_session.add(
PrintArchive(
filename="benchy.3mf",
file_path="/data/benchy.3mf",
file_size=2048,
content_hash="abc123",
started_at=datetime(2026, 3, 1, 10, 0, 0),
deleted_at=datetime(2026, 3, 4, 8, 0, 0),
)
)
await db_session.commit()
tally = _CategoryTally()
await _service()._restore_archives(db_session, {"archives": [self._entry()]}, True, tally, {})
await db_session.commit()
row = (await db_session.execute(select(PrintArchive))).scalar_one()
assert row.deleted_at == datetime(2026, 3, 4, 8, 0, 0), "an old backup must not resurrect a deleted archive"
assert not any("visible again" in note for note in _messages(tally))
@pytest.mark.asyncio
async def test_overwrite_still_clears_an_owner_the_backup_explicitly_nulls(self, db_session):
"""Control: absent is ignored, but an explicit null is still honoured.
A current-format backup of an unowned archive has to be able to say so,
or overwrite stops meaning "make the local row match the backup".
"""
bob = await self._user(db_session, "bob")
db_session.add(
PrintArchive(
filename="benchy.3mf",
file_path="/data/benchy.3mf",
file_size=2048,
content_hash="abc123",
started_at=datetime(2026, 3, 1, 10, 0, 0),
created_by_id=bob.id,
)
)
await db_session.commit()
await _service()._restore_archives(
db_session, {"archives": [self._entry(created_by_id=None)]}, True, _CategoryTally(), {}
)
await db_session.commit()
row = (await db_session.execute(select(PrintArchive))).scalar_one()
assert row.created_by_id is None
@pytest.mark.asyncio
async def test_overwrite_still_undeletes_when_the_backup_explicitly_nulls(self, db_session):
"""Control for the deleted_at half, with the note that goes with it."""
db_session.add(
PrintArchive(
filename="benchy.3mf",
file_path="/data/benchy.3mf",
file_size=2048,
content_hash="abc123",
started_at=datetime(2026, 3, 1, 10, 0, 0),
deleted_at=datetime(2026, 3, 4, 8, 0, 0),
)
)
await db_session.commit()
tally = _CategoryTally()
await _service()._restore_archives(db_session, {"archives": [self._entry(deleted_at=None)]}, True, tally, {})
await db_session.commit()
row = (await db_session.execute(select(PrintArchive))).scalar_one()
assert row.deleted_at is None
assert any("visible again" in note for note in _messages(tally))
@pytest.mark.asyncio
async def test_owner_survives_collect_then_restore(self, db_session):
"""Both halves, because each looks harmless alone.