fix(restore): coerce pg bigint counts to Number before comparing (PR #596 round 2)
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.
This commit is contained in:
@@ -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: `<something>.count <op> <something>`
|
||||
// where <op> 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
|
||||
|
||||
@@ -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) {
|
||||
|
||||
Reference in New Issue
Block a user