ReviewOS

stacks/reportshq

Postgres: the remaining dialect differences the new CI job found

#32
Closed chris opened this 8 days ago ·
8 days ago

Production moved to Postgres. The app had only ever run on SQLite, so the first run of the new test-postgres CI job surfaced 83 failures across four classes. Most are fixed; this tracks the rest.

test-postgres is continue-on-error: true for now, because gating on a known-red suite means no deploys at all. It goes back to a hard gate the moment this reaches zero — a non-blocking check nobody reads is the same as no check.

Fixed

  • ? placeholders. Raw SQL with ? is a syntax error on Postgres, reported against the next token (syntax error at or near "AND"). Fixed in the library: toDialectPlaceholders in bun-query-builder 0.2.32, quoting-aware so a ? inside a literal is left alone.
  • SQLite-only strftime / json_extract throughout the query engine. Fixed in the library: jsonExtract / dateBucket / booleanLiteral joined the DialectDriver interface in 0.2.31, implemented for every dialect. The app now carries no dialect knowledge at all.
  • Booleans written as 1 and 0. Nine statements, plus two test fixtures. Postgres refuses them; SQLite accepts them.
  • schema.text() producing varchar(255). text and string shared a branch in the migration generator's type inference, so text() was a no-op unless it also carried a large .max(). That is what put JSON blobs in 255-char columns. Fixed in bun-query-builder 0.2.33.
  • A literal 'owner' meeting an enum column in a UNION and two CASE expressions. Postgres coerces the literal into the enum and refuses it, because the owner is synthesised rather than being a project_members row. Both sides are now explicitly CAST(... AS TEXT).

Remaining (24 failures)

Integers come back as strings. expect(result.projectId).toBe(248) gets "248". The Postgres driver returns bigint as a string, which is correct for a driver — JS cannot hold every bigint — and wrong for an id the application compares with ===. Needs a decision: coerce at the boundary in the app, or have the query builder normalise integer-typed columns. The second is better if it can be done without lying about large values.

LIMIT must not be negative (3). Somewhere a limit is computed as negative and SQLite silently treated it as unbounded. That is a real bug in either case and worth finding regardless of dialect.

A few value too long for character varying(255) remain on tables other than the ones already widened. Each needs the column checked against what the model says it holds.

Why this is worth the noise

Every one of these passed the whole SQLite suite. They are exactly the bugs a single-dialect test suite cannot see, and the reason the job exists.

8 days ago

The integers-as-strings difference is user-visible in the production API, not only in tests. A freshly created project comes back as:

{"project":{"id":"1","owner_id":"2","auto_reports_enabled":true,...}}

id and owner_id are strings; auto_reports_enabled is a proper boolean now. So this is an API contract change caused by the database move, not just a test-assertion problem, and any client doing project.id === 1 breaks.

That raises the priority and argues for fixing it in the query builder rather than coercing at each call site: normalise integer-typed columns to numbers when the value is within Number.MAX_SAFE_INTEGER, and leave it a string when it genuinely is not representable. Coercing per call site means every new query is a chance to forget, and the failure is silent.

Verified on production after the deploy: register, log in, create a project, verify the key, and ingest an event all succeed. The smoke-test rows have been deleted.

8 days ago

Zero failures on both dialects, and test-postgres is back in the deploy's needs with continue-on-error removed. 551 pass on Postgres, 551 on SQLite, and the gated run deployed green.

The last four:

  • report_revisions.snapshot held a whole report's blocks in a varchar(255). Now text.
  • LIMIT -1 OFFSET n, the SQLite idiom for "everything after n", which Postgres refuses. Rewritten as "not in the newest N", which also collapses an N+1 delete loop into one statement.
  • Integers as strings. acceptInvite returned the raw project_id, so the value never matched the number its own type promised, and the project API responses had it too. Coerced at both boundaries.
  • events.value was schema.number(), which maps to an integer column, so 99.5 stored as 100 with no error anywhere. It says float() now.

The one worth reading

That last fix first went in as a library change: map every number() to decimal. Regenerating the schema against it produced 70 ALTER statements turning grid coordinates, view counts, byte sizes, an hour-of-day and a foreign key into decimal(10,2) — an arbitrary two-place scale and a ceiling of 99,999,999.99 on columns that are conceptually whole numbers.

It was reverted in bun-query-builder 0.2.35. A default cannot read intent; the model can say what it means. The two spellings of number are now pinned by a test so the split is documented rather than discovered.

Still outstanding

Production's schema is the old one — events.value is integer, and properties/snapshot/query are varchar(255). The new migrations are CREATE TABLE IF NOT EXISTS, so deploying them is a no-op against those tables and the wrong types stay. The database holds 0 users and 0 events, so recreating it costs nothing today and stops being free the moment a real signup lands. Waiting on a decision before touching it.

8 days ago

Closing the loop on the part that made all of this expensive: nothing in the stack could see it.

migrate decides what to do by asking which migration files have not executed. It never asks whether the database resembles the models. So the wrong schema reported "nothing to migrate — your database is already up to date" on every run, and a corrected CREATE TABLE IF NOT EXISTS deployed as a no-op against it. The only symptom either bug produced was a number that came back different from the one written.

bun-query-builder 0.2.36 adds auditSchemaDrift, comparing the plan built from the models against the live schema. It compares by type family, not by SQL string — dialects spell one type several ways (character varying, varchar, int4, integer, real, float4) and an audit that reported those would be noise, which is the fastest way to teach people to ignore it. Widenings pass for the same reason; a varchar model in a text column loses nothing. What it refuses is the narrowing direction, which is both of these bugs.

stacksjs/stacks 34853c3 wires it into buddy migrate, beside the existing foreign-key probe and on the same reasoning: migrate is the highest-context moment to say so. It warns rather than fails, because repairing drift can mean rewriting a table and that is the operator's call to schedule.

Verified against a database drifted to match production exactly:

The live schema does not match the models.
  • events.properties is character varying, models declare text
  • events.value is integer, models declare fractional

Migrations will NOT fix this on their own: the runner only applies files that have
not run yet, and a corrected `CREATE TABLE IF NOT EXISTS` is a no-op against a table
that already exists. Regenerate and apply the difference, or rebuild the schema.

And silent against the corrected schema — 14 models, no false positives.

Production has been rebuilt. Dumped first, dropped, recreated from the corrected migrations, verified end to end: register, log in, create a project, ingest an event with value: 99.5, and read back 99.5 rather than 100. Test rows deleted; the database is empty again.

8 days ago

Zero failures on both dialects, test-postgres is a hard gate again, and the class of bug that caused this is now detectable: buddy migrate warns when the live schema stops matching the models (bun-query-builder 0.2.36 + stacksjs/stacks 34853c3). Production has been rebuilt and verified end to end.

Sign in to comment on this issue.