ReviewOS

stacks/bun-query-builder

auditSchemaDrift cannot see naive-vs-aware timestamps, so a timestamptz column under a datetime model reports clean

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

auditSchemaDrift compares type families, and naive-vs-aware timestamps collapse into one family. So a live timestamptz column under a model declaring datetime is reported as clean, in both directions, on every dialect. It is the one drift class the auditor cannot express, and it is also the one that silently shifts every value an application writes.

Why this one matters more than the usual drift

A naive UTC literal written into an aware column is interpreted in the session timezone, so the stored instant is shifted by that offset and the same row read from two sessions yields two different times. Stacks writes naive UTC literals everywhere, so on a timestamptz column every timestamp is wrong by the server's offset. We have that live right now: three applications are 8 hours off, and the login path 500s on one of them (stacksjs/stacks#2902).

The auditor is exactly the tool that should have caught it, and it has been reporting those schemas clean on every buddy migrate since they were created. Worth saying plainly: bqb did not create these columns. columnTypeForPostgres maps a declared datetime to plain timestamp and only emits timestamptz when a model asks for it by name. The only failure here is not noticing.

Reproduction

Live Postgres 16.14, a table whose expires_at is timestamptz and whose created_at is naive, audited against a plan declaring both as datetime:

expires_at declared datetime, live timestamptz   clean=true   mismatches=[]
expires_at declared timestamptz, live timestamptz  clean=true   mismatches=[]
created_at declared timestamptz, live naive      clean=true   mismatches=[]
CONTROL amount declared decimal, live integer    clean=false  mismatches=["amount:integer"]

The control is on the same table through the same call, so the harness is live and the auditor is otherwise working. formatSchemaDrift returns an empty string for the first three.

const plan = { tables: [{ table: 'oauth_access_tokens', columns: [{ name: 'expires_at', type: 'datetime' }] }] }
const drift = await auditSchemaDrift(plan, { dialect: 'postgres' })
// drift.clean === true, against a live `expires_at timestamptz`

Root cause

Two routes to the same pass, in src/actions/schema-drift.ts:

  1. familyOfDeclared maps date, datetime and timestamptz all to 'date' (lines 77-80). familyOfSqlType maps anything matching /^(?:date|time|timestamp|datetime)\b/ to 'date' (line 103). Postgres information_schema.columns.data_type reports a timestamptz column as timestamp with time zone, which matches that branch, so declared and actual are both 'date' and satisfies returns true on its first line.

  2. The literal spelling timestamptz is classified 'other', because \b after timestamp cannot match before the t of tz. satisfies then passes it through the "unknown is not evidence of drift" rule at line 169. This is the path a SQLite column declared timestamptz takes, since PRAGMA table_info returns the declared text.

Verified spelling, same server:

naive      -> timestamp without time zone
aware      -> timestamp with time zone
awarelong  -> timestamp with time zone

Suggested fix

Give awareness its own family rather than widening satisfies:

  • familyOfDeclared: timestamptz returns 'timestamptz'; date and datetime keep 'date'.
  • familyOfSqlType: test /with time zone/ and the bare timestamptz spelling before the generic date branch, and return 'timestamptz'.
  • satisfies: neither family satisfies the other. Nothing is lost in a naive-to-aware move, so the existing widening rationale does not apply: the value changes.

One subtlety that makes this more than a two-line change. familyOfSqlType currently takes no dialect, but for the bare word timestamp awareness is dialect-dependent: on Postgres timestamp is naive, while MySQL's TIMESTAMP is converted to and from the session timezone and DATETIME is the naive one. So MySQL has this same bug in mirror image, and the classifier needs the dialect threaded in to call it correctly. The existing SQLite carve-out at lines 141-146 also needs timestamptz added alongside date, or every SQLite database starts reporting drift.

Happy to send the patch if you would rather not take it on.

9 days ago

I filed #1177 on the same finding a few minutes after this and have closed it as a duplicate. This one is better evidenced: it drives auditSchemaDrift itself with a control column, and it locates both routes in the source.

One cell worth adding, because it widens route 2 beyond the timestamptz spelling. On 0.3.11, familyOfDeclared('timestamp') is also 'other':

familyOfDeclared('timestamp')    -> "other"      // the declared-side switch knows datetime, timestamptz, date — not timestamp
familyOfSqlType('timestamptz')   -> "other"      // the live-side \b failure you identified

Since satisfies treats 'other' as compatible in both directions, a model declaring the bare word timestamp is unaudited against every live type, not just against awareness:

   declared / live                              families       mismatch reported
   timestamp / timestamp with time zone         other/date     false
   timestamp / integer                          other/integer  false

So the "unknown is not evidence of drift" rule at line 169 is reachable from the declared side as well as the live side, and the two classifiers have complementary gaps: timestamp is missing from one, bare timestamptz from the other. Worth covering in the same pass, since your fix already has to touch both functions.

The full compatibility shape, for reference:

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

Identity on the diagonal plus other as a universal wildcard. I checked the dialect argument too: the naive-versus-aware collapse is silent on postgres, mysql and sqlite alike, so the third parameter does not rescue it. The one place it changes the answer is datetime / text, which reports on postgres and mysql and does not on sqlite, which looks like the carve-out you mention at lines 141-146.

One caution on the framing, since it is the thing that cost stacksjs/stacks#2902 three rounds and a wrong close. "Three applications are 8 hours off" needs its hosts named before it is leaned on: a timestamptz column cannot exist in SQLite, so for a SQLite app the 8h has to come from the client-side read rather than from Postgres interpreting a naive literal in the session zone. Route 2 here does cover a SQLite column whose declared text is timestamptz, so the classification bug is real on SQLite; it is the shift that would need a different mechanism. Separating the two keeps this issue clean if someone later cannot reproduce the shift on one of those three.

Sign in to comment on this issue.