From 1a7d533f297b9a6e77d2bfa2955b5a88fcbfb1d5 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 10:11:35 +0000 Subject: [PATCH 1/5] fix(driver-sql): make datetime filters storage-form aware on SQLite (#3912) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A SQLite `Field.datetime` column is mixed-form. A JS `Date` binds as an INTEGER epoch, but a REST/JSON write carries an ISO string (JSON has no `Date`), a `NOW()` default stamps ISO TEXT, and the platform's own `created_at`/`updated_at` are stamped as ISO strings too. The filter path coerced the COMPARAND to epoch ms purely from the DECLARED type, so any TEXT-stored row failed the affinity compare: a two-sided window collapsed to zero rows (a dashboard `last_30_days` read 0 with 29 rows in range), and a one-sided `>=` matched every TEXT row regardless of the bound. Normalise the COLUMN instead of guessing at the comparand, mirroring what the bucketing path already does via `typeof()` (#3773): - `sqliteEpochMsSql` reads either stored form as epoch ms. julianday() is used rather than `unixepoch(x,'subsec')` (SQLite 3.42+) and is exact to the millisecond, so equality filters keep matching. - `filterColumnExpr` returns that expression only for a SQLite `Field.datetime`; every other column/dialect keeps the plain, index-friendly `col op ?`. - `applyNormalizedComparison` compiles the value comparisons (=, !=, <, <=, >, >=, in, nin, between) against it, and declines everything whose meaning does not depend on the stored form — null predicates, the LIKE family, an empty in/nin set, a malformed between — so those keep their existing handling and error messages. Wired into all three filter-compile paths (`applyFilters` object form, `applyAstComparison` array form, `applyFilterCondition` Mongo form). The analytics native-SQL strategy binds its own WHERE, so it needs the same treatment: `temporalFilterColumnSql` is the column-side companion of the existing `temporalFilterValue`, threaded through as `StrategyContext.coerceTemporalFilterColumn` and applied to the scalar comparisons, `in`/`notIn`, and the `timeDimensions` dateRange BETWEEN — the dashboard shape from the report. Absent hook → byte-identical SQL, so Postgres/MySQL and non-SQL drivers are untouched. Tests: a new suite writing the way REST does (ISO TEXT) plus a mixed-storage suite holding both forms in one column — 13 of its 16 cases fail without this change — and strategy-level coverage that the column hook lands on exactly the value comparisons. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TPxNwPnjcn599ujXpU3ibJ --- .../src/sql-driver-analytics-datetime.test.ts | 93 ++++++ ...river-datetime-filter-text-storage.test.ts | 287 ++++++++++++++++++ packages/plugins/driver-sql/src/sql-driver.ts | 162 +++++++++- .../native-sql-datetime-filter-column.test.ts | 148 +++++++++ .../src/analytics-service.ts | 9 + .../services/service-analytics/src/plugin.ts | 32 ++ .../src/strategies/native-sql-strategy.ts | 59 +++- .../spec/src/contracts/analytics-service.ts | 26 ++ 8 files changed, 798 insertions(+), 18 deletions(-) create mode 100644 packages/plugins/driver-sql/src/sql-driver-datetime-filter-text-storage.test.ts create mode 100644 packages/services/service-analytics/src/__tests__/native-sql-datetime-filter-column.test.ts diff --git a/packages/plugins/driver-sql/src/sql-driver-analytics-datetime.test.ts b/packages/plugins/driver-sql/src/sql-driver-analytics-datetime.test.ts index efa031ccdd..c1e53de12f 100644 --- a/packages/plugins/driver-sql/src/sql-driver-analytics-datetime.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-analytics-datetime.test.ts @@ -100,3 +100,96 @@ describe('Analytics datetime filter — SQLite epoch storage (E2E repro)', () => expect(driver.temporalFilterValue(TABLE, 'title', 'hello')).toBe('hello'); }); }); + +/** + * #3912 — coercing the comparand is necessary but NOT sufficient. + * + * The fixture above writes every row with a JS `Date`, so the column is uniformly + * INTEGER epoch. A production table is not: REST/JSON writes carry ISO strings + * (JSON has no `Date`) and `NOW()` defaults stamp ISO TEXT, so the SAME column + * holds both forms. An epoch comparand then matches the INTEGER half and misses + * every TEXT row — a dashboard `last_30_days` reading 0 with rows in range. + * + * `temporalFilterColumnSql` is the companion hook that normalises the COLUMN, so + * the comparison is form-agnostic. These tests bind exactly what the analytics + * strategy binds, against a real mixed-storage SQLite table. + */ +describe('Analytics datetime filter — MIXED storage (#3912)', () => { + let driver: SqlDriver; + const TABLE = 'compliance_assessment'; + const CUTOFF = '2025-06-18'; + + beforeEach(async () => { + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + await driver.initObjects([ + { + name: TABLE, + fields: { title: { type: 'string' }, assessed_at: { type: 'datetime' } }, + }, + ]); + + const rows = [ + ['i1', new Date('2024-01-01T00:00:00Z')], // INTEGER, before cutoff + ['t1', '2024-02-01T00:00:00.000Z'], // TEXT, before cutoff + ['i2', new Date('2025-09-01T09:00:00Z')], // INTEGER, after + ['t2', '2025-10-01T09:00:00.000Z'], // TEXT, after + ['t3', '2026-01-15T09:00:00.000Z'], // TEXT, after + ] as const; + for (const [id, at] of rows) { + await driver.create(TABLE, { id, title: id, assessed_at: at }, { bypassTenantAudit: true }); + } + }); + + afterEach(async () => { + await driver.disconnect(); + }); + + /** + * The `dateRange` shape the dashboard actually emits — a two-sided window, with + * both bounds coerced, optionally reading the column through the fix. + */ + const countInWindow = async (col: string, from: string, to: string, useColumnHook: boolean) => { + const ref = useColumnHook + ? driver.temporalFilterColumnSql(TABLE, col, `"${col}"`) + : `"${col}"`; + const res: any = await driver.execute( + `SELECT count(*) AS n FROM "${TABLE}" WHERE ${ref} >= ? AND ${ref} <= ?`, + [ + driver.temporalFilterValue(TABLE, col, from), + driver.temporalFilterValue(TABLE, col, to), + ], + ); + const row = Array.isArray(res) ? res[0] : res?.rows?.[0] ?? res; + return Number(row.n); + }; + + it('the fixture really is mixed-form', async () => { + const res: any = await driver.execute( + `SELECT typeof(assessed_at) AS t, count(*) AS n FROM "${TABLE}" GROUP BY 1 ORDER BY 1`, + ); + const rows = Array.isArray(res) ? res : res?.rows ?? []; + expect(rows.map((r: any) => [r.t, Number(r.n)])).toEqual([['integer', 2], ['text', 3]]); + }); + + it('BUG: coercing only the comparand empties the window on a TEXT-stored row', async () => { + // SQLite orders INTEGER before TEXT, so a TEXT row passes `>= ` and + // then fails `<= ` — the two-sided window collapses to nothing. That + // is the reported symptom: 0 rows where 3 exist. + expect(await countInWindow('assessed_at', CUTOFF, '2026-12-31', false)).toBe(1); // i2 only + expect(await countInWindow('assessed_at', '2025-10-01', '2026-12-31', false)).toBe(0); + }); + + it('FIX: normalising the column finds rows of BOTH forms in the window', async () => { + expect(await countInWindow('assessed_at', CUTOFF, '2026-12-31', true)).toBe(3); // i2, t2, t3 + expect(await countInWindow('assessed_at', '2025-10-01', '2026-12-31', true)).toBe(2); // t2, t3 + expect(await countInWindow('assessed_at', '2024-01-01', '2024-12-31', true)).toBe(2); // i1, t1 + }); + + it('leaves a non-temporal column reference untouched', () => { + expect(driver.temporalFilterColumnSql(TABLE, 'title', '"title"')).toBe('"title"'); + }); +}); diff --git a/packages/plugins/driver-sql/src/sql-driver-datetime-filter-text-storage.test.ts b/packages/plugins/driver-sql/src/sql-driver-datetime-filter-text-storage.test.ts new file mode 100644 index 0000000000..afffe9e625 --- /dev/null +++ b/packages/plugins/driver-sql/src/sql-driver-datetime-filter-text-storage.test.ts @@ -0,0 +1,287 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Regression #3912 — the OTHER half of the SQLite `Field.datetime` storage mix. + * + * `sql-driver-datetime-filter.test.ts` covers rows written with a JS `Date`, so + * better-sqlite3 stores them as INTEGER epoch ms. But the REST / JSON write path + * cannot produce a `Date` (JSON has no such type) and a `defaultValue: 'NOW()'` + * slot stamps an ISO string, so a production table is dominated by ISO **TEXT** + * — and the filter path coerced its comparand to epoch ms purely from the + * DECLARED type. Every datetime window filter then compared INTEGER-vs-TEXT and + * returned nothing: a dashboard `last_30_days` on `created_date` read 0 while 29 + * rows matched. + * + * These tests write the way REST does (ISO strings) and assert the same filters + * that already pass for `Date`-written rows. The mixed-storage suite at the end + * is the real production shape: one table holding both forms at once. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { SqlDriver } from '../src/index.js'; + +const OBJECT = { + name: 'lead', + fields: { + name: { type: 'string' }, + created_date: { type: 'datetime' }, + closed_on: { type: 'date' }, + }, +} as any; + +describe('SqlDriver datetime filters on ISO-TEXT-stored columns (#3912)', () => { + let driver: SqlDriver; + + beforeEach(async () => { + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + await driver.initObjects([OBJECT]); + + // Written the way a REST/JSON create writes: ISO strings, which + // better-sqlite3 binds as TEXT. + await driver.create('lead', { + id: 'l1', name: 'Old', created_date: '2025-01-15T00:00:00.000Z', closed_on: '2025-01-15', + }, { bypassTenantAudit: true }); + await driver.create('lead', { + id: 'l2', name: 'New', created_date: '2026-03-20T12:00:00.000Z', closed_on: '2026-03-20', + }, { bypassTenantAudit: true }); + await driver.create('lead', { + id: 'l3', name: 'Newer', created_date: '2026-05-25T08:30:15.250Z', closed_on: '2026-05-25', + }, { bypassTenantAudit: true }); + }); + + afterEach(async () => { + await driver.disconnect(); + }); + + it('stores the ISO write as TEXT (the premise these tests rest on)', async () => { + const rows: any = await (driver as any).knex.raw( + `select id, typeof(created_date) as t from lead order by id`, + ); + expect(rows.map((r: any) => r.t)).toEqual(['text', 'text', 'text']); + }); + + it('matches $gte against an ISO date string', async () => { + const rows = await driver.find('lead', { + where: { created_date: { $gte: '2026-01-01' } }, + orderBy: [{ field: 'id', order: 'asc' }], + }); + expect(rows.map((r: any) => r.id)).toEqual(['l2', 'l3']); + }); + + it('matches a $gte / $lt window — the dashboard dateRange shape', async () => { + const rows = await driver.find('lead', { + where: { created_date: { $gte: '2026-01-01', $lt: '2026-05-01' } }, + }); + expect(rows.map((r: any) => r.id)).toEqual(['l2']); + }); + + it('matches a full ISO timestamp comparand, to the millisecond', async () => { + const inclusive = await driver.find('lead', { + where: { created_date: { $gte: '2026-05-25T08:30:15.250Z' } }, + }); + expect(inclusive.map((r: any) => r.id)).toEqual(['l3']); + + const exclusive = await driver.find('lead', { + where: { created_date: { $gt: '2026-05-25T08:30:15.250Z' } }, + }); + expect(exclusive).toEqual([]); + }); + + it('matches a JS Date comparand against a TEXT-stored row', async () => { + const rows = await driver.find('lead', { + where: { created_date: { $gte: new Date('2026-05-01T00:00:00Z') } }, + }); + expect(rows.map((r: any) => r.id)).toEqual(['l3']); + }); + + it('matches equality on the exact stored instant', async () => { + const rows = await driver.find('lead', { + where: { created_date: '2026-03-20T12:00:00.000Z' }, + }); + expect(rows.map((r: any) => r.id)).toEqual(['l2']); + }); + + it('matches $between, $in and $nin', async () => { + const between = await driver.find('lead', { + where: { created_date: { $between: ['2026-01-01', '2026-04-01'] } }, + }); + expect(between.map((r: any) => r.id)).toEqual(['l2']); + + const isIn = await driver.find('lead', { + where: { created_date: { $in: ['2026-03-20T12:00:00.000Z', '2026-05-25T08:30:15.250Z'] } }, + orderBy: [{ field: 'id', order: 'asc' }], + }); + expect(isIn.map((r: any) => r.id)).toEqual(['l2', 'l3']); + + const notIn = await driver.find('lead', { + where: { created_date: { $nin: ['2026-03-20T12:00:00.000Z'] } }, + orderBy: [{ field: 'id', order: 'asc' }], + }); + expect(notIn.map((r: any) => r.id)).toEqual(['l1', 'l3']); + }); + + it('matches $ne and the null predicates', async () => { + const ne = await driver.find('lead', { + where: { created_date: { $ne: '2026-03-20T12:00:00.000Z' } }, + orderBy: [{ field: 'id', order: 'asc' }], + }); + expect(ne.map((r: any) => r.id)).toEqual(['l1', 'l3']); + + await driver.create('lead', { id: 'l4', name: 'Undated' }, { bypassTenantAudit: true }); + const missing = await driver.find('lead', { where: { created_date: { $null: true } } }); + expect(missing.map((r: any) => r.id)).toEqual(['l4']); + + const present = await driver.find('lead', { + where: { created_date: { $null: false } }, + orderBy: [{ field: 'id', order: 'asc' }], + }); + expect(present.map((r: any) => r.id)).toEqual(['l1', 'l2', 'l3']); + }); + + it('matches the AST array filter form', async () => { + const rows = await driver.find('lead', { + where: [['created_date', '>=', '2026-01-01'], ['created_date', '<', '2026-05-01']], + } as any); + expect(rows.map((r: any) => r.id)).toEqual(['l2']); + }); + + it('matches inside an $or branch', async () => { + const rows = await driver.find('lead', { + where: { $or: [{ created_date: { $lt: '2025-06-01' } }, { name: 'Newer' }] }, + orderBy: [{ field: 'id', order: 'asc' }], + }); + expect(rows.map((r: any) => r.id)).toEqual(['l1', 'l3']); + }); + + it('leaves the Field.date column on its own YYYY-MM-DD rule', async () => { + const rows = await driver.find('lead', { + where: { closed_on: { $gte: '2026-01-01' } }, + orderBy: [{ field: 'id', order: 'asc' }], + }); + expect(rows.map((r: any) => r.id)).toEqual(['l2', 'l3']); + }); + +}); + +describe('SqlDriver datetime filters on the created_at audit column (#3912)', () => { + let driver: SqlDriver; + + beforeEach(async () => { + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + // `withAuditFields` (objectql registry) declares `created_at` / `updated_at` + // as `datetime` on every audited object, so the driver sees them as declared + // datetime columns — exactly as spelled here. + await driver.initObjects([ + { + name: 'ticket', + fields: { + subject: { type: 'string' }, + created_at: { type: 'datetime' }, + updated_at: { type: 'datetime' }, + }, + }, + ] as any); + await driver.create('ticket', { id: 't1', subject: 'Stamped' }); + }); + + afterEach(async () => { + await driver.disconnect(); + }); + + it('filters created_at even though the driver stamps it as ISO TEXT', async () => { + // The driver stamps the audit columns with `new Date().toISOString()` — never + // a `Date` — so they are TEXT on EVERY object in the system while being + // declared `datetime`. Filtering on them was unconditionally broken, not an + // edge case. + const stamped: any = await (driver as any).knex.raw( + `select typeof(created_at) as c, typeof(updated_at) as u from ticket`, + ); + expect(stamped[0]).toEqual({ c: 'text', u: 'text' }); + + const matched = await driver.find('ticket', { + where: { created_at: { $gte: '2000-01-01' } }, + }); + expect(matched.map((r: any) => r.id)).toEqual(['t1']); + + // …and a window that starts after the stamp excludes it, so the match above + // is a real comparison rather than "the predicate was dropped". + const future = await driver.find('ticket', { + where: { created_at: { $gte: '2999-01-01' } }, + }); + expect(future).toEqual([]); + }); +}); + +describe('SqlDriver datetime filters on a MIXED-storage column (#3912)', () => { + let driver: SqlDriver; + + beforeEach(async () => { + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + await driver.initObjects([OBJECT]); + + // The production shape: seeds/imports bind a `Date` (INTEGER epoch) while the + // REST API writes ISO strings (TEXT) — into the same column. + await driver.create('lead', { + id: 'int-old', name: 'int-old', created_date: new Date('2025-02-01T00:00:00Z'), + }, { bypassTenantAudit: true }); + await driver.create('lead', { + id: 'txt-old', name: 'txt-old', created_date: '2025-02-02T00:00:00.000Z', + }, { bypassTenantAudit: true }); + await driver.create('lead', { + id: 'int-new', name: 'int-new', created_date: new Date('2026-06-01T00:00:00Z'), + }, { bypassTenantAudit: true }); + await driver.create('lead', { + id: 'txt-new', name: 'txt-new', created_date: '2026-06-02T00:00:00.000Z', + }, { bypassTenantAudit: true }); + }); + + afterEach(async () => { + await driver.disconnect(); + }); + + it('really does hold both storage forms', async () => { + const rows: any = await (driver as any).knex.raw( + `select id, typeof(created_date) as t from lead order by id`, + ); + expect(Object.fromEntries(rows.map((r: any) => [r.id, r.t]))).toEqual({ + 'int-new': 'integer', 'int-old': 'integer', 'txt-new': 'text', 'txt-old': 'text', + }); + }); + + it('returns rows of BOTH forms from one window filter', async () => { + const rows = await driver.find('lead', { + where: { created_date: { $gte: '2026-01-01' } }, + orderBy: [{ field: 'id', order: 'asc' }], + }); + expect(rows.map((r: any) => r.id)).toEqual(['int-new', 'txt-new']); + }); + + it('excludes rows of BOTH forms that fall outside the window', async () => { + const rows = await driver.find('lead', { + where: { created_date: { $lt: '2026-01-01' } }, + orderBy: [{ field: 'id', order: 'asc' }], + }); + expect(rows.map((r: any) => r.id)).toEqual(['int-old', 'txt-old']); + }); + + it('counts rows of both forms in an aggregate window', async () => { + const rows: any = await driver.aggregate('lead', { + aggregations: [{ function: 'count', alias: 'n' }], + where: { created_date: { $gte: '2026-01-01' } }, + } as any); + // 4 means the window was dropped; 1 means only one storage form matched. + expect(Number(rows[0].n)).toBe(2); + }); +}); diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index 8b7a311195..8e4684a707 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -3646,6 +3646,117 @@ export class SqlDriver implements IDataDriver { }; } + /** + * SQL that reads a SQLite `Field.datetime` column as epoch **milliseconds**, + * whatever form the row actually stored it in. + * + * A `Field.datetime` column on SQLite is genuinely MIXED-form, and always has + * been: a value bound as a JS `Date` lands as INTEGER epoch ms, while a REST / + * JSON write (JSON has no `Date`, so the payload carries an ISO string) and a + * `defaultValue: 'NOW()'` slot — including the platform's own `created_at` / + * `updated_at` audit stamps — land as ISO TEXT. `formatOutput` → + * `normalizeSqliteDatetimeOutput` already repairs that mix on read, and + * {@link sqliteTemporalArg} already dispatches on `typeof()` for bucketing + * (#3773). The filter path had no equivalent: it coerced the COMPARAND to + * epoch ms purely from the DECLARED type, so an ISO-TEXT-stored row failed the + * TEXT-vs-INTEGER affinity compare and every datetime window filter returned + * empty (#3912) — the exact failure the epoch coercion was added to prevent, + * just with the two storage forms swapped. + * + * Normalising the COLUMN (rather than guessing at the comparand) is what makes + * the comparison correct for both forms at once. The julian-day round trip is + * used instead of `unixepoch(x, 'subsec')` because the latter needs SQLite + * 3.42+; `julianday()` is exact to the millisecond on every version (SQLite + * carries the julian day as integer ms internally), so `round()` recovers the + * epoch exactly and equality filters keep matching. An unparseable TEXT value + * yields NULL, which compares false — the same non-match it produced before. + */ + protected sqliteEpochMsSql(columnSql: string): string { + return ( + `(case when typeof(${columnSql}) in ('integer','real') then ${columnSql} ` + + `else cast(round((julianday(${columnSql}) - 2440587.5) * 86400000.0) as integer) end)` + ); + } + + /** + * The left-hand side of a filter comparison on `column`, normalised to the + * storage form {@link coerceFilterValue} coerces the comparand into. + * + * `null` — the overwhelmingly common answer — means the plain column + * identifier is already correct, so the caller keeps using the ordinary Knex + * builder call (and its index-friendly `col op ?` SQL). Only a SQLite + * `Field.datetime` needs the {@link sqliteEpochMsSql} CASE. + */ + protected filterColumnExpr( + table: string | null | undefined, + field: string, + column: string, + ): { sql: string; bindings: any[] } | null { + if (!this.isEpochStoredDatetime(table, field)) return null; + return { sql: this.sqliteEpochMsSql('??'), bindings: [column, column, column] }; + } + + /** + * Compile one VALUE comparison against a storage-normalised column expression + * ({@link filterColumnExpr}), for the operators where the stored form actually + * matters. + * + * Returns `false` — "not handled, carry on" — for everything else, so the + * caller's normal Knex path still owns: null predicates (`IS NULL` reads the + * raw column and is form-independent), the `LIKE` family (a substring match on + * an instant is meaningless, and the raw column is what the user typed + * against), an empty `in`/`nin` set (Knex's `1 = 0` / `1 = 1` shortcuts), and a + * malformed `between` (so the caller still throws its descriptive error). + */ + private applyNormalizedComparison( + builder: any, + join: 'and' | 'or', + expr: { sql: string; bindings: any[] }, + op: string, + value: unknown, + ): boolean { + const raw = join === 'or' ? 'orWhereRaw' : 'whereRaw'; + const binary = (sqlOp: string): boolean => { + // A null comparand is a null PREDICATE, not a comparison — hand it back so + // the caller compiles `IS NULL` / `IS NOT NULL` as it always has. + if (value == null) return false; + builder[raw](`${expr.sql} ${sqlOp} ?`, [...expr.bindings, value]); + return true; + }; + const list = (sqlOp: 'in' | 'not in'): boolean => { + if (!Array.isArray(value) || value.length === 0) return false; + const placeholders = value.map(() => '?').join(', '); + builder[raw](`${expr.sql} ${sqlOp} (${placeholders})`, [...expr.bindings, ...value]); + return true; + }; + + switch (op) { + case '=': case '==': case '$eq': + return binary('='); + case '!=': case '<>': case '$ne': + return binary('<>'); + case '>': case '$gt': + return binary('>'); + case '>=': case '$gte': + return binary('>='); + case '<': case '$lt': + return binary('<'); + case '<=': case '$lte': + return binary('<='); + case 'in': case '$in': + return list('in'); + case 'nin': case 'not_in': case 'notin': case '$nin': + return list('not in'); + case 'between': case '$between': { + if (!Array.isArray(value) || value.length !== 2) return false; + builder[raw](`${expr.sql} between ? and ?`, [...expr.bindings, value[0], value[1]]); + return true; + } + default: + return false; + } + } + /** * Public, dialect-correct temporal filter-value coercion for callers that * build SQL *outside* the normal `find()`/`applyFilters()` path — chiefly the @@ -3670,6 +3781,25 @@ export class SqlDriver implements IDataDriver { return this.coerceFilterValue(objectName, field, value); } + /** + * The companion of {@link temporalFilterValue} for the same outside-the-builder + * callers: given the SQL they were going to put on the LEFT of the comparison + * (an already-quoted, possibly join-qualified column reference), return the SQL + * they must use instead so the column reads in the storage form the coerced + * comparand is in. + * + * Everything but a SQLite `Field.datetime` gets its `columnSql` back verbatim. + * That one case gets the {@link sqliteEpochMsSql} CASE, because the column is + * mixed INTEGER-epoch / ISO-TEXT and coercing only the value matches whichever + * half the writer happened to produce (#3912). Coercing the value is therefore + * necessary but NOT sufficient — a caller that binds `temporalFilterValue` + * must wrap its column with this too, or it keeps half the bug. + */ + public temporalFilterColumnSql(objectName: string, field: string, columnSql: string): string { + if (!this.isEpochStoredDatetime(objectName, field)) return columnSql; + return this.sqliteEpochMsSql(columnSql); + } + protected applyFilters(builder: Knex.QueryBuilder, filters: any) { if (!filters) return; const table = this.coercionKey(builder); @@ -3690,7 +3820,11 @@ export class SqlDriver implements IDataDriver { for (const [key, value] of Object.entries(filters)) { if (['limit', 'offset', 'fields', 'orderBy'].includes(key)) continue; - builder.where(this.remoteColumn(table, key, key), this.coerceFilterValue(table, key, value) as any); + const column = this.remoteColumn(table, key, key); + const coerced = this.coerceFilterValue(table, key, value); + const expr = this.filterColumnExpr(table, key, column); + if (expr && this.applyNormalizedComparison(builder, 'and', expr, '=', coerced)) continue; + builder.where(column, coerced as any); } return; } @@ -3714,7 +3848,10 @@ export class SqlDriver implements IDataDriver { const localField = this.mapSortField(fieldRaw); const field = this.remoteColumn(table, fieldRaw, localField); const coerced = this.coerceFilterValue(table, localField, value); - this.applyAstComparison(builder, nextJoin, field, op, value, coerced); + this.applyAstComparison( + builder, nextJoin, field, op, value, coerced, + this.filterColumnExpr(table, localField, field), + ); } else { const method = nextJoin === 'or' ? 'orWhere' : 'where'; (builder as any)[method]((qb: any) => { @@ -3779,6 +3916,12 @@ export class SqlDriver implements IDataDriver { * null predicates compile to a real `IS NULL` / `IS NOT NULL` (unified with * the `{field, equals, null}` path), and any operator off the whitelist * throws instead of ever reaching Knex. + * + * `columnExpr` (from {@link filterColumnExpr}) is the storage-normalised form + * of `field` — non-null only for a SQLite `Field.datetime`, where comparing the + * raw column would compare against whichever of the two stored forms the writer + * happened to produce (#3912). It is optional so the protected signature stays + * source-compatible for subclasses; omitting it just keeps the raw column. */ protected applyAstComparison( builder: any, @@ -3787,12 +3930,18 @@ export class SqlDriver implements IDataDriver { op: string, rawValue: unknown, coerced: unknown, + columnExpr?: { sql: string; bindings: any[] } | null, ): void { const where = join === 'or' ? 'orWhere' : 'where'; const whereNull = join === 'or' ? 'orWhereNull' : 'whereNull'; const whereNotNull = join === 'or' ? 'orWhereNotNull' : 'whereNotNull'; const opLower = String(op).toLowerCase(); + // Value comparisons on a mixed-storage column read it through the CASE; every + // other operator (null predicates, the LIKE family, a malformed `between`) + // declines and falls through to the ordinary handling below. + if (columnExpr && this.applyNormalizedComparison(builder, join, columnExpr, opLower, coerced)) return; + switch (opLower) { // Equality — 2-arg form so Knex renders `IS NULL` for a null comparand, // keeping the `{field, equals, null}` path working. @@ -3937,9 +4086,13 @@ export class SqlDriver implements IDataDriver { } else if (typeof value === 'object' && value !== null && !Array.isArray(value)) { const localField = this.mapSortField(key); const field = this.remoteColumn(table, key, localField); + // Non-null only for a SQLite `Field.datetime`, whose two stored forms + // (INTEGER epoch / ISO TEXT) must be unified before comparing (#3912). + const columnExpr = this.filterColumnExpr(table, localField, field); for (const [op, opValue] of Object.entries(value as Record)) { const method = logicalOp === 'or' ? 'orWhere' : 'where'; const coerced = this.coerceFilterValue(table, localField, opValue); + if (columnExpr && this.applyNormalizedComparison(builder, logicalOp, columnExpr, op, coerced)) continue; switch (op) { case '$eq': (builder as any)[method](field, coerced); @@ -4025,7 +4178,10 @@ export class SqlDriver implements IDataDriver { const localField = this.mapSortField(key); const field = this.remoteColumn(table, key, localField); const method = logicalOp === 'or' ? 'orWhere' : 'where'; - (builder as any)[method](field, this.coerceFilterValue(table, localField, value) as any); + const coerced = this.coerceFilterValue(table, localField, value); + const columnExpr = this.filterColumnExpr(table, localField, field); + if (columnExpr && this.applyNormalizedComparison(builder, logicalOp, columnExpr, '=', coerced)) continue; + (builder as any)[method](field, coerced as any); } } } diff --git a/packages/services/service-analytics/src/__tests__/native-sql-datetime-filter-column.test.ts b/packages/services/service-analytics/src/__tests__/native-sql-datetime-filter-column.test.ts new file mode 100644 index 0000000000..18d9a9a583 --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/native-sql-datetime-filter-column.test.ts @@ -0,0 +1,148 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Regression #3912 — the COLUMN half of the datetime storage-form fix. + * + * `native-sql-datetime-filter.test.ts` covers coercing the comparand to epoch ms. + * That alone is not enough: a SQLite `Field.datetime` column holds an INTEGER + * epoch (a `Date` write) and ISO TEXT (a REST/JSON write, a `NOW()` default) at + * the same time, so an epoch comparand matches the INTEGER rows and misses every + * TEXT one — a dashboard `dateRange: last_30_days` reading 0 with rows in range. + * + * The fix threads a companion `StrategyContext.coerceTemporalFilterColumn` hook + * that lets the driver normalise the column reference. These tests assert the + * strategy applies it to exactly the value comparisons, leaves the null and LIKE + * predicates on the raw column, and emits byte-identical SQL when the hook is + * absent (Postgres, non-SQL drivers, legacy wiring). + */ + +import { describe, it, expect } from 'vitest'; +import type { Cube } from '@objectstack/spec/data'; +import type { AnalyticsQuery, StrategyContext } from '@objectstack/spec/contracts'; +import { NativeSQLStrategy } from '../strategies/native-sql-strategy.js'; + +const cube: Cube = { + name: 'compliance', + title: 'Compliance', + sql: 'compliance_assessment', + measures: { total: { name: 'total', label: 'Total', type: 'count', sql: '*' } }, + dimensions: { + // Dimension id deliberately differs from the column, so a hook that fires on + // `assessed_at` proves the storage target resolved the real column. + assessed: { name: 'assessed', label: 'Assessed', type: 'time', sql: 'assessed_at' }, + title: { name: 'title', label: 'Title', type: 'string', sql: 'title' }, + }, + public: false, +}; + +/** Stand-in for `SqlDriver.temporalFilterColumnSql` under better-sqlite3. */ +function sqliteColumnHook(object: string, field: string, columnSql: string): string { + if (object === 'compliance_assessment' && field === 'assessed_at') { + return `EPOCH_MS(${columnSql})`; + } + return columnSql; // date text / non-temporal / native timestamp → unchanged +} + +function ctxWith(overrides: Partial): StrategyContext { + return { + getCube: (name) => (name === 'compliance' ? cube : undefined), + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }), + executeRawSql: async () => [], + ...overrides, + }; +} + +const withHook = () => ctxWith({ coerceTemporalFilterColumn: sqliteColumnHook }); + +const gen = async (query: AnalyticsQuery, ctx: StrategyContext) => + new NativeSQLStrategy().generateSql(query, ctx); + +describe('NativeSQLStrategy — datetime filter column normalisation (#3912)', () => { + it('normalises the column on a scalar comparison', async () => { + const { sql } = await gen( + { cube: 'compliance', measures: ['total'], where: { assessed: { $gte: '2025-06-18' } } }, + withHook(), + ); + expect(sql).toContain('EPOCH_MS(assessed_at) >= $1'); + }); + + it('normalises the column on an `in` set', async () => { + const { sql } = await gen( + { + cube: 'compliance', + measures: ['total'], + where: { assessed: { $in: ['2025-06-18', '2025-06-19'] } }, + }, + withHook(), + ); + expect(sql).toContain('EPOCH_MS(assessed_at) IN ($1, $2)'); + }); + + it('normalises the column on a timeDimension dateRange — the dashboard shape', async () => { + const { sql } = await gen( + { + cube: 'compliance', + measures: ['total'], + timeDimensions: [{ dimension: 'assessed', dateRange: ['2025-06-18', '2025-07-01'] }], + }, + withHook(), + ); + expect(sql).toContain( + 'EPOCH_MS(assessed_at) BETWEEN $1 AND $2', + ); + }); + + it('leaves the null predicates on the raw column (storage-independent)', async () => { + const set = await gen( + { cube: 'compliance', measures: ['total'], where: { assessed: { $exists: true } } }, + withHook(), + ); + expect(set.sql).toContain('assessed_at IS NOT NULL'); + expect(set.sql).not.toContain('EPOCH_MS'); + + const notSet = await gen( + { cube: 'compliance', measures: ['total'], where: { assessed: null } }, + withHook(), + ); + expect(notSet.sql).toContain('assessed_at IS NULL'); + expect(notSet.sql).not.toContain('EPOCH_MS'); + }); + + it('leaves a LIKE match on the raw column (a substring match reads the text)', async () => { + const { sql } = await gen( + { cube: 'compliance', measures: ['total'], where: { assessed: { $contains: '2025-06' } } }, + withHook(), + ); + expect(sql).toContain('assessed_at LIKE $1'); + expect(sql).not.toContain('EPOCH_MS'); + }); + + it('does not touch a non-temporal column', async () => { + const { sql } = await gen( + { cube: 'compliance', measures: ['total'], where: { title: { $eq: 'SOC2' } } }, + withHook(), + ); + expect(sql).toContain('title = $1'); + expect(sql).not.toContain('EPOCH_MS'); + }); + + it('is backward-compatible: no hook → the bare column, exactly as before', async () => { + const query: AnalyticsQuery = { + cube: 'compliance', + measures: ['total'], + where: { assessed: { $gte: '2025-06-18' } }, + timeDimensions: [{ dimension: 'assessed', dateRange: ['2025-06-18', '2025-07-01'] }], + }; + const { sql } = await gen(query, ctxWith({})); + expect(sql).toContain('assessed_at >= $1'); + expect(sql).toContain('assessed_at BETWEEN $2 AND $3'); + }); + + it('falls back to the bare column when the hook returns nothing usable', async () => { + const { sql } = await gen( + { cube: 'compliance', measures: ['total'], where: { assessed: { $gte: '2025-06-18' } } }, + ctxWith({ coerceTemporalFilterColumn: () => '' as unknown as string }), + ); + expect(sql).toContain('assessed_at >= $1'); + }); +}); diff --git a/packages/services/service-analytics/src/analytics-service.ts b/packages/services/service-analytics/src/analytics-service.ts index b45b3e058e..45c1fb26db 100644 --- a/packages/services/service-analytics/src/analytics-service.ts +++ b/packages/services/service-analytics/src/analytics-service.ts @@ -175,6 +175,14 @@ export interface AnalyticsServiceConfig { * `StrategyContext.coerceTemporalFilterValue` for the full rationale. */ coerceTemporalFilterValue?: (objectName: string, fieldName: string, value: unknown) => unknown; + /** + * Normalise the COLUMN side of the same comparison to that storage form — the + * other half of the fix, needed because a SQLite `Field.datetime` holds both an + * INTEGER epoch (a `Date` write) and ISO TEXT (a REST/JSON write, a `NOW()` + * default) at once, so coercing only the comparand matches one of them and + * misses the other (#3912). See `StrategyContext.coerceTemporalFilterColumn`. + */ + coerceTemporalFilterColumn?: (objectName: string, fieldName: string, columnSql: string) => string; /** * ADR-0062 D6 — report whether an object is federated (external datasource). * Threaded into the StrategyContext so `NativeSQLStrategy` declines external @@ -331,6 +339,7 @@ export class AnalyticsService implements IAnalyticsService { this.datasetRegistry.get(cubeName)?.allowedRelationships ?? config.getAllowedRelationships?.(cubeName), coerceTemporalFilterValue: config.coerceTemporalFilterValue, + coerceTemporalFilterColumn: config.coerceTemporalFilterColumn, isExternalObject: config.isExternalObject, }; diff --git a/packages/services/service-analytics/src/plugin.ts b/packages/services/service-analytics/src/plugin.ts index 44d7e9646e..5adbb5a582 100644 --- a/packages/services/service-analytics/src/plugin.ts +++ b/packages/services/service-analytics/src/plugin.ts @@ -61,6 +61,14 @@ interface DriverLike { * timestamp / non-temporal → unchanged). Optional — only SqlDriver implements it. */ temporalFilterValue?(objectName: string, field: string, value: unknown): unknown; + /** + * Normalise the column reference to that same storage form. Required alongside + * `temporalFilterValue` on any dialect whose column is mixed-form — a SQLite + * `Field.datetime` holds an INTEGER epoch and ISO TEXT at once, so coercing + * only the value fixes one half and leaves the other empty (#3912). Optional — + * only SqlDriver implements it. + */ + temporalFilterColumnSql?(objectName: string, field: string, columnSql: string): string; } /** @@ -473,6 +481,29 @@ export class AnalyticsServicePlugin implements Plugin { return value; }; + // The column half of the same fix (#3912). A SQLite `Field.datetime` column + // holds BOTH storage forms — INTEGER epoch from a `Date` write, ISO TEXT from + // a REST/JSON write or a `NOW()` default — so coercing the comparand alone + // matched whichever half the writer produced and returned an empty window for + // the other. Ask the driver for the column expression that normalises both. + const coerceTemporalFilterColumn = ( + objectName: string, + fieldName: string, + columnSql: string, + ): string => { + try { + const svc = ctx.getService('data'); + const driver = svc?.getDriverForObject?.(objectName); + if (driver && typeof driver.temporalFilterColumnSql === 'function') { + return driver.temporalFilterColumnSql(objectName, fieldName, columnSql); + } + } catch { + // Same tiering as above — an unresolvable driver emits the bare column, + // which is today's behaviour and correct on every non-mixed dialect. + } + return columnSql; + }; + const config: AnalyticsServiceConfig = { cubes: this.options.cubes, logger: ctx.logger, @@ -483,6 +514,7 @@ export class AnalyticsServicePlugin implements Plugin { getReadScope, getAllowedRelationships: this.options.getAllowedRelationships, coerceTemporalFilterValue, + coerceTemporalFilterColumn, relationshipResolver, labelResolver, // ADR-0053 — source-field currency metadata for the measure currency chain. diff --git a/packages/services/service-analytics/src/strategies/native-sql-strategy.ts b/packages/services/service-analytics/src/strategies/native-sql-strategy.ts index 13f982f333..a9a35543c3 100644 --- a/packages/services/service-analytics/src/strategies/native-sql-strategy.ts +++ b/packages/services/service-analytics/src/strategies/native-sql-strategy.ts @@ -119,13 +119,18 @@ export class NativeSQLStrategy implements AnalyticsStrategy { if (range.length === 2) { // Same epoch-vs-text root cause as buildFilterClause: a dateRange on a // SQLite `Field.datetime` column compares ISO TEXT against an INTEGER - // epoch and matches nothing. Coerce both bounds to the storage form. + // epoch and matches nothing. Coerce both bounds to the storage form — + // and normalise the column to that form too, because the column holds + // BOTH forms at once and coercing only the bounds still empties the + // half the writer stored the other way (#3912). const td2 = this.resolveStorageTarget(cube, td.dimension, tableName); params.push( this.coerceTemporal(ctx, td2, range[0]), this.coerceTemporal(ctx, td2, range[1]), ); - whereClauses.push(`${colExpr} BETWEEN $${params.length - 1} AND $${params.length}`); + whereClauses.push( + `${this.temporalColumn(ctx, td2, colExpr)} BETWEEN $${params.length - 1} AND $${params.length}`, + ); } } } @@ -436,8 +441,28 @@ export class NativeSQLStrategy implements AnalyticsStrategy { return coerceFilterValueForSql(value); } - private buildFilterClause( + /** + * The column side of {@link coerceTemporal}: normalise the reference so it + * reads in the storage form the comparand was coerced into. + * + * A SQLite `Field.datetime` column carries an INTEGER epoch (a `Date` write) + * and ISO TEXT (a REST/JSON write, a `NOW()` default — including the platform's + * own `created_at`) at the SAME time, so coercing the value alone fixes one half + * and empties the other. That is #3912: a `dateRange: last_30_days` on + * `created_date` read 0 with 29 rows in range. Every other column and dialect + * gets its reference back verbatim. + */ + private temporalColumn( + ctx: StrategyContext, + target: { object: string; field: string }, col: string, + ): string { + if (typeof ctx.coerceTemporalFilterColumn !== 'function') return col; + return ctx.coerceTemporalFilterColumn(target.object, target.field, col) || col; + } + + private buildFilterClause( + rawCol: string, operator: string, values: string[] | undefined, params: unknown[], @@ -449,8 +474,11 @@ export class NativeSQLStrategy implements AnalyticsStrategy { contains: 'LIKE', notContains: 'NOT LIKE', }; - if (operator === 'set') return `${col} IS NOT NULL`; - if (operator === 'notSet') return `${col} IS NULL`; + // Null predicates and the LIKE family read the column as stored — the former + // is storage-independent, the latter is a substring match on the raw text — + // so only the value comparisons take the normalised reference. + if (operator === 'set') return `${rawCol} IS NOT NULL`; + if (operator === 'notSet') return `${rawCol} IS NULL`; if (operator === 'in' || operator === 'notIn') { if (!values || values.length === 0) return null; @@ -458,7 +486,7 @@ export class NativeSQLStrategy implements AnalyticsStrategy { // KPI), so coerce each element to the column's storage form too — same // SQLite epoch-vs-text root cause as the scalar operators below. const placeholders = values.map(v => { params.push(this.coerceTemporal(ctx, target, v)); return `$${params.length}`; }).join(', '); - return `${col} ${operator === 'in' ? 'IN' : 'NOT IN'} (${placeholders})`; + return `${this.temporalColumn(ctx, target, rawCol)} ${operator === 'in' ? 'IN' : 'NOT IN'} (${placeholders})`; } const sqlOp = opMap[operator]; @@ -466,16 +494,17 @@ export class NativeSQLStrategy implements AnalyticsStrategy { if (operator === 'contains' || operator === 'notContains') { params.push(`%${values[0]}%`); - } else { - // Coerce so booleans/numbers bind as their native SQL types AND so a - // relative-date / ISO-string comparand on a SQLite `Field.datetime` - // column is converted to its INTEGER epoch storage form. Without this a - // dashboard filter like `assessed_at >= '2025-06-18'` compiles to a - // TEXT-vs-INTEGER affinity compare that is always false → "No rows", - // even though the rows exist (the confirmed time-series chart bug). - params.push(this.coerceTemporal(ctx, target, values[0])); + return `${rawCol} ${sqlOp} $${params.length}`; } - return `${col} ${sqlOp} $${params.length}`; + + // Coerce so booleans/numbers bind as their native SQL types AND so a + // relative-date / ISO-string comparand on a SQLite `Field.datetime` + // column is converted to its INTEGER epoch storage form. Without this a + // dashboard filter like `assessed_at >= '2025-06-18'` compiles to a + // TEXT-vs-INTEGER affinity compare that is always false → "No rows", + // even though the rows exist (the confirmed time-series chart bug). + params.push(this.coerceTemporal(ctx, target, values[0])); + return `${this.temporalColumn(ctx, target, rawCol)} ${sqlOp} $${params.length}`; } private extractObjectName(cube: Cube): string { diff --git a/packages/spec/src/contracts/analytics-service.ts b/packages/spec/src/contracts/analytics-service.ts index 2deb994ac9..2db04ffb91 100644 --- a/packages/spec/src/contracts/analytics-service.ts +++ b/packages/spec/src/contracts/analytics-service.ts @@ -394,6 +394,32 @@ export interface StrategyContext { */ coerceTemporalFilterValue?(objectName: string, fieldName: string, value: unknown): unknown; + /** + * The companion of {@link StrategyContext.coerceTemporalFilterValue} for the + * LEFT side of the same comparison: given the SQL the strategy was going to + * emit for the column (an already-quoted, possibly alias-qualified reference), + * return the SQL it must emit instead so the column reads in the same storage + * form the coerced comparand is in. + * + * Why coercing the value alone is not enough: a SQLite `Field.datetime` column + * is MIXED-form in practice. A JS `Date` binds as an INTEGER epoch, but a REST + * / JSON write carries an ISO string (JSON has no `Date`) and a `NOW()` default + * — including the platform's own `created_at` / `updated_at` stamps — lands as + * ISO TEXT. Coercing the comparand to epoch ms therefore fixes the INTEGER rows + * and breaks the TEXT ones, which is why a dashboard `dateRange: last_30_days` + * still read 0 while the rows existed (#3912). The driver answers with an + * expression that normalises whatever is stored, so both halves match. + * + * Everything else — `Field.date`, native-timestamp dialects, non-temporal + * columns — gets `columnSql` back verbatim, and when the hook is absent the + * strategy emits the bare column exactly as before, so it is purely additive. + * + * @param objectName Logical object / table backing the cube. + * @param fieldName Bare column name the filter targets. + * @param columnSql The SQL reference the strategy resolved for that column. + */ + coerceTemporalFilterColumn?(objectName: string, fieldName: string, columnSql: string): string; + /** * ADR-0062 D6 — is `objectName` a federated (external-datasource) object? * From 6c7a214bcaf2ae8daa0fc052fb3a34aaab80140a Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 11:08:58 +0000 Subject: [PATCH 2/5] =?UTF-8?q?refactor(driver-sql):=20give=20Field.dateti?= =?UTF-8?q?me=20ONE=20storage=20form=20=E2=80=94=20canonical=20UTC=20text?= =?UTF-8?q?=20(#3912)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The filter-level fix (normalise the column inside the comparison) made queries correct but left the disease: two write paths producing two shapes in one column. That still mis-ordered ORDER BY (#3928), still cost an unindexable expression on every temporal predicate, and still meant storage disagreed with what find() presents. So fix the storage. `Field.datetime` is now stored as `YYYY-MM-DDTHH:MM:SS.sssZ`. `canonicalUtcDatetime()` is the one function producing it, applied on write (formatInput) and to every filter comparand (coerceFilterValue), so the two sides of a comparison cannot disagree about shape. Chosen over an epoch integer because lexicographic order IS chronological order: range filters and ORDER BY read the column directly and can use an index, strftime parses it so the bucket expression needs no CASE, it is what formatOutput already presented, it matches the Field.date convention, and it was already the majority of rows on disk. Evaluated the other dialects rather than assuming, against a real PostgreSQL 16 with TimeZone=Asia/Shanghai: - The INTEGER/TEXT storage mix is SQLite-only — knex's table.timestamp creates timestamptz on PG (useTz defaults true, verified), so a Date and an ISO-Z string always meant the same instant there. - But PG had the same family of bug, quieter. A zone-naive write bound into timestamptz resolves against the SERVER's timezone: '2026-03-20 12:00:00' stored as 04:00Z, 8 hours off the instant SQLite records. And an un-anchored `YYYY-MM-DD` comparand meant the server's local midnight, so the identical query over the identical instant put a row on a DIFFERENT calendar day than SQLite did. Making the comparand rule dialect-independent fixes both; PG needed no migration. - MySQL is NOT covered: table.timestamp emits TIMESTAMP (range ends 2038-01-19) and the connection sets no timezone, so mysql2 serialises a Date in the process's local zone. Separate defect, needs a real MySQL to verify and a column-type migration; filed on its own. Existing rows converge in backfillCanonicalDatetimes, run from initObjects. One UPDATE per column whose SET expression IS sqliteCanonicalDatetimeSql — the same expression the read paths use, so the migration cannot drift from the repair it retires. `col IS NOT ` is the whole WHERE: null-safe and type-aware, so epoch rows match, canonical rows are skipped, and unparseable values fall through coalesce unchanged rather than being destroyed. One scan, zero writes, idempotent. The backfill is allowed to fail. It logs, marks nothing, and the read paths keep sqliteCanonicalDatetimeSql — so an un-migrated column still compares and buckets correctly, just unindexed. Correctness must never be contingent on a migration having run, and the repair stays permanently for external/unmanaged tables that never get one. needsLegacyDatetimeRepair (replacing isEpochStoredDatetime, whose name no longer described anything) is the single predicate the filter, bucket and analytics paths all ask, so they cannot drift about whether a column is clean. ADR-0053 recorded "datetime stays stored as UTC epoch ms" as Phase 1 step 4; addendum D-B1/D-B2/D-B3 revises it with the measurements above. Tests: canonical-storage suite (every input shape folds to one stored string; ORDER BY is chronological across write shapes; backfill converges, is idempotent, preserves junk, and returns identical rows before/after); an opt-in live-Postgres suite behind OS_TEST_POSTGRES_URL that asserts it is pointed at a non-UTC server so it cannot pass vacuously (3 of its 5 cases fail without this change). Suites that pinned the mixed-storage premise now assert the new invariant, with the legacy forms moved to a LegacyStorageDriver test double — seeded raw with the canonical marker cleared, the honest simulation of a database whose backfill has not run. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TPxNwPnjcn599ujXpU3ibJ --- docs/adr/0053-date-and-datetime-semantics.md | 114 ++++- .../src/legacy-datetime-storage.testkit.ts | 51 +++ ...l-driver-aggregate-temporal-output.test.ts | 56 ++- .../src/sql-driver-analytics-datetime.test.ts | 112 +++-- .../sql-driver-date-bucket-storage.test.ts | 79 ++-- ...-driver-datetime-canonical-storage.test.ts | 211 +++++++++ ...river-datetime-filter-text-storage.test.ts | 62 ++- ...-driver-datetime-postgres-timezone.test.ts | 121 +++++ .../src/sql-driver-temporal-dialect.test.ts | 102 ++++- ...river-user-datetime-default-format.test.ts | 43 +- packages/plugins/driver-sql/src/sql-driver.ts | 415 ++++++++++++------ 11 files changed, 1065 insertions(+), 301 deletions(-) create mode 100644 packages/plugins/driver-sql/src/legacy-datetime-storage.testkit.ts create mode 100644 packages/plugins/driver-sql/src/sql-driver-datetime-canonical-storage.test.ts create mode 100644 packages/plugins/driver-sql/src/sql-driver-datetime-postgres-timezone.test.ts diff --git a/docs/adr/0053-date-and-datetime-semantics.md b/docs/adr/0053-date-and-datetime-semantics.md index 3f0b041add..3dfd1e78fb 100644 --- a/docs/adr/0053-date-and-datetime-semantics.md +++ b/docs/adr/0053-date-and-datetime-semantics.md @@ -1,6 +1,6 @@ # ADR-0053: `date` is a timezone-naive calendar day; `datetime` is an instant rendered in a reference timezone -**Status**: Accepted (2026-06-16) — Phase 1 + addendum D-A1 implemented (`sql-driver.ts` `toDateOnly` write/read/filter normalization; analytics `coerceTemporalFilterValue`), Phase 2 landing incrementally; D-A2 (`temporalFilterValue` promotion onto the `IDataDriver` contract) still open as the ADR predicted. +**Status**: Accepted (2026-06-16) — Phase 1 + addendum D-A1 implemented (`sql-driver.ts` `toDateOnly` write/read/filter normalization; analytics `coerceTemporalFilterValue`), Phase 2 landing incrementally; D-A2 (`temporalFilterValue` promotion onto the `IDataDriver` contract) still open as the ADR predicted. **Partly superseded (2026-07-29, addendum D-B1):** Phase 1's "`Field.datetime` stays stored as UTC epoch ms" is replaced by a canonical `YYYY-MM-DDTHH:MM:SS.sssZ` text storage form, applied on write and to filter comparands on every dialect — see the final addendum (#3912). **Deciders**: ObjectStack Protocol Architects **Builds on**: [ADR-0032](./0032-unified-expression-layer.md) (unified expression layer — CEL dialect, `today()`/`daysFromNow()`), [ADR-0014](./0014-record-form-field-type.md) (field types) **Consumers**: `@objectstack/spec` (`Field.date`/`Field.datetime`), `@objectstack/driver-sql` (`coerceFilterValue`, `formatInput`/`formatOutput`, `dateFields`/`datetimeFields`), `@objectstack/formula` (`stdlib` time functions, `cel-engine` hydration), `@objectstack/objectql` (`applyFormulaPlan`), schedule/cron executors, report/analytics date bucketing, `sys-user-preference.timezone`. @@ -402,3 +402,115 @@ time, the matrix proves runtime correctness across drivers. inventing a semantic" stance. No change to Phase 2's reference-timezone plan. - Until D-A2 lands, the hook depends on a duck-typed driver method — a known, intentionally-temporary seam tracked here. + +--- + +## Addendum (2026-07-29) — `Field.datetime` has ONE storage form: canonical UTC text + +> **Status:** landed. This addendum **revises** the storage half of Phase 1 (which +> left `Field.datetime` "stored as UTC epoch ms" on SQLite) and **extends** +> D-A1 from the 2026-06-18 addendum to the column side of the comparison. +> Closes #3912; supersedes the epoch-ms convention referenced there. + +### What the epoch-ms convention actually was + +It was never a convention — it was a description of what better-sqlite3 happened +to do with a bound JS `Date`. Nothing enforced it: `formatInput` deliberately left +`datetime` untouched, so the storage form was decided by whichever writer got +there first. + +- A JS `Date` (seed loader, import, server-side code) → INTEGER epoch ms. +- A REST/JSON write → ISO **TEXT**, because JSON has no `Date` type. +- A `defaultValue: 'NOW()'` slot → ISO TEXT. +- The platform's own `created_at` / `updated_at` → ISO TEXT (they are stamped + with `toISOString()`), on **every** object in the system. + +One column therefore held both forms at once, while the read path coerced filter +comparands to epoch ms purely from the DECLARED type. On SQLite's type ordering +(`INTEGER < TEXT`) that made a two-sided window collapse to zero rows and a +one-sided `>=` match every TEXT row regardless of the bound — the reported symptom +in #3912 (a dashboard `last_30_days` reading 0 with 29 rows in range). It also +left `ORDER BY` sorting all INTEGER rows before all TEXT ones (#3928). + +### D-B1 — The canonical storage form is `YYYY-MM-DDTHH:MM:SS.sssZ` + +`Field.datetime` is stored as fixed-width, zone-explicit UTC text on SQLite, and +written to Postgres/MySQL as that same string. `canonicalUtcDatetime()` is the one +function that produces it, applied on write (`formatInput`) and to every filter +comparand (`coerceFilterValue`) so the two sides of a comparison cannot disagree +about shape. + +Chosen over the epoch integer because: + +- Lexicographic order **is** chronological order, so range filters and `ORDER BY` + read the column directly and can use an index. An epoch convention forces an + expression wrapper on every temporal predicate — it moves the cost rather than + removing it. +- `strftime`/`julianday` parse it, so the date-bucket expression needs no + epoch↔text CASE (#3773). +- It is what `formatOutput` already presents, so storage and presentation stop + disagreeing — the asymmetry Phase 1 removed for `date`, now removed for + `datetime`. +- It matches the `Field.date` convention, so the platform has one temporal + storage story instead of one per field type. +- It was already the majority of rows on disk, so the migration is the smaller + one. + +### D-B2 — The comparand rule is dialect-INDEPENDENT, which fixes Postgres too + +`coerceFilterValue` previously canonicalised only on SQLite and passed the value +through on native-timestamp dialects. That was not neutral. Measured against +PostgreSQL 16 with `TimeZone = Asia/Shanghai`: + +- A zone-**naive** write (`'2026-03-20 12:00:00'`) bound into `timestamptz` was + resolved against the SERVER's timezone and stored as `2026-03-20T04:00:00Z` — + 8 hours off the instant SQLite records for the same write, in direct violation + of this ADR's "a naive wall clock is UTC" rule. +- A bare `YYYY-MM-DD` comparand (what a `{30_days_ago}` token expands to) meant + midnight in the SERVER's timezone, so the identical query over the identical + instant put a row on a **different calendar day** than it did on SQLite. + +Stating the `Z` on both sides removes the server's timezone from the answer. +Postgres storage itself needed no change — knex's `table.timestamp` already +creates `timestamptz` (`useTz` defaults true, verified) — so this is a write- and +comparand-side fix there, not a migration. Regression cover is +`sql-driver-datetime-postgres-timezone.test.ts`, opt-in via `OS_TEST_POSTGRES_URL` +because CI provisions no server; it asserts it is pointed at a non-UTC server so +it cannot pass vacuously. + +### D-B3 — Existing rows converge at schema sync; correctness never depends on it + +`backfillCanonicalDatetimes` runs inside `initObjects`, rewriting SQLite rows that +are not already canonical (INTEGER/REAL epoch, zone-naive text, offset-bearing +text). It is idempotent, skips freshly-created tables entirely, and preserves +values SQLite cannot parse rather than nulling them. + +It is allowed to fail. On error it logs and marks nothing, and the read paths keep +the `sqliteCanonicalDatetimeSql` repair that makes an un-migrated column compare +and bucket correctly — just without an index. A migration must never be able to +take boot down, and query correctness must never be contingent on one having run. +The corollary is that the repair expression stays in the codebase permanently: it +also covers external/unmanaged tables (ADR-0015), which never get a backfill. + +`needsLegacyDatetimeRepair` is the single predicate every read path asks, so the +filter, bucket and analytics surfaces cannot drift apart about whether a column +is clean. + +### Consequences + +- D-A2 (promote `temporalFilterValue` onto the `IDataDriver` contract) now has a + second method to carry with it: `temporalFilterColumnSql`, the column-side + companion threaded to analytics as + `StrategyContext.coerceTemporalFilterColumn`. Coercing the value alone is not + sufficient on a mixed-form column, so any surface that binds a comparand into + raw SQL must wrap its column reference too. Both remain duck-typed until D-A2. +- D-A3's conformance matrix should gain a **storage-form** axis (canonical, + legacy-epoch, legacy-naive) and a **server-timezone** axis, since both are now + known to have produced dialect-divergent row results. +- #3928 (datetime `ORDER BY` mis-sorted on mixed storage) is closed by + construction rather than by a sort-side fix. +- MySQL is **not** covered here. `table.timestamp` emits a MySQL `TIMESTAMP`, + whose range ends at 2038-01-19, and the connection sets no `timezone`, so + mysql2 serialises a `Date` in the Node process's local zone. That is a separate + defect from #3912 and is filed on its own; it needs a real MySQL to verify and + a column-type migration to fix. diff --git a/packages/plugins/driver-sql/src/legacy-datetime-storage.testkit.ts b/packages/plugins/driver-sql/src/legacy-datetime-storage.testkit.ts new file mode 100644 index 0000000000..e842b7ce82 --- /dev/null +++ b/packages/plugins/driver-sql/src/legacy-datetime-storage.testkit.ts @@ -0,0 +1,51 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Test double for a SQLite database that still holds PRE-canonical + * `Field.datetime` values — the shape every deployment had before #3912, and the + * shape one still has when `backfillCanonicalDatetimes` could not run (it logs + * and swallows, deliberately, so a migration failure cannot take boot down). + * + * Since #3912 the write path canonicalises, so the legacy forms cannot be + * produced through `create()` any more. They have to be inserted RAW, and the + * column's canonical marker has to be cleared, or the driver would correctly + * conclude the column is clean and skip the read-side repair. Both halves are + * needed for the fixture to be honest — which is exactly why they live in one + * helper rather than being re-improvised per suite. + * + * Not exported from the package entry (`index.ts`): this is test-only. + */ + +import { SqlDriver } from './sql-driver.js'; + +export class LegacyStorageDriver extends SqlDriver { + /** + * Insert rows bypassing `formatInput`, so each `datetime` value lands in + * whatever form it is given (a number → INTEGER epoch ms, a string → TEXT), + * then mark `field` as NOT known-canonical so the read paths apply their + * repair — the state of an un-migrated database. + */ + async seedLegacyRows( + table: string, + field: string, + rows: Array>, + ): Promise { + await this.knex(table).insert(rows); + this.forgetCanonical(table, field); + } + + /** Drop the "already backfilled" marker for one column. */ + forgetCanonical(table: string, field: string): void { + this.canonicalDatetimeFields[table]?.delete(field); + } + + /** Raw stored form of a column, for asserting on the fixture's premise. */ + async storedForms(table: string, field: string): Promise> { + const res: any = await this.knex.raw( + `select id, typeof(??) as t, ?? as v from ?? order by id`, + [field, field, table], + ); + const rows = Array.isArray(res) ? res : (res?.rows ?? []); + return rows.map((r: any) => ({ id: r.id, type: r.t, value: r.v })); + } +} diff --git a/packages/plugins/driver-sql/src/sql-driver-aggregate-temporal-output.test.ts b/packages/plugins/driver-sql/src/sql-driver-aggregate-temporal-output.test.ts index 69a3a2ce36..048fa982a7 100644 --- a/packages/plugins/driver-sql/src/sql-driver-aggregate-temporal-output.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-aggregate-temporal-output.test.ts @@ -22,6 +22,7 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { SqlDriver } from '../src/index.js'; +import { LegacyStorageDriver } from '../src/legacy-datetime-storage.testkit.js'; const TABLE = 'deal'; @@ -43,7 +44,7 @@ describe('temporal values leaving aggregate()/distinct() (#3797)', () => { { name: TABLE, fields: { - closed_at: { type: 'datetime' }, // INTEGER epoch ms under better-sqlite3 + closed_at: { type: 'datetime' }, // canonical ISO-Z TEXT since #3912 closed_on: { type: 'date' }, // YYYY-MM-DD TEXT region: { type: 'string' }, amount: { type: 'number' }, @@ -100,27 +101,38 @@ describe('temporal values leaving aggregate()/distinct() (#3797)', () => { }); it('collapses two storage forms of the same instant into one value', async () => { - // SQL `DISTINCT` compares STORED values, and one SQLite `Field.datetime` - // column holds both forms — `formatInput` passes datetime values through, - // so a `Date` lands as INTEGER epoch ms and an ISO string lands as TEXT. - // Two rows recording the SAME instant therefore survive `DISTINCT` as two - // rows and then present identically, which is a duplicate unless the - // presented values are re-deduplicated. - await driver.create( - TABLE, - { id: 'd4', closed_at: ISO, closed_on: '2026-01-10', region: 'east', amount: 8 }, - { bypassTenantAudit: true }, - ); - - const raw: any = await driver.execute( - `SELECT DISTINCT typeof("closed_at") AS t FROM "${TABLE}" ORDER BY t`, - ); - const forms = (Array.isArray(raw) ? raw : (raw?.rows ?? [])).map((r: any) => r.t); - expect(forms).toContain('text'); // the row just written - expect(forms.some((f: string) => f === 'integer' || f === 'real')).toBe(true); - - // Three stored rows for two instants → two values, not three. - expect((await driver.distinct(TABLE, 'closed_at')).sort()).toEqual([ISO, ISO_LATER]); + // SQL `DISTINCT` compares STORED values. A pre-#3912 SQLite + // `Field.datetime` column held both forms — `formatInput` passed datetime + // values through, so a `Date` landed as INTEGER epoch ms and an ISO string + // as TEXT. Two rows recording the SAME instant therefore survived + // `DISTINCT` as two rows and then presented identically: a duplicate + // unless the presented values are re-deduplicated. + // + // Canonical storage means new writes can no longer produce that pair, so + // the fixture is an UN-MIGRATED database — where this dedup is still what + // stands between a legacy row and a duplicated chart axis label. + const legacy = new LegacyStorageDriver({ + client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true, + }); + try { + await legacy.initObjects([ + { name: TABLE, fields: { closed_at: { type: 'datetime' }, region: { type: 'string' } } }, + ]); + await legacy.seedLegacyRows(TABLE, 'closed_at', [ + { id: 'l1', closed_at: Date.parse(ISO), region: 'east' }, // INTEGER epoch + { id: 'l2', closed_at: ISO, region: 'east' }, // canonical TEXT + { id: 'l3', closed_at: ISO_LATER, region: 'west' }, + ]); + + const forms = (await legacy.storedForms(TABLE, 'closed_at')).map((r) => r.type); + expect(forms).toContain('text'); + expect(forms.some((f) => f === 'integer' || f === 'real')).toBe(true); + + // Three stored rows for two instants → two values, not three. + expect((await legacy.distinct(TABLE, 'closed_at')).sort()).toEqual([ISO, ISO_LATER]); + } finally { + await legacy.disconnect(); + } }); }); diff --git a/packages/plugins/driver-sql/src/sql-driver-analytics-datetime.test.ts b/packages/plugins/driver-sql/src/sql-driver-analytics-datetime.test.ts index c1e53de12f..b88d129d65 100644 --- a/packages/plugins/driver-sql/src/sql-driver-analytics-datetime.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-analytics-datetime.test.ts @@ -1,26 +1,29 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * End-to-end repro of the dashboard time-series "No rows" bug at the storage - * level, and proof of the fix. + * End-to-end cover of the dashboard time-series "No rows" bug at the storage + * level, and proof of the fix — now that the fix is canonical STORAGE (#3912) + * rather than a comparand cast. * * The analytics `NativeSQLStrategy` compiles dashboard relative-date tokens * (e.g. `{12_months_ago}`) to ISO date strings and binds them into a raw * `SELECT … WHERE col >= ?` that it runs through the driver's `execute()` — * bypassing the normal `find()` filter coercion. Under better-sqlite3 a - * `Field.datetime` column is stored as an INTEGER epoch (ms), so the ISO TEXT - * comparand never matches (TEXT sorts after every INTEGER) → 0 rows, even though - * the rows exist. A `Field.date` column stores ISO TEXT and matches fine. + * `Field.datetime` column USED to be stored as an INTEGER epoch (ms) whenever a + * `Date` was bound, so an ISO TEXT comparand never matched it (TEXT sorts after + * every INTEGER) → 0 rows even though the rows existed. * - * This test reproduces both the broken (raw ISO bind → 0) and fixed (epoch bind - * via the driver's public `temporalFilterValue` → N) behaviour against a real - * SQLite database, mirroring exactly what the analytics strategy now does. + * Both sides are one shape now: `formatInput` writes canonical UTC text and + * `temporalFilterValue` canonicalises the comparand to the same. These tests run + * that against a real SQLite database, and the MIXED-storage suite below covers + * what an un-migrated database still needs from the read-side repair. */ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { SqlDriver } from '../src/index.js'; +import { LegacyStorageDriver } from '../src/legacy-datetime-storage.testkit.js'; -describe('Analytics datetime filter — SQLite epoch storage (E2E repro)', () => { +describe('Analytics datetime filter — SQLite canonical storage (E2E)', () => { let driver: SqlDriver; const TABLE = 'compliance_assessment'; const CUTOFF = '2025-06-18'; // ISO date token the dashboard expands to @@ -37,15 +40,15 @@ describe('Analytics datetime filter — SQLite epoch storage (E2E repro)', () => name: TABLE, fields: { title: { type: 'string' }, - assessed_at: { type: 'datetime' }, // stored as INTEGER epoch ms - assessed_on: { type: 'date' }, // stored as YYYY-MM-DD text + assessed_at: { type: 'datetime' }, // canonical ISO-Z text since #3912 + assessed_on: { type: 'date' }, // YYYY-MM-DD text }, }, ]); // Four assessments AFTER the cutoff, one well before — inserted with real - // Date objects so better-sqlite3 stores `assessed_at` as INTEGER epoch ms, - // exactly the path the seed loader takes. + // Date objects, the path the seed loader takes. That used to land as INTEGER + // epoch ms; `formatInput` now canonicalises it on the way in. const rows = [ ['a1', new Date('2024-01-01T00:00:00Z'), '2024-01-01'], // before cutoff ['a2', new Date('2025-06-18T09:00:00Z'), '2025-06-18'], // on/after @@ -75,22 +78,33 @@ describe('Analytics datetime filter — SQLite epoch storage (E2E repro)', () => return Number(row.n); }; - it('BUG: a raw ISO comparand against the epoch datetime column returns 0 rows', async () => { - // This is what the type-blind strategy used to bind — the silent failure. - expect(await countWhere('assessed_at', CUTOFF)).toBe(0); + it('the datetime column is stored as canonical text, like the date column', async () => { + const res: any = await driver.execute( + `SELECT DISTINCT typeof("assessed_at") AS t FROM "${TABLE}"`, + ); + const rows = Array.isArray(res) ? res : res?.rows ?? []; + expect(rows.map((r: any) => r.t)).toEqual(['text']); }); - it('FIX: the driver-coerced epoch comparand returns the 4 matching rows', async () => { - // `temporalFilterValue` is exactly the hook NativeSQLStrategy now calls. + it('FIX: the canonicalised comparand returns the 4 matching rows', async () => { + // `temporalFilterValue` is exactly the hook NativeSQLStrategy calls. const coerced = driver.temporalFilterValue(TABLE, 'assessed_at', CUTOFF); - expect(typeof coerced).toBe('number'); // epoch ms, not the ISO string + expect(coerced).toBe('2025-06-18T00:00:00.000Z'); // canonical text, not epoch ms expect(await countWhere('assessed_at', coerced)).toBe(4); }); + it('the bare ISO date matches too — both sides are text now (index-friendly)', async () => { + // Canonical storage is fixed-width UTC, so `'2025-06-18' <= '2025-06-18T…'` + // lexicographically. The comparand cast stops being load-bearing for a + // day-granular bound, which is what keeps the emitted SQL a plain + // `col >= ?` against an indexable column. + expect(await countWhere('assessed_at', CUTOFF)).toBe(4); + }); + it('CONTROL: the `Field.date` text column already matched the raw ISO comparand', async () => { // Proves the date/text path was never broken and is left untouched. const coerced = driver.temporalFilterValue(TABLE, 'assessed_on', CUTOFF); - expect(typeof coerced).toBe('string'); // YYYY-MM-DD, NOT coerced to epoch + expect(coerced).toBe('2025-06-18'); // YYYY-MM-DD, not an instant expect(await countWhere('assessed_on', coerced)).toBe(4); // and the raw ISO bind matches identically (no coercion needed for text) expect(await countWhere('assessed_on', CUTOFF)).toBe(4); @@ -102,25 +116,28 @@ describe('Analytics datetime filter — SQLite epoch storage (E2E repro)', () => }); /** - * #3912 — coercing the comparand is necessary but NOT sufficient. + * #3912 — what an UN-MIGRATED database still needs. * - * The fixture above writes every row with a JS `Date`, so the column is uniformly - * INTEGER epoch. A production table is not: REST/JSON writes carry ISO strings - * (JSON has no `Date`) and `NOW()` defaults stamp ISO TEXT, so the SAME column - * holds both forms. An epoch comparand then matches the INTEGER half and misses - * every TEXT row — a dashboard `last_30_days` reading 0 with rows in range. + * Before canonical storage, one column legitimately held both forms: REST/JSON + * writes carry ISO strings (JSON has no `Date`) and `NOW()` defaults stamp ISO + * TEXT, while a bound `Date` landed as INTEGER epoch. An epoch comparand then + * matched the INTEGER half and missed every TEXT row — a dashboard + * `last_30_days` reading 0 with rows in range. * - * `temporalFilterColumnSql` is the companion hook that normalises the COLUMN, so - * the comparison is form-agnostic. These tests bind exactly what the analytics - * strategy binds, against a real mixed-storage SQLite table. + * New writes cannot produce that pair any more, and `backfillCanonicalDatetimes` + * converges existing rows at schema sync. But the backfill is allowed to fail + * (it logs and swallows, so a migration can never take boot down), and an + * external/unmanaged table never gets one — so `temporalFilterColumnSql` still + * has to make the comparison form-agnostic. These tests bind exactly what the + * analytics strategy binds, against a real mixed-storage SQLite table. */ -describe('Analytics datetime filter — MIXED storage (#3912)', () => { - let driver: SqlDriver; +describe('Analytics datetime filter — legacy MIXED storage (#3912)', () => { + let driver: LegacyStorageDriver; const TABLE = 'compliance_assessment'; const CUTOFF = '2025-06-18'; beforeEach(async () => { - driver = new SqlDriver({ + driver = new LegacyStorageDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true, @@ -132,16 +149,13 @@ describe('Analytics datetime filter — MIXED storage (#3912)', () => { }, ]); - const rows = [ - ['i1', new Date('2024-01-01T00:00:00Z')], // INTEGER, before cutoff - ['t1', '2024-02-01T00:00:00.000Z'], // TEXT, before cutoff - ['i2', new Date('2025-09-01T09:00:00Z')], // INTEGER, after - ['t2', '2025-10-01T09:00:00.000Z'], // TEXT, after - ['t3', '2026-01-15T09:00:00.000Z'], // TEXT, after - ] as const; - for (const [id, at] of rows) { - await driver.create(TABLE, { id, title: id, assessed_at: at }, { bypassTenantAudit: true }); - } + await driver.seedLegacyRows(TABLE, 'assessed_at', [ + { id: 'i1', title: 'i1', assessed_at: Date.parse('2024-01-01T00:00:00Z') }, // INTEGER, before + { id: 't1', title: 't1', assessed_at: '2024-02-01T00:00:00.000Z' }, // TEXT, before + { id: 'i2', title: 'i2', assessed_at: Date.parse('2025-09-01T09:00:00Z') }, // INTEGER, after + { id: 't2', title: 't2', assessed_at: '2025-10-01T09:00:00.000Z' }, // TEXT, after + { id: 't3', title: 't3', assessed_at: '2026-01-15T09:00:00.000Z' }, // TEXT, after + ]); }); afterEach(async () => { @@ -175,12 +189,14 @@ describe('Analytics datetime filter — MIXED storage (#3912)', () => { expect(rows.map((r: any) => [r.t, Number(r.n)])).toEqual([['integer', 2], ['text', 3]]); }); - it('BUG: coercing only the comparand empties the window on a TEXT-stored row', async () => { - // SQLite orders INTEGER before TEXT, so a TEXT row passes `>= ` and - // then fails `<= ` — the two-sided window collapses to nothing. That - // is the reported symptom: 0 rows where 3 exist. - expect(await countInWindow('assessed_at', CUTOFF, '2026-12-31', false)).toBe(1); // i2 only - expect(await countInWindow('assessed_at', '2025-10-01', '2026-12-31', false)).toBe(0); + it('BUG: reading the raw column silently drops the rows in the OTHER form', async () => { + // SQLite orders INTEGER before TEXT, so against a canonical TEXT comparand + // every legacy epoch row fails `>=` outright. The window quietly returns a + // SUBSET — the shape of #3912, and the reason the read-side repair cannot + // just be deleted once writes are canonical: a database that has not been + // backfilled still holds the other form. + expect(await countInWindow('assessed_at', CUTOFF, '2026-12-31', false)).toBe(2); // t2,t3 — i2 lost + expect(await countInWindow('assessed_at', '2024-01-01', '2024-06-30', false)).toBe(1); // t1 — i1 lost }); it('FIX: normalising the column finds rows of BOTH forms in the window', async () => { diff --git a/packages/plugins/driver-sql/src/sql-driver-date-bucket-storage.test.ts b/packages/plugins/driver-sql/src/sql-driver-date-bucket-storage.test.ts index 7f161a1eb6..42770a03d2 100644 --- a/packages/plugins/driver-sql/src/sql-driver-date-bucket-storage.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-date-bucket-storage.test.ts @@ -5,22 +5,26 @@ * * `sql-driver-date-bucket.test.ts` builds its fixture with * `knex.schema.createTable` + `t.string('ts')`, so every value it buckets is ISO - * TEXT. That is only half of what SQLite actually holds: a `Field.datetime` - * declared through `initObjects` becomes INTEGER epoch **milliseconds** (knex - * binds a JS `Date` as `.getTime()`), and `strftime` reads a bare integer as a - * Julian day number — epoch ms is orders of magnitude outside the legal range, - * so every row bucketed as NULL and any datetime trend chart rendered as one - * `(null)` bar carrying the whole total. + * TEXT. That was only half of what SQLite actually held: a `Field.datetime` + * declared through `initObjects` used to become INTEGER epoch **milliseconds** + * (knex binds a JS `Date` as `.getTime()`), and `strftime` reads a bare integer + * as a Julian day number — epoch ms is orders of magnitude outside the legal + * range, so every row bucketed as NULL and any datetime trend chart rendered as + * one `(null)` bar carrying the whole total. * - * So this suite goes through `driver.initObjects([...])` — the path a real - * object takes — and sweeps every supported granularity against BOTH storage - * forms, asserting against the same `bucketDateValue` labels the in-memory - * fallback produces. Anything that buckets differently depending on how the - * column happens to be stored is the bug this file exists to catch. + * Since #3912 writes are canonicalised, so a freshly written column is uniform + * TEXT — but the epoch form still sits in every database written by an earlier + * build, so both must keep bucketing identically. This suite goes through + * `driver.initObjects([...])` — the path a real object takes — and sweeps every + * supported granularity against BOTH storage forms, asserting against the same + * `bucketDateValue` labels the in-memory fallback produces. Anything that + * buckets differently depending on how the column happens to be stored is the + * bug this file exists to catch. */ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { SqlDriver } from '../src/index.js'; +import { LegacyStorageDriver } from '../src/legacy-datetime-storage.testkit.js'; type Granularity = 'day' | 'month' | 'quarter' | 'year'; @@ -115,7 +119,7 @@ describe('SqlDriver date bucketing is storage-form independent (#3773)', () => { { name: TABLE, fields: { - closed_at: { type: 'datetime' }, // INTEGER epoch ms under better-sqlite3 + closed_at: { type: 'datetime' }, // canonical ISO-Z TEXT since #3912 closed_on: { type: 'date' }, // YYYY-MM-DD TEXT amount: { type: 'number' }, }, @@ -137,20 +141,25 @@ describe('SqlDriver date bucketing is storage-form independent (#3773)', () => { await driver.disconnect(); }); - it('really does store the two columns in the two different forms', async () => { - // The premise of this whole file. If better-sqlite3 ever stops binding a - // `Date` as an integer, this fails first and explains the rest. + it('stores both temporal columns as canonical text (#3912)', async () => { + // The premise of this whole file. Since #3912 `formatInput` canonicalises a + // `Field.datetime` write to `YYYY-MM-DDTHH:MM:SS.sssZ`, so a bound `Date` no + // longer lands as an INTEGER epoch — the storage form is now the one + // `Field.date` already used, with a time part. The legacy epoch form still + // exists in un-migrated databases and is covered by the MIXED-form suite + // below, which writes it the only way it can still be produced: raw SQL. const res: any = await driver.execute( - `SELECT typeof("closed_at") AS at_t, typeof("closed_on") AS on_t FROM "${TABLE}" WHERE id = 'r3'`, + `SELECT typeof("closed_at") AS at_t, "closed_at" AS at_v, typeof("closed_on") AS on_t FROM "${TABLE}" WHERE id = 'r3'`, ); const row = Array.isArray(res) ? res[0] : (res?.rows?.[0] ?? res); - expect(['integer', 'real']).toContain(row.at_t); + expect(row.at_t).toBe('text'); + expect(row.at_v).toMatch(/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$/); expect(row.on_t).toBe('text'); }); for (const g of GRANULARITIES) { describe(`granularity '${g}'`, () => { - it('buckets the epoch-stored datetime column', async () => { + it('buckets the datetime column', async () => { expect(await bucketSums(driver, 'closed_at', g)).toEqual(expectedBuckets(g)); }); @@ -179,17 +188,23 @@ describe('SqlDriver date bucketing is storage-form independent (#3773)', () => { }); describe('SqlDriver date bucketing over a MIXED-form datetime column (#3773)', () => { - // One SQLite `Field.datetime` column legitimately holds both forms at once: - // `formatInput` leaves datetime values alone, so a `Date` lands as INTEGER - // epoch ms while an ISO string (what an unresolved `defaultValue: 'NOW()'` - // slot and any string-valued write produce) lands as TEXT. A bucket - // expression that assumed epoch for the whole column would divide the TEXT by - // 1000 — `'2026-01-10T…' / 1000.0` is 2.026 seconds past the epoch — and file - // live rows under 1970, which is worse than the NULL it replaced. - let driver: SqlDriver; + // A legacy, UN-MIGRATED database: before #3912 `formatInput` left datetime + // values alone, so a `Date` landed as INTEGER epoch ms while an ISO string + // (an unresolved `defaultValue: 'NOW()'` slot, any string-valued write) landed + // as TEXT — in the same column. A bucket expression that assumed epoch for the + // whole column would divide the TEXT by 1000 — `'2026-01-10T…' / 1000.0` is + // 2.026 seconds past the epoch — and file live rows under 1970, which is worse + // than the NULL it replaced. + // + // Writes are canonical now, so the only way to reach this state is a database + // whose backfill has not run: rows inserted raw, with the column's canonical + // marker cleared. That is exactly the state `backfillCanonicalDatetimes` logs + // and swallows into when it cannot run — where queries must stay CORRECT via + // the read-side repair, just unindexed. + let driver: LegacyStorageDriver; beforeEach(async () => { - driver = new SqlDriver({ + driver = new LegacyStorageDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true, @@ -197,10 +212,12 @@ describe('SqlDriver date bucketing over a MIXED-form datetime column (#3773)', ( await driver.initObjects([ { name: TABLE, fields: { closed_at: { type: 'datetime' }, amount: { type: 'number' } } }, ]); - await driver.create(TABLE, { id: 'int', closed_at: new Date('2026-01-10T09:00:00Z'), amount: 1 }, { bypassTenantAudit: true }); - await driver.create(TABLE, { id: 'txt', closed_at: '2026-02-14T09:00:00Z', amount: 2 }, { bypassTenantAudit: true }); - await driver.create(TABLE, { id: 'naive', closed_at: '2026-02-20 09:00:00', amount: 4 }, { bypassTenantAudit: true }); - await driver.create(TABLE, { id: 'nil', closed_at: null, amount: 8 }, { bypassTenantAudit: true }); + await driver.seedLegacyRows(TABLE, 'closed_at', [ + { id: 'int', closed_at: Date.parse('2026-01-10T09:00:00Z'), amount: 1 }, // INTEGER epoch ms + { id: 'txt', closed_at: '2026-02-14T09:00:00Z', amount: 2 }, // zone-explicit TEXT + { id: 'naive', closed_at: '2026-02-20 09:00:00', amount: 4 }, // CURRENT_TIMESTAMP TEXT + { id: 'nil', closed_at: null, amount: 8 }, + ]); }); afterEach(async () => { diff --git a/packages/plugins/driver-sql/src/sql-driver-datetime-canonical-storage.test.ts b/packages/plugins/driver-sql/src/sql-driver-datetime-canonical-storage.test.ts new file mode 100644 index 0000000000..8ac02bb52b --- /dev/null +++ b/packages/plugins/driver-sql/src/sql-driver-datetime-canonical-storage.test.ts @@ -0,0 +1,211 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #3912 — `Field.datetime` has ONE storage form: canonical UTC text + * (`YYYY-MM-DDTHH:MM:SS.sssZ`). + * + * The filter-level fix that came first (normalise the column in the comparison) + * made queries correct but left the disease: two write paths producing two + * shapes in one column. That still mis-ordered `ORDER BY` (#3928), still cost an + * unindexable expression on every window, and still meant storage disagreed with + * the value `find()` presents. + * + * So the write path canonicalises, the comparand canonicalises to the same + * function, and `backfillCanonicalDatetimes` converges existing rows at schema + * sync. This suite covers the three claims that buys: + * 1. every write shape lands as one canonical string; + * 2. the backfill converges a legacy database, and the read paths then drop + * their repair expression; + * 3. lexicographic order is chronological order, so ORDER BY is correct. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { SqlDriver } from '../src/index.js'; +import { LegacyStorageDriver } from '../src/legacy-datetime-storage.testkit.js'; + +const CANONICAL = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$/; + +const makeDriver = (Ctor: new (cfg: any) => T): T => + new Ctor({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }); + +const OBJECT = { + name: 'evt', + fields: { label: { type: 'string' }, at: { type: 'datetime' } }, +} as any; + +describe('Field.datetime writes land in ONE canonical form (#3912)', () => { + let driver: SqlDriver; + + beforeEach(async () => { + driver = makeDriver(SqlDriver); + await driver.initObjects([OBJECT]); + }); + + afterEach(async () => { + await driver.disconnect(); + }); + + const storedOf = async (id: string) => { + const res: any = await driver.execute(`select typeof(at) as t, at as v from evt where id = ?`, [id]); + const row = Array.isArray(res) ? res[0] : (res?.rows?.[0] ?? res); + return { type: row.t, value: row.v }; + }; + + it('folds every accepted input shape onto the same stored string', async () => { + const INSTANT = '2026-03-20T12:00:00.000Z'; + const inputs: Array<[string, unknown]> = [ + ['date-obj', new Date(INSTANT)], + ['iso-z', INSTANT], + ['iso-no-ms', '2026-03-20T12:00:00Z'], + ['offset', '2026-03-20T20:00:00+08:00'], // same instant, other offset + ['naive', '2026-03-20 12:00:00'], // wall clock IS UTC (ADR-0074) + ['epoch-num', Date.parse(INSTANT)], + ['epoch-str', String(Date.parse(INSTANT))], + ]; + for (const [id, at] of inputs) { + await driver.create('evt', { id, label: id, at }, { bypassTenantAudit: true }); + } + + for (const [id] of inputs) { + const stored = await storedOf(id); + expect(stored.type, `${id} stored form`).toBe('text'); + expect(stored.value, `${id} stored value`).toBe(INSTANT); + } + }); + + it('stores a bare calendar day as UTC midnight, not local midnight', async () => { + await driver.create('evt', { id: 'day', label: 'day', at: '2026-03-20' }, { bypassTenantAudit: true }); + expect((await storedOf('day')).value).toBe('2026-03-20T00:00:00.000Z'); + }); + + it('leaves an uninterpretable value alone rather than inventing an instant', async () => { + await driver.create('evt', { id: 'junk', label: 'junk', at: 'not-a-date' }, { bypassTenantAudit: true }); + expect((await storedOf('junk')).value).toBe('not-a-date'); + }); + + it('sorts chronologically as TEXT — lexicographic order IS time order (#3928)', async () => { + // The property that makes canonical UTC text the right choice over an epoch + // integer: ORDER BY needs no expression, so it can use an index, and a + // `Date`-written row can no longer sort ahead of an ISO-written one. + const instants = [ + ['e-1969', '1969-12-31T23:59:59.999Z'], + ['e-2025', '2025-11-15T09:00:00.000Z'], + ['e-2026a', '2026-01-10T09:00:00.000Z'], + ['e-2026b', '2026-06-30T23:59:59.000Z'], + ] as const; + // Alternate the write shape so a form-sensitive sort would interleave wrongly. + for (const [i, [id, iso]] of instants.entries()) { + await driver.create( + 'evt', { id, label: id, at: i % 2 === 0 ? new Date(iso) : iso }, { bypassTenantAudit: true }, + ); + } + + const rows = await driver.find('evt', { orderBy: [{ field: 'at', order: 'asc' }] }); + expect(rows.map((r: any) => r.id)).toEqual(instants.map(([id]) => id)); + + // …and the raw column sorts identically, i.e. the DB did the ordering. + const res: any = await driver.execute(`select id from evt order by at asc`); + const raw = Array.isArray(res) ? res : (res?.rows ?? []); + expect(raw.map((r: any) => r.id)).toEqual(instants.map(([id]) => id)); + }); + + it('emits a plain indexable comparison — no repair expression (#3912)', async () => { + // A table created in this process is canonical by construction, so + // `needsLegacyDatetimeRepair` is false and the SQL has no CASE in it. + const sql = (driver as any).knex('evt').where('at', '>=', '2026-01-01').toString(); + expect(sql).not.toContain('typeof'); + const compiled = (driver as any).knex.queryBuilder(); + (driver as any).applyFilters(compiled.table('evt'), { at: { $gte: '2026-01-01' } }); + expect(compiled.toString()).not.toContain('typeof'); + expect(compiled.toString()).toContain('2026-01-01T00:00:00.000Z'); + }); +}); + +describe('backfillCanonicalDatetimes converges a legacy database (#3912)', () => { + let driver: LegacyStorageDriver; + + beforeEach(async () => { + driver = makeDriver(LegacyStorageDriver); + await driver.initObjects([OBJECT]); + await driver.seedLegacyRows('evt', 'at', [ + { id: 'epoch', label: 'epoch', at: Date.parse('2026-03-20T12:00:00.000Z') }, + { id: 'naive', label: 'naive', at: '2026-03-20 12:00:00' }, + { id: 'offset', label: 'offset', at: '2026-03-20T20:00:00+08:00' }, + { id: 'canon', label: 'canon', at: '2026-03-20T12:00:00.000Z' }, + { id: 'junk', label: 'junk', at: 'not-a-date' }, + { id: 'nil', label: 'nil', at: null }, + ]); + }); + + afterEach(async () => { + await driver.disconnect(); + }); + + it('rewrites every interpretable row to the canonical form', async () => { + await (driver as any).backfillCanonicalDatetimes('evt', true); + + const forms = Object.fromEntries( + (await driver.storedForms('evt', 'at')).map((r) => [r.id, r.value]), + ); + // All four interpretable shapes describe the same instant, so they converge + // on one byte-identical string. + expect(forms.epoch).toBe('2026-03-20T12:00:00.000Z'); + expect(forms.naive).toBe('2026-03-20T12:00:00.000Z'); + expect(forms.offset).toBe('2026-03-20T12:00:00.000Z'); + expect(forms.canon).toBe('2026-03-20T12:00:00.000Z'); + }); + + it('preserves what it cannot interpret instead of nulling it', async () => { + await (driver as any).backfillCanonicalDatetimes('evt', true); + const forms = Object.fromEntries( + (await driver.storedForms('evt', 'at')).map((r) => [r.id, r.value]), + ); + expect(forms.junk).toBe('not-a-date'); + expect(forms.nil).toBeNull(); + }); + + it('marks the column clean, so the read paths drop the repair expression', async () => { + expect((driver as any).needsLegacyDatetimeRepair('evt', 'at')).toBe(true); + expect((driver as any).filterColumnExpr('evt', 'at', 'at')).not.toBeNull(); + + await (driver as any).backfillCanonicalDatetimes('evt', true); + + expect((driver as any).needsLegacyDatetimeRepair('evt', 'at')).toBe(false); + expect((driver as any).filterColumnExpr('evt', 'at', 'at')).toBeNull(); + expect(driver.temporalFilterColumnSql('evt', 'at', '"at"')).toBe('"at"'); + }); + + it('is idempotent — a second run rewrites nothing', async () => { + await (driver as any).backfillCanonicalDatetimes('evt', true); + const before = await driver.storedForms('evt', 'at'); + driver.forgetCanonical('evt', 'at'); + await (driver as any).backfillCanonicalDatetimes('evt', true); + expect(await driver.storedForms('evt', 'at')).toEqual(before); + }); + + it('makes a window filter return the same rows before and after (#3912)', async () => { + // The repair keeps the un-migrated database CORRECT; the backfill only makes + // it fast. Both must agree, or the migration would be observable as a change + // in results — which is exactly what it must never be. + const window = { at: { $gte: '2026-03-20T00:00:00.000Z', $lte: '2026-03-21T00:00:00.000Z' } }; + const before = (await driver.find('evt', { where: window })).map((r: any) => r.id).sort(); + expect(before).toEqual(['canon', 'epoch', 'naive', 'offset']); + + await (driver as any).backfillCanonicalDatetimes('evt', true); + + const after = (await driver.find('evt', { where: window })).map((r: any) => r.id).sort(); + expect(after).toEqual(before); + }); + + it('runs automatically at schema sync', async () => { + // The fixture cleared the marker by hand; a real boot re-runs `initObjects`, + // which is where the backfill is wired in. + expect((driver as any).needsLegacyDatetimeRepair('evt', 'at')).toBe(true); + await driver.initObjects([OBJECT]); + expect((driver as any).needsLegacyDatetimeRepair('evt', 'at')).toBe(false); + for (const row of await driver.storedForms('evt', 'at')) { + if (row.id === 'junk' || row.id === 'nil') continue; + expect(String(row.value)).toMatch(CANONICAL); + } + }); +}); diff --git a/packages/plugins/driver-sql/src/sql-driver-datetime-filter-text-storage.test.ts b/packages/plugins/driver-sql/src/sql-driver-datetime-filter-text-storage.test.ts index afffe9e625..c3c2753b1e 100644 --- a/packages/plugins/driver-sql/src/sql-driver-datetime-filter-text-storage.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-datetime-filter-text-storage.test.ts @@ -3,22 +3,24 @@ /** * Regression #3912 — the OTHER half of the SQLite `Field.datetime` storage mix. * - * `sql-driver-datetime-filter.test.ts` covers rows written with a JS `Date`, so - * better-sqlite3 stores them as INTEGER epoch ms. But the REST / JSON write path - * cannot produce a `Date` (JSON has no such type) and a `defaultValue: 'NOW()'` - * slot stamps an ISO string, so a production table is dominated by ISO **TEXT** - * — and the filter path coerced its comparand to epoch ms purely from the - * DECLARED type. Every datetime window filter then compared INTEGER-vs-TEXT and - * returned nothing: a dashboard `last_30_days` on `created_date` read 0 while 29 - * rows matched. + * `sql-driver-datetime-filter.test.ts` covers rows written with a JS `Date`, + * which better-sqlite3 used to store as INTEGER epoch ms. But the REST / JSON + * write path cannot produce a `Date` (JSON has no such type) and a + * `defaultValue: 'NOW()'` slot stamps an ISO string, so a production table was + * dominated by ISO **TEXT** — and the filter path coerced its comparand to epoch + * ms purely from the DECLARED type. Every datetime window filter then compared + * INTEGER-vs-TEXT and returned nothing: a dashboard `last_30_days` on + * `created_date` read 0 while 29 rows matched. * - * These tests write the way REST does (ISO strings) and assert the same filters - * that already pass for `Date`-written rows. The mixed-storage suite at the end - * is the real production shape: one table holding both forms at once. + * The fix made the storage form itself canonical, so these tests write the way + * REST does (ISO strings) and assert that every filter shape works against it — + * which is now the same shape a `Date` write produces. The legacy suite at the + * end covers the un-migrated database: one column holding both forms at once. */ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { SqlDriver } from '../src/index.js'; +import { LegacyStorageDriver } from '../src/legacy-datetime-storage.testkit.js'; const OBJECT = { name: 'lead', @@ -220,31 +222,29 @@ describe('SqlDriver datetime filters on the created_at audit column (#3912)', () }); }); -describe('SqlDriver datetime filters on a MIXED-storage column (#3912)', () => { - let driver: SqlDriver; +describe('SqlDriver datetime filters on a legacy MIXED-storage column (#3912)', () => { + let driver: LegacyStorageDriver; beforeEach(async () => { - driver = new SqlDriver({ + driver = new LegacyStorageDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true, }); await driver.initObjects([OBJECT]); - // The production shape: seeds/imports bind a `Date` (INTEGER epoch) while the - // REST API writes ISO strings (TEXT) — into the same column. - await driver.create('lead', { - id: 'int-old', name: 'int-old', created_date: new Date('2025-02-01T00:00:00Z'), - }, { bypassTenantAudit: true }); - await driver.create('lead', { - id: 'txt-old', name: 'txt-old', created_date: '2025-02-02T00:00:00.000Z', - }, { bypassTenantAudit: true }); - await driver.create('lead', { - id: 'int-new', name: 'int-new', created_date: new Date('2026-06-01T00:00:00Z'), - }, { bypassTenantAudit: true }); - await driver.create('lead', { - id: 'txt-new', name: 'txt-new', created_date: '2026-06-02T00:00:00.000Z', - }, { bypassTenantAudit: true }); + // A database written by a pre-#3912 build: seeds/imports bound a `Date` + // (INTEGER epoch) while the REST API wrote ISO strings (TEXT) — into the same + // column. `formatInput` cannot produce this any more, and the backfill + // converges it at schema sync, so the fixture is seeded raw with the + // canonical marker cleared: the state of a database whose backfill has not + // run (it is allowed to fail — correctness must not depend on it). + await driver.seedLegacyRows('lead', 'created_date', [ + { id: 'int-old', name: 'int-old', created_date: Date.parse('2025-02-01T00:00:00Z') }, + { id: 'txt-old', name: 'txt-old', created_date: '2025-02-02T00:00:00.000Z' }, + { id: 'int-new', name: 'int-new', created_date: Date.parse('2026-06-01T00:00:00Z') }, + { id: 'txt-new', name: 'txt-new', created_date: '2026-06-02T00:00:00.000Z' }, + ]); }); afterEach(async () => { @@ -252,10 +252,8 @@ describe('SqlDriver datetime filters on a MIXED-storage column (#3912)', () => { }); it('really does hold both storage forms', async () => { - const rows: any = await (driver as any).knex.raw( - `select id, typeof(created_date) as t from lead order by id`, - ); - expect(Object.fromEntries(rows.map((r: any) => [r.id, r.t]))).toEqual({ + const forms = await driver.storedForms('lead', 'created_date'); + expect(Object.fromEntries(forms.map((r) => [r.id, r.type]))).toEqual({ 'int-new': 'integer', 'int-old': 'integer', 'txt-new': 'text', 'txt-old': 'text', }); }); diff --git a/packages/plugins/driver-sql/src/sql-driver-datetime-postgres-timezone.test.ts b/packages/plugins/driver-sql/src/sql-driver-datetime-postgres-timezone.test.ts new file mode 100644 index 0000000000..6d62c1a4e9 --- /dev/null +++ b/packages/plugins/driver-sql/src/sql-driver-datetime-postgres-timezone.test.ts @@ -0,0 +1,121 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #3912 on Postgres — the half that is NOT about storage form. + * + * SQLite's mixed INTEGER/TEXT storage is dialect-specific: `Field.datetime` maps + * to a real `timestamptz` here (knex's `table.timestamp` defaults `useTz: true`), + * so a `Date` and an ISO-`Z` string always resolved to the same instant. What + * Postgres HAD instead was a timezone bug of the same family, and it was quieter: + * + * - A zone-NAIVE value bound into `timestamptz` is resolved against the + * SERVER's `TimeZone`, not UTC. On an `Asia/Shanghai` server + * `'2026-03-20 12:00:00'` was stored as `2026-03-20T04:00:00Z` — 8 hours off + * the instant SQLite records for the same write (ADR-0053/ADR-0074: a naive + * wall clock IS UTC). + * - The filter comparand was passed through untouched, so a bare `YYYY-MM-DD` + * from a `{30_days_ago}` token meant midnight in the SERVER's timezone. The + * identical query over the identical instant put a row on a different + * calendar day than it did on SQLite — a silently wrong window, not an empty + * one. + * + * `canonicalUtcDatetime` closes both: every write and every comparand carries an + * explicit `Z`, so the server's timezone stops participating in the answer. + * + * Opt-in — needs a real server, which CI does not provision: + * + * OS_TEST_POSTGRES_URL=postgres://user@host:5432/db pnpm --filter @objectstack/driver-sql test + * + * Point it at a server whose `TimeZone` is NOT UTC to actually exercise the bug; + * the suite asserts that and tells you if it cannot. + */ + +import { describe, it, expect, beforeAll, beforeEach, afterEach } from 'vitest'; +import { SqlDriver } from '../src/index.js'; + +const URL = process.env.OS_TEST_POSTGRES_URL; +const TABLE = 'os3912_probe'; + +/** 20:00Z is 04:00 the NEXT day at +08:00 — so a day window discriminates. */ +const BOUNDARY = '2026-03-20T20:00:00.000Z'; +const MIDDAY = '2026-03-20T12:00:00.000Z'; + +describe.skipIf(!URL)('Field.datetime on Postgres is timezone-independent (#3912)', () => { + let driver: SqlDriver; + let serverTimeZone = ''; + + beforeAll(async () => { + const probe = new SqlDriver({ client: 'pg', connection: URL }); + const res: any = await probe.execute(`select current_setting('TimeZone') as tz`); + serverTimeZone = ((res?.rows ?? res)[0] as any).tz; + await probe.disconnect(); + }); + + beforeEach(async () => { + driver = new SqlDriver({ client: 'pg', connection: URL }); + await driver.execute(`drop table if exists "${TABLE}" cascade`); + await driver.initObjects([ + { name: TABLE, fields: { label: { type: 'string' }, at: { type: 'datetime' } } }, + ]); + }); + + afterEach(async () => { + await driver.execute(`drop table if exists "${TABLE}" cascade`).catch(() => {}); + await driver.disconnect(); + }); + + /** Epoch ms of the stored instant, read straight out of Postgres. */ + const storedEpochMs = async (id: string): Promise => { + const res: any = await driver.execute( + `select (extract(epoch from at) * 1000)::bigint as ms from "${TABLE}" where id = ?`, + [id], + ); + return Number(((res?.rows ?? res)[0] as any).ms); + }; + + it('is pointed at a non-UTC server (otherwise this suite proves nothing)', () => { + expect( + serverTimeZone, + 'set the server TimeZone to something like Asia/Shanghai — on UTC the bug is invisible', + ).not.toMatch(/^(UTC|Etc\/UTC|GMT)$/i); + }); + + it('maps Field.datetime to timestamptz, not a naive timestamp', async () => { + const res: any = await driver.execute( + `select data_type from information_schema.columns where table_name = ? and column_name = 'at'`, + [TABLE], + ); + expect(((res?.rows ?? res)[0] as any).data_type).toBe('timestamp with time zone'); + }); + + it('records the same instant for a Date, an ISO-Z string and a NAIVE string', async () => { + await driver.create(TABLE, { id: 'date-obj', label: 'a', at: new Date(MIDDAY) }, { bypassTenantAudit: true }); + await driver.create(TABLE, { id: 'iso-z', label: 'b', at: MIDDAY }, { bypassTenantAudit: true }); + // The one that used to be resolved in the server's timezone. + await driver.create(TABLE, { id: 'naive', label: 'c', at: '2026-03-20 12:00:00' }, { bypassTenantAudit: true }); + + const expected = Date.parse(MIDDAY); + for (const id of ['date-obj', 'iso-z', 'naive']) { + expect(await storedEpochMs(id), `${id} stored instant`).toBe(expected); + } + }); + + it('anchors a bare calendar-day comparand to UTC midnight, like SQLite', async () => { + await driver.create(TABLE, { id: 'b1', label: 'x', at: new Date(BOUNDARY) }, { bypassTenantAudit: true }); + + const day = async (from: string, to: string) => + (await driver.find(TABLE, { where: { at: { $gte: from, $lt: to } } })).map((r: any) => r.id); + + // 20:00Z belongs to 2026-03-20 in UTC. Read against the server's local + // midnight (Asia/Shanghai) it would fall on the 21st instead — which is + // exactly how the same dashboard window disagreed with SQLite. + expect(await day('2026-03-20', '2026-03-21')).toEqual(['b1']); + expect(await day('2026-03-21', '2026-03-22')).toEqual([]); + }); + + it('presents the stored instant as canonical UTC on read', async () => { + await driver.create(TABLE, { id: 'r', label: 'r', at: '2026-03-20 12:00:00' }, { bypassTenantAudit: true }); + const row: any = await driver.findOne(TABLE, 'r', { bypassTenantAudit: true }); + expect(new Date(row.at).toISOString()).toBe(MIDDAY); + }); +}); diff --git a/packages/plugins/driver-sql/src/sql-driver-temporal-dialect.test.ts b/packages/plugins/driver-sql/src/sql-driver-temporal-dialect.test.ts index 1215eb23a6..23b4369e95 100644 --- a/packages/plugins/driver-sql/src/sql-driver-temporal-dialect.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-temporal-dialect.test.ts @@ -2,11 +2,22 @@ /** * Dialect-correctness of `temporalFilterValue` (the hook the analytics layer - * uses). The datetime → epoch-ms coercion is SQLite-ONLY: SQLite stores - * `Field.datetime` as an INTEGER epoch, but Postgres/MySQL map it to a native - * TIMESTAMP where an ISO string / Date binds correctly. Coercing to an epoch - * integer on a native-timestamp dialect would compare INTEGER vs TIMESTAMP and - * break the query — the exact Postgres regression we must NOT introduce. + * uses). + * + * Since #3912 a `Field.datetime` comparand takes ONE rule on every dialect: + * canonical UTC ISO — the same form `formatInput` now writes. It replaced a + * SQLite-only coercion to epoch ms, which assumed a storage form the write path + * never actually guaranteed. + * + * Making it uniform is a FIX on Postgres/MySQL too, not just a simplification. + * The comparand used to be passed through untouched there, so a bare + * `YYYY-MM-DD` from a `{30_days_ago}` token meant midnight in the SERVER's + * timezone — measured against a real PostgreSQL 16 on `Asia/Shanghai`, the same + * instant landed on a different calendar day than it did on SQLite. Stating the + * `Z` removes the server's timezone from the comparison. + * + * What must NOT come back is the epoch integer: binding one against a native + * TIMESTAMP compares INTEGER vs TIMESTAMP and breaks the query outright. * * No DB connection is needed: we seed the field-type maps the way `initObjects` * would and exercise the pure coercion logic across dialects. @@ -23,6 +34,10 @@ class ProbeDriver extends SqlDriver { seedDate(table: string, field: string): void { (this.dateFields[table] ??= new Set()).add(field); } + /** Mark a column as backfilled, the way `backfillCanonicalDatetimes` does. */ + markCanonical(table: string, field: string): void { + (this.canonicalDatetimeFields[table] ??= new Set()).add(field); + } } function makeDriver(client: string): ProbeDriver { @@ -31,25 +46,52 @@ function makeDriver(client: string): ProbeDriver { } const ISO = '2025-06-18'; -const EPOCH = Date.parse('2025-06-18T00:00:00.000Z'); +const CANONICAL = '2025-06-18T00:00:00.000Z'; describe('temporalFilterValue dialect gating', () => { - it('SQLite: datetime ISO comparand → epoch ms', () => { - const d = makeDriver('better-sqlite3'); - d.seedDatetime('t', 'at'); - expect(d.temporalFilterValue('t', 'at', ISO)).toBe(EPOCH); + it('every dialect canonicalises a bare calendar day to UTC midnight (#3912)', () => { + // The uniformity IS the contract now. On Postgres this is the fix for the + // measured cross-dialect divergence: an un-anchored '2025-06-18' was read as + // midnight in the server's timezone, so a window put the same instant on a + // different day than SQLite did. + for (const client of ['better-sqlite3', 'pg', 'mysql2']) { + const d = makeDriver(client); + d.seedDatetime('t', 'at'); + expect(d.temporalFilterValue('t', 'at', ISO)).toBe(CANONICAL); + } }); - it('Postgres: datetime ISO comparand is LEFT UNCHANGED (no epoch coercion → no regression)', () => { - const d = makeDriver('pg'); + it('never binds an epoch integer — that would break a native TIMESTAMP compare', () => { + for (const client of ['better-sqlite3', 'pg', 'mysql2']) { + const d = makeDriver(client); + d.seedDatetime('t', 'at'); + for (const input of [ISO, new Date(CANONICAL), Date.parse(CANONICAL), CANONICAL]) { + expect(typeof d.temporalFilterValue('t', 'at', input)).toBe('string'); + } + } + }); + + it('folds every accepted input shape onto the same canonical instant', () => { + const d = makeDriver('better-sqlite3'); d.seedDatetime('t', 'at'); - expect(d.temporalFilterValue('t', 'at', ISO)).toBe(ISO); + for (const input of [ + new Date(CANONICAL), // JS Date + Date.parse(CANONICAL), // epoch ms number + String(Date.parse(CANONICAL)), // epoch ms as text + '2025-06-18', // bare calendar day + '2025-06-18 00:00:00', // zone-naive wall clock (CURRENT_TIMESTAMP) + '2025-06-18T00:00:00Z', // zone-explicit, no millis + '2025-06-18T08:00:00+08:00', // same instant, offset form + ]) { + expect(d.temporalFilterValue('t', 'at', input)).toBe(CANONICAL); + } }); - it('MySQL: datetime ISO comparand is left unchanged', () => { - const d = makeDriver('mysql2'); + it('leaves an uninterpretable comparand alone rather than inventing an instant', () => { + const d = makeDriver('better-sqlite3'); d.seedDatetime('t', 'at'); - expect(d.temporalFilterValue('t', 'at', ISO)).toBe(ISO); + expect(d.temporalFilterValue('t', 'at', 'not-a-date')).toBe('not-a-date'); + expect(d.temporalFilterValue('t', 'at', '')).toBe(''); }); it('Field.date normalises to YYYY-MM-DD text on every dialect', () => { @@ -70,7 +112,7 @@ describe('temporalFilterValue dialect gating', () => { /** * The same dialect gating for the aggregate BUCKET expression (#3773). It shares - * `isEpochStoredDatetime` with the filter coercion above, so the two can only be + * `needsLegacyDatetimeRepair` with the filter path above, so the two can only be * wrong together — which is the point: a window and a bucket that disagree about * storage is how the epoch column ended up correctly filtered and then entirely * bucketed as NULL. @@ -87,26 +129,42 @@ describe('buildDateBucketExpr dialect gating (#3773)', () => { const expr = (d: ProbeDriver, field: string, g: string, table?: string) => (d as any).buildDateBucketExpr(field, g, table) as { sql: string; bindings: any[] } | null; - it('SQLite: a declared Field.datetime is normalised from epoch ms', () => { + it('SQLite: an UN-BACKFILLED Field.datetime is repaired to canonical text', () => { + // `seedDatetime` mimics `initObjects` declaring the column WITHOUT the + // backfill having marked it canonical — an un-migrated database. const d = makeDriver('better-sqlite3'); d.seedDatetime('t', 'at'); for (const g of GRANULARITIES) { const e = expr(d, 'at', g, 't')!; - expect(e.sql).toContain(`julianday(??/1000.0, 'unixepoch')`); + expect(e.sql).toContain(`strftime('%Y-%m-%dT%H:%M:%fZ', ??/1000.0, 'unixepoch')`); // Real division, not integer: `-1 / 1000` truncates to 0 and moves a // pre-1970 instant forward a day. expect(e.sql).not.toContain('/1000,'); } }); + it('SQLite: a BACKFILLED Field.datetime drops the repair entirely (#3912)', () => { + // The payoff of canonicalising storage: once a column is known clean the + // bucket expression is a bare column again, so it can use an index. + const d = makeDriver('better-sqlite3'); + d.seedDatetime('t', 'at'); + d.markCanonical('t', 'at'); + for (const g of GRANULARITIES) { + const e = expr(d, 'at', g, 't')!; + expect(e.sql).not.toContain('unixepoch'); + expect(e.sql).not.toContain('typeof'); + expect(e.bindings).toEqual(expect.arrayContaining(['at'])); + } + }); + it('SQLite: Field.date and undeclared columns keep the plain column form', () => { const d = makeDriver('better-sqlite3'); d.seedDate('t', 'on'); for (const g of GRANULARITIES) { - expect(expr(d, 'on', g, 't')!.sql).not.toContain('julianday'); - expect(expr(d, 'anything', g, 't')!.sql).not.toContain('julianday'); + expect(expr(d, 'on', g, 't')!.sql).not.toContain('unixepoch'); + expect(expr(d, 'anything', g, 't')!.sql).not.toContain('unixepoch'); // No table key at all (a caller outside the aggregate path) → plain form. - expect(expr(d, 'at', g)!.sql).not.toContain('julianday'); + expect(expr(d, 'at', g)!.sql).not.toContain('unixepoch'); } }); diff --git a/packages/plugins/driver-sql/src/sql-driver-user-datetime-default-format.test.ts b/packages/plugins/driver-sql/src/sql-driver-user-datetime-default-format.test.ts index 7590ec4476..f717ad0ed6 100644 --- a/packages/plugins/driver-sql/src/sql-driver-user-datetime-default-format.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-user-datetime-default-format.test.ts @@ -90,31 +90,38 @@ describe('User NOW()-default temporal fields — canonical format (SQLite)', () // ── Read presentation: mixed storage → one canonical instant ──────────────── - it('an explicit Date (stored as INTEGER epoch ms) reads back as canonical ISO-8601-Z', async () => { + it('an explicit Date is STORED canonical, not just presented canonical (#3912)', async () => { const when = new Date('2026-03-20T12:34:56.789Z'); await driver.create('event', { id: 'e3', label: 'C', starts_at: when }, { bypassTenantAudit: true }); - // Raw on disk is the INTEGER epoch (better-sqlite3 binds a Date as getTime()). + // On disk it used to be the INTEGER epoch (better-sqlite3 binds a Date as + // getTime()); `formatInput` now canonicalises the write, so storage and + // presentation are the same string. That equality is the whole point — it is + // what lets a window filter and an ORDER BY read the column directly. const rawRow = await raw('event').where('id', 'e3').first(); - expect(typeof rawRow.starts_at).toBe('number'); - expect(rawRow.starts_at).toBe(when.getTime()); + expect(typeof rawRow.starts_at).toBe('string'); + expect(rawRow.starts_at).toBe('2026-03-20T12:34:56.789Z'); - // …but formatOutput presents the canonical instant. const row: any = await driver.findOne('event', 'e3', { bypassTenantAudit: true }); - expect(typeof row.starts_at).toBe('string'); - expect(row.starts_at).toBe('2026-03-20T12:34:56.789Z'); + expect(row.starts_at).toBe(rawRow.starts_at); }); - it('CONSISTENT PRESENTATION: an explicit-Date row and a defaulted row both read back as ISO-Z, despite genuinely mixed on-disk storage', async () => { + it('CONSISTENT STORAGE: an explicit-Date row and a defaulted row land in the SAME form (#3912)', async () => { await driver.create('event', { id: 'explicit', label: 'X', starts_at: new Date('2026-01-02T03:04:05.006Z') }, { bypassTenantAudit: true }); await driver.create('event', { id: 'defaulted', label: 'Y' }, { bypassTenantAudit: true }); // omitted → DDL default - // On disk: one INTEGER, one TEXT — exactly the mixed storage the fix targets. + // This assertion is the inverse of the one it replaces. The two write paths + // used to produce INTEGER and TEXT in one column — the mixed storage that + // broke every window filter (#3912) and still mis-orders ORDER BY (#3928). + // `formatInput` and `nowColumnDefault` now agree on one shape. const rawRows = await raw('event').whereIn('id', ['explicit', 'defaulted']).select('id', 'starts_at'); - const onDiskTypes = new Set(rawRows.map((r: any) => typeof r.starts_at)); - expect(onDiskTypes).toEqual(new Set(['number', 'string'])); + expect(new Set(rawRows.map((r: any) => typeof r.starts_at))).toEqual(new Set(['string'])); + for (const r of rawRows as any[]) expect(r.starts_at).toMatch(ISO_Z); + + // Lexicographic order is chronological order — what fixed-width UTC buys. + const sorted = [...rawRows].sort((a: any, b: any) => String(a.starts_at).localeCompare(String(b.starts_at))); + expect(sorted.map((r: any) => r.id)).toEqual(['explicit', 'defaulted']); - // On read: uniform canonical ISO-Z, both parse to a real instant. for (const id of ['explicit', 'defaulted']) { const row: any = await driver.findOne('event', id, { bypassTenantAudit: true }); expect(row.starts_at).toMatch(ISO_Z); @@ -122,6 +129,18 @@ describe('User NOW()-default temporal fields — canonical format (SQLite)', () } }); + it('still repairs a LEGACY epoch row on read (un-migrated database)', async () => { + // Rows written by a pre-#3912 build are INTEGER epoch ms. The read-side + // repair that presented them canonically has not gone anywhere — a database + // whose backfill has not run must keep reading correctly. + await raw('event').insert({ id: 'legacy', label: 'L', starts_at: Date.parse('2026-03-20T12:34:56.789Z') }); + const rawRow = await raw('event').where('id', 'legacy').first(); + expect(typeof rawRow.starts_at).toBe('number'); + + const row: any = await driver.findOne('event', 'legacy', { bypassTenantAudit: true }); + expect(row.starts_at).toBe('2026-03-20T12:34:56.789Z'); + }); + it('an explicit ISO-8601-Z string is preserved (idempotent) on read', async () => { const iso = '2026-05-25T08:00:00.000Z'; await driver.create('event', { id: 'e4', label: 'D', starts_at: iso }, { bypassTenantAudit: true }); diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index 8e4684a707..7b1818a7a8 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -202,6 +202,74 @@ function normalizeSqliteDatetimeOutput(value: unknown): unknown { return repairNaiveUtcAuditTimestamp(s); } +/** + * The CANONICAL on-disk form of a `Field.datetime` value: a fixed-width, + * zone-explicit UTC instant — `YYYY-MM-DDTHH:MM:SS.sssZ` (`Date#toISOString`). + * + * ADR-0053 already declares `datetime` to be "an instant stored as UTC"; this is + * the function that makes the STORAGE match the declaration instead of leaving + * it to whatever the caller happened to pass. It is applied on write + * ({@link SqlDriver.formatInput}) and to filter comparands + * ({@link SqlDriver.coerceFilterValue}) so both sides of every comparison are + * the same shape, on every dialect. + * + * Why THIS form (#3912): + * - Fixed width + UTC means lexicographic order IS chronological order, so a + * SQLite TEXT column sorts and range-compares correctly *through an index* — + * no expression wrapper, which is what an epoch-integer convention forces. + * - `strftime`/`julianday` parse it directly, so the date-bucket expression + * needs no epoch↔text CASE (#3773). + * - It is what `formatOutput`/`normalizeSqliteDatetimeOutput` ALREADY present + * on read, so storage and presentation stop disagreeing. + * - It matches the `Field.date` convention (ISO TEXT), so the platform has one + * temporal storage story rather than one per field type. + * - Postgres parses it into `timestamptz` unambiguously, which is precisely + * what a zone-naive string does NOT do (it is read in the SERVER's + * timezone — an 8-hour shift on an Asia/Shanghai server). + * + * Distinct from {@link repairNaiveUtcAuditTimestamp}, which is deliberately + * idempotent on any zone-EXPLICIT string and so preserves a `+08:00` offset. + * That is right for a read repair and wrong for a storage canon: `'…T12:00+08:00'` + * and `'…T04:00Z'` are the same instant but sort differently as text. Everything + * lands in `Z` here. + * + * Total: `null`/`undefined`, empty strings and unparseable junk pass through + * untouched rather than becoming `Invalid Date` — a value the driver cannot + * interpret is never silently rewritten. + */ +function canonicalUtcDatetime(value: unknown): unknown { + if (value == null) return value; + if (value instanceof Date) { + return Number.isNaN(value.getTime()) ? value : value.toISOString(); + } + if (typeof value === 'number') { + if (!Number.isFinite(value)) return value; + const d = new Date(value); + return Number.isNaN(d.getTime()) ? value : d.toISOString(); + } + if (typeof value !== 'string') return value; + const s = value.trim(); + if (s === '') return value; + // A bare integer (in either JS or string form) is epoch milliseconds — the + // shape better-sqlite3 wrote for every `Date` bound before this convention. + if (/^-?\d+$/.test(s)) { + const d = new Date(Number(s)); + return Number.isNaN(d.getTime()) ? value : d.toISOString(); + } + // A bare calendar day means midnight UTC. Stated explicitly so it cannot be + // re-read as midnight in the server's local zone — the Postgres divergence + // where the same query lands a row on a different calendar day. + const iso = /^\d{4}-\d{2}-\d{2}$/.test(s) + ? `${s}T00:00:00.000Z` + // Zone-naive `YYYY-MM-DD[ T]HH:MM[:SS[.fff]]` → its wall-clock IS UTC, the + // same rule `CURRENT_TIMESTAMP`-written rows take on read (ADR-0074). + : /^\d{4}-\d{2}-\d{2}[ T]\d{2}:\d{2}(:\d{2}(\.\d+)?)?$/.test(s) + ? `${s.replace(' ', 'T')}Z` + : s; + const ms = Date.parse(iso); + return Number.isFinite(ms) ? new Date(ms).toISOString() : value; +} + // ── Introspection Types ────────────────────────────────────────────────────── export interface IntrospectedColumn { @@ -343,6 +411,13 @@ export class SqlDriver implements IDataDriver { protected numericFields: Record = {}; protected dateFields: Record> = {}; protected datetimeFields: Record> = {}; + /** + * SQLite `Field.datetime` columns proven to hold ONLY canonical UTC text — + * either backfilled by {@link backfillCanonicalDatetimes} or created empty in + * this process. Read by {@link needsLegacyDatetimeRepair} to drop the repair + * expression, so a migrated deployment gets plain indexable `col op ?` SQL. + */ + protected canonicalDatetimeFields: Record> = {}; protected timeFields: Record> = {}; /** * Federation read path (ADR-0015). For external objects whose physical @@ -2458,6 +2533,11 @@ export class SqlDriver implements IDataDriver { if (exists) { await this.reconcileAndWarnDrift(tableName, obj.fields ?? {}, declaredIndexes); } + + // #3912: converge this table's `Field.datetime` columns on the canonical + // UTC-text storage form. A table this call just CREATED has no rows, so it + // is canonical by construction — record that without touching the disk. + await this.backfillCanonicalDatetimes(tableName, exists); } // Pre-create the auto_number counter table now, while we hold a fresh pooled @@ -2476,6 +2556,80 @@ export class SqlDriver implements IDataDriver { } } + /** + * Converge one table's `Field.datetime` columns on the canonical UTC-text + * storage form (#3912), then mark them clean so the read paths can drop their + * repair expression. + * + * SQLite only, and this is the whole reason the convention is affordable. + * Postgres/MySQL store a real temporal type, so their rows are already one + * shape — there is nothing on disk to rewrite. (What Postgres CANNOT recover is + * an instant written from a zone-naive string before this change: it was + * resolved against the server's timezone at write time and the original wall + * clock is gone. `formatInput` stops producing those going forward; existing + * rows are simply the instants the server recorded.) + * + * ONE `UPDATE` per column, whose SET expression is the very same + * {@link sqliteCanonicalDatetimeSql} the read paths use — so "what canonical + * means" has a single definition and the migration cannot drift from the repair + * it retires. It converts every non-canonical shape in one pass: INTEGER/REAL + * epoch ms, zone-naive `CURRENT_TIMESTAMP` output, an offset-bearing `+08:00` + * value, a bare `YYYY-MM-DD`. + * + * `col IS NOT ` is the whole `WHERE`. `IS NOT` rather than `<>` + * because it is null-safe AND type-aware: an INTEGER value is never equal to + * the text the expression yields, so epoch rows match; an already-canonical + * string equals it exactly and is skipped. A value SQLite cannot parse falls + * through the expression's `coalesce` unchanged, compares equal to itself, and + * is left alone rather than destroyed. So a converged table costs one scan and + * zero writes, and re-running is a no-op. + * + * Failures are logged and swallowed: the column simply stays un-marked, the + * read paths keep their repair, and queries stay CORRECT (just unindexed). A + * migration that cannot run must never be able to take the process down at + * boot, and correctness must never be contingent on one having run. + */ + protected async backfillCanonicalDatetimes(table: string, tableExisted: boolean): Promise { + const fields = this.datetimeFields[table]; + if (!this.isSqlite || !fields || fields.size === 0) return; + + const clean = (this.canonicalDatetimeFields[table] ??= new Set()); + // A table created by this very call is empty, so every datetime column in it + // is canonical without a single row being read. + if (!tableExisted) { + for (const field of fields) clean.add(field); + return; + } + + const canonical = this.sqliteCanonicalDatetimeSql('??'); + // The expression spells `??` 4×, and the statement uses it twice (the SET + // value and the WHERE guard) — hence the column name repeated per use. + const exprBindings = (field: string) => [field, field, field, field]; + for (const field of fields) { + try { + const res = await this.knex.raw( + `update ?? set ?? = ${canonical} where ?? is not null and ?? is not ${canonical}`, + [table, field, ...exprBindings(field), field, field, ...exprBindings(field)], + ); + const converted = (res as any)?.changes ?? 0; + if (converted) { + this.logger.info?.( + `[sql-driver] canonicalised datetime storage (#3912) for ${table}.${field}`, + { rowsConverted: converted }, + ); + } + clean.add(field); + } catch (err) { + // Correctness does not depend on this succeeding — only performance does. + this.logger.warn( + `[sql-driver] could not canonicalise datetime storage for ${table}.${field}; ` + + `queries stay correct via the read-side repair`, + { error: err instanceof Error ? err.message : String(err) }, + ); + } + } + } + // ── Managed-schema drift & reconcile (#2186) ─────────────────────────────── /** Canonical dialect name for the drift differ. */ @@ -3439,111 +3593,89 @@ export class SqlDriver implements IDataDriver { } /** - * Normalise a filter value for a single column so the comparison the - * driver sends to SQLite matches the on-disk representation. + * Put a filter comparand into the same canonical form the column is STORED in, + * so a comparison can never be decided by the two sides' shapes disagreeing. * - * The platform stores `Field.datetime()` values as INTEGER milliseconds - * (the result of passing a JS `Date` through better-sqlite3) but date - * macros like `{last_quarter_start}` expand to an ISO `YYYY-MM-DD` string - * client-side. Without coercion the SQL becomes `published_at >= '2026-…'` - * which collapses to a TEXT-vs-INTEGER affinity compare and never - * matches. We translate the ISO/Date/numeric inputs into the storage - * type so the comparison works. + * `Field.datetime` → canonical UTC ISO ({@link canonicalUtcDatetime}), the + * exact function `formatInput` applies on write. One rule, every dialect: + * - It is what a SQLite column now holds, as TEXT that sorts chronologically, + * so the comparison is a plain indexable string compare. + * - It is unambiguous for a Postgres `timestamptz`. This part is a FIX, not a + * no-op: the comparand used to be passed through untouched there, so a bare + * `YYYY-MM-DD` from a `{30_days_ago}` token meant midnight in the SERVER's + * timezone. Measured on an `Asia/Shanghai` server, the identical query put + * the identical instant on a different calendar day than SQLite did — a + * silently wrong window rather than an empty one. * - * For `Field.date()` we keep ISO TEXT but normalise Date objects to - * `YYYY-MM-DD` for the same reason. + * The previous rule coerced to an epoch INTEGER on SQLite only, which assumed + * a storage form the write path never guaranteed; see #3912 for why that could + * not be made correct without also rewriting what the writer produces. + * + * `Field.date` keeps ISO TEXT, normalised to `YYYY-MM-DD` (ADR-0053 Phase 1). */ protected coerceFilterValue(table: string | null, field: string, value: any): any { if (value == null || !table) return value; if (Array.isArray(value)) return value.map((v) => this.coerceFilterValue(table, field, v)); - const isDatetime = this.datetimeFields[table]?.has(field); - const isDate = this.dateFields[table]?.has(field); - if (!isDatetime && !isDate) return value; - - const toMs = (v: any): number | null => { - if (v instanceof Date) return v.getTime(); - if (typeof v === 'number' && Number.isFinite(v)) return v; - if (typeof v === 'string') { - const trimmed = v.trim(); - if (trimmed === '') return null; - if (/^-?\d+$/.test(trimmed)) { - const n = Number(trimmed); - if (Number.isFinite(n)) return n; - } - // Treat bare YYYY-MM-DD as start-of-day UTC; full ISO is parsed - // as-is so timezones round-trip correctly. - const iso = /^\d{4}-\d{2}-\d{2}$/.test(trimmed) ? `${trimmed}T00:00:00.000Z` : trimmed; - const n = Date.parse(iso); - return Number.isFinite(n) ? n : null; - } - return null; - }; - - if (isDatetime) { - // Only SQLite stores `Field.datetime` as an INTEGER epoch (better-sqlite3 - // binds a JS `Date` as `.getTime()`); there the ISO/text comparand MUST be - // coerced to epoch ms or it collapses to a TEXT-vs-INTEGER affinity compare - // that never matches. Postgres/MySQL map datetime to a native TIMESTAMP - // (see `defineColumn` → `table.timestamp`), where Knex binds an ISO string - // or `Date` correctly — coercing to an epoch integer there would compare an - // INTEGER against a TIMESTAMP and break the query. So gate on the storage - // form, via the predicate the bucket expression reads too. - if (!this.isEpochStoredDatetime(table, field)) return value; - const ms = toMs(value); - return ms == null ? value : ms; - } - - // Field.date — normalise the comparand to YYYY-MM-DD (ADR-0053 Phase 1). - return this.toDateOnly(value); + const kind = this.temporalFieldKind(table, field); + if (!kind) return value; + return kind === 'datetime' ? canonicalUtcDatetime(value) : this.toDateOnly(value); } /** - * Does this column store instants as epoch **milliseconds** rather than as a - * temporal type SQL understands natively? + * Might this SQLite `Field.datetime` column still hold values written BEFORE + * the canonical-UTC-text convention (#3912) — an INTEGER/REAL epoch from a + * bound JS `Date`, a zone-naive `CURRENT_TIMESTAMP` string, an offset-bearing + * `+08:00` string — and therefore need + * {@link sqliteCanonicalDatetimeSql} wrapped around it before it is compared + * or bucketed? + * + * This is the ONE predicate for that question. Every consumer of it has already + * been bitten by disagreeing about storage — the filter comparand (#2034, then + * #3912) and the aggregate bucket expression (#3773) — so they share it rather + * than each carrying a copy of the rule. * - * True for a SQLite `Field.datetime` and nothing else: better-sqlite3 binds a - * JS `Date` as `.getTime()`, while Postgres/MySQL get a real TIMESTAMP column - * (`defineColumn` → `table.timestamp`) and `Field.date` is ISO TEXT on every - * dialect. + * `false` in three cases, and the third is the point of the whole exercise: + * - not SQLite (Postgres/MySQL have a real temporal type); + * - not a declared `Field.datetime`; + * - the column has been BACKFILLED to the canonical form in this process + * ({@link backfillCanonicalDatetimes}), so every row is already canonical + * text and the repair would only cost an unindexable expression. * - * This is the ONE predicate for that storage convention. Both consumers of it - * have already been bitten by disagreeing about storage — the filter comparand - * (#2034: an ISO string compared against an INTEGER column matched nothing) - * and the aggregate bucket expression (#3773: `strftime` read the same INTEGER - * as a Julian day and bucketed everything as NULL) — so they share it rather - * than each carrying their own copy of the rule. + * That last exit is what makes the convention pay off rather than just move the + * cost around: after migration the emitted SQL is a plain `col >= ?` again. */ - protected isEpochStoredDatetime(table: string | null | undefined, field: string): boolean { + protected needsLegacyDatetimeRepair(table: string | null | undefined, field: string): boolean { if (!table || !this.isSqlite) return false; - return this.datetimeFields[table]?.has(field) === true; + if (this.datetimeFields[table]?.has(field) !== true) return false; + return this.canonicalDatetimeFields[table]?.has(field) !== true; } /** - * The value expression to hand SQLite's `strftime()` for a bucketed column. - * - * A TEXT-stored column is passed straight through — `strftime` already parses - * `YYYY-MM-DD`, `YYYY-MM-DDTHH:MM:SSZ` and the zone-naive `CURRENT_TIMESTAMP` - * form. An epoch-stored `Field.datetime` (see {@link isEpochStoredDatetime}) - * must first be converted, or `strftime` reads the bare integer as a Julian - * day number — epoch ms is far outside the legal range, so every row buckets - * as NULL and a trend chart collapses into a single `(null)` bar. + * Read a possibly-legacy SQLite `Field.datetime` column as canonical UTC text + * — the SQL twin of {@link canonicalUtcDatetime}, for rows written before the + * convention existed and not yet backfilled. * - * The conversion dispatches on the STORED value's type, not just the declared - * one, because a SQLite `Field.datetime` column is genuinely mixed-form: an - * explicit value bound as a JS `Date` lands as INTEGER/REAL epoch ms, while a - * `defaultValue: 'NOW()'` slot lands as TEXT (the same mix `formatOutput` → - * `normalizeSqliteDatetimeOutput` already repairs on read). Dividing a TEXT - * timestamp by 1000 would coerce it to its leading year — `'2026-01-10T…'/1000.0` - * is `2.026` seconds past the epoch, bucketing real rows into 1970 — which is - * strictly worse than the NULL it replaces, so the CASE is load-bearing. + * `strftime('%Y-%m-%dT%H:%M:%fZ', …)` is the same format string + * {@link nowColumnDefault} already writes, so a repaired value is + * byte-identical to a freshly written one — which is what lets the comparison + * be a plain text compare. The `typeof()` dispatch is load-bearing: an epoch + * INTEGER handed to `strftime` unconverted is read as a Julian day (#3773), + * while `'unixepoch'` applied to text would read its leading year as seconds. * - * Normalising to a julian day (rather than emitting `strftime(fmt, x/1000.0, - * 'unixepoch')`) keeps ONE reusable scalar the caller can drop into any format - * string, including the two-reference quarter expression. `/1000.0` is real - * division on purpose: integer `/1000` truncates toward zero, which pushes a - * pre-1970 instant forward a day (`-1` → 1970-01-01 instead of 1969-12-31). + * `coalesce(…, col)` preserves genuinely uninterpretable junk instead of + * turning it into NULL, matching `canonicalUtcDatetime`'s totality — a value + * the driver cannot parse keeps failing the comparison, rather than silently + * becoming "no value". */ + protected sqliteCanonicalDatetimeSql(columnSql: string): string { + return ( + `(case when typeof(${columnSql}) in ('integer','real') ` + + `then strftime('%Y-%m-%dT%H:%M:%fZ', ${columnSql}/1000.0, 'unixepoch') ` + + `else coalesce(strftime('%Y-%m-%dT%H:%M:%fZ', ${columnSql}), ${columnSql}) end)` + ); + } + /** * Which temporal presentation rule, if any, a declared field takes — * `null` for everything that is not a `Field.datetime` / `Field.date`. @@ -3635,65 +3767,51 @@ export class SqlDriver implements IDataDriver { return rows; } - protected sqliteTemporalArg( - field: string, - table: string | null | undefined, - ): { sql: string; bindings: any[] } { - if (!this.isEpochStoredDatetime(table, field)) return { sql: '??', bindings: [field] }; - return { - sql: `(case when typeof(??) in ('integer','real') then julianday(??/1000.0, 'unixepoch') else julianday(??) end)`, - bindings: [field, field, field], - }; - } - /** - * SQL that reads a SQLite `Field.datetime` column as epoch **milliseconds**, - * whatever form the row actually stored it in. + * The value expression to hand SQLite's `strftime()` for a bucketed column. * - * A `Field.datetime` column on SQLite is genuinely MIXED-form, and always has - * been: a value bound as a JS `Date` lands as INTEGER epoch ms, while a REST / - * JSON write (JSON has no `Date`, so the payload carries an ISO string) and a - * `defaultValue: 'NOW()'` slot — including the platform's own `created_at` / - * `updated_at` audit stamps — land as ISO TEXT. `formatOutput` → - * `normalizeSqliteDatetimeOutput` already repairs that mix on read, and - * {@link sqliteTemporalArg} already dispatches on `typeof()` for bucketing - * (#3773). The filter path had no equivalent: it coerced the COMPARAND to - * epoch ms purely from the DECLARED type, so an ISO-TEXT-stored row failed the - * TEXT-vs-INTEGER affinity compare and every datetime window filter returned - * empty (#3912) — the exact failure the epoch coercion was added to prevent, - * just with the two storage forms swapped. + * Canonical UTC text (#3912) is passed straight through — `strftime` parses it, + * `YYYY-MM-DD`, and the zone-naive `CURRENT_TIMESTAMP` form alike. A column + * that may still hold PRE-canonical values gets {@link sqliteCanonicalDatetimeSql} + * wrapped around it, which is likewise text, so `strftime` sees one shape + * either way. * - * Normalising the COLUMN (rather than guessing at the comparand) is what makes - * the comparison correct for both forms at once. The julian-day round trip is - * used instead of `unixepoch(x, 'subsec')` because the latter needs SQLite - * 3.42+; `julianday()` is exact to the millisecond on every version (SQLite - * carries the julian day as integer ms internally), so `round()` recovers the - * epoch exactly and equality filters keep matching. An unparseable TEXT value - * yields NULL, which compares false — the same non-match it produced before. + * Without that repair an epoch INTEGER reaches `strftime` as a bare number, + * which SQLite reads as a Julian DAY — epoch ms is far outside the legal range, + * so every row buckets as NULL and a trend chart collapses to one `(null)` bar + * (#3773). The `typeof()` dispatch inside the repair is equally load-bearing in + * the other direction: dividing a TEXT timestamp by 1000 coerces it to its + * leading year (`'2026-01-10T…'/1000.0` = 2.026 seconds past the epoch), + * bucketing live rows into 1970 — strictly worse than the NULL it replaces. */ - protected sqliteEpochMsSql(columnSql: string): string { - return ( - `(case when typeof(${columnSql}) in ('integer','real') then ${columnSql} ` + - `else cast(round((julianday(${columnSql}) - 2440587.5) * 86400000.0) as integer) end)` - ); + protected sqliteTemporalArg( + field: string, + table: string | null | undefined, + ): { sql: string; bindings: any[] } { + if (!this.needsLegacyDatetimeRepair(table, field)) return { sql: '??', bindings: [field] }; + return { sql: this.sqliteCanonicalDatetimeSql('??'), bindings: [field, field, field, field] }; } /** - * The left-hand side of a filter comparison on `column`, normalised to the - * storage form {@link coerceFilterValue} coerces the comparand into. + * The left-hand side of a filter comparison on `column`, read in the same + * canonical form {@link coerceFilterValue} puts the comparand in. * - * `null` — the overwhelmingly common answer — means the plain column - * identifier is already correct, so the caller keeps using the ordinary Knex - * builder call (and its index-friendly `col op ?` SQL). Only a SQLite - * `Field.datetime` needs the {@link sqliteEpochMsSql} CASE. + * `null` — the answer for every dialect, every non-datetime column, and every + * SQLite datetime column that has been backfilled — means the plain identifier + * is already correct, so the caller keeps the ordinary Knex builder call and + * its indexable `col op ?` SQL. Only a column that may still hold PRE-canonical + * values ({@link needsLegacyDatetimeRepair}) is wrapped. */ protected filterColumnExpr( table: string | null | undefined, field: string, column: string, ): { sql: string; bindings: any[] } | null { - if (!this.isEpochStoredDatetime(table, field)) return null; - return { sql: this.sqliteEpochMsSql('??'), bindings: [column, column, column] }; + if (!this.needsLegacyDatetimeRepair(table, field)) return null; + return { + sql: this.sqliteCanonicalDatetimeSql('??'), + bindings: [column, column, column, column], + }; } /** @@ -3796,8 +3914,8 @@ export class SqlDriver implements IDataDriver { * must wrap its column with this too, or it keeps half the bug. */ public temporalFilterColumnSql(objectName: string, field: string, columnSql: string): string { - if (!this.isEpochStoredDatetime(objectName, field)) return columnSql; - return this.sqliteEpochMsSql(columnSql); + if (!this.needsLegacyDatetimeRepair(objectName, field)) return columnSql; + return this.sqliteCanonicalDatetimeSql(columnSql); } protected applyFilters(builder: Knex.QueryBuilder, filters: any) { @@ -4533,12 +4651,43 @@ export class SqlDriver implements IDataDriver { } } + // ADR-0053: a `Field.datetime` is an instant stored as UTC. Make the STORAGE + // say so — collapse every accepted input shape (JS `Date`, epoch number, ISO + // string, zone-naive wall clock, bare calendar day) to one canonical + // `YYYY-MM-DDTHH:MM:SS.sssZ` before it hits the wire (#3912). + // + // This is the write half of the fix; `coerceFilterValue` applies the SAME + // function to comparands, so the two sides of a comparison can no longer + // disagree about shape. It runs on EVERY dialect, for two different reasons: + // - SQLite has no temporal type, so whatever is bound is what is stored. A + // `Date` landed as INTEGER epoch and a REST/JSON write as ISO TEXT, in + // the same column — the mixed storage that made every window filter + // return the wrong rows, and that still makes ORDER BY sort all INTEGER + // rows before all TEXT ones (#3928). + // - Postgres/MySQL do have one, but a zone-NAIVE string bound into it is + // interpreted in the SERVER's timezone, not UTC — measured at 8 hours off + // on an `Asia/Shanghai` server. Sending an explicit `Z` removes the + // server's timezone from the write path entirely. + const datetimeFields = this.datetimeFields[object]; + if (datetimeFields && datetimeFields.size > 0 && copy && typeof copy === 'object') { + for (const field of datetimeFields) { + const v = copy[field]; + if (v == null) continue; + // `NOW()` was already replaced with an ISO instant above; anything else + // that is not interpretable as a time passes through untouched. + const normalized = canonicalUtcDatetime(v); + if (normalized !== v) { + if (!copied) { copy = { ...copy }; copied = true; } + copy[field] = normalized; + } + } + } + // ADR-0053 Phase 1: a `Field.date` is a timezone-naive calendar day, not // an instant. Collapse any `Date` or full-ISO value to `YYYY-MM-DD` before // it hits the wire so storage matches the date-only contract the filter // layer (`coerceFilterValue`) already enforces — the write/filter // asymmetry was the root cause of the silent date-equality miss. - // `Field.datetime` is untouched (it keeps full-instant semantics). const dateFields = this.dateFields[object]; if (dateFields && dateFields.size > 0 && copy && typeof copy === 'object') { for (const field of dateFields) { From 0b92b581b753f3b30b36f05bd65e8f02394b5ce3 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 12:25:14 +0000 Subject: [PATCH 3/5] fix(driver-sql): store Field.datetime as DATETIME(3) UTC on MySQL (#3942) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the last dialect gap from #3912 — and a regression that change introduced. Measured on MariaDB 10.11 with default_time_zone='+08:00' and the process on America/New_York. MySQL accepts neither the `T` separator nor the `Z` suffix in a datetime literal, so the canonical ISO form fails the statement outright with "Incorrect datetime value". An ISO comparand or REST/JSON write had ALWAYS failed that way — JSON has no Date type, so datetime writes over REST were broken on MySQL before any of this — but canonicalising the write path in #3912 extended the failure to Date writes too. Baseline before #3912: ISO string rejected, Date accepted but truncated to whole seconds, zone-naive string stored 12 hours off (host zone and server zone compounding), nothing outside 1970..2038 storable at all. Three coordinated pieces, keeping the logical canon and changing only the physical spelling: - Field.datetime maps to DATETIME(3), not TIMESTAMP. TIMESTAMP is a 32-bit epoch (a 2040 contract end date is simply rejected), carries no fractional digits, and converts using the session timezone. DATETIME does none of that, so the column holds the UTC wall clock the driver writes — the ServiceNow model. Postgres deliberately keeps timestamptz: precision 3 there would REDUCE it from microseconds. created_at / updated_at take the same type; the registry declares them datetime and they are what most list views sort by. - The connection is pinned to UTC on both layers — connection.timezone for mysql2, SET time_zone='+00:00' via pool.afterCreate for the server. That is what makes the wall clock BE the instant, and it keeps a not-yet-migrated TIMESTAMP column correct too. An explicit host choice is left alone; an existing afterCreate is chained, not replaced. - storageDatetimeValue respells the canonical instant as a MySQL literal for the bind, on the write path and the filter path alike, so the two cannot disagree. Deliberately strict: only an exactly-canonical string is rewritten, so unparseable values and years outside 1000..9999 reach MySQL untouched and fail loudly rather than being reinterpreted. migrateMysqlDatetimeColumns widens legacy TIMESTAMP columns at schema sync, restating the audit DEFAULT because MySQL drops it on MODIFY. Same failure policy as the SQLite backfill: logged and swallowed, because a TIMESTAMP column keeps working (same literal, UTC session) and merely keeps its range and precision limits. Verified on a real server: the ALTER moves no stored instant, correctly-stored legacy rows round-trip exactly, post-2038 and pre-1970 instants then store, and re-running is a no-op. As on Postgres, the migration cannot repair instants the old timezone-ambiguous write path recorded wrongly — it preserves what is on disk. Tests: sql-driver-datetime-mysql-storage.test.ts, opt-in via OS_TEST_MYSQL_URL and asserting a non-UTC server so it cannot pass vacuously; 10 of its 13 cases fail without this change. The two existing suites that pinned the old MySQL behaviour now assert the new one — the comparand is dialect-spelled, and the connection carries the UTC pin. ADR-0053 addendum D-B4. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TPxNwPnjcn599ujXpU3ibJ --- docs/adr/0053-date-and-datetime-semantics.md | 59 +++- .../src/sql-driver-connect-bound.test.ts | 28 ++ .../sql-driver-datetime-mysql-storage.test.ts | 273 ++++++++++++++++++ .../src/sql-driver-temporal-dialect.test.ts | 39 ++- packages/plugins/driver-sql/src/sql-driver.ts | 218 +++++++++++++- 5 files changed, 595 insertions(+), 22 deletions(-) create mode 100644 packages/plugins/driver-sql/src/sql-driver-datetime-mysql-storage.test.ts diff --git a/docs/adr/0053-date-and-datetime-semantics.md b/docs/adr/0053-date-and-datetime-semantics.md index 3dfd1e78fb..b5ab1d8358 100644 --- a/docs/adr/0053-date-and-datetime-semantics.md +++ b/docs/adr/0053-date-and-datetime-semantics.md @@ -1,6 +1,6 @@ # ADR-0053: `date` is a timezone-naive calendar day; `datetime` is an instant rendered in a reference timezone -**Status**: Accepted (2026-06-16) — Phase 1 + addendum D-A1 implemented (`sql-driver.ts` `toDateOnly` write/read/filter normalization; analytics `coerceTemporalFilterValue`), Phase 2 landing incrementally; D-A2 (`temporalFilterValue` promotion onto the `IDataDriver` contract) still open as the ADR predicted. **Partly superseded (2026-07-29, addendum D-B1):** Phase 1's "`Field.datetime` stays stored as UTC epoch ms" is replaced by a canonical `YYYY-MM-DDTHH:MM:SS.sssZ` text storage form, applied on write and to filter comparands on every dialect — see the final addendum (#3912). +**Status**: Accepted (2026-06-16) — Phase 1 + addendum D-A1 implemented (`sql-driver.ts` `toDateOnly` write/read/filter normalization; analytics `coerceTemporalFilterValue`), Phase 2 landing incrementally; D-A2 (`temporalFilterValue` promotion onto the `IDataDriver` contract) still open as the ADR predicted. **Partly superseded (2026-07-29, addendum D-B1..D-B4):** Phase 1's "`Field.datetime` stays stored as UTC epoch ms" is replaced by one canonical UTC instant per dialect — `YYYY-MM-DDTHH:MM:SS.sssZ` text on SQLite, `timestamptz` on Postgres, `DATETIME(3)` on MySQL — applied on write and to filter comparands alike (#3912, #3942). **Deciders**: ObjectStack Protocol Architects **Builds on**: [ADR-0032](./0032-unified-expression-layer.md) (unified expression layer — CEL dialect, `today()`/`daysFromNow()`), [ADR-0014](./0014-record-form-field-type.md) (field types) **Consumers**: `@objectstack/spec` (`Field.date`/`Field.datetime`), `@objectstack/driver-sql` (`coerceFilterValue`, `formatInput`/`formatOutput`, `dateFields`/`datetimeFields`), `@objectstack/formula` (`stdlib` time functions, `cel-engine` hydration), `@objectstack/objectql` (`applyFormulaPlan`), schedule/cron executors, report/analytics date bucketing, `sys-user-preference.timezone`. @@ -405,7 +405,7 @@ time, the matrix proves runtime correctness across drivers. --- -## Addendum (2026-07-29) — `Field.datetime` has ONE storage form: canonical UTC text +## Addendum (2026-07-29) — `Field.datetime` has ONE storage form per dialect, always a UTC instant > **Status:** landed. This addendum **revises** the storage half of Phase 1 (which > left `Field.datetime` "stored as UTC epoch ms" on SQLite) and **extends** @@ -509,8 +509,53 @@ is clean. known to have produced dialect-divergent row results. - #3928 (datetime `ORDER BY` mis-sorted on mixed storage) is closed by construction rather than by a sort-side fix. -- MySQL is **not** covered here. `table.timestamp` emits a MySQL `TIMESTAMP`, - whose range ends at 2038-01-19, and the connection sets no `timezone`, so - mysql2 serialises a `Date` in the Node process's local zone. That is a separate - defect from #3912 and is filed on its own; it needs a real MySQL to verify and - a column-type migration to fix. +### D-B4 — MySQL stores the same instant, spelled the way MySQL parses it + +MySQL was the third dialect, and measurement (MariaDB 10.11, `default_time_zone += '+08:00'`, process on `America/New_York`) found it the worst off — including +one defect D-B1 *introduced*: + +- MySQL accepts neither the `T` separator nor the `Z` suffix in a datetime + literal, so the canonical form failed the statement outright with *Incorrect + datetime value*. An ISO comparand or REST/JSON write had **always** failed this + way; canonicalising the write path extended it to `Date` writes too. +- `table.timestamp` emits MySQL `TIMESTAMP`: a 32-bit epoch that cannot hold an + instant outside 1970..2038 (a contract end date in 2040 was rejected), carries + no fractional digits so the canonical form's milliseconds were truncated, and + converts on read/write using the session timezone. +- mysql2's `connection.timezone` defaults to the HOST's local zone and + `@@session.time_zone` to the server's, so a zone-naive value landed 12 hours + off with the two misconfigurations compounding. + +The resolution keeps the logical canon and changes only the physical spelling: + +1. `Field.datetime` maps to **`DATETIME(3)`** — range 1000..9999, milliseconds + kept, and no timezone conversion of its own, so the column holds the UTC wall + clock the driver writes. This is the ServiceNow model. Postgres deliberately + keeps `timestamptz`: asking for precision 3 there would *reduce* it from + microseconds. The builtin `created_at`/`updated_at` take the same type — the + registry declares them `Field.datetime`, and they are what most list views + sort by. +2. The connection is **pinned to UTC on both layers** — `connection.timezone = + 'Z'` for mysql2 and `SET time_zone = '+00:00'` via `pool.afterCreate` for the + server. This is what makes the wall clock *be* the instant, and it keeps a + not-yet-migrated `TIMESTAMP` column correct too. An explicit host choice is + left alone; an existing `afterCreate` is chained, not replaced. +3. `storageDatetimeValue` respells the canonical instant as a MySQL literal for + the bind, on the write path and the filter path alike. Deliberately strict: + only an exactly-canonical string is rewritten, so an unparseable value or a + year outside 1000..9999 reaches MySQL untouched and fails loudly. + +`migrateMysqlDatetimeColumns` widens legacy `TIMESTAMP` columns at schema sync, +under the same failure policy as D-B3 — a `TIMESTAMP` column keeps *working* +(the driver binds the same literal and the session is UTC), it merely keeps the +range and precision limits. Verified on a real server: the ALTER moves no stored +instant, correctly-stored legacy rows round-trip exactly, and it is idempotent. + +The same caveat as Postgres applies and is worth stating plainly: the migration +**cannot repair instants the old timezone-ambiguous write path recorded wrongly** +— that information is gone. It preserves what is on disk. + +Regression cover is `sql-driver-datetime-mysql-storage.test.ts`, opt-in via +`OS_TEST_MYSQL_URL` (CI provisions no server), asserting a non-UTC server so it +cannot pass vacuously. 10 of its 13 cases fail without this change. diff --git a/packages/plugins/driver-sql/src/sql-driver-connect-bound.test.ts b/packages/plugins/driver-sql/src/sql-driver-connect-bound.test.ts index 6b3fb4a25c..851532ee19 100644 --- a/packages/plugins/driver-sql/src/sql-driver-connect-bound.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-connect-bound.test.ts @@ -71,9 +71,37 @@ describe('SqlDriver — connection-attempt bound (framework#3769)', () => { expect((d as any).knex.client.config.connection).toEqual({ uri: 'mysql://u:p@host:3306/d', connectTimeout: 10_000, + // #3942 — mysql2 renders a bound `Date` and parses a returned DATETIME in + // `connection.timezone`, which defaults to the HOST's local zone. Pinned + // to UTC so the recorded instant cannot depend on which machine wrote it. + timezone: 'Z', }); }); + it('pins MySQL to UTC on both layers, and leaves an explicit choice alone (#3942)', () => { + const pinned = make({ client: 'mysql2', connection: { host: 'db', database: 'app' } }); + expect((pinned as any).knex.client.config.connection.timezone).toBe('Z'); + // The server side is the other half: `@@session.time_zone` decides how a + // zone-naive literal is read for a legacy TIMESTAMP column, and what + // CURRENT_TIMESTAMP renders. Measured 8h off on a `+08:00` server. + expect(typeof (pinned as any).knex.client.config.pool.afterCreate).toBe('function'); + + const explicit = make({ + client: 'mysql2', + connection: { host: 'db', database: 'app', timezone: '+08:00' }, + }); + expect((explicit as any).knex.client.config.connection.timezone).toBe('+08:00'); + }); + + it('does not touch the connection timezone on non-MySQL dialects', () => { + // Postgres resolves an explicit-offset literal itself and SQLite has no + // session zone at all, so neither needs (or should get) the mysql2 knob. + for (const client of ['pg', 'better-sqlite3']) { + const d = make({ client, connection: { host: 'db', database: 'app', filename: ':memory:' } }); + expect((d as any).knex.client.config.connection.timezone).toBeUndefined(); + } + }); + it('adds the timeout to an object connection without disturbing its fields', () => { const d = make({ client: 'pg', diff --git a/packages/plugins/driver-sql/src/sql-driver-datetime-mysql-storage.test.ts b/packages/plugins/driver-sql/src/sql-driver-datetime-mysql-storage.test.ts new file mode 100644 index 0000000000..08b4f0a3fd --- /dev/null +++ b/packages/plugins/driver-sql/src/sql-driver-datetime-mysql-storage.test.ts @@ -0,0 +1,273 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #3942 — `Field.datetime` on MySQL. + * + * MySQL was the dialect the #3912 canonical-storage work did not reach, and it + * was the worst off of the three. Measured on MariaDB 10.11 before this change: + * + * - An ISO-8601 comparand or write value FAILED the statement outright — + * MySQL accepts neither the `T` separator nor the `Z` suffix in a datetime + * literal, so every REST/JSON write of a datetime errored with *Incorrect + * datetime value*, and canonicalising the write path in #3912 extended that + * to `Date` writes too. + * - A `Date` that did land was truncated to whole seconds: `table.timestamp` + * emits MySQL `TIMESTAMP`, which carries no fractional digits by default. + * - A zone-naive string landed at the wrong instant — 4 hours off with the + * process on `America/New_York` — because mysql2 renders and parses in the + * HOST's zone unless told otherwise. + * - Nothing outside 1970..2038 could be stored at all: `TIMESTAMP` is a 32-bit + * epoch. A contract end date in 2040 was simply rejected. + * + * The fix is three coordinated pieces, all asserted below: `DATETIME(3)` instead + * of `TIMESTAMP`, a connection pinned to UTC on both the mysql2 and the server + * layer, and a MySQL-spelled bind carrying the same UTC wall clock. + * + * Opt-in — needs a real server, which CI does not provision: + * + * OS_TEST_MYSQL_URL=mysql://root@127.0.0.1:3306/test pnpm --filter @objectstack/driver-sql test + * + * Point it at a server whose `time_zone` is NOT UTC (e.g. `default_time_zone = + * '+08:00'`) and run with a non-UTC `TZ`; the suite asserts the server side and + * tells you if it cannot prove anything. + */ + +import { describe, it, expect, beforeAll, beforeEach, afterEach } from 'vitest'; +import { SqlDriver } from '../src/index.js'; + +const URL = process.env.OS_TEST_MYSQL_URL; +const TABLE = 'os3942_probe'; + +const MIDDAY = '2026-03-20T12:34:56.789Z'; +/** 20:00Z is 04:00 the NEXT day at +08:00 — so a day window discriminates. */ +const BOUNDARY = '2026-03-20T20:00:00.000Z'; + +/** A driver whose connection this suite does NOT pin, to read raw server state. */ +const rawDriver = () => new SqlDriver({ client: 'mysql2', connection: URL }); + +describe.skipIf(!URL)('Field.datetime on MySQL (#3942)', () => { + let driver: SqlDriver; + let serverTimeZone = ''; + + beforeAll(async () => { + const probe = rawDriver(); + const rows = await rowsOf(probe, `select @@global.time_zone as tz`); + serverTimeZone = String((rows[0] as any).tz); + await probe.disconnect(); + }); + + beforeEach(async () => { + driver = new SqlDriver({ client: 'mysql2', connection: URL }); + await driver.execute(`drop table if exists ${TABLE}`); + await driver.initObjects([ + { name: TABLE, fields: { label: { type: 'string' }, at: { type: 'datetime' } } }, + ]); + }); + + afterEach(async () => { + await driver.execute(`drop table if exists ${TABLE}`).catch(() => {}); + await driver.disconnect(); + }); + + it('is pointed at a non-UTC server (otherwise this suite proves less)', () => { + expect( + serverTimeZone, + "set default_time_zone='+08:00' — on a UTC server the timezone bugs are invisible", + ).not.toMatch(/^(UTC|\+00:00|SYSTEM)$/i); + }); + + it('creates DATETIME(3), not TIMESTAMP — no 2038 ceiling, milliseconds kept', async () => { + const cols = await columnTypes(driver, TABLE); + expect(cols.at).toBe('datetime(3)'); + // The audit columns are declared `Field.datetime` by the objectql registry + // and are what most list views sort by, so they take the same type. + expect(cols.created_at).toBe('datetime(3)'); + expect(cols.updated_at).toBe('datetime(3)'); + }); + + it('pins the session to UTC, so the server timezone stops participating', async () => { + const rows = await rowsOf(driver, `select @@session.time_zone as tz`); + expect(String((rows[0] as any).tz)).toBe('+00:00'); + }); + + it('accepts an ISO write at all — it used to fail the statement', async () => { + // The headline regression: a REST/JSON payload carries an ISO string, and + // MySQL rejects it verbatim. This asserts the write completes; the next test + // asserts it landed on the right instant. + await expect( + driver.create(TABLE, { id: 'iso', label: 'iso', at: MIDDAY }, { bypassTenantAudit: true }), + ).resolves.toBeDefined(); + }); + + it('records the same instant for a Date, an ISO-Z string and a NAIVE string', async () => { + await driver.create(TABLE, { id: 'date-obj', label: 'a', at: new Date(MIDDAY) }, { bypassTenantAudit: true }); + await driver.create(TABLE, { id: 'iso-z', label: 'b', at: MIDDAY }, { bypassTenantAudit: true }); + await driver.create(TABLE, { id: 'naive', label: 'c', at: '2026-03-20 12:34:56.789' }, { bypassTenantAudit: true }); + await driver.create(TABLE, { id: 'offset', label: 'd', at: '2026-03-20T20:34:56.789+08:00' }, { bypassTenantAudit: true }); + + const expected = Date.parse(MIDDAY); + for (const id of ['date-obj', 'iso-z', 'naive', 'offset']) { + expect(await storedEpochMs(driver, id), `${id} stored instant`).toBe(expected); + } + }); + + it('keeps milliseconds — TIMESTAMP silently truncated them to whole seconds', async () => { + await driver.create(TABLE, { id: 'ms', label: 'ms', at: MIDDAY }, { bypassTenantAudit: true }); + expect(await storedEpochMs(driver, 'ms')).toBe(Date.parse(MIDDAY)); + expect(Date.parse(MIDDAY) % 1000).toBe(789); // the fixture would be vacuous otherwise + }); + + it('stores instants outside the 1970..2038 TIMESTAMP range', async () => { + // Asserted through the driver's own read rather than `unix_timestamp()`, + // which is itself a 32-bit epoch function and returns 0 for exactly the + // instants this test is about. + for (const [id, iso] of [['future', '2040-06-01T00:00:00.000Z'], ['past', '1960-06-01T00:00:00.000Z']] as const) { + await driver.create(TABLE, { id, label: id, at: iso }, { bypassTenantAudit: true }); + expect(await readInstant(driver, id), id).toBe(iso); + } + }); + + it('anchors a bare calendar-day comparand to UTC midnight, like the other dialects', async () => { + await driver.create(TABLE, { id: 'b1', label: 'x', at: BOUNDARY }, { bypassTenantAudit: true }); + const day = async (from: string, to: string) => + (await driver.find(TABLE, { where: { at: { $gte: from, $lt: to } } })).map((r: any) => r.id); + + // 20:00Z belongs to 2026-03-20 in UTC; on a `+08:00` server read as local + // midnight it would fall on the 21st — the divergence #3912 measured on PG. + expect(await day('2026-03-20', '2026-03-21')).toEqual(['b1']); + expect(await day('2026-03-21', '2026-03-22')).toEqual([]); + }); + + it('round-trips the instant through a read', async () => { + await driver.create(TABLE, { id: 'r', label: 'r', at: MIDDAY }, { bypassTenantAudit: true }); + const row: any = await driver.findOne(TABLE, 'r', { bypassTenantAudit: true }); + expect(new Date(row.at).toISOString()).toBe(MIDDAY); + }); +}); + +describe.skipIf(!URL)('MySQL TIMESTAMP → DATETIME(3) migration (#3942)', () => { + const LEGACY = 'os3942_legacy'; + const GOOD = '2026-03-20T12:34:56.000Z'; + let driver: SqlDriver; + + beforeEach(async () => { + // Build the table the way a pre-#3942 build did: TIMESTAMP columns, rows in + // it. The legacy connection is pinned to UTC so the fixture's instants are + // the ones we intend — the migration's job is to preserve them, not to + // second-guess what an older, timezone-ambiguous writer recorded. + const legacy = rawDriver(); + await legacy.execute(`drop table if exists ${LEGACY}`); + await legacy.execute( + `create table ${LEGACY} ( + id varchar(255) not null primary key, + created_at timestamp null default current_timestamp, + updated_at timestamp null default current_timestamp, + label varchar(255) null, + at timestamp null + )`, + ); + await legacy.execute(`set time_zone = '+00:00'`); + await legacy.execute(`insert into ${LEGACY} (id, label, at) values (?, ?, ?)`, [ + 'old', 'old', '2026-03-20 12:34:56', + ]); + await legacy.disconnect(); + + driver = new SqlDriver({ client: 'mysql2', connection: URL }); + }); + + afterEach(async () => { + await driver.execute(`drop table if exists ${LEGACY}`).catch(() => {}); + await driver.disconnect(); + }); + + it('widens the columns at schema sync without moving any instant', async () => { + const before = await storedEpochMs(driver, 'old', LEGACY); + expect((await columnTypes(driver, LEGACY)).at).toBe('timestamp'); + + await driver.initObjects([ + { name: LEGACY, fields: { label: { type: 'string' }, at: { type: 'datetime' } } }, + ]); + + const cols = await columnTypes(driver, LEGACY); + expect(cols.at).toBe('datetime(3)'); + expect(cols.created_at).toBe('datetime(3)'); + expect(cols.updated_at).toBe('datetime(3)'); + // The instant is what must not move. A migration that silently reinterprets + // stored data is worse than one that never runs. + expect(await storedEpochMs(driver, 'old', LEGACY)).toBe(before); + expect(before).toBe(Date.parse(GOOD)); + }); + + it('lets the migrated column hold what TIMESTAMP could not', async () => { + await driver.initObjects([ + { name: LEGACY, fields: { label: { type: 'string' }, at: { type: 'datetime' } } }, + ]); + await driver.create(LEGACY, { id: 'future', label: 'f', at: '2040-06-01T00:00:00.000Z' }, { bypassTenantAudit: true }); + expect(await readInstant(driver, 'future', LEGACY)).toBe('2040-06-01T00:00:00.000Z'); + }); + + it('is idempotent — a second sync leaves the schema alone', async () => { + const shape = { name: LEGACY, fields: { label: { type: 'string' }, at: { type: 'datetime' } } }; + await driver.initObjects([shape]); + const first = await columnTypes(driver, LEGACY); + await driver.initObjects([shape]); + expect(await columnTypes(driver, LEGACY)).toEqual(first); + }); + + it('keeps the audit default, so inserts do not start writing NULL', async () => { + await driver.initObjects([ + { name: LEGACY, fields: { label: { type: 'string' }, at: { type: 'datetime' } } }, + ]); + await driver.create(LEGACY, { id: 'fresh', label: 'f' }, { bypassTenantAudit: true }); + const row: any = await driver.findOne(LEGACY, 'fresh', { bypassTenantAudit: true }); + expect(row.created_at ?? null, 'created_at must still default').not.toBeNull(); + }); +}); + +// ── helpers ───────────────────────────────────────────────────────────────── + +/** mysql2 hands back `[rows, fields]`; normalise to just the rows. */ +async function rowsOf(driver: SqlDriver, sql: string, bindings: unknown[] = []): Promise { + const res: any = await driver.execute(sql, bindings as any); + if (Array.isArray(res) && Array.isArray(res[0])) return res[0]; + return Array.isArray(res) ? res : (res?.rows ?? []); +} + +/** column → its full MySQL type, lower-cased and case-insensitive on the key. */ +async function columnTypes(driver: SqlDriver, table: string): Promise> { + const rows = await rowsOf( + driver, + `select column_name, column_type from information_schema.columns + where table_schema = database() and table_name = ?`, + [table], + ); + const out: Record = {}; + for (const r of rows as any[]) { + out[String(r.COLUMN_NAME ?? r.column_name)] = String(r.COLUMN_TYPE ?? r.column_type).toLowerCase(); + } + return out; +} + +/** + * The instant the driver presents for a row, as canonical ISO. The user-facing + * contract, and the only check that works outside 1970..2038 — `unix_timestamp()` + * is itself a 32-bit epoch function and returns 0 there. + */ +async function readInstant(driver: SqlDriver, id: string, table = TABLE): Promise { + const row: any = await driver.findOne(table, id, { bypassTenantAudit: true }); + return new Date(row.at).toISOString(); +} + +/** + * The stored instant as epoch ms, read under an explicitly-UTC session so the + * assertion cannot be satisfied by a compensating session-timezone error. + */ +async function storedEpochMs(driver: SqlDriver, id: string, table = TABLE): Promise { + await driver.execute(`set time_zone = '+00:00'`); + const rows = await rowsOf( + driver, + `select cast(unix_timestamp(at) * 1000 as signed) as ms from ${table} where id = ?`, + [id], + ); + return Number((rows[0] as any).ms); +} diff --git a/packages/plugins/driver-sql/src/sql-driver-temporal-dialect.test.ts b/packages/plugins/driver-sql/src/sql-driver-temporal-dialect.test.ts index 23b4369e95..ec7a941d6a 100644 --- a/packages/plugins/driver-sql/src/sql-driver-temporal-dialect.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-temporal-dialect.test.ts @@ -47,20 +47,45 @@ function makeDriver(client: string): ProbeDriver { const ISO = '2025-06-18'; const CANONICAL = '2025-06-18T00:00:00.000Z'; +/** The same UTC wall clock, spelled the only way MySQL parses it (#3942). */ +const MYSQL_LITERAL = '2025-06-18 00:00:00.000'; + +/** What each dialect physically binds for one canonical instant. */ +const PHYSICAL: Record = { + 'better-sqlite3': CANONICAL, + pg: CANONICAL, + mysql2: MYSQL_LITERAL, +}; describe('temporalFilterValue dialect gating', () => { - it('every dialect canonicalises a bare calendar day to UTC midnight (#3912)', () => { - // The uniformity IS the contract now. On Postgres this is the fix for the - // measured cross-dialect divergence: an un-anchored '2025-06-18' was read as - // midnight in the server's timezone, so a window put the same instant on a - // different day than SQLite did. - for (const client of ['better-sqlite3', 'pg', 'mysql2']) { + it('every dialect anchors a bare calendar day to UTC midnight (#3912)', () => { + // The SEMANTICS are uniform — one instant, midnight UTC — even though MySQL + // spells it differently. On Postgres this is the fix for the measured + // cross-dialect divergence: an un-anchored '2025-06-18' was read as midnight + // in the server's timezone, so a window put the same instant on a different + // day than SQLite did. + for (const [client, expected] of Object.entries(PHYSICAL)) { const d = makeDriver(client); d.seedDatetime('t', 'at'); - expect(d.temporalFilterValue('t', 'at', ISO)).toBe(CANONICAL); + expect(d.temporalFilterValue('t', 'at', ISO), client).toBe(expected); } }); + it('MySQL gets the same instant in a MySQL-parseable literal (#3942)', () => { + // MySQL rejects both the `T` separator and the `Z` suffix — an ISO-8601 + // comparand fails the statement outright with *Incorrect datetime value* + // (measured on MariaDB 10.11), so the canonical form has to be respelled for + // the bind. It is the SAME UTC wall clock: the column is `DATETIME(3)`, which + // does no timezone conversion, and the connection is pinned to UTC. + const d = makeDriver('mysql2'); + d.seedDatetime('t', 'at'); + expect(d.temporalFilterValue('t', 'at', ISO)).toBe(MYSQL_LITERAL); + expect(d.temporalFilterValue('t', 'at', '2025-06-18T08:00:00+08:00')).toBe(MYSQL_LITERAL); + expect(d.temporalFilterValue('t', 'at', new Date(CANONICAL))).toBe(MYSQL_LITERAL); + // Never ISO — that is precisely what MySQL cannot parse. + expect(String(d.temporalFilterValue('t', 'at', CANONICAL))).not.toMatch(/[TZ]/); + }); + it('never binds an epoch integer — that would break a native TIMESTAMP compare', () => { for (const client of ['better-sqlite3', 'pg', 'mysql2']) { const d = makeDriver(client); diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index 7b1818a7a8..dea3ef3f16 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -270,6 +270,35 @@ function canonicalUtcDatetime(value: unknown): unknown { return Number.isFinite(ms) ? new Date(ms).toISOString() : value; } +/** + * The canonical instant rendered as a MySQL datetime literal — the same UTC wall + * clock, spelled the only way MySQL will parse it (#3942). + * + * MySQL/MariaDB reject the ISO-8601 the rest of the platform speaks: neither the + * `T` separator nor the `Z` suffix is accepted in a datetime literal, so + * `'2026-03-20T12:34:56.789Z'` fails the INSERT outright with *Incorrect datetime + * value* (measured on MariaDB 10.11). MySQL 8.0.19+ added `±HH:MM` offsets, but + * still not `Z`, and the platform supports older servers — so the offset is + * dropped and the value is stored as the UTC wall clock in a `DATETIME(3)` + * column, which does no timezone conversion of its own. + * + * This is a PHYSICAL spelling, not a semantic change: the column still holds the + * same instant, and every layer above the bind — API payloads, filter authoring, + * CEL — keeps the canonical `…Z` form. Reads convert back (the connection is + * pinned to UTC, so mysql2 reconstructs the instant correctly). + * + * Total, and deliberately strict: only an exactly-canonical string is rewritten. + * Anything else — an unparseable value `canonicalUtcDatetime` passed through, or + * a year outside MySQL's 1000..9999 range, which `toISOString` renders in + * expanded `+0YYYYY` form — is handed to MySQL untouched, so it fails loudly + * rather than being silently reinterpreted. + */ +function mysqlDatetimeLiteral(canonical: unknown): unknown { + if (typeof canonical !== 'string') return canonical; + const m = /^(\d{4}-\d{2}-\d{2})T(\d{2}:\d{2}:\d{2}\.\d{3})Z$/.exec(canonical); + return m ? `${m[1]} ${m[2]}` : canonical; +} + // ── Introspection Types ────────────────────────────────────────────────────── export interface IntrospectedColumn { @@ -742,7 +771,59 @@ export class SqlDriver implements IDataDriver { } // A function-valued `connection` (knex's per-acquire provider) is left // alone: the host is building each connection itself and owns its timeouts. - return bounded; + return SqlDriver.withUtcSession(bounded); + } + + /** + * Pin a MySQL connection to UTC, in both directions (#3942). + * + * MySQL is the one supported dialect where the SESSION timezone participates + * in what a datetime means, on two independent layers, and both default to + * something machine-dependent: + * + * - **mysql2** (`connection.timezone`, default `'local'`) decides how a bound + * JS `Date` is rendered and how a returned `DATETIME` string is parsed back. + * Left at `'local'`, two app servers in different zones write the same + * instant as different values, and read the same row as different instants. + * - **The server** (`@@session.time_zone`, default `SYSTEM`) decides how a + * zone-naive literal is interpreted for a `TIMESTAMP` column, and what + * `CURRENT_TIMESTAMP` renders. Measured at 8 hours off on a server + * configured `+08:00`. + * + * Setting both to UTC makes the wall clock the driver writes *be* the instant, + * which is what {@link mysqlDatetimeLiteral} relies on — and it keeps legacy + * `TIMESTAMP` columns correct too, so a deployment stays right whether or not + * the `DATETIME(3)` migration has run. + * + * A host that set either one explicitly is left alone; an existing + * `pool.afterCreate` is chained rather than replaced, since it is a documented + * knex extension point the host may already be using. + */ + private static withUtcSession(knexConfig: Record): Record { + const client = String(knexConfig.client ?? ''); + if (client !== 'mysql' && client !== 'mysql2') return knexConfig; + + const out: Record = { ...knexConfig }; + const conn = out.connection; + if (conn && typeof conn === 'object' && (conn as any).timezone === undefined) { + out.connection = { ...(conn as object), timezone: 'Z' }; + } + + const pool = (out.pool ?? {}) as Record; + const hostAfterCreate = pool.afterCreate as + | ((conn: unknown, done: (err?: unknown) => void) => void) + | undefined; + out.pool = { + ...pool, + afterCreate(connection: any, done: (err?: unknown, conn?: unknown) => void) { + connection.query(`SET time_zone = '+00:00'`, (err: unknown) => { + if (err) return done(err); + if (!hostAfterCreate) return done(undefined, connection); + hostAfterCreate(connection, (hostErr?: unknown) => done(hostErr, connection)); + }); + }, + }; + return out; } /** @@ -2154,8 +2235,8 @@ export class SqlDriver implements IDataDriver { if (!exists) { await this.knex.schema.createTable(shardName, (table) => { table.string('id').primary(); - table.timestamp('created_at').defaultTo(this.knex.fn.now()); - table.timestamp('updated_at').defaultTo(this.knex.fn.now()); + this.createAuditTimestampColumn(table, 'created_at'); + this.createAuditTimestampColumn(table, 'updated_at'); for (const [name, field] of Object.entries(obj.fields ?? {})) { if (builtinColumns.has(name)) continue; this.createColumn(table, name, field); @@ -2470,8 +2551,8 @@ export class SqlDriver implements IDataDriver { if (!exists) { await this.knex.schema.createTable(tableName, (table) => { table.string('id').primary(); - table.timestamp('created_at').defaultTo(this.knex.fn.now()); - table.timestamp('updated_at').defaultTo(this.knex.fn.now()); + this.createAuditTimestampColumn(table, 'created_at'); + this.createAuditTimestampColumn(table, 'updated_at'); if (obj.fields) { for (const [name, field] of Object.entries(obj.fields)) { if (builtinColumns.has(name)) continue; @@ -2538,6 +2619,8 @@ export class SqlDriver implements IDataDriver { // UTC-text storage form. A table this call just CREATED has no rows, so it // is canonical by construction — record that without touching the disk. await this.backfillCanonicalDatetimes(tableName, exists); + // #3942: the MySQL twin — widen legacy `TIMESTAMP` columns to `DATETIME(3)`. + if (exists) await this.migrateMysqlDatetimeColumns(tableName, obj.fields ?? {}); } // Pre-create the auto_number counter table now, while we hold a fresh pooled @@ -2630,6 +2713,80 @@ export class SqlDriver implements IDataDriver { } } + /** + * Widen a table's legacy MySQL `TIMESTAMP` datetime columns to `DATETIME(3)` + * (#3942) — the MySQL counterpart of {@link backfillCanonicalDatetimes}. + * + * `TIMESTAMP` cannot hold an instant past 2038-01-19 or before 1970, keeps no + * milliseconds, and converts using the session timezone. `ALTER … MODIFY` moves + * the column to `DATETIME(3)`, which has none of those properties. The instants + * survive because the connection pins `@@session.time_zone` to UTC + * ({@link withUtcSession}): MySQL renders each `TIMESTAMP` in the session zone + * to produce the `DATETIME` wall clock, so UTC in gives the UTC wall clock the + * driver's own writes use. + * + * Only the audit columns and declared `Field.datetime` columns are touched, and + * only when they are still `timestamp` — so this is idempotent and re-running + * costs one `information_schema` lookup. + * + * Failures are logged and swallowed, like the SQLite backfill and for the same + * reason: a `TIMESTAMP` column keeps working (the driver binds the same UTC + * wall-clock literal either way, and the session is pinned to UTC), it merely + * keeps the range and precision limits. Correctness must not depend on a + * migration having run, and a migration must never take boot down. + */ + protected async migrateMysqlDatetimeColumns( + table: string, + fields: Record, + ): Promise { + if (!this.isMysql) return; + const candidates = new Set(AUDIT_TIMESTAMP_COLUMNS); + for (const [name, field] of Object.entries(fields)) { + if ((field?.type ?? 'string') === 'datetime' && !field?.multiple) candidates.add(name); + } + if (candidates.size === 0) return; + + try { + const res: any = await this.knex.raw( + `select column_name, is_nullable from information_schema.columns + where table_schema = database() and table_name = ? and data_type = 'timestamp'`, + [table], + ); + // mysql2 returns [rows, fields]; column names vary in case by server. + const rows: any[] = Array.isArray(res?.[0]) ? res[0] : (res?.rows ?? res ?? []); + const legacy = rows + .map((r) => ({ + name: String(r.COLUMN_NAME ?? r.column_name ?? ''), + nullable: String(r.IS_NULLABLE ?? r.is_nullable ?? 'YES').toUpperCase() !== 'NO', + })) + .filter((c) => c.name && candidates.has(c.name)); + if (legacy.length === 0) return; + + for (const col of legacy) { + // The default is re-stated because MySQL drops a column's DEFAULT when + // MODIFY does not repeat it, and an audit column without + // `CURRENT_TIMESTAMP(3)` would start inserting NULL. + const isAudit = (AUDIT_TIMESTAMP_COLUMNS as readonly string[]).includes(col.name); + const nullClause = col.nullable ? 'null' : 'not null'; + const defaultClause = isAudit ? ' default current_timestamp(3)' : ''; + await this.knex.raw( + `alter table ?? modify column ?? datetime(3) ${nullClause}${defaultClause}`, + [table, col.name], + ); + } + this.logger.info?.( + `[sql-driver] widened MySQL TIMESTAMP → DATETIME(3) (#3942) on ${table}`, + { columns: legacy.map((c) => c.name) }, + ); + } catch (err) { + this.logger.warn( + `[sql-driver] could not widen MySQL datetime columns on ${table}; ` + + `writes stay correct, but the 2038 ceiling and millisecond truncation remain`, + { error: err instanceof Error ? err.message : String(err) }, + ); + } + } + // ── Managed-schema drift & reconcile (#2186) ─────────────────────────────── /** Canonical dialect name for the drift differ. */ @@ -3619,7 +3776,7 @@ export class SqlDriver implements IDataDriver { const kind = this.temporalFieldKind(table, field); if (!kind) return value; - return kind === 'datetime' ? canonicalUtcDatetime(value) : this.toDateOnly(value); + return kind === 'datetime' ? this.storageDatetimeValue(value) : this.toDateOnly(value); } /** @@ -3645,6 +3802,21 @@ export class SqlDriver implements IDataDriver { * That last exit is what makes the convention pay off rather than just move the * cost around: after migration the emitted SQL is a plain `col >= ?` again. */ + /** + * The physical form a `Field.datetime` value takes on THIS dialect — what is + * actually bound, on both the write path (`formatInput`) and the filter path + * (`coerceFilterValue`), so the two can never disagree. + * + * The logical canon is {@link canonicalUtcDatetime}'s `…Z` string everywhere. + * MySQL is the one dialect that cannot parse it, so it gets the same instant + * spelled as a MySQL literal (see {@link mysqlDatetimeLiteral}); SQLite stores + * the canonical string as-is and Postgres parses it straight into `timestamptz`. + */ + protected storageDatetimeValue(value: unknown): unknown { + const canonical = canonicalUtcDatetime(value); + return this.isMysql ? mysqlDatetimeLiteral(canonical) : canonical; + } + protected needsLegacyDatetimeRepair(table: string | null | undefined, field: string): boolean { if (!table || !this.isSqlite) return false; if (this.datetimeFields[table]?.has(field) !== true) return false; @@ -4416,6 +4588,25 @@ export class SqlDriver implements IDataDriver { } } + /** + * DDL for a builtin `created_at` / `updated_at` audit column. + * + * These are created directly rather than through {@link createColumn} (they are + * not declared fields), but the objectql registry DOES declare them as + * `Field.datetime` on every audited object — so they are filtered, sorted and + * bucketed like one, and must take the same physical type or they inherit the + * `TIMESTAMP` problems on MySQL: no milliseconds, and a 2038 ceiling on the + * column every list view sorts by (#3942). `CURRENT_TIMESTAMP` has to carry + * matching precision for a `DATETIME(3)` default, hence `now(3)`. + */ + protected createAuditTimestampColumn(table: Knex.CreateTableBuilder, name: string): void { + if (this.isMysql) { + table.datetime(name, { precision: 3 }).defaultTo(this.knex.fn.now(3)); + return; + } + table.timestamp(name).defaultTo(this.knex.fn.now()); + } + protected createColumn(table: Knex.CreateTableBuilder, name: string, field: any) { if (field.multiple) { table.json(name); @@ -4469,7 +4660,18 @@ export class SqlDriver implements IDataDriver { col = table.date(name); break; case 'datetime': - col = table.timestamp(name); + // MySQL's `TIMESTAMP` is a 32-bit epoch: it cannot represent an instant + // outside 1970-01-01..2038-01-19 (a contract end date, a subscription + // expiry, a retention horizon — all rejected outright), it carries no + // fractional seconds by default so the canonical form's milliseconds are + // silently truncated, and it CONVERTS on read/write using the session + // timezone, which makes the stored instant depend on server config. + // `DATETIME(3)` has none of those properties: 1000..9999, milliseconds + // kept, and stored verbatim — so the column holds the UTC wall clock the + // driver writes, exactly as ServiceNow stores its MySQL timestamps + // (#3942). Postgres deliberately keeps `table.timestamp` → `timestamptz`: + // asking for precision 3 there would REDUCE it from microseconds. + col = this.isMysql ? table.datetime(name, { precision: 3 }) : table.timestamp(name); break; case 'time': col = table.time(name); @@ -4675,7 +4877,7 @@ export class SqlDriver implements IDataDriver { if (v == null) continue; // `NOW()` was already replaced with an ISO instant above; anything else // that is not interpretable as a time passes through untouched. - const normalized = canonicalUtcDatetime(v); + const normalized = this.storageDatetimeValue(v); if (normalized !== v) { if (!copied) { copy = { ...copy }; copied = true; } copy[field] = normalized; From 55d21f972223ec744dd079c60306c305f8abd701 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 12:45:18 +0000 Subject: [PATCH 4/5] docs(driver-sql): add the changeset, and correct two stale datetime storage claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Check Changeset` was red — the release notes need a changeset for the driver-sql / service-analytics / spec changes on this branch. Two hand-written docs asserted the storage form this branch replaces, so they are corrected here rather than left to the drift-check advisory: - `date-macros.mdx` said the driver translates a comparand into "SQLite epoch-ms"; it now names the real per-dialect form. - `webhooks.mdx` described `created_at`/`updated_at` as a "Native TIMESTAMP column", which was already loose on SQLite and is wrong on MySQL now that they are DATETIME(3). Restated as the contract — a UTC instant — which is what the row was contrasting against the adjacent epoch-ms number columns anyway. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TPxNwPnjcn599ujXpU3ibJ --- .changeset/datetime-canonical-utc-storage.md | 56 ++++++++++++++++++++ content/docs/automation/webhooks.mdx | 4 +- content/docs/references/data/date-macros.mdx | 8 ++- 3 files changed, 64 insertions(+), 4 deletions(-) create mode 100644 .changeset/datetime-canonical-utc-storage.md diff --git a/.changeset/datetime-canonical-utc-storage.md b/.changeset/datetime-canonical-utc-storage.md new file mode 100644 index 0000000000..85f8fdb3b6 --- /dev/null +++ b/.changeset/datetime-canonical-utc-storage.md @@ -0,0 +1,56 @@ +--- +"@objectstack/driver-sql": minor +"@objectstack/service-analytics": patch +"@objectstack/spec": patch +--- + +fix(driver-sql): give `Field.datetime` one UTC storage form per dialect (#3912, #3942) + +Any window filter on a `Field.datetime` column returned an empty set on SQLite — +a dashboard `dateRange: last_30_days` on `created_date` read 0 while 29 matching +rows existed. + +There was never a storage *convention*, only a description of what better-sqlite3 +happened to do with a bound JS `Date`. Nothing enforced it — `formatInput` +deliberately left `datetime` untouched — so the form was decided by whichever +writer got there first: a JS `Date` landed as INTEGER epoch ms, while a REST/JSON +write (JSON has no `Date` type), a `defaultValue: 'NOW()'` slot, and the +platform's own `created_at` / `updated_at` all landed as ISO **TEXT**. One column +held both forms while the read path coerced comparands to epoch ms purely from +the *declared* type. On SQLite's type ordering (`INTEGER < TEXT`) a two-sided +window collapsed to zero rows, and a one-sided `>=` matched every TEXT row +regardless of the bound. + +`Field.datetime` now has one canonical instant per dialect, produced by one +function applied on write **and** to every filter comparand, so the two sides of +a comparison cannot disagree about shape: + +- **SQLite** — `YYYY-MM-DDTHH:MM:SS.sssZ` text. Lexicographic order *is* + chronological order, so range filters and `ORDER BY` read the column directly + and can use an index; `strftime` parses it, so the date-bucket expression needs + no CASE. +- **Postgres** — `timestamptz`, unchanged. The fix here is on the write and + comparand side: a zone-naive write was previously resolved against the + *server's* timezone (measured 8 hours off on `Asia/Shanghai`), and an + un-anchored `YYYY-MM-DD` comparand meant the server's local midnight, so the + identical query over the identical instant landed a row on a different calendar + day than SQLite did. +- **MySQL** — `DATETIME(3)` instead of `TIMESTAMP`, a connection pinned to UTC on + both the mysql2 and the server layer, and a MySQL-spelled bind carrying the + same UTC wall clock. MySQL accepts neither the `T` separator nor the `Z` suffix + in a datetime literal, so datetime writes over REST had always failed outright; + `TIMESTAMP` additionally truncated milliseconds and could not store an instant + outside 1970..2038. + +Existing rows converge at schema sync. Both migrations are allowed to fail: they +log, mark nothing, and the read paths keep a repair expression, so an un-migrated +column still compares and buckets **correctly** — just unindexed. Neither can +repair instants the old timezone-ambiguous write path recorded wrongly; they +preserve what is on disk. + +Also closes #3928 (datetime `ORDER BY` mis-sorted on mixed storage) by +construction. Rationale is recorded as ADR-0053 addendum D-B1..D-B4. + +The analytics change is additive: a `coerceTemporalFilterColumn` companion to the +existing `coerceTemporalFilterValue` hook, so a raw-SQL strategy can normalise the +column side too. Absent hook → byte-identical SQL. diff --git a/content/docs/automation/webhooks.mdx b/content/docs/automation/webhooks.mdx index 6548cde80b..2d83fc5ba7 100644 --- a/content/docs/automation/webhooks.mdx +++ b/content/docs/automation/webhooks.mdx @@ -148,8 +148,8 @@ writable. | `response_code` | number | Last HTTP status code received. | | `response_body` | textarea | Truncated to the first 16 KB. | | `error` | textarea | Last transport-level error (DNS, connect, timeout). | -| `created_at` | datetime | Native TIMESTAMP column — written as a `Date`, not an epoch-ms number. | -| `updated_at` | datetime | Native TIMESTAMP column — written as a `Date`, not an epoch-ms number. | +| `created_at` | datetime | A `Field.datetime` UTC instant — not an epoch-ms number like the columns above. | +| `updated_at` | datetime | A `Field.datetime` UTC instant — not an epoch-ms number like the columns above. | > **Why store full payload?** Receivers may be down for hours; we must > retry the *exact* bytes we promised to send. Recomputing payload from diff --git a/content/docs/references/data/date-macros.mdx b/content/docs/references/data/date-macros.mdx index 938aeb2506..fe996f91f8 100644 --- a/content/docs/references/data/date-macros.mdx +++ b/content/docs/references/data/date-macros.mdx @@ -53,9 +53,13 @@ Either way the DRIVER only ever sees ISO date / timestamp strings, never `\{tokens\}`. Translating an ISO comparand into a column's on-disk -form (SQLite epoch-ms, `YYYY-MM-DD` text, native timestamp) is the +form — canonical UTC text on SQLite, a native `timestamptz` on Postgres, -driver's job — see `SqlDriver.temporalFilterValue`. +a `DATETIME(3)` literal on MySQL, and `YYYY-MM-DD` text for a calendar + +day on every dialect — is the driver's job; see + +`SqlDriver.temporalFilterValue`. A token OUTSIDE this vocabulary is rejected rather than passed From b1fc8324c04be73ddf7405630644c3d32a4c8a84 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 12:59:21 +0000 Subject: [PATCH 5/5] fix(spec): correct the datetime storage note at its source, not in the generated doc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `TypeScript Type Check` was red on the generated-docs gate: my previous commit hand-edited `content/docs/references/data/date-macros.mdx`, which is GENERATED from `packages/spec/src/data/date-macros.zod.ts` and committed — so the check regenerated it and found a difference. Reverted the hand-edit, corrected the doc comment in the Zod source (it still said the driver translates a comparand into "SQLite epoch-ms"), and regenerated. The `webhooks.mdx` edit in the previous commit stands — that one is hand-written. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TPxNwPnjcn599ujXpU3ibJ --- content/docs/references/data/date-macros.mdx | 6 +++--- packages/spec/src/data/date-macros.zod.ts | 6 ++++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/content/docs/references/data/date-macros.mdx b/content/docs/references/data/date-macros.mdx index fe996f91f8..547991e5c8 100644 --- a/content/docs/references/data/date-macros.mdx +++ b/content/docs/references/data/date-macros.mdx @@ -53,11 +53,11 @@ Either way the DRIVER only ever sees ISO date / timestamp strings, never `\{tokens\}`. Translating an ISO comparand into a column's on-disk -form — canonical UTC text on SQLite, a native `timestamptz` on Postgres, +form — canonical UTC text on SQLite, a native `timestamptz` on -a `DATETIME(3)` literal on MySQL, and `YYYY-MM-DD` text for a calendar +Postgres, a `DATETIME(3)` literal on MySQL, and `YYYY-MM-DD` text for -day on every dialect — is the driver's job; see +a calendar day on every dialect — is the driver's job; see `SqlDriver.temporalFilterValue`. diff --git a/packages/spec/src/data/date-macros.zod.ts b/packages/spec/src/data/date-macros.zod.ts index 5f8e9c3f08..35ecef1bb5 100644 --- a/packages/spec/src/data/date-macros.zod.ts +++ b/packages/spec/src/data/date-macros.zod.ts @@ -33,8 +33,10 @@ import { z } from 'zod'; * * Either way the DRIVER only ever sees ISO date / timestamp strings, * never `{tokens}`. Translating an ISO comparand into a column's on-disk - * form (SQLite epoch-ms, `YYYY-MM-DD` text, native timestamp) is the - * driver's job — see `SqlDriver.temporalFilterValue`. + * form — canonical UTC text on SQLite, a native `timestamptz` on + * Postgres, a `DATETIME(3)` literal on MySQL, and `YYYY-MM-DD` text for + * a calendar day on every dialect — is the driver's job; see + * `SqlDriver.temporalFilterValue`. * * A token OUTSIDE this vocabulary is rejected rather than passed * through: `@objectstack/lint`'s `validate-filter-tokens` fails the