feat(balance): convert priced accounts to detailed (v16) (#211) #220

Merged
maximus merged 1 commit from issue-211-conversion-v16 into main 2026-06-10 01:07:47 +00:00
Owner

Resolves #211

Stacked on #219 (base issue-210-schema-migrations).

What

Migration v16 — pure data conversion (additive, guarded, atomic) for the Bilan détail-par-titre stack (Étape 2). Converts each existing single-security "priced" account into a detailed account holding exactly one position, with zero data loss.

  1. Mint securities — one shared balance_securities per priced account symbol (normalized UPPER(TRIM), deduped via ON CONFLICT(symbol) DO NOTHING on the COLLATE NOCASE UNIQUE), only for accounts whose category has a real asset_type (balance_securities.asset_type is NOT NULL — a priced account whose category has asset_type IS NULL has no valid routing).
  2. Mirror holdings — one holding per existing priced line (qty/unit_price/value/price_source/price_fetched_at copied verbatim; book_cost stays NULL — no retroactive acquisition cost). UNIQUE(snapshot_line_id, security_id) + ON CONFLICT DO NOTHING ⇒ re-run is a strict no-op.
  3. Collapse line — NULL the per-line qty/unit_price only where a holding now exists (the 🔴 security fix: a line that got no holding — priced-without-asset_type — is never NULLed ⇒ no silent loss). NULLing both columns together preserves the lines' (both NULL | both NOT NULL) CHECK.

A trailing TEMP-table CHECK(ok = 1) asserts the invariant "qty NULLed ⇒ has a holding" and ABORTS on breach, rolling back the whole v16 transaction (sqlx wraps each migration in its own transaction). Priced accounts without asset_type or without a symbol are left fully intact.

Tests (cargo, in-memory SQLite, v10→v16 via execute_batch — mirrors #210 style)

  • convertible account → security + mirror holding, line qty NULLed, value + history preserved
  • priced-without-asset_type account untouched (qty intact, no holding)
  • re-run idempotent (no dup security/holding)
  • injected-failure mid-v16 aborts on the guard; transaction rollback restores pre-v16 state (zero securities/holdings, quantity intact)
  • full v10→v16 chain on empty data applies cleanly (guard passes vacuously)

Quality gate

cargo check · cargo test (94) · npm run build · npm test (552)

Notes

  • No BEGIN/COMMIT inside the migration SQL: sqlx 0.8.6 (tauri-plugin-sql 2.3.2) already wraps each migration in a transaction and rolls back on error, so the guard abort is sufficient. The rollback test reproduces this with an explicit rusqlite transaction dropped on error (SQLite ABORT alone only undoes the offending statement).
  • No CHANGELOG / consolidated_schema / i18n change: v16 is invisible on its own (the detailed-account UI lands in #214/#216; #218 covers ADR 0015 + docs + CHANGELOG). New profiles ship zero priced accounts ⇒ v16 is a vacuous no-op there.

Generated autonomously by /autopilot run of 2026-06-06

Resolves #211 Stacked on #219 (base `issue-210-schema-migrations`). ## What Migration **v16** — pure data conversion (additive, guarded, atomic) for the Bilan détail-par-titre stack (Étape 2). Converts each existing single-security "priced" account into a `detailed` account holding exactly one position, with **zero data loss**. 1. **Mint securities** — one shared `balance_securities` per priced account symbol (normalized `UPPER(TRIM)`, deduped via `ON CONFLICT(symbol) DO NOTHING` on the `COLLATE NOCASE` UNIQUE), **only** for accounts whose category has a real `asset_type` (`balance_securities.asset_type` is `NOT NULL` — a priced account whose category has `asset_type IS NULL` has no valid routing). 2. **Mirror holdings** — one holding per existing priced line (qty/unit_price/value/price_source/price_fetched_at copied verbatim; `book_cost` stays NULL — no retroactive acquisition cost). `UNIQUE(snapshot_line_id, security_id)` + `ON CONFLICT DO NOTHING` ⇒ re-run is a strict no-op. 3. **Collapse line** — NULL the per-line qty/unit_price **only where a holding now exists** (the 🔴 security fix: a line that got no holding — priced-without-`asset_type` — is never NULLed ⇒ no silent loss). NULLing both columns together preserves the lines' `(both NULL | both NOT NULL)` CHECK. A trailing TEMP-table `CHECK(ok = 1)` asserts the invariant *"qty NULLed ⇒ has a holding"* and **ABORTS** on breach, rolling back the whole v16 transaction (sqlx wraps each migration in its own transaction). Priced accounts without `asset_type` or without a `symbol` are left fully intact. ## Tests (cargo, in-memory SQLite, v10→v16 via execute_batch — mirrors #210 style) - convertible account → security + mirror holding, line qty NULLed, **value + history preserved** - priced-without-`asset_type` account **untouched** (qty intact, no holding) - re-run **idempotent** (no dup security/holding) - **injected-failure** mid-v16 aborts on the guard; transaction rollback restores pre-v16 state (zero securities/holdings, quantity intact) - full v10→v16 chain on empty data applies cleanly (guard passes vacuously) ## Quality gate `cargo check` ✅ · `cargo test` ✅ (94) · `npm run build` ✅ · `npm test` ✅ (552) ## Notes - No `BEGIN/COMMIT` inside the migration SQL: sqlx 0.8.6 (tauri-plugin-sql 2.3.2) already wraps each migration in a transaction and rolls back on error, so the guard abort is sufficient. The rollback test reproduces this with an explicit rusqlite transaction dropped on error (SQLite ABORT alone only undoes the offending statement). - No CHANGELOG / consolidated_schema / i18n change: v16 is invisible on its own (the detailed-account UI lands in #214/#216; #218 covers ADR 0015 + docs + CHANGELOG). New profiles ship zero priced accounts ⇒ v16 is a vacuous no-op there. Generated autonomously by /autopilot run of 2026-06-06
maximus added the
status:review
autopilot:pending-human
labels 2026-06-06 17:01:21 +00:00
maximus added 1 commit 2026-06-06 17:01:22 +00:00
v16 is a purely additive, guarded, atomic data migration (Bilan détail par
titre, Étape 2). It converts each existing single-security priced account into
a detailed account holding exactly one position, with zero data loss.

  1. Mints one shared balance_securities row per priced account symbol
     (normalized UPPER(TRIM), deduped via ON CONFLICT(symbol) DO NOTHING on the
     COLLATE NOCASE UNIQUE), ONLY for accounts whose category carries a real
     asset_type — balance_securities.asset_type is NOT NULL, and a priced
     account whose category has asset_type IS NULL has no valid routing.
  2. Mirrors each existing priced line into one holding (qty/unit_price/value/
     price_source/price_fetched_at copied; book_cost stays NULL — no
     retroactive acquisition cost). UNIQUE(line, security) + ON CONFLICT DO
     NOTHING makes a re-run a strict no-op.
  3. Collapses the now-redundant per-line qty/unit_price to NULL ONLY where a
     holding now exists (the security fix — a line that got no holding, i.e.
     priced-without-asset_type, is never NULLed, so no silent data loss).
     NULLing both columns together preserves the lines' (both NULL | both NOT
     NULL) CHECK.

A trailing TEMP-table CHECK(ok = 1) asserts the invariant 'qty NULLed ⇒ has a
holding' and ABORTS on breach, rolling back the whole v16 transaction (sqlx
wraps each migration in a transaction). Priced accounts without asset_type or
without a symbol are left fully intact.

Integration tests (in-memory SQLite, apply v10→v16 via execute_batch, mirroring
the #210 migration-test style): convertible account gains a security + holding
with values/history preserved and its line qty NULLed; non-convertible priced
account untouched (qty intact, no holding); re-run idempotent; injected-failure
mid-v16 aborts on the guard and a transaction rollback restores the pre-v16
state (zero securities/holdings, quantity intact).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Author
Owner

Adversarial review — PR #220 (v16 priced→detailed conversion)

Verdict: 🔴 REQUEST_CHANGES — one must-fix (startup-bricking guard false-positive). The conversion logic and atomicity are otherwise correct and well-reasoned; the data-loss guard is sound on the priced-account axis but over-broad on its trigger predicate.


🔴 MUST-FIX — guard false-positive bricks app startup for simple accounts carrying a symbol

src-tauri/src/lib.rs:309 (guard WHERE), mirrored in test const V16_SQL.

The abort guard keys on a.symbol IS NOT NULL rather than on convertibility (c.asset_type IS NOT NULL). That makes it fire on a state v16 is not responsible for:

A simple-category account that carries a non-NULL symbol. Its snapshot lines have quantity IS NULL by constructionsaveSnapshot forces simple-kind lines to NULL qty/unit_price (balance.service.ts ~L899-928, comment L703), and the lines CHECK enforces it. Trace through v16 for such a line:

  • Step 1 skips it (c.asset_type IS NULL) → no security.
  • Step 2 skips it (sl.quantity IS NULL) → no holding.
  • Step 3 skips it (quantity IS NOT NULL predicate) → not NULLed, no data loss.
  • Guard matches: a.symbol IS NOT NULL AND sl.quantity IS NULL AND NOT EXISTS (holding) → inserts 0CHECK(ok = 1) fails → the whole v16 migration aborts and rolls back → the app fails to launch for that profile.

Reachability is real, via the normal UI (not just the raw service):

  • AccountForm.tsx:232-249 renders the symbol <input> unconditionally; isPriced only swaps the placeholder/hint. On save (AccountForm.tsx:158) it sends symbol: trimmedSymbol || null regardless of kind.
  • updateBalanceAccount (balance.service.ts:451-456) preserves the existing symbol when re-categorizing. So: create a priced account AAPL, then move it to a simple category (cash) → symbol persists, account is now simple, its lines are qty-NULL → guard trips on next launch.
  • Neither the service (createBalanceAccount L409 / updateBalanceAccount L451) nor any DB CHECK gates symbol by category kind — "simple ⇒ symbol NULL" is convention only.

No data is lost (sqlx rolls back cleanly — verified below), but the user cannot start the app to clear the symbol, so this is a hard startup brick.

Fix: scope the guard's invariant to lines v16 actually owns — i.e. convertible accounts only. Join balance_categories c ON c.id = a.balance_category_id in the guard subquery and add AND c.asset_type IS NOT NULL. (Equivalently: a line should only be expected to have a holding when its account's category has a real asset_type and the line had a quantity.) Apply the identical change to the production Migration { version: 16 } SQL and the V16_SQL test constant.

Test gap that hid this: db_pre_v16() seeds only priced accounts (both acc_a and acc_b). Add a third account — simple category (cash), non-NULL symbol, a qty-NULL snapshot line — and assert v16 applies cleanly (guard does not abort) and that line is left fully intact.


Verified correct (the hard parts)

  • Anti-data-loss, steps 1/2/3 mutual consistency — A line is NULLed (step 3) only if id IN (holdings). A holding exists (step 2) only if a matching security exists (step 1 ⇒ c.asset_type IS NOT NULL) and sl.quantity IS NOT NULL. So a priced-without-asset_type account mints no security → no holding → step 3 never NULLs it → qty/price retained. Confirmed against schema: balance_securities.asset_type is NOT NULL CHECK(IN('stock','crypto')) (v14), balance_categories.asset_type is nullable (v10, set only on seed stock/crypto).
  • Symbol normalizationUPPER(TRIM(a.symbol)) is byte-identical in step 1 and step 2's JOIN; dedupe via ON CONFLICT(symbol) relies on v14 symbol TEXT NOT NULL COLLATE NOCASE UNIQUE — present and correct.
  • CHECK preservation — step 3 NULLs quantity and unit_price together → (both NULL | both NOT NULL) CHECK on balance_snapshot_lines upheld.
  • Holdings NOT-NULL columnssl.quantity IS NOT NULL ⇒ (lines CHECK) unit_price NOT NULL; balance_snapshot_lines.value is NOT NULL always → holdings.quantity/unit_price/value (NOT NULL) satisfied. book_cost left NULL (nullable) — fine.
  • Abort + atomicity (the flagged false-positive worry) — The guard manufactures a CHECK violation → execute() returns Err → migration rolls back. Verified at source: sqlx-sqlite 0.8.6 Sqlite::apply() does let mut tx = self.begin().await?; tx.execute(&migration.sql)…?; … tx.commit() with no no_tx branch — every migration runs in a transaction, rolled back on any error. So the production SQL correctly omits BEGIN/COMMIT, and the guard runs after step 3. The rollback test's rusqlite conn.transaction() dropped-without-commit is a faithful proxy (SQLite ABRT alone would only undo the offending statement; the enclosing tx is what guarantees whole-migration atomicity — the test's framing is accurate, not self-deluding).
  • Idempotence — re-run: ON CONFLICT(symbol) DO NOTHING + ON CONFLICT(snapshot_line_id, security_id) DO NOTHING no-op; step 3's quantity IS NOT NULL means already-NULLed lines are skipped; guard re-passes (every NULLed line still has its holding). Confirmed by migration_v16_is_idempotent_on_rerun.
  • Value preserved — aggregated line keeps value; the mirror holding carries qty/price/value verbatim (migration_v16_converts… asserts both).
  • Migration hygiene — purely additive (+393/-0, single file); v1–v15 untouched (checksum-safe); single version: 16; consolidated_schema/CHANGELOG/i18n correctly deferred (v16 is UI-invisible). db_through_v13 + V14_SQL/V15_SQL helpers exist and the v9→v15 chain is applied before seeding, so all columns are present.

