feat: import an iOS or cross-fork NOOP backup by row-copy instead of rejecting it#746
feat: import an iOS or cross-fork NOOP backup by row-copy instead of rejecting it#746tanarchytan wants to merge 1 commit into
Conversation
ryanbr
left a comment
There was a problem hiding this comment.
Really nicely architected — the copy-live-first / atomic-swap / snapshot-rollback / post-swap re-verify design is careful, the row-copy is properly transactional, housekeeping/identity tables are correctly left out, and the Swift twin actually passes CI on macOS (better than the "unverified" caveat suggested). Two blockers before it can land, plus a few smaller items. The first is a data-loss hole I verified end to end.
🔴 Blocker 1 — CRITICAL: constant-filling a key column can silently destroy a table's rows
planRowCopyImport (DataBackup.kt:866-869) fills a source-absent NOT NULL-no-default target column with a constant typed-zero, with no PK/UNIQUE awareness (SchemaColumn doesn't read the pk flag). If a foreign backup names any primary-key or UNIQUE-component column differently from this build's schema:
- the key column is filled with the same constant for every row;
DELETE FROM t+INSERT OR IGNORE(:880-881) then treats every row after the first as a PK collision and silently skips it → the table collapses to one row;- the post-swap guard is only a structural
quick_check(:380), which a valid 1-row table passes; rollbackFile.delete()(:426) then discards the only intact copy. There is no row-count verification anywhere.
Scenario: a cross-fork (or iOS/GRDB) backup whose hrSample timestamp is timestamp vs this build's ts → thousands of samples reduced to one, rollback deleted, and the only signal is a reassuring "hrSample: filled ts with defaults." The trigger is narrow for iOS↔Android when PK column names happen to align, but cross-fork import is an explicit target here and has zero defense.
Fix: make the fill PK/UNIQUE-aware — read pk from PRAGMA table_info and unique indexes from PRAGMA index_list/index_info; if a source-absent NOT-NULL-no-default column is a key member, abort that table (keep the live rows) or fail the import loudly — never constant-fill a key column. Independently, add a source-vs-landed row-count sanity check before deleting the rollback snapshot, so any large shortfall aborts instead of committing. (The same INSERT OR IGNORE fill path also silently drops rows on a CHECK-constraint violation — same fix.)
🔴 Blocker 2 — CI red: i18n gate, 14 new hardcoded Android literals
The confirm dialog + warning strings in BackupSyncScreen.kt/SettingsScreen.kt are hardcoded Compose Text("…") literals rather than stringResource. The diff-scoped gate reports FAIL 14 NEW hardcoded literal(s). They need extracting to strings.xml and translating for the focus locales — note that's de/es/fr and pt-PT now (#769 landed pt-PT as a focus locale).
🟠 Parity — typedZeroLiteral diverges on an untyped column
DataBackup.kt:895 returns x'' for an empty/untyped declared type; the Swift twin (ForeignBackupImport.swift:210) falls through to 0. Same .noopbak → byte-different filled rows depending on platform, which breaks the pure-planner parity contract. Neither test suite feeds an untyped-type column, so the shared golden vectors miss it. Likely unreachable in today's real Room/GRDB schemas (both always emit an affinity keyword), but please align the two and add a test vector for the empty-type case.
🟠 Medium — SettingsScreen success path doesn't auto-restart (#57 stale-DAO)
importFrom leaves Room closed after the swap. BackupSyncScreen force-relaunches on NeedsRestart, but SettingsScreen.runImport only shows a toast. This PR routes the new cross-fork import through SettingsScreen, so a successful import lands the app in exactly the #57 state (running app, closed DAOs, empty-history ENDs acking/trimming the strap). Please call restartNoop on the NeedsRestart success branch there too, matching BackupSyncScreen.
Lower / hardening (non-blocking)
- Parity, low:
holdsDatafor an unknown-origin file differs — Apple reconciles it, Android refuses (DataBackup.swift:345vsDataBackup.kt:613-616/:259). Only affects non-NOOP files, but it's an accept-vs-reject asymmetry. - Low: source table/column names from the foreign file are interpolated into
PRAGMAwith backtick-only quoting and no validation (readSchema~:914). Impact is bounded (single-statementrawQuery; all mutating SQL uses trusted target names), so worst case is a thrown exception → safeFailed. Still worth doubling embedded backticks / validating against[A-Za-z0-9_]+.
What's solid (verified, no action)
Confirmation gate runs before anything touches the live DB; WAL checkpoint(TRUNCATE) + close before the copy (quiescent, not a torn read); the multi-table row-copy is a single transaction; SQL errors throw and the scratch file is cleaned up on any failure; INSERT OR IGNORE never overwrites; REPLACE clears / MERGE keeps; housekeeping + identity tables are never copied (Room identity is inherited from the live-store copy); and the planner is at parity on statement ordering (deterministic .sorted()), the housekeeping set, and the column-intersection rule.
The design is close — fix the key-column fill (with a row-count backstop) and extract the strings, and this becomes a genuinely valuable feature. Happy to re-review once those land.
Restore refused any .noopbak that was not this app's own Room store, so a backup from the iOS/Mac build or another fork was hard-rejected. Add an opt-in reconcile path: when a picked backup is foreign but its data can be brought over, copy the live store (keeping this app's schema and Room identity), row-copy the backup's shared tables and columns into that copy, then land it through the normal snapshot, swap and rollback path. Same-app restores are unchanged and still take the fast file swap. Routing is by schema content, never by version: GRDB reports user_version 0 and forks reuse the same Room version integers, so the classifier keys off the tables and columns a backup actually carries. A row copy fills a NOT NULL column the source lacks with a typed zero so pre-column rows are kept, and every insert is INSERT OR IGNORE. Includes the Swift twin for the parity contract (unverified on macOS) and pure planner + detection tests on both platforms.
|
Thanks for the thorough review. Rebased onto main and addressed everything. Blocker 1 (key-column data loss). Blocker 2 (i18n). Extracted the confirm/warning/failed dialog strings in typedZeroLiteral parity. Aligned the Kotlin untyped case to the Swift #57 stale-DAO. PRAGMA quoting. Added a Swift twin is updated to match but I can't build it (no Mac), same caveat as before. Also opened #775 for a Room/GRDB schema-parity test, since this import relies on the two schemas agreeing on shared column/key names (that is where the rrInterval |
acd61bb to
f4cca45
Compare
|
Re-reviewed the update — both blockers from the last pass are resolved, nicely:
One trivial thing is keeping CI red —
Low-severity nit while you're in there: the Really close — fix the |
ryanbr
left a comment
There was a problem hiding this comment.
Careful piece of work. Routing by schema content rather than version is the right call given GRDB always reports user_version 0, and I verified the two claims that matter most rather than taking them:
- The destructive path never touches the live store.
reconcileForeignBackupcopiesliveDbFileinto a scratch file incacheDirand the REPLACEDELETE FROM main.*runs against that copy, with the backup ATTACHed as source. Live DB is only ever read. - It survives today's schema change. #830 landed Room v24 / GRDB v30 this morning, adding a nullable
ordtorrInterval. BecauseplanRowCopyImportreadsPRAGMA table_infoat runtime instead of a fixed column list, an older backup simply yields NULL there — which is exactly #830's "emission order unknown". Nothing to update.
Three things:
-
CI is red on one line.
ForeignBackupImportTests.swift:363—ReconcileErrorneeds: Equatable. Precisely the class of thing you flagged as unverifiable on a non-Apple host. -
Taking your split offer. Even after that one-word fix, 446 lines of Swift plus 411 of tests have never executed anywhere. The Android half is unit-tested and device-validated on a real SM-G996B; the Swift half is unverified. Land Android first, Swift twin as a follow-up once its tests actually run on a Mac. The parity rule allows deferring with a stated reason, and "the twin has never been executed" is a good one.
-
Yes please to the androidTest. This is the one path that can destroy a user's history — worth having a real end-to-end run in CI rather than only the pure planner tests, and worth me doing the instrumented
reconcileForeignBackuprun you asked for before it lands.
Also worth a rebase — main has moved a fair way in three days.
What
Restore currently refuses any
.noopbakthat isn't this app's own Room store. Pick a backup from the iOS/Mac build (GRDB) or from another Android NOOP fork and you get a hard rejection, because a raw file swap of a schema this build can't migrate would strand or wipe the store.This adds an opt-in path. When the picked backup is foreign but its data can be brought over, the app offers to reconcile it. On confirmation it copies your live store (so the result keeps this app's exact schema and Room identity) and row-copies the backup's data into that copy, then lands the file through the normal snapshot, swap and rollback path. Same-app restores are untouched and still take the fast file swap.
Routing is by schema content, never by version: GRDB always reports
user_version0 and forks reuse the same Room version integers, so the classifier keys off the tables and columns a backup actually carries (grdb_migrationsmeans the iOS/Mac store; a Room store with a table or column this build's schema lacks means another Android fork). A backup that is merely an older or behind version of this app still restores the ordinary way, and a file that holds data but carries neither migrator's bookkeeping is still refused.Why
Users move between the iOS and Android builds, and between forks, and expect their history to come with them. The only prior answer was "export a WHOOP CSV on the other device", which drops everything the CSV does not carry. A row copy keyed on the shared tables and columns preserves far more, without pretending a foreign schema is migratable.
Data safety
INSERT OR IGNORE, so a primary-key clash is skipped, never overwritten. REPLACE clears each shared table first, MERGE keeps existing rows. Housekeeping tables (identity, autoincrement, migration ledger) are never copied.Files
android/app/src/main/java/com/noop/data/DataBackup.kt: content classifier (foreignBackupKind), the pure version-agnostic planner (planRowCopyImport), the SQLite executor (reconcileForeignBackup), and theNeedsConfirmation/FailedNeedsRestartresults the UI keys on.android/app/src/main/java/com/noop/ui/BackupSyncScreen.kt,android/app/src/main/java/com/noop/ui/SettingsScreen.kt: the confirm dialog and the post-restore warning surface.Strand/Data/DataBackup.swift,Strand/Screens/SettingsView.swift: the Swift app layer, same routing and confirm flow.Packages/WhoopStore/Sources/WhoopStore/ForeignBackupImport.swift: the Swift twin for the parity contract, the same pure planner plus a GRDB executor.Tests
CrossForkImportPlanTest.kt: the planner, four source schema shapes against this app's own store as the target, asserting the exact emitted statements, REPLACE clears-then-inserts ordering, MERGE emits no DELETE, every insert is OR IGNORE, the typed-zero fill for a source-absent NOT NULL column, and the warnings.CrossForkImportDetectionTest.kt: the content classifier (foreignBackupKindcross-checked againstbackupOriginOfandholdsData).CrossForkSchemaFixtures.kt: the shared table-to-columns descriptors that drive both pure tests with no live SQLite.ForeignBackupImportTests.swift: the Swift twin, the pure planner against the same synthetic schemas, plus a GRDB executor test on real files.Verification
./gradlew testFullDebugUnitTest --tests "com.noop.data.CrossFork*"), and the whole module compiles.reconcileForeignBackupon this tree before merge is still worth doing, and I am happy to add an androidTest for it.swift test --filter ForeignBackupImportTestson a Mac before merge. Happy to split the Swift twin into its own PR if you would rather land the Android change first.Known minor items (not blockers)