Skip to content

feat: import an iOS or cross-fork NOOP backup by row-copy instead of rejecting it#746

Open
tanarchytan wants to merge 1 commit into
ryanbr:mainfrom
tanarchytan:feat/cross-fork-backup-import
Open

feat: import an iOS or cross-fork NOOP backup by row-copy instead of rejecting it#746
tanarchytan wants to merge 1 commit into
ryanbr:mainfrom
tanarchytan:feat/cross-fork-backup-import

Conversation

@tanarchytan

Copy link
Copy Markdown

What

Restore currently refuses any .noopbak that 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_version 0 and forks reuse the same Room version integers, so the classifier keys off the tables and columns a backup actually carries (grdb_migrations means 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

  • The confirmation gate runs before anything touches the live DB, so declining (or any failure during reconcile) leaves the existing store exactly as it was, and the app restarts cleanly on the intact live DB.
  • Schema probes open the file read-only, fall back to a read-write open for WAL files that cannot be opened read-only on older SQLite, and return a null descriptor (refused, not a silent swap) when a file genuinely cannot be read.
  • A reconcile that hits a SQL error throws rather than returning a half-written file, so a torn copy can never masquerade as a good restore. Its scratch file is cleaned up on any failure.
  • The row copy fills NOT NULL columns the source lacks with a typed zero, so a fork that predates a column keeps its rows instead of dropping them on insert. Every insert is 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 the NeedsConfirmation / FailedNeedsRestart results 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 (foreignBackupKind cross-checked against backupOriginOf and holdsData).
  • 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

  • Android JVM unit tests pass locally (./gradlew testFullDebugUnitTest --tests "com.noop.data.CrossFork*"), and the whole module compiles.
  • The row-copy design (content routing plus the table/column intersection) was exercised end to end on a Samsung SM-G996B (Android 15): a GRDB backup reconciled into a Room v-current store and the result reopened through Room. A maintainer instrumented run of reconcileForeignBackup on this tree before merge is still worth doing, and I am happy to add an androidTest for it.
  • The Swift twin is included for the parity contract but is unverified on macOS (authored on a non-Apple host; WhoopStore is GRDB-linked so it does not build on Linux). It needs swift test --filter ForeignBackupImportTests on 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)

  • The typed-zero fill is byte-identical across Android and Swift for every column in the schema. The only divergence would be an untyped declared column, which neither Room nor GRDB ever emits, so it is unreachable.
  • Filled-column warning wording differs slightly between the two platforms; the data outcome is identical.

@ryanbr ryanbr left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: holdsData for an unknown-origin file differs — Apple reconciles it, Android refuses (DataBackup.swift:345 vs DataBackup.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 PRAGMA with backtick-only quoting and no validation (readSchema ~:914). Impact is bounded (single-statement rawQuery; all mutating SQL uses trusted target names), so worst case is a thrown exception → safe Failed. 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.
@tanarchytan

Copy link
Copy Markdown
Author

Thanks for the thorough review. Rebased onto main and addressed everything.

Blocker 1 (key-column data loss). SchemaColumn/ColumnInfo now read pk from PRAGMA table_info plus UNIQUE indexes from index_list/index_info. A source-absent NOT NULL-no-default column that is a key is no longer constant-filled. Rather than skip the table (which drops all its rows), it is filled with the source rowid, a per-row-unique value, so the rows import and INSERT OR IGNORE can never collapse them. That matters in practice: an older or other-fork backup can lack only a disambiguator key (e.g. rrInterval's seq), and skipping would drop the whole R-R series. Added a row-count backstop in the reconcile too: on a REPLACE, if a copied table lands fewer rows than the source held it throws before the live store is touched (catches a CHECK violation or a source-side UNIQUE collision).

Blocker 2 (i18n). Extracted the confirm/warning/failed dialog strings in BackupSyncScreen and SettingsScreen to strings.xml, translated for de, es, fr and pt-PT. The diff-scoped gate is clean.

typedZeroLiteral parity. Aligned the Kotlin untyped case to the Swift 0, and added the empty-type vector to both suites.

#57 stale-DAO. SettingsScreen.runImport now relaunches on the success branch (matching BackupSyncScreen), and its warnings dialog relaunches on dismiss.

PRAGMA quoting. Added a quoteId that doubles embedded backticks, used wherever a foreign identifier is interpolated.

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 seq gap traces back to).

@tanarchytan
tanarchytan force-pushed the feat/cross-fork-backup-import branch from acd61bb to f4cca45 Compare July 24, 2026 09:53
@ryanbr

ryanbr commented Jul 24, 2026

Copy link
Copy Markdown
Owner

Re-reviewed the update — both blockers from the last pass are resolved, nicely:

  • Data-loss (the critical one): the fill is now PK/UNIQUE-aware (keyColumns() off PRAGMA table_info.pk + index_list/index_info) on both platforms, and a source-absent key column is filled with the source rowid (per-row-unique) instead of a constant, so INSERT OR IGNORE no longer collapses the table — plus the row-count backstop. That closes the silent mass-loss hole. (Minor: a rowid-filled key column gives those rows synthetic key values — only on the rare cross-fork renamed-key case, and it's tracked in synthKeyCols + warned, so acceptable over dropping rows.)
  • i18n gate: clean now — --ci reports no new hardcoded literals.

One trivial thing is keeping CI red — test (WhoopStore) is a compile failure, not a logic failure:

ForeignBackupImportTests.swift:363: error: XCTAssertEqual requires that
'ForeignBackupImport.ReconcileError' conform to 'Equatable'
    XCTAssertEqual(error as? ForeignBackupImport.ReconcileError, .noLiveStore)

ReconcileError (enum ReconcileError: Error, LocalizedError) isn't Equatable, so that new assertion doesn't build — which takes down the whole WhoopStore test target and leaves the Swift twin unverified in CI. One-line fix: add Equatable to the enum (or switch the test to if case .noLiveStore = error).

Low-severity nit while you're in there: the typedZeroLiteral untyped/empty-type case still looks divergent — Swift comments "NUMERIC / untyped → 0" (ForeignBackupImport.swift:277) vs the Android x'' path. Niche (real Room/GRDB schemas always carry an affinity, and the key path makes it rarer still), but worth aligning + a shared fixture so the pure planner stays byte-identical.

Really close — fix the Equatable conformance so the Swift tests compile and CI goes green, and I think this is landable.

@ryanbr ryanbr left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. reconcileForeignBackup copies liveDbFile into a scratch file in cacheDir and the REPLACE DELETE 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 ord to rrInterval. Because planRowCopyImport reads PRAGMA table_info at 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:

  1. CI is red on one line. ForeignBackupImportTests.swift:363ReconcileError needs : Equatable. Precisely the class of thing you flagged as unverifiable on a non-Apple host.

  2. 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.

  3. 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 reconcileForeignBackup run you asked for before it lands.

Also worth a rebase — main has moved a fair way in three days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants