diff --git a/backend/app/services/github_restore.py b/backend/app/services/github_restore.py index 899ad96bc..0b8293219 100644 --- a/backend/app/services/github_restore.py +++ b/backend/app/services/github_restore.py @@ -123,13 +123,45 @@ _KNOWN_NOZZLES = {"0.2", "0.4", "0.6", "0.8"} def _parse_dt(value) -> datetime | None: - """Best-effort parse of a datetime the backup wrote via ``str(...)``.""" + """Best-effort parse of a datetime the backup wrote via ``str(...)``. + + Normalised to naive UTC, because that is what every ``DateTime`` column + here holds: the models write ``datetime.now(timezone.utc)`` into naive + columns and both dialects drop the offset on the way in. Carrying an aware + value through would store the wrong wall clock, and comparing one against a + value read back out of a naive column raises ``TypeError``. The collector + only ever writes naive strings, so this is a guard on hand-edited or + foreign backups rather than a path Bambuddy takes itself. + """ if not value or not isinstance(value, str): return None try: - return datetime.fromisoformat(value) + parsed = datetime.fromisoformat(value) except ValueError: return None + if parsed.tzinfo is not None: + parsed = parsed.astimezone(timezone.utc).replace(tzinfo=None) + return parsed + + +def _created_at_matches(row, created_at: datetime | None) -> bool: + """Does ``row.created_at`` equal a timestamp read out of a backup? + + Compared in Python, not in SQL, and that is the whole point. Every + ``created_at`` these callers dedupe on is ``server_default=func.now()``, so + SQLite fills it from ``CURRENT_TIMESTAMP``, which has second precision and + stores ``'2026-08-02 11:28:41'``. SQLAlchemy binds a Python datetime as + ``'2026-08-02 11:28:41.000000'``, and SQLite compares the two as strings — + so ``Model.created_at == created_at`` never matches a row the application + itself created, not even when handed that row's own value straight back. + Every dedupe keyed on it misses, and the restore inserts a duplicate of + everything instead of recognising what is already there. + + Reading the candidates back and comparing the parsed datetimes sidesteps + the bind format entirely, and is equally correct on PostgreSQL (where the + column keeps microseconds and the SQL comparison happened to work). + """ + return created_at is not None and row.created_at == created_at def _is_blocked_setting_key(key: str) -> bool: @@ -1200,16 +1232,19 @@ class GitHubRestoreService: created_at = _parse_dt(entry.get("created_at")) if created_at is None: return None, None + # created_at is filtered in Python, not here — see _created_at_matches. result = await db.execute( select(Spool).where( - Spool.created_at == created_at, Spool.material == (entry.get("material") or "PLA"), Spool.brand == entry.get("brand"), Spool.subtype == entry.get("subtype"), Spool.color_name == entry.get("color_name"), ) ) - return result.scalars().first(), None + for row in result.scalars(): + if _created_at_matches(row, created_at): + return row, None + return None, None @staticmethod async def _guard_tag_overwrite(db: AsyncSession, existing: Spool, fields: dict, matched_on: str | None) -> int: @@ -1295,16 +1330,20 @@ class GitHubRestoreService: created_at = _parse_dt(entry.get("created_at")) # Usage history has no natural key of its own, so dedupe on the - # tuple that makes a consumption event unique in practice. + # tuple that makes a consumption event unique in practice. As in + # _find_spool, created_at is compared in Python — see + # _created_at_matches. An entry carrying no created_at at all + # cannot be recognised and is re-inserted, which is what the + # IS NULL comparison this replaced did too: the column is + # non-nullable, so it never matched either. existing = await db.execute( select(SpoolUsageHistory).where( SpoolUsageHistory.spool_id == spool_id, - SpoolUsageHistory.created_at == created_at, SpoolUsageHistory.weight_used == (entry.get("weight_used") or 0), SpoolUsageHistory.print_name == entry.get("print_name"), ) ) - if existing.scalars().first() is not None: + if any(_created_at_matches(row, created_at) for row in existing.scalars()): tally.skipped += 1 continue diff --git a/backend/tests/unit/test_github_restore.py b/backend/tests/unit/test_github_restore.py index d6b7f8431..62f2a84c4 100644 --- a/backend/tests/unit/test_github_restore.py +++ b/backend/tests/unit/test_github_restore.py @@ -6,7 +6,7 @@ dependent rows, overwrite-vs-skip, the settings credential blocklist, and the K-profile paths that depend on live printers. """ -from datetime import datetime +from datetime import datetime, timedelta from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock, patch @@ -67,6 +67,13 @@ class TestParseDt: def test_returns_none_for_junk(self, value): assert _parse_dt(value) is None + def test_an_offset_is_normalised_to_naive_utc(self): + """Every DateTime column here is naive UTC; an aware value cannot be + written to one without silently shifting the wall clock, nor compared + against one without raising.""" + assert _parse_dt("2026-07-27T08:02:05+02:00") == datetime(2026, 7, 27, 6, 2, 5) + assert _parse_dt("2026-07-27T06:02:05+00:00").tzinfo is None + class TestSettingKeyBlocklist: @pytest.mark.parametrize( @@ -976,6 +983,142 @@ class TestRestoreSpools: assert row.printer_id is None +class TestServerDefaultCreatedAtDedupe: + """Dedupe against rows whose ``created_at`` came from the server default. + + Every test above seeds its "existing" row through the restore itself, which + binds ``created_at`` explicitly — so both sides end up in SQLAlchemy's + microsecond format and a SQL ``==`` matches. Rows the *application* created + do not: SQLite fills ``server_default=func.now()`` from + ``CURRENT_TIMESTAMP``, which has second precision, and the two strings + never compare equal. That is the ordinary case — a user's own spools and + their print history — and it duplicated the lot on every restore. + """ + + @staticmethod + async def _native_spool(db_session, **kwargs): + """A spool created the way the app creates one: no explicit created_at.""" + spool = Spool(material="PLA", brand="Bambu Lab", subtype="Basic", color_name="Jade White", **kwargs) + db_session.add(spool) + await db_session.commit() + await db_session.refresh(spool) + return spool + + def _entry_for(self, spool, **overrides): + """The backup entry the collector writes for ``spool``.""" + entry = { + "id": 41, + "material": spool.material, + "brand": spool.brand, + "subtype": spool.subtype, + "color_name": spool.color_name, + "created_at": str(spool.created_at), + } + entry.update(overrides) + return entry + + @pytest.mark.asyncio + async def test_find_spool_matches_on_the_composite_fallback(self, db_session): + spool = await self._native_spool(db_session) + + found, matched_on = await _service()._find_spool(db_session, self._entry_for(spool)) + + assert found is not None and found.id == spool.id + assert matched_on is None # the composite, not a tag column + + @pytest.mark.asyncio + async def test_a_tagless_spool_is_not_duplicated(self, db_session): + spool = await self._native_spool(db_session) + payload = {"spools": [self._entry_for(spool)]} + tally = _CategoryTally() + + await _service()._restore_spools(db_session, payload, None, False, tally, {}) + await db_session.commit() + + assert len((await db_session.execute(select(Spool))).scalars().all()) == 1 + assert tally.skipped == 1 + + @pytest.mark.asyncio + async def test_overwrite_updates_the_original_instead_of_inserting(self, db_session): + spool = await self._native_spool(db_session) + payload = {"spools": [self._entry_for(spool, weight_used=250.0)]} + + await _service()._restore_spools(db_session, payload, None, True, _CategoryTally(), {}) + await db_session.commit() + + row = (await db_session.execute(select(Spool))).scalar_one() + assert row.id == spool.id + assert row.weight_used == 250.0 + + @pytest.mark.asyncio + async def test_a_second_spool_added_later_stays_distinct(self, db_session): + """The composite is only unique because of created_at, so the Python + comparison has to stay exact — not a same-day tolerance.""" + spool = await self._native_spool(db_session) + twin = Spool(material=spool.material, brand=spool.brand, subtype=spool.subtype, color_name=spool.color_name) + twin.created_at = spool.created_at + timedelta(hours=1) + db_session.add(twin) + await db_session.commit() + + found, _ = await _service()._find_spool(db_session, self._entry_for(spool)) + + assert found.id == spool.id + + @pytest.mark.asyncio + async def test_existing_usage_history_is_not_re_inserted(self, db_session): + spool = await self._native_spool(db_session, tag_uid="AABBCCDD") + usage_row = SpoolUsageHistory(spool_id=spool.id, print_name="b.3mf", weight_used=5.0) + db_session.add(usage_row) + await db_session.commit() + await db_session.refresh(usage_row) + + tally = _CategoryTally() + inventory = {"spools": [self._entry_for(spool, tag_uid="AABBCCDD")]} + usage = { + "usage_history": [ + { + "spool_id": 41, + "print_name": "b.3mf", + "weight_used": 5.0, + "created_at": str(usage_row.created_at), + } + ] + } + + await _service()._restore_spools(db_session, inventory, usage, False, tally, {}) + await db_session.commit() + + rows = (await db_session.execute(select(SpoolUsageHistory))).scalars().all() + assert len(rows) == 1 + assert tally.skipped == 2 # the spool and its one usage row + + @pytest.mark.asyncio + async def test_a_genuinely_new_usage_row_still_lands(self, db_session): + """Dedupe by timestamp must not swallow a repeat of the same print.""" + spool = await self._native_spool(db_session, tag_uid="AABBCCDD") + usage_row = SpoolUsageHistory(spool_id=spool.id, print_name="b.3mf", weight_used=5.0) + db_session.add(usage_row) + await db_session.commit() + await db_session.refresh(usage_row) + + inventory = {"spools": [self._entry_for(spool, tag_uid="AABBCCDD")]} + usage = { + "usage_history": [ + { + "spool_id": 41, + "print_name": "b.3mf", + "weight_used": 5.0, + "created_at": str(usage_row.created_at + timedelta(days=1)), + } + ] + } + + await _service()._restore_spools(db_session, inventory, usage, False, _CategoryTally(), {}) + await db_session.commit() + + assert len((await db_session.execute(select(SpoolUsageHistory))).scalars().all()) == 2 + + class TestRestoreArchives: def _archive_entry(self, **overrides): entry = {