diff --git a/backend/app/services/github_restore.py b/backend/app/services/github_restore.py index 09dadc981..6cd0ba1f1 100644 --- a/backend/app/services/github_restore.py +++ b/backend/app/services/github_restore.py @@ -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", diff --git a/backend/tests/unit/test_github_restore.py b/backend/tests/unit/test_github_restore.py index 020484061..7a97e4317 100644 --- a/backend/tests/unit/test_github_restore.py +++ b/backend/tests/unit/test_github_restore.py @@ -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.