fix(core): make snapshot generation dialect-aware so backups work on Postgres#2234
fix(core): make snapshot generation dialect-aware so backups work on Postgres#2234esaa-egypt wants to merge 1 commit into
Conversation
…Postgres
generateSnapshot() reached for three SQLite-only constructs with no dialect
branch, so every snapshot-backed feature failed on PostgreSQL with 42P01:
- table discovery via `SELECT name FROM sqlite_master ... LIKE 'ec_%'`
- column discovery via `PRAGMA table_info("<table>")`
- the published-content filter via `strftime('%Y-%m-%dT%H:%M:%fZ', 'now')`
That took down Settings -> Backups (Download backup, Back up now, and the
daily scheduled run) and the /_emdash/api/snapshot preview endpoint. The
scheduled path is the worst of the three: saving the settings succeeds and
the API keeps reporting `{"enabled":true,...,"storageAvailable":true}`, so
the admin shows daily backups as armed while every tick throws and the
archive list stays empty.
Table discovery and the status filter now go through the existing
`listTablesLike` and `buildStatusCondition` helpers. Column discovery needed
a new `listTableColumns` helper alongside them.
`listTableColumns` returns SQLite storage classes on BOTH dialects rather
than the raw `information_schema` `data_type`. Snapshots are consumed by
SQLite targets — the Durable Object preview database recreates every table
from these strings — and that consumer allowlists TEXT/INTEGER/REAL/BLOB/JSON
and falls back to TEXT. Passing Postgres type names through would therefore
be silently lossy (`integer` survives, but `bigint`, `boolean`, `jsonb` and
`double precision` would all collapse to TEXT), so the Postgres branch maps
them explicitly instead.
Table discovery keeps its deterministic ordering: `listTablesLike` has no
ORDER BY, so the result is sorted in JS to preserve the previous output.
Tests use the repo's describeEachDialect harness, so both new files run
against SQLite and against a real Postgres connection; their Postgres
variants fail against the pre-fix implementation.
Fixes emdash-cms#2233
🦋 Changeset detectedLatest commit: f2f38c3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
github-actions
Bot
added
review/awaiting-author
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
The approach is sound and addresses the right problem. Backups/snapshots failing on Postgres because of SQLite-only catalog queries is a real bug, and routing table/column/status discovery through the existing dialect-helpers.ts pattern is the idiomatic fix in this codebase. The core change is minimal and preserves existing SQLite behavior.
I read the diff, the full changed files, the existing snapshot tests, the DO preview consumer (packages/cloudflare/src/db/do-class.ts), and the schema registry's column-type declarations. The source fix looks correct: listTablesLike and buildStatusCondition already existed for exactly this, and the new listTableColumns uses parameterized queries and scopes Postgres catalog reads to current_schema(). The snapshot consumer already uppercases and allowlists the type strings, so the type-mapping strategy is safe.
Headline conclusion: the production fix is good, but two issues in the new tests/helper contract need fixing before this can merge — otherwise CI will fail on SQLite and one dialect test assertion is structurally wrong.
-
listTableColumnsreturns raw, lowercase SQLite declared types, contradicting its interface and breaking the new tests on SQLite. ItsTableColumnInfo.typedocstring promises "SQLite storage class — TEXT, INTEGER, REAL, BLOB, or JSON", and the Postgres branch normalizes accordingly, but the SQLite branch returns whateverPRAGMA table_inforeports (e.g."text","integer"). The newlist-table-columns.test.tsandsnapshot-dialect.test.tsassertions expect uppercase, so they will fail on SQLite. The consumer uppercases on its own, so snapshots still work, but the helper's contract is inconsistent. -
The new
snapshot-dialect.test.tsassertssnapshot.tables.ec_post).toEqual([]), butgenerateSnapshotonly assignstables[tableName]whenrows.length > 0. With the default published filter and no rows,ec_postis omitted entirely (undefined), not an empty array. This mirrors the existingsnapshot.test.tsbehavior ("Schema should include ec_post ... tables._emdash_collections should have 2 rows"), so the test expectation should match it.
Both are small, mechanical fixes.
| const result = await sql<{ name: string; type: string }>` | ||
| SELECT name, type FROM pragma_table_info(${tableName}) | ||
| `.execute(db); | ||
| return result.rows.map((r) => ({ name: r.name, type: r.type || "TEXT" })); |
There was a problem hiding this comment.
[needs fixing] The SQLite branch returns the raw r.type string, which for EmDash-created tables is lowercase ("text", "integer"). That contradicts the TableColumnInfo docstring above, which promises an uppercase SQLite storage class (TEXT, INTEGER, REAL, BLOB, JSON), and it differs from the Postgres branch which normalizes to those same uppercase strings. The preview consumer happens to uppercase before checking, so production snapshots survive, but the helper's own contract is dialect-dependent in case and the new list-table-columns.test.ts assertions (INTEGER, TEXT, etc.) will fail on SQLite.
Normalize the SQLite branch to return uppercase storage classes, matching Postgres and the tests:
return result.rows.map((r) => ({ name: r.name, type: (r.type || "TEXT").toUpperCase() }));| // endpoint uses, and the only one that evaluates the status filter. | ||
| const snapshot = await generateSnapshot(ctx.db); | ||
|
|
||
| expect(snapshot.tables.ec_post).toEqual([]); |
There was a problem hiding this comment.
[needs fixing] generateSnapshot only assigns tables[tableName] when rows.length > 0, so an empty content table is omitted from snapshot.tables entirely (undefined), not represented as []. This matches the existing snapshot.test.ts behavior, but this new assertion expects an empty array and will fail.
Fix the expectation to match the actual (and pre-existing) behavior:
expect(snapshot.tables.ec_post).toBeUndefined();Or, if you want to assert the filter behavior more directly, insert a draft/scheduled row and verify it is excluded.
What does this PR do?
generateSnapshot()reaches for three SQLite-only constructs with no dialect branch, so every snapshot-backed feature fails on PostgreSQL:snapshot.ts:250SELECT name FROM sqlite_master ... LIKE 'ec_%'42P01snapshot.ts:275PRAGMA table_info("<table>")42601snapshot.ts:303strftime('%Y-%m-%dT%H:%M:%fZ', 'now')42883Only the first appears in a traceback, because it fails first — the other two are on the same path behind it. Together they take out Settings → Backups (Download backup, Back up now, and the daily scheduled run) and
/_emdash/api/snapshot(the preview-mode export). Thestrftimeone is in the default no-drafts/no-trash branch, which is exactly the path preview uses, so this is not only a backups bug.The scheduled path is the one worth fixing carefully: saving the settings succeeds and
GET /_emdash/api/settings/backupskeeps returning{"enabled":true,"retention":30,...,"storageAvailable":true}, so the admin presents daily backups as armed while every tick throws and the archive list stays empty. An operator on Postgres gets no signal that they have no backups.What changed. Table discovery and the status filter now go through the helpers that already exist for exactly this —
listTablesLikeandbuildStatusCondition. Column discovery had no equivalent, so this addslistTableColumnsnext to them indialect-helpers.ts.listTableColumnsreturns SQLite storage classes on both dialects, not the rawinformation_schema.data_type. That is the one design decision here worth reviewing: snapshots are consumed by SQLite targets —EmDashPreviewDB.applySnapshotrecreates every table from these strings — and that consumer allowlistsTEXT/INTEGER/REAL/BLOB/JSON, silently falling back toTEXT. Passing Postgres type names straight through would "work" while being lossy:integerhappens to survive, butbigint,boolean,jsonbanddouble precisionwould all land asTEXT. The Postgres branch maps them explicitly instead. If you'd rather that mapping lived in the consumer, say so and I'll move it.One behavioural detail preserved:
listTablesLikehas noORDER BY, so the call site sorts in JS to keep the previous deterministic table order.Closes #2233
Type of change
Checklist
pnpm typecheckpasses — not run locally, see note belowpnpm lintpasses — not run locally, see note belowpnpm testpasses (or targeted tests for my change) — not run locally, see note belowpnpm formathas been run — not run locally, see note belowOn the four unchecked boxes: I could not run
pnpm install/build/typecheck/lint/testlocally. The machine that has the PostgreSQL deployment is disk-constrained (~2.6 GB free, and the monorepo install pulls Playwright browsers), so I verified the runtime behaviour there instead — see below — and wrote the tests to the conventions inAGENTS.mdand the neighbouringlist-tables-like.test.tswithout executing them. I'd rather say that plainly than tick boxes I didn't earn. Please treat CI as the authority on the test files specifically; I'm happy to fix anything it flags.AI-generated code disclosure
Screenshots / test output
End-to-end verification on a real deployment. I applied these three changes to the built
0.31.1server on a live Node + PostgreSQL 16 instance (Astro 6.4.8,@astrojs/node, ~30_emdash_*/ec_*tables at migration053, R2 storage) and ran it on an isolated port against the production database:The resulting archive is well-formed:
That last line is the
SAFE_OPTIONS_PREFIXESallowlist still holding — noplugin:,*secret*, orpasskey_pending:keys in the export.Tests added. Two files using
describeEachDialect, so they run against SQLite and against a real Postgres connection; their Postgres variants fail against the pre-fix implementation:tests/integration/database/list-table-columns.test.ts— declaration ordering, the storage-class invariant, integer mapping, and the missing-table casetests/integration/snapshot/snapshot-dialect.test.ts—generateSnapshotacross both dialects, covering the default (preview) path that evaluates the status filter and theincludeTrashedpath the backup handlers use