mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-10-05 13:41:36 +02:00
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.
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
@@ -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 = {
|
||||
|
||||
Reference in New Issue
Block a user