Minor (non-blocking, optional)

  • Cross-account symbol collision — two priced accounts sharing a symbol but with different asset_type (e.g. ETH as stock in one, crypto in another) → step 1 SELECT DISTINCT … ON CONFLICT(symbol) DO NOTHING keeps the first-inserted asset_type (order non-deterministic); step 2 attaches both lines' holdings to that one security. No data loss (qty/price/value preserved), but a possible mis-typing. This is inherent to the v14 single-symbol catalogue design (one security per symbol), not a v16 regression — flagging for awareness. A typed account sharing a symbol with an untyped-category account will also convert the untyped one (it gets a holding via the shared security); again no loss, arguably desirable.

Once the guard predicate is scoped to convertible accounts (and a simple-account-with-symbol test is added), this is a clean APPROVE.

## Adversarial review — PR #220 (v16 priced→detailed conversion) **Verdict: 🔴 REQUEST_CHANGES** — one must-fix (startup-bricking guard false-positive). The conversion logic and atomicity are otherwise correct and well-reasoned; the data-loss guard is sound on the priced-account axis but over-broad on its trigger predicate. --- ### 🔴 MUST-FIX — guard false-positive bricks app startup for simple accounts carrying a symbol `src-tauri/src/lib.rs:309` (guard `WHERE`), mirrored in test const `V16_SQL`. The abort guard keys on **`a.symbol IS NOT NULL`** rather than on *convertibility* (`c.asset_type IS NOT NULL`). That makes it fire on a state v16 is **not** responsible for: A **simple-category** account that carries a non-NULL `symbol`. Its snapshot lines have `quantity IS NULL` **by construction** — `saveSnapshot` forces simple-kind lines to NULL qty/unit_price (`balance.service.ts` ~L899-928, comment L703), and the lines CHECK enforces it. Trace through v16 for such a line: - **Step 1** skips it (`c.asset_type IS NULL`) → no security. ✅ - **Step 2** skips it (`sl.quantity IS NULL`) → no holding. ✅ - **Step 3** skips it (`quantity IS NOT NULL` predicate) → not NULLed, no data loss. ✅ - **Guard** matches: `a.symbol IS NOT NULL` ✅ `AND sl.quantity IS NULL` ✅ `AND NOT EXISTS (holding)` ✅ → inserts `0` → `CHECK(ok = 1)` fails → **the whole v16 migration aborts and rolls back → the app fails to launch** for that profile. **Reachability is real, via the normal UI** (not just the raw service): - `AccountForm.tsx:232-249` renders the symbol `<input>` **unconditionally**; `isPriced` only swaps the placeholder/hint. On save (`AccountForm.tsx:158`) it sends `symbol: trimmedSymbol || null` regardless of kind. - `updateBalanceAccount` (`balance.service.ts:451-456`) preserves the existing symbol when re-categorizing. So: create a priced account `AAPL`, then move it to a simple category (`cash`) → symbol persists, account is now simple, its lines are qty-NULL → guard trips on next launch. - Neither the service (`createBalanceAccount` L409 / `updateBalanceAccount` L451) nor any DB CHECK gates `symbol` by category kind — "simple ⇒ symbol NULL" is convention only. No data is lost (sqlx rolls back cleanly — verified below), but the user cannot start the app to clear the symbol, so this is a hard startup brick. **Fix:** scope the guard's invariant to lines v16 actually owns — i.e. convertible accounts only. Join `balance_categories c ON c.id = a.balance_category_id` in the guard subquery and add `AND c.asset_type IS NOT NULL`. (Equivalently: a line should only be expected to have a holding when its account's category has a real asset_type *and* the line had a quantity.) Apply the identical change to the production `Migration { version: 16 }` SQL **and** the `V16_SQL` test constant. **Test gap that hid this:** `db_pre_v16()` seeds only *priced* accounts (both `acc_a` and `acc_b`). Add a third account — simple category (`cash`), non-NULL `symbol`, a qty-NULL snapshot line — and assert v16 applies cleanly (guard does not abort) and that line is left fully intact. --- ### ✅ Verified correct (the hard parts) - **Anti-data-loss, steps 1/2/3 mutual consistency** — A line is NULLed (step 3) *only* if `id IN (holdings)`. A holding exists (step 2) *only* if a matching security exists (step 1 ⇒ `c.asset_type IS NOT NULL`) and `sl.quantity IS NOT NULL`. So a priced-without-`asset_type` account mints no security → no holding → step 3 never NULLs it → qty/price retained. Confirmed against schema: `balance_securities.asset_type` is `NOT NULL CHECK(IN('stock','crypto'))` (v14), `balance_categories.asset_type` is nullable (v10, set only on seed `stock`/`crypto`). - **Symbol normalization** — `UPPER(TRIM(a.symbol))` is byte-identical in step 1 and step 2's JOIN; dedupe via `ON CONFLICT(symbol)` relies on v14 `symbol TEXT NOT NULL COLLATE NOCASE UNIQUE` — present and correct. - **CHECK preservation** — step 3 NULLs `quantity` **and** `unit_price` together → `(both NULL | both NOT NULL)` CHECK on `balance_snapshot_lines` upheld. - **Holdings NOT-NULL columns** — `sl.quantity IS NOT NULL` ⇒ (lines CHECK) `unit_price NOT NULL`; `balance_snapshot_lines.value` is `NOT NULL` always → `holdings.quantity/unit_price/value (NOT NULL)` satisfied. `book_cost` left NULL (nullable) — fine. - **Abort + atomicity (the flagged false-positive worry)** — The guard manufactures a CHECK violation → `execute()` returns `Err` → migration rolls back. Verified at source: `sqlx-sqlite 0.8.6` `Sqlite::apply()` does `let mut tx = self.begin().await?; tx.execute(&migration.sql)…?; … tx.commit()` with **no `no_tx` branch** — every migration runs in a transaction, rolled back on any error. So the production SQL correctly omits BEGIN/COMMIT, and the guard runs **after** step 3. The rollback test's rusqlite `conn.transaction()` dropped-without-commit is a *faithful* proxy (SQLite ABRT alone would only undo the offending statement; the enclosing tx is what guarantees whole-migration atomicity — the test's framing is accurate, not self-deluding). - **Idempotence** — re-run: `ON CONFLICT(symbol) DO NOTHING` + `ON CONFLICT(snapshot_line_id, security_id) DO NOTHING` no-op; step 3's `quantity IS NOT NULL` means already-NULLed lines are skipped; guard re-passes (every NULLed line still has its holding). Confirmed by `migration_v16_is_idempotent_on_rerun`. - **Value preserved** — aggregated line keeps `value`; the mirror holding carries qty/price/value verbatim (`migration_v16_converts…` asserts both). - **Migration hygiene** — purely additive (`+393/-0`, single file); v1–v15 untouched (checksum-safe); single `version: 16`; consolidated_schema/CHANGELOG/i18n correctly deferred (v16 is UI-invisible). `db_through_v13` + `V14_SQL`/`V15_SQL` helpers exist and the v9→v15 chain is applied before seeding, so all columns are present. ### Minor (non-blocking, optional) - **Cross-account symbol collision** — two priced accounts sharing a symbol but with different `asset_type` (e.g. `ETH` as `stock` in one, `crypto` in another) → step 1 `SELECT DISTINCT … ON CONFLICT(symbol) DO NOTHING` keeps the first-inserted asset_type (order non-deterministic); step 2 attaches both lines' holdings to that one security. No data loss (qty/price/value preserved), but a possible mis-typing. This is inherent to the v14 single-`symbol` catalogue design (one security per symbol), not a v16 regression — flagging for awareness. A typed account sharing a symbol with an *untyped*-category account will also convert the untyped one (it gets a holding via the shared security); again no loss, arguably desirable. --- Once the guard predicate is scoped to convertible accounts (and a simple-account-with-symbol test is added), this is a clean APPROVE.
Author
Owner

