ReviewOS

stacks/bun-query-builder

auditSchemaDrift cannot report a naive-vs-aware timestamp mismatch, and `other` passes everything

#1177
Closed glennmichael123 opened this 9 days ago ·
9 days ago

auditSchemaDrift reports a clean schema on databases whose timestamp columns are wrong, because the two type classifiers it compares collapse distinctions that matter and because one of their output families matches everything.

Measured on 0.3.11 by calling the public exports, so none of this is read off minified code.

The chain

auditSchemaDrift compares type families, not types:

let B = familyOfDeclared(column.type)        // declared side
let X = familyOfSqlType(live.sqlType)        // introspected side
if (!satisfies(B, X, dialect))
  typeMismatches.push({ table, column, expected: B, actual: X, actualSqlType: live.sqlType })

familyOfDeclared, familyOfSqlType and satisfies are all exported, and auditSchemaDrift.toString() contains exactly one call to each.

Defect 1: naive and aware are the same family

familyOfDeclared('datetime')                    -> "date"
familyOfSqlType('timestamp without time zone')  -> "date"
familyOfSqlType('timestamp with time zone')     -> "date"

So a model declaring datetime over a live timestamp with time zone column produces "date" on both sides and no mismatch, on every dialect:

dialect = postgres
   datetime / timestamp without time zone      date/date     mismatch reported = false
   datetime / timestamp with time zone         date/date     mismatch reported = false
   timestamptz / timestamp without time zone   date/date     mismatch reported = false
   timestamptz / timestamp with time zone      date/date     mismatch reported = false

The last two are the inverse case and are equally silent: a model that explicitly asks for timestamptz over a naive column is also accepted.

This is not a cosmetic gap. The naive/aware distinction is the one that changes stored values. Stacks writes a naive UTC literal; Postgres interprets one written into an aware column in the session timezone, so every timestamp in such a column is shifted by that offset. @stacksjs/stacks has a dedicated repair for it (ensureUtcDatetimeColumns, with STACKS_CONVERT_AWARE_TIMESTAMPS=1), and that repair is currently the only thing that notices. buddy migrate calls auditSchemaDrift and reports the schema as clean on exactly the databases the repair exists to fix. Related symptom thread: stacksjs/stacks#2902.

Defect 2: "other" is a universal wildcard, and both classifiers route real spellings into it

satisfies is identity on the diagonal plus "other" matching anything, in both directions:

satisfies(declared, live)
        date   text   numbe  boole  json   binar  other
date    true   false  false  false  false  false  true
text    false  true   false  false  false  false  true
numbe   false  false  true   false  false  false  true
boole   false  false  false  true   false  false  true
json    false  false  false  false  true   false  true
binar   false  false  false  false  false  true   true
other   true   true   true   true   true   true   true

That would be harmless if nothing real landed in "other". Two things do, and the two classifiers have complementary gaps:

familyOfDeclared('timestamp')     -> "other"     // declared side knows datetime, timestamptz, date, not timestamp
familyOfSqlType('timestamptz')    -> "other"     // live side knows timestamp…, date, datetime, not bare timestamptz

So:

   timestamp / timestamp with time zone        other/date    mismatch reported = false
   timestamp / integer                         other/integer mismatch reported = false

A model declaring timestamp is compatible with any live column type, integer included. Those columns are not awareness-blind, they are unaudited. familyOfSqlType matches on /^(?:date|time|timestamp|datetime)\b/, and timestamptz fails that \b because the word does not end after timestamp, which is why the live side misses the one spelling Postgres uses in some introspection paths.

For contrast, datetime / text is reported on postgres and mysql, and is not reported on sqlite, which looks deliberate.

Reproduction

No database required:

import { familyOfDeclared, familyOfSqlType, satisfies } from 'bun-query-builder'

for (const [d, l] of [
  ['datetime', 'timestamp with time zone'],
  ['timestamptz', 'timestamp without time zone'],
  ['timestamp', 'integer'],
]) {
  const B = familyOfDeclared(d), X = familyOfSqlType(l)
  console.log(d, '/', l, `${B}/${X}`, 'mismatch =', !satisfies(B, X, 'postgres'))
}

Suggested direction

The package already knows the difference elsewhere: its SQL-type-to-attribute normalizer treats timestamptz as distinct (f === 'timestamptz' || /\bwith time zone\b/.test(c)). There are two classifiers and the drift auditor uses the blind one.

Two things would close it:

  1. Give awareness its own family, or carry it as a flag beside the family, so date versus date-aware can disagree. Then datetime over timestamptz reports, and the inverse reports too.
  2. Stop "other" from passing. Either make it match nothing and fix the two classifier gaps (timestamp on the declared side, bare timestamptz on the live side), or keep the wildcard but have auditSchemaDrift surface other/* as "not audited" rather than folding it into clean: true. Silently clean and knowingly unaudited are different answers, and only one of them is safe to act on.

A table of declared spelling against live spelling with the expected verdict would pin both, and needs no database.

Not bqb's fault, worth recording

bqb does not create these columns: its Postgres DDL maps a datetime attribute to plain timestamp and emits timestamptz only when a model asks for it by name. And db.unsafe() is a verbatim pass-through to the Bun.SQL instance (unsafe(g, f) { return _t(g, f), R.unsafe(g, f) }, where _t is only the $1-versus-? placeholder warning), with no row mapping or decode, so reads through it are not touched here either. The failure is only that the auditor does not notice columns that arrived some other way.

9 days ago

Duplicate of #1176, which was filed a few minutes earlier and is better evidenced: it drives auditSchemaDrift itself with a control column rather than probing the classifiers, and it locates both routes in src/actions/schema-drift.ts with line numbers.

The one thing here that was additive, that familyOfDeclared('timestamp') is also 'other' so a model declaring the bare word is unaudited against every type, is now a comment on #1176. Closing in favour of that one.

Sign in to comment on this issue.