From 354fbed18220b47167b9bf6b11b3881fc120b7ae Mon Sep 17 00:00:00 2001 From: Luca <102960244+Luca-Timo@users.noreply.github.com> Date: Mon, 1 Jun 2026 22:23:26 +0200 Subject: [PATCH] fix(restore): coerce pg bigint counts to Number before comparing (PR #596 round 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pg-driver serialises `bigint` (which is what `COUNT(*)` returns) as a JavaScript STRING to preserve precision for huge counts. The manifest stores `expected.rowCount` as a JS number (parseInt'd at databaseBackup.js:118). Strict `!==` in performPostRestoreVerification flagged every match as a mismatch on PG: Table activity_logs row count mismatch: expected 16, got 16 Table admin_users row count mismatch: expected 1, got 1 Table app_settings row count mismatch: expected 165, got 165 ... (every table, all matching) Symptom matched the preservedMeta scope leak from round 1: install- from-backup logged FAILED, trigger file wasn't cleaned, data was actually intact. Caught on PR #596 e2e re-run. Cure: coerce both sides with `Number(...)` at the comparison AND in the interpolated value so the warning text renders `16` not `"16"`. Pre-emptive: lines 448 + 458-459 had the same string-vs-number issue masked by `>` (JS coerces operands for `>`), but the warning text printed `"5"` on PG vs `5` on SQLite, and a future patch changing `>` to `=== 0` or `!== expectedCount` would silently break on PG. Coerced at the read site into `eventCountN` / `activeUsersN` locals + added a comment block explaining the contract so future edits don't drop the Number() calls without re-auditing. New source-inspection test: pins the contract that every `.count` result in restoreService.js MUST be wrapped in `Number(...)` when used in a comparison (===/!==/>/=/<=). Same source-inspection pattern as the preservedMeta test added round 1 — pragmatic until the real-PG integration test follow-up lands. The maintainer's audit of the rest of the backup/restore surface (_installFromBackupBoot, _restoreSettingsBoot, _backupPathsBoot, backupCoverageService, backupIntegrityService, backupService, databaseBackup) confirmed no other bigint-as-string sites — the class is now closed in the audited scope. --- .../restoreService.pgBranch.test.js | 42 +++++++++++++++++++ backend/src/services/restoreService.js | 38 +++++++++++++---- 2 files changed, 72 insertions(+), 8 deletions(-) diff --git a/backend/__tests__/integration/restoreService.pgBranch.test.js b/backend/__tests__/integration/restoreService.pgBranch.test.js index 6b0342e9..e91ab75d 100644 --- a/backend/__tests__/integration/restoreService.pgBranch.test.js +++ b/backend/__tests__/integration/restoreService.pgBranch.test.js @@ -109,6 +109,48 @@ describe('restoreService — PG branch scope contract (PR #596 review)', () => { expect(declarationLine).toBeLessThan(splitLineOneBased); }); + it('every .count() result is coerced to Number before comparison', () => { + // PR #596 review caught a second PG-only landmine: pg-driver + // returns COUNT(*) as a string ("16" not 16) to preserve bigint + // precision. The original code compared `result.count !== + // expected.rowCount` and every match flagged as a mismatch on PG. + // + // The fix coerces with `Number(...)` at every comparison + + // interpolation site. This test catches a future regression where + // a refactor uses `.count` directly in a `===` / `!==` / `>` / + // `<` comparison without coercing. + // + // Heuristic: find every `.count` access in the file and make sure + // the line either: + // (a) wraps it in `Number(...)`, or + // (b) is purely an interpolation that already coerced upstream + // (e.g. `validation.warnings.push(`... ${eventCountN} ...`)` + // where eventCountN is the coerced local), or + // (c) is the docstring/comment line (filtered separately). + // + // We approximate this by listing every `.count` reference site + // and asserting that lines doing comparisons (`===`/`!==`/`>`/ + // `<`/`>=`/`<=`) on a raw `.count` access without `Number(...)` + // around it are zero. + const dangerousLines = lines + .map((l, i) => ({ line: i + 1, text: l })) + // Filter to lines that compare a .count result + .filter(({ text }) => { + // Skip comments + if (/^\s*(\/\/|\*)/.test(text)) return false; + // Detect a `.count` (followed by `)` for `?.count` or by space/operator) + // being directly compared via ===/!==/>/<. + // Match the BAD pattern: `.count ` + // where is === / !== / > / < / >= / <= + const bareCountInComparison = /\w+\??\.count\s*(?:!==|===|>=?|<=?)\s+/; + // ALLOW if the .count is preceded by `Number(` in the same line + const wrappedInNumber = /Number\(\s*\w+\??\.count/; + return bareCountInComparison.test(text) && !wrappedInNumber.test(text); + }); + + expect(dangerousLines).toEqual([]); + }); + it('the replay block reads preservedMeta outside the SQLite/PG branch', () => { // The replay block lives near the end of performDatabaseRestore. // It must NOT be guarded by `this.dbType === 'postgresql'` — the diff --git a/backend/src/services/restoreService.js b/backend/src/services/restoreService.js index bba57295..8c87b6f2 100644 --- a/backend/src/services/restoreService.js +++ b/backend/src/services/restoreService.js @@ -442,21 +442,32 @@ class RestoreService { } } - // Check if restoring would overwrite existing data + // Check if restoring would overwrite existing data. + // + // NOTE: pg-driver returns `count('* as count')` as a STRING (it + // serialises `bigint` to string to avoid JS precision loss for + // huge counts) — see PR #596 review for the `bigint`-as-string + // discussion. Both blocks below coerce to `Number` before + // comparing AND before interpolating into the warning text, so + // the count renders as `5` not `"5"` regardless of DB driver. + // Don't drop the `Number()` calls without also re-auditing the + // strict-equality call sites flagged in the same review. if (options.restoreType === 'full' || options.restoreType === 'database') { const eventCount = await db('events').count('* as count').first(); - if (eventCount && eventCount.count > 0) { - validation.warnings.push(`Database contains ${eventCount.count} existing events that will be overwritten`); + const eventCountN = Number(eventCount?.count || 0); + if (eventCountN > 0) { + validation.warnings.push(`Database contains ${eventCountN} existing events that will be overwritten`); } } - // Check for active users + // Check for active users (same coercion contract as above). const activeUsers = await db('admin_users') .where('is_active', formatBoolean(true)) .count('* as count') .first(); - if (activeUsers && activeUsers.count > 0) { - validation.warnings.push(`There are ${activeUsers.count} active admin users`); + const activeUsersN = Number(activeUsers?.count || 0); + if (activeUsersN > 0) { + validation.warnings.push(`There are ${activeUsersN} active admin users`); } } catch (error) { @@ -1240,9 +1251,20 @@ END $$;` for (const [table, expected] of Object.entries(manifest.database.row_counts)) { try { const result = await db(table).count('* as count').first(); - if (result.count !== expected.rowCount) { + // pg-driver serialises `bigint` as string to preserve + // precision for huge counts, so `result.count` on PG is + // e.g. `"16"` while the manifest's `expected.rowCount` + // is the JS number `16`. Strict `!==` flagged every + // match as a mismatch on PG. Caught on PR #596 review: + // `Table activity_logs row count mismatch: + // expected 16, got 16` + // every table, all "matching". Coerce both sides to + // Number to compare reliably across SQLite (number) and + // PG (string). + const actual = Number(result.count); + if (actual !== expected.rowCount) { verification.errors.push( - `Table ${table} row count mismatch: expected ${expected.rowCount}, got ${result.count}` + `Table ${table} row count mismatch: expected ${expected.rowCount}, got ${actual}` ); } } catch (error) {