Disposition (décision Max) : merge + fix-forward.

La review a relevé un vrai bug (garde d'abort v16 trop large → blocage de lancement pour un compte simple à symbole résiduel). La logique de conversion elle-même est correcte. Plutôt que de réécrire la chaîne stackée, le fix est différé en fix-forward sur main après le merge de la chaîne et avant le tag de release — v16 n'a jamais été appliquée à un profil persistant, donc corriger sa SQL avant sa première application respecte la règle d'immutabilité des migrations.

Fix tracké dans #228 (type:bug, status:ready, avec SQL + test de régression). Label de l'issue #211 repassé à status:approved pour débloquer le merge de la chaîne.

Autopilot review — 2026-06-06.

**Disposition (décision Max) : merge + fix-forward.** La review a relevé un vrai bug (garde d'abort v16 trop large → blocage de lancement pour un compte simple à symbole résiduel). La logique de conversion elle-même est correcte. Plutôt que de réécrire la chaîne stackée, le fix est **différé en fix-forward sur `main`** après le merge de la chaîne et avant le tag de release — v16 n'a jamais été appliquée à un profil persistant, donc corriger sa SQL avant sa première application respecte la règle d'immutabilité des migrations. Fix tracké dans #228 (type:bug, status:ready, avec SQL + test de régression). Label de l'issue #211 repassé à `status:approved` pour débloquer le merge de la chaîne. _Autopilot review — 2026-06-06._
maximus changed target branch from issue-210-schema-migrations to main 2026-06-10 01:07:46 +00:00
maximus merged commit 4e4e4bd0d2 into main 2026-06-10 01:07:47 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: maximus/Simpl-Resultat#220
No description provided.