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) {