Преглед изворни кода

fix(backup): dedupe spools and usage history on a comparison that can match (#2656)

Both `created_at` columns the restore dedupes on are
`server_default=func.now()`. SQLite fills those from `CURRENT_TIMESTAMP`,
which has second precision and stores `'2026-08-02 11:28:41'`, while
SQLAlchemy binds a Python datetime as `'2026-08-02 11:28:41.000000'`.
SQLite compares the two as strings, so `Model.created_at == created_at`
never matched a row the application itself created — not even when handed
that row's own value straight back out of the ORM.

Every dedupe keyed on it therefore missed, on the ordinary case rather
than an edge one:

* `_find_spool`'s composite fallback duplicated every tag-less spool on
  each restore, and `overwrite_existing=True` never reached the original;
* the usage-history dedupe re-inserted the user's entire consumption
  history on each restore.

Rows the restore itself had inserted did match, because those carry an
explicit bind in the same microsecond format — which is why the existing
repeat-restore tests passed throughout.

Fixed by filtering the candidates in SQL and comparing `created_at` in
Python, which sidesteps the bind format and behaves identically on
PostgreSQL, where the column keeps microseconds and the SQL comparison
happened to work. `_parse_dt` now also normalises an offset-bearing value
to naive UTC, matching what the naive columns actually hold; the collector
never writes one, so that guards hand-edited and foreign backups.

Seven tests, six of which fail without the fix. They seed the "existing"
row the way the application does — no explicit `created_at` — which is
what the existing coverage was missing.
jmoore-skild пре 1 месец
родитељ
комит
329b506240
2 измењених фајлова са 190 додато и 8 уклоњено
  1. 46 7
      backend/app/services/github_restore.py
  2. 144 1
      backend/tests/unit/test_github_restore.py

+ 46 - 7
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
 

+ 144 - 1
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 = {