From 1b054d6ead9491d788c77c61e54b8e617f32ed9f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 00:46:38 +0000 Subject: [PATCH 1/2] feat(spec,ci): temporal hooks onto the IDataDriver contract; conformance job with live non-UTC servers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the two debts the datetime storage work (#3912/#3942/#3954) left open in ADR-0053. D-A2 — contract promotion. temporalFilterValue and temporalFilterColumnSql were duck-typed: analytics probed `typeof driver.x === 'function'` against a locally-invented interface, and nothing at the type level said a driver must implement both or neither. #3912's lesson is precisely that coercing the comparand without normalising the column reintroduces half the bug, so a driver implementing one hook alone would silently regress. Both are now optional members of IDataDriver, documented as a PAIR with "absent = identity" semantics for drivers whose storage form is the wire form. SqlDriver `implements IDataDriver`, so its signatures are compile-checked from here on; analytics Picks the contract instead of inventing a local shape, keeping the runtime typeof guards as the correct way to consume an optional member. coerceFilterValueForSql already sits behind the hook as the last-resort boolean/number recovery — the demotion D-A2 asked for — and stays in exactly that role. ADR-0053 records the resolution. D-A3 — the conformance backstop. The live-server suites from #3912/#3942 are opt-in (OS_TEST_POSTGRES_URL / OS_TEST_MYSQL_URL) and skip without a server, so nothing ran them in CI and the seam could regress silently. New `temporal-conformance` job: postgres:16 and mysql:8.0 service containers, servers pointed at +08:00 post-start (services cannot override the image command), the Node process at America/New_York, and assertions in UTC — the three-way skew that caught every bug in this family, with both suites asserting a non-UTC server so a mis-provisioned service fails loudly rather than passing vacuously. The WHOLE driver-sql suite runs under the skewed zone, so a TZ-sensitive assumption anywhere in the driver's tests fails here before it ships. mysql:8.0 also covers the other half of the #3942 compatibility claim — the hands-on verification ran on MariaDB 10.11, the stricter dialect. Verified locally with the exact CI command against live PG 16 (Asia/Shanghai) and MariaDB (+08:00): 47 files, 480/480, zero skips. Full pnpm build + test green (132 tasks); spec doc gates green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TPxNwPnjcn599ujXpU3ibJ --- .changeset/temporal-hooks-on-contract.md | 25 ++++ .github/workflows/ci.yml | 120 ++++++++++++++++++ docs/adr/0053-date-and-datetime-semantics.md | 23 +++- .../services/service-analytics/src/plugin.ts | 40 +++--- packages/spec/src/contracts/data-driver.ts | 41 ++++++ 5 files changed, 223 insertions(+), 26 deletions(-) create mode 100644 .changeset/temporal-hooks-on-contract.md diff --git a/.changeset/temporal-hooks-on-contract.md b/.changeset/temporal-hooks-on-contract.md new file mode 100644 index 0000000000..4a8992c485 --- /dev/null +++ b/.changeset/temporal-hooks-on-contract.md @@ -0,0 +1,25 @@ +--- +"@objectstack/spec": minor +"@objectstack/service-analytics": patch +--- + +feat(spec): promote the temporal storage hooks onto the IDataDriver contract (ADR-0053 D-A2) + +`temporalFilterValue` and `temporalFilterColumnSql` — the pair that closed +#3912's storage-form drift — were duck-typed: analytics probed +`typeof driver.x === 'function'` against a locally-invented interface, and +nothing at the type level said a driver must implement both or neither. The +lesson of #3912 is precisely that coercing the comparand without normalising +the column reintroduces half the bug, so a driver implementing one hook alone +would silently regress. + +Both are now optional members of `IDataDriver` +(`@objectstack/spec/contracts`), documented as a pair with "absent = identity" +semantics for drivers whose storage form is the wire form (memory, mongo). +`SqlDriver implements IDataDriver`, so its signatures are compile-checked from +here on; analytics derives its driver seam by `Pick`-ing the contract instead +of a local duck type. Runtime `typeof` guards remain — that is the correct way +to consume an optional contract member — but the shape they guard now has one +authoritative definition. + +No runtime behaviour change. ADR-0053 D-A2 is recorded as resolved. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8edab5b992..d688e7592d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -196,6 +196,126 @@ jobs: path: .turbo/cache key: ${{ runner.os }}-turbo-${{ github.job }}-${{ github.ref_name }}-${{ github.sha }} + + # ── Temporal conformance against live, non-UTC servers (ADR-0053 D-A3) ───── + # + # The datetime storage work (#3912/#3942) was verified against real servers + # because every one of its bugs was invisible on all-UTC infrastructure: a + # zone-naive write resolved in the SERVER's zone on Postgres, mysql2 rendered + # a Date in the HOST's zone, and a bare YYYY-MM-DD comparand meant a + # different midnight per dialect. The committed suites are opt-in + # (OS_TEST_POSTGRES_URL / OS_TEST_MYSQL_URL) and skip without a server, so + # without this job they would never run in CI and the seam could regress + # silently — D-A3's exact concern. + # + # Every timezone here is deliberately DIFFERENT: servers at +08:00, the Node + # process at America/New_York, assertions in UTC. Both suites assert they + # are pointed at a non-UTC server, so a mis-provisioned service fails loudly + # instead of letting the job pass vacuously. + temporal-conformance: + name: Temporal Conformance (live PG + MySQL) + needs: filter + if: needs.filter.outputs.core == 'true' + runs-on: ubuntu-latest + permissions: + contents: read + + services: + postgres: + image: postgres:16 + env: + POSTGRES_PASSWORD: postgres + ports: + - 5432:5432 + options: >- + --health-cmd="pg_isready -U postgres" + --health-interval=5s + --health-timeout=5s + --health-retries=12 + mysql: + # Real MySQL 8.0. The hands-on verification of #3942 ran on MariaDB + # 10.11 — the stricter dialect for datetime literals — so this job is + # the other half of the compatibility claim. `-h 127.0.0.1` forces the + # ping over TCP: the image's init phase runs mysqld with networking + # disabled, so a socket ping would report healthy before init finishes. + image: mysql:8.0 + env: + MYSQL_ROOT_PASSWORD: root + MYSQL_DATABASE: conformance + ports: + - 3306:3306 + options: >- + --health-cmd="mysqladmin ping -h 127.0.0.1 -uroot -proot" + --health-interval=5s + --health-timeout=5s + --health-retries=24 + + steps: + - name: Checkout repository + uses: actions/checkout@v7 + + # Service containers cannot override the image command, so the non-UTC + # zones are set post-start through each server's own mechanism. Echoed + # back so a provisioning failure is visible in the log — though the + # suites' own non-UTC guards are the real gate. + - name: Point both servers at a non-UTC timezone + run: | + docker exec ${{ job.services.postgres.id }} psql -U postgres -c "ALTER SYSTEM SET timezone='Asia/Shanghai'" + docker exec ${{ job.services.postgres.id }} psql -U postgres -c "SELECT pg_reload_conf()" + docker exec ${{ job.services.postgres.id }} psql -U postgres -tAc "SHOW timezone" + docker exec ${{ job.services.mysql.id }} mysql -uroot -proot -e "SET GLOBAL time_zone = '+08:00'" + docker exec ${{ job.services.mysql.id }} mysql -uroot -proot -N -e "SELECT @@global.time_zone" + + - name: Setup Node.js + uses: actions/setup-node@v7 + with: + node-version: '22' + + - name: Enable Corepack + run: corepack enable + + - name: Get pnpm store directory + shell: bash + run: | + echo "STORE_PATH=$(pnpm store path --silent)" >> $GITHUB_ENV + + - name: Setup pnpm cache + uses: actions/cache@v6 + with: + path: ${{ env.STORE_PATH }} + key: ${{ runner.os }}-pnpm-store-v3-${{ hashFiles('**/pnpm-lock.yaml') }} + restore-keys: | + ${{ runner.os }}-pnpm-store-v3- + + # Restore-only (same policy as every other job); falls back to the Test + # Core namespace because that job builds a superset of what this one + # needs and its cache is seeded from main. + - name: Restore Turbo cache + uses: actions/cache/restore@v6 + with: + path: .turbo/cache + key: ${{ runner.os }}-turbo-${{ github.job }}-${{ github.ref_name }}-${{ github.sha }} + restore-keys: | + ${{ runner.os }}-turbo-${{ github.job }}- + ${{ runner.os }}-turbo-test-${{ github.ref_name }}- + ${{ runner.os }}-turbo-test- + + - name: Install dependencies + run: pnpm install --frozen-lockfile + + - name: Build driver-sql and its dependencies + run: pnpm exec turbo run build --filter=@objectstack/driver-sql... --concurrency=4 + + # The whole driver-sql suite runs under the skewed process zone — not + # just the live-server files — so a TZ-sensitive assumption anywhere in + # the driver's tests fails here before it can ship. + - name: Run driver-sql suite against both live servers + env: + TZ: America/New_York + OS_TEST_POSTGRES_URL: postgres://postgres:postgres@127.0.0.1:5432/postgres + OS_TEST_MYSQL_URL: mysql://root:root@127.0.0.1:3306/conformance + run: pnpm --filter @objectstack/driver-sql test + dogfood: # Sharded 2-way: the suite is ~60 independent test files, each booting its # own in-process app, and a single 4-vCPU runner needed ~7½ minutes for the diff --git a/docs/adr/0053-date-and-datetime-semantics.md b/docs/adr/0053-date-and-datetime-semantics.md index b5ab1d8358..7c0a22c5e9 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..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). +**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 resolved 2026-07-30: `temporalFilterValue` + `temporalFilterColumnSql` are optional `IDataDriver` contract members with identity semantics, and analytics types its driver seam from the contract. **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`. @@ -387,6 +387,19 @@ that is not on the driver contract. Promote it to a first-class retire it)** once the contract method is universal. (If an in-flight `IDataDriver`-interface change is open, align this with it; do not block on it.) +> **Resolved (2026-07-30).** Both hooks — `temporalFilterValue` and the D-B +> column-side companion `temporalFilterColumnSql` — are optional members of +> `IDataDriver` (`spec/contracts/data-driver.ts`), documented as a PAIR: +> implementing one without the other reintroduces half of #3912, so the +> contract says implement both or neither, with "absent = identity" semantics +> for drivers whose storage form is the wire form. `SqlDriver implements +> IDataDriver`, so its signatures are compile-checked; analytics derives its +> driver seam by `Pick`-ing the contract instead of a local duck type, keeping +> the runtime `typeof` guards as the (correct) way to consume an optional +> member. `coerceFilterValueForSql` already sits behind the hook as the +> last-resort boolean/number recovery for non-temporal columns — the demotion +> this decision asked for — and is retained for exactly that role. + **D-A3 — Add a temporal conformance matrix as the runtime regression backstop.** Cover `field-type {date, datetime} × operator {eq, gte/lte/gt/lt, in, dateRange} × relative-token {today, N_days_ago, N_months_ago, …} × driver {SQLite, Postgres at @@ -400,8 +413,9 @@ time, the matrix proves runtime correctness across drivers. - The `datetime`-on-raw-SQL filter bug is closed at the driver boundary, mirroring Phase 1's "align the consumer with the driver's existing contract rather than 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. +- ~~Until D-A2 lands, the hook depends on a duck-typed driver method — a known, + intentionally-temporary seam tracked here.~~ Landed 2026-07-30; see the + resolution note under D-A2. --- @@ -503,7 +517,8 @@ is clean. 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. + raw SQL must wrap its column reference too. Both promoted onto the contract + 2026-07-30 — see the resolution note under 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. diff --git a/packages/services/service-analytics/src/plugin.ts b/packages/services/service-analytics/src/plugin.ts index 5adbb5a582..b44adf61cc 100644 --- a/packages/services/service-analytics/src/plugin.ts +++ b/packages/services/service-analytics/src/plugin.ts @@ -3,7 +3,7 @@ import type { Plugin, PluginContext } from '@objectstack/core'; import type { Cube, FilterCondition } from '@objectstack/spec/data'; import type { ExecutionContext } from '@objectstack/spec/kernel'; -import type { IAnalyticsService } from '@objectstack/spec/contracts'; +import type { IAnalyticsService, IDataDriver } from '@objectstack/spec/contracts'; import { AnalyticsService } from './analytics-service.js'; import type { AnalyticsServiceConfig } from './analytics-service.js'; import type { DriverCapabilities } from './strategies/types.js'; @@ -45,31 +45,27 @@ interface DataEngineLike { } | undefined; /** * Resolve the storage driver backing an object (public ObjectQL accessor). - * Used to delegate temporal filter-value coercion to the driver, which is the + * Used to delegate temporal storage-form coercion to the driver, which is the * single source of truth for how a `Field.date`/`Field.datetime` is stored on - * the active dialect. The driver may expose `temporalFilterValue(object, field, - * value)` (SqlDriver does); when absent we leave the value untouched. + * the active dialect. When the hooks are absent, values and column SQL pass + * through untouched — the contract's identity semantics. */ - getDriverForObject?(objectName: string): DriverLike | undefined; + getDriverForObject?(objectName: string): TemporalDriverSurface | undefined; } -/** Minimal driver surface the analytics layer probes for temporal coercion. */ -interface DriverLike { - /** - * Coerce a filter comparand to the column's on-disk storage form - * (SQLite `Field.datetime` → epoch ms; `Field.date` → YYYY-MM-DD; native - * 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; -} +/** + * The slice of the `IDataDriver` CONTRACT the analytics layer consumes — + * `temporalFilterValue` / `temporalFilterColumnSql` are first-class contract + * members since ADR-0053 D-A2, no longer a duck-typed local invention. Picked + * (rather than using `IDataDriver` whole) because `getDriverForObject` hands + * back whatever the engine registered, and this seam only needs the temporal + * surface; the runtime `typeof` guards below remain the correct way to consume + * an optional contract member. + */ +type TemporalDriverSurface = Pick< + IDataDriver, + 'temporalFilterValue' | 'temporalFilterColumnSql' +>; /** * Configuration for AnalyticsServicePlugin. diff --git a/packages/spec/src/contracts/data-driver.ts b/packages/spec/src/contracts/data-driver.ts index 73380a443b..9db000c45f 100644 --- a/packages/spec/src/contracts/data-driver.ts +++ b/packages/spec/src/contracts/data-driver.ts @@ -127,6 +127,47 @@ export interface IDataDriver { /** Delete multiple records matching a query (optional) */ deleteMany?(object: string, query: QueryAST, options?: DriverOptions): Promise; + // =========================================================================== + // Temporal Storage Convention (ADR-0053 D-A1/D-A2, #3912) + // =========================================================================== + // + // A driver is the single source of truth for how a `Field.date` / + // `Field.datetime` value is physically stored on its dialect. Any surface + // that builds queries OUTSIDE the driver's own find()/filter path — the + // analytics native-SQL strategy today, any future raw-query strategy — must + // route its temporal comparands AND its column references through these two + // hooks rather than re-deriving the storage form from the value's textual + // shape (the drift that produced #3912). + // + // The two hooks are a pair on purpose: #3912's lesson is that coercing the + // comparand alone is NOT sufficient on a dialect whose stored form can + // diverge from the bound form — the column side of the comparison has to be + // normalised by the same authority. A driver that implements one without + // the other reintroduces half the bug, so implement both or neither. + // + // Both are optional with identity semantics: a driver whose storage form IS + // the wire form (memory, mongo) simply omits them, and callers treat + // "absent" exactly like "returns the input unchanged". + + /** + * Coerce a filter comparand to the on-disk storage form of `field` on + * `objectName` — e.g. an ISO instant for a canonical-text datetime column, + * `YYYY-MM-DD` text for a `Field.date`, a dialect-spelled datetime literal + * where the dialect cannot parse ISO-8601. Non-temporal fields and + * uninterpretable values are returned unchanged. + */ + temporalFilterValue?(objectName: string, field: string, value: unknown): unknown; + + /** + * The companion for the LEFT side of the same comparison: given the SQL the + * caller was going to emit for the column (an already-quoted, possibly + * qualified reference), return the SQL it must emit instead so the column + * reads in the same storage form {@link temporalFilterValue} coerces the + * comparand into. Returns `columnSql` verbatim for every column that needs + * no normalisation — which is every column on a fully-converged database. + */ + temporalFilterColumnSql?(objectName: string, field: string, columnSql: string): string; + // =========================================================================== // Transaction Management // =========================================================================== From 4eab066b610ab0d7d1f312e24c00f5b2803ac77e Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 03:11:40 +0000 Subject: [PATCH 2/2] =?UTF-8?q?fix(driver-sql):=20Field.time=20gets=20a=20?= =?UTF-8?q?canonical=20storage=20form=20=E2=80=94=20HH:MM:SS[.fff]=20wall-?= =?UTF-8?q?clock=20text=20on=20every=20dialect=20(#3994)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Field.time repeated the pre-#3912 Field.datetime pattern: writes were never normalised, only reads were repaired, so one SQLite column accumulated bare time-of-day TEXT, full-timestamp TEXT and INTEGER epoch ms side by side. find() looked right; everything comparing the STORED form was wrong — measured: a business-hours window filter silently dropped 4 of 7 rows, ORDER BY sorted 14:30 before 08:00, a full-ISO write failed the statement on both Postgres and MySQL, a bound Date stored a process-timezone wall clock on pg, MySQL's bare TIME rounded fractional seconds, and NOW() defaults read three different clocks across the three dialects. The #3912→#3942→#3954 construction, transplanted (ADR-0053 D-C1..D-C3): - One canonicalTimeOfDay — HH:MM:SS, .fff only when non-zero; Date/epoch/ full-timestamp fold to the UTC time-of-day — applied on write (formatInput), to filter comparands (coerceFilterValue → temporalFilterValue) and on read (toTimeOnly). - SQLite: backfillCanonicalTimes converges legacy columns at schema sync (IS NOT-guarded UPDATE, log-and-swallow); until then filters wrap the column in sqliteCanonicalTimeSql — correct, just unindexed. os migrate plan lists the work as normalize_time_storage with a row count. - MySQL: new time columns are TIME(3); legacy TIME(0) widens at schema sync (migrateMysqlTimeColumns, plan kind widen_time_columns) since zero-precision TIME rounds fractional writes. - NOW() defaults read the UTC clock on every dialect; MySQL 8.0 rejects a plain CURRENT_TIMESTAMP default on TIME entirely, so the expression default is also a compatibility fix. - distinct()/aggregate() present time columns exactly as find() does (ReadPresentationKind gains 'time'). HH:MM:SS writes round-trip byte-identically (field-zoo f_time, #2022); a minutes-only HH:MM completes to HH:MM:00; uninterpretable values pass through untouched. Verified on live servers: SQLite, PG 16 at Asia/Shanghai, MariaDB 10.11 at +08:00, Node at America/New_York — 505/505 driver tests with zero skips. Closes #3994 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01TPxNwPnjcn599ujXpU3ibJ --- .changeset/field-time-canonical-storage.md | 40 ++ docs/adr/0053-date-and-datetime-semantics.md | 77 ++- .../schema-migrate.pending-render.test.ts | 11 +- packages/cli/src/utils/schema-migrate.ts | 9 +- .../src/legacy-datetime-storage.testkit.ts | 18 + .../plugins/driver-sql/src/schema-drift.ts | 21 +- .../sql-driver-time-canonical-storage.test.ts | 263 ++++++++++ .../src/sql-driver-time-live-dialects.test.ts | 215 ++++++++ .../src/sql-driver-time-of-day.test.ts | 42 +- packages/plugins/driver-sql/src/sql-driver.ts | 463 ++++++++++++++++-- packages/spec/src/contracts/data-driver.ts | 6 +- 11 files changed, 1084 insertions(+), 81 deletions(-) create mode 100644 .changeset/field-time-canonical-storage.md create mode 100644 packages/plugins/driver-sql/src/sql-driver-time-canonical-storage.test.ts create mode 100644 packages/plugins/driver-sql/src/sql-driver-time-live-dialects.test.ts diff --git a/.changeset/field-time-canonical-storage.md b/.changeset/field-time-canonical-storage.md new file mode 100644 index 0000000000..02c9e15bf7 --- /dev/null +++ b/.changeset/field-time-canonical-storage.md @@ -0,0 +1,40 @@ +--- +"@objectstack/driver-sql": minor +"@objectstack/cli": patch +"@objectstack/spec": patch +--- + +fix(driver-sql): `Field.time` gets a canonical storage form — `HH:MM:SS[.fff]` wall-clock text on every dialect (#3994) + +`Field.time` repeated the pre-#3912 `Field.datetime` pattern: writes were never +normalised and only reads were repaired, so one SQLite column accumulated bare +time-of-day TEXT, full-timestamp TEXT and INTEGER epoch ms side by side. +`find()` looked right; everything that compared the STORED form was wrong — +measured: a business-hours window filter silently dropped 4 of 7 rows, ORDER BY +sorted 14:30 before 08:00, a full-ISO write failed the statement outright on +both Postgres and MySQL, a bound `Date` stored a process-timezone wall clock on +pg, MySQL's bare `TIME` rounded `…00.500` up to `…01`, and a `NOW()` default +resolved against three different clocks on the three dialects. + +The #3912→#3942→#3954 construction, transplanted (ADR-0053 D-C1..D-C3): + +- One `canonicalTimeOfDay` — `HH:MM:SS`, `.fff` only when non-zero; `Date`/ + epoch/full-timestamp fold to the UTC time-of-day — applied on write + (`formatInput`), to filter comparands (`coerceFilterValue`, and thereby the + `temporalFilterValue` contract hook) and on read (`toTimeOnly`). +- SQLite: legacy columns converge at schema sync (`backfillCanonicalTimes`, + same `IS NOT`-guarded UPDATE, same log-and-swallow policy); until then the + filter paths wrap the column in the repair expression — correct, just + unindexed. `os migrate plan` lists the work as `normalize_time_storage` with + a row count. +- MySQL: new time columns are `TIME(3)`; legacy `TIME(0)` columns widen at + schema sync (`migrateMysqlTimeColumns`, plan kind `widen_time_columns`), + since zero-precision TIME *rounds* fractional writes. +- `NOW()` defaults read the UTC clock on every dialect (Postgres previously + used the server zone, MySQL the inserting session's zone — and MySQL 8.0 + rejects a plain `CURRENT_TIMESTAMP` default on TIME entirely). +- `distinct()`/`aggregate()` present time columns exactly as `find()` does. + +`HH:MM:SS` writes round-trip byte-identically (the field-zoo `f_time` +contract); a minutes-only `HH:MM` now completes to `HH:MM:00`, and uninterpretable +values still pass through untouched. diff --git a/docs/adr/0053-date-and-datetime-semantics.md b/docs/adr/0053-date-and-datetime-semantics.md index 7c0a22c5e9..aa07c8ab5c 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 resolved 2026-07-30: `temporalFilterValue` + `temporalFilterColumnSql` are optional `IDataDriver` contract members with identity semantics, and analytics types its driver seam from the contract. **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). +**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 resolved 2026-07-30: `temporalFilterValue` + `temporalFilterColumnSql` are optional `IDataDriver` contract members with identity semantics, and analytics types its driver seam from the contract. **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). **Extended (2026-07-30, addendum D-C1..D-C3):** `Field.time` takes the same construction — canonical `HH:MM:SS[.fff]` wall-clock text, one function on write/filter/read, `TIME(3)` on MySQL, UTC `NOW()` defaults on every dialect (#3994). **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`. @@ -574,3 +574,78 @@ The same caveat as Postgres applies and is worth stating plainly: the migration 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. + +--- + +## Addendum (2026-07-30) — `Field.time` gets the same storage convention (D-C1..D-C3, #3994) + +`Field.time` was the last temporal field type with **no storage convention at +all** — the exact meta-problem #3912 exposed for `Field.datetime`. `formatInput` +never touched it, `coerceFilterValue` returned its comparands unchanged, and the +read paths papered over the drift so well (`toTimeOnly`) that `find()` always +looked right. Measured on real servers (SQLite; PG 16 at `Asia/Shanghai`; +MariaDB 10.11 / MySQL 8.0 at `+08:00`; Node at `America/New_York`), the same +failure family followed: a business-hours window filter silently dropped 4 of 7 +rows on SQLite (INTEGER epoch rows fail `>= '09:00'` because `INTEGER < TEXT`; +full-timestamp text fails `<= '18:00'` lexicographically), ORDER BY put 14:30 +before 08:00, a full-ISO write failed the statement outright on PG **and** +MySQL, a bound `Date` stored a process-timezone wall clock on pg, MySQL's bare +`TIME` rounded `…00.500` up to `…01`, and a `NOW()` default resolved against +three different clocks on the three dialects. + +### D-C1 — The canonical form is `HH:MM:SS`, `.fff` only when non-zero + +A `Field.time` is a **timezone-naive wall-clock time-of-day** (#2004), so the +canonical text carries no zone: `HH:MM:SS`, extended to `HH:MM:SS.fff` exactly +when the milliseconds are non-zero. One function (`canonicalTimeOfDay`) produces +it and is applied on write (`formatInput`), to filter comparands +(`coerceFilterValue`/`temporalFilterValue`) and on read (`toTimeOnly`), the +D-B1 construction transplanted. + +Why variable-width rather than datetime's fixed `.sss`: `.` sorts below every +digit, so lexicographic order is still chronological order across mixed widths +(`'14:30:00.100' < '14:30:01'`), and the zero-millisecond spelling `HH:MM:SS` +is what every dialect's native TIME emits — the field-zoo `f_time` round-trip +(#2022) keeps holding byte-for-byte. Determinism is what matters for equality +and `distinct()`, and each wall clock has exactly one spelling. A minutes-only +`'14:30'` completes to `'14:30:00'`. + +A `Date` / epoch-ms / full-timestamp value folds to its **UTC** time-of-day — +matching the platform's instant semantics, the SQLite read repair's historical +behaviour, and `nowColumnDefault`; never the process or server timezone. +Uninterpretable values (including out-of-range wall clocks like `'25:00'`) pass +through untouched. + +### D-C2 — Physical form per dialect + +- **SQLite**: canonical TEXT (no native type). Legacy columns converge at + schema sync via `backfillCanonicalTimes` — one `UPDATE` per column whose SET + expression IS the read-repair SQL (`sqliteCanonicalTimeSql`), `IS NOT` guard, + log-and-swallow failure policy, `os migrate plan` row-counted entry + (`normalize_time_storage`) — D-B3 verbatim. Until it runs, filters wrap the + column in the repair expression: correct, just unindexed. +- **Postgres**: native `time` (µs precision). The canonical literal binds + unambiguously; the pre-fix failures were the *input* shapes, not the column. +- **MySQL**: `TIME(3)` — the `DATETIME(3)` precedent applied: bare `TIME` is + zero-precision and **rounds** fractional literals, changing the stored wall + clock. Legacy `TIME(0)` columns widen at schema sync + (`migrateMysqlTimeColumns`, plan kind `widen_time_columns`), restating a + `NOW()` expression default where declared. + +### D-C3 — `NOW()` defaults are the UTC wall clock on every dialect + +`knex.fn.now()` compiles to `CURRENT_TIMESTAMP`, which a TIME column resolves +in the **server's** zone on Postgres and the **inserting session's** zone on +MySQL (so the same column's default depended on who inserted). MySQL 8.0 +additionally rejects a plain `CURRENT_TIMESTAMP` default on TIME. The defaults +are now expression-spelled per dialect — `strftime('%H:%M:%f','now')` with a +canonical `.000` trim on SQLite, `timezone('utc', now())::time(3)` on Postgres, +`(cast(utc_timestamp(3) as time(3)))` on MySQL (8.0.13+/MariaDB 10.2+ for the +expression-default syntax) — all reading the UTC clock. + +The D-B3 caveat applies unchanged: the backfill converges *shapes*; a wall +clock the old path never recorded correctly (a pg `Date` bind serialised in the +host's zone) is not recoverable. Regression cover: +`sql-driver-time-canonical-storage.test.ts`, `sql-driver-time-of-day.test.ts` +(SQLite), `sql-driver-time-live-dialects.test.ts` (live PG + MySQL, in the CI +temporal-conformance job's non-UTC matrix). diff --git a/packages/cli/src/utils/schema-migrate.pending-render.test.ts b/packages/cli/src/utils/schema-migrate.pending-render.test.ts index bf992e4b5f..3fd6fe5226 100644 --- a/packages/cli/src/utils/schema-migrate.pending-render.test.ts +++ b/packages/cli/src/utils/schema-migrate.pending-render.test.ts @@ -41,6 +41,8 @@ const ADDITIVE: PendingSchemaWork[] = [ const IN_PLACE: PendingSchemaWork[] = [ { table: 'evt', kind: 'normalize_datetime_storage', columns: ['at'], rows: 1234567 }, { table: 'legacy', kind: 'widen_datetime_columns', columns: ['at', 'created_at'], rows: 42 }, + { table: 'shift', kind: 'normalize_time_storage', columns: ['starts_at'], rows: 7 }, + { table: 'shift_my', kind: 'widen_time_columns', columns: ['starts_at'], rows: 9 }, ]; describe('renderPendingSchemaWork (#3954)', () => { @@ -74,6 +76,11 @@ describe('renderPendingSchemaWork (#3954)', () => { expect(out()).toContain('widen_datetime_columns: at, created_at'); // A MySQL widen is ALTER … MODIFY — a rebuild, said outright. expect(out()).toContain('42 row table rebuild'); + // The Field.time twins (#3994) take the same rendering rules. + expect(out()).toContain('normalize_time_storage: starts_at'); + expect(out()).toContain('7 row update(s)'); + expect(out()).toContain('widen_time_columns: starts_at'); + expect(out()).toContain('9 row table rebuild'); }); it('shows both sections when both kinds are pending', () => { @@ -102,7 +109,7 @@ describe('summarizePendingSchemaWork (#3954)', () => { const summary = summarizePendingSchemaWork([...ADDITIVE, ...IN_PLACE]); expect(summary).toContain('1 table(s) to create'); expect(summary).toContain('1 column(s) to add'); - expect(summary).toContain('3 datetime column(s) to converge in place'); - expect(summary).toContain('~1,234,609 rows'); + expect(summary).toContain('5 temporal column(s) to converge in place'); + expect(summary).toContain('~1,234,625 rows'); }); }); diff --git a/packages/cli/src/utils/schema-migrate.ts b/packages/cli/src/utils/schema-migrate.ts index b967c51152..4d5ab18487 100644 --- a/packages/cli/src/utils/schema-migrate.ts +++ b/packages/cli/src/utils/schema-migrate.ts @@ -285,17 +285,14 @@ export function renderPendingSchemaWork(pending: PendingSchemaWork[]): void { if (inPlace.length > 0) { console.log(` ${chalk.bold('In place (existing rows converged when you apply)')}`); for (const p of inPlace) { - const label = p.kind === 'normalize_datetime_storage' - ? 'normalize_datetime_storage' - : 'widen_datetime_columns'; // A MySQL widen is `ALTER … MODIFY`, i.e. a full table rebuild holding a // metadata lock — worth saying outright, not just implying via the count. - const cost = p.kind === 'widen_datetime_columns' + const cost = p.kind === 'widen_datetime_columns' || p.kind === 'widen_time_columns' ? `${formatRows(p.rows)} row table rebuild` : `${formatRows(p.rows)} row update(s)`; console.log( ` ${chalk.yellow('~')} ${chalk.yellow(p.table)} ` + - `${chalk.dim(`[${label}: ${p.columns.join(', ')} — ${cost}]`)}`, + `${chalk.dim(`[${p.kind}: ${p.columns.join(', ')} — ${cost}]`)}`, ); } console.log(''); @@ -320,7 +317,7 @@ export function summarizePendingSchemaWork(pending: PendingSchemaWork[]): string if (inPlace.length > 0) { const cols = inPlace.reduce((n, p) => n + p.columns.length, 0); const rows = inPlace.reduce((n, p) => n + (p.rows ?? 0), 0); - parts.push(`${cols} datetime column(s) to converge in place (~${formatRows(rows)} rows)`); + parts.push(`${cols} temporal column(s) to converge in place (~${formatRows(rows)} rows)`); } return parts.join(', '); } diff --git a/packages/plugins/driver-sql/src/legacy-datetime-storage.testkit.ts b/packages/plugins/driver-sql/src/legacy-datetime-storage.testkit.ts index e842b7ce82..3227d25241 100644 --- a/packages/plugins/driver-sql/src/legacy-datetime-storage.testkit.ts +++ b/packages/plugins/driver-sql/src/legacy-datetime-storage.testkit.ts @@ -39,6 +39,24 @@ export class LegacyStorageDriver extends SqlDriver { this.canonicalDatetimeFields[table]?.delete(field); } + /** + * The `Field.time` twin (#3994): insert raw pre-canonical time values and + * clear the column's canonical marker. + */ + async seedLegacyTimeRows( + table: string, + field: string, + rows: Array>, + ): Promise { + await this.knex(table).insert(rows); + this.forgetCanonicalTime(table, field); + } + + /** Drop the "already backfilled" marker for one `Field.time` column. */ + forgetCanonicalTime(table: string, field: string): void { + this.canonicalTimeFields[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( diff --git a/packages/plugins/driver-sql/src/schema-drift.ts b/packages/plugins/driver-sql/src/schema-drift.ts index f328def2b9..8a84676d09 100644 --- a/packages/plugins/driver-sql/src/schema-drift.ts +++ b/packages/plugins/driver-sql/src/schema-drift.ts @@ -128,23 +128,26 @@ export interface PendingSchemaWork { /** * What kind of physical work a {@link PendingSchemaWork} entry represents. * - * The first two are purely additive and never touch existing rows. The datetime - * pair is NOT: `normalize_datetime_storage` rewrites rows in place (the SQLite - * canonical-UTC backfill) and `widen_datetime_columns` rebuilds a column (the - * MySQL `TIMESTAMP` → `DATETIME(3)` widening) — both from #3912/#3942. They are - * rendered under their own heading for that reason: the additive section tells - * the operator the work is never data-losing, and that claim must not silently - * come to cover a row rewrite. + * The first two are purely additive and never touch existing rows. The rest are + * NOT: `normalize_datetime_storage` / `normalize_time_storage` rewrite rows in + * place (the SQLite canonical-text backfills, #3912/#3994) and + * `widen_datetime_columns` / `widen_time_columns` rebuild a column (the MySQL + * `TIMESTAMP` → `DATETIME(3)` and `TIME` → `TIME(3)` widenings, #3942/#3994). + * They are rendered under their own heading for that reason: the additive + * section tells the operator the work is never data-losing, and that claim must + * not silently come to cover a row rewrite. */ export type PendingSchemaWorkKind = | 'create_table' | 'add_columns' | 'normalize_datetime_storage' - | 'widen_datetime_columns'; + | 'normalize_time_storage' + | 'widen_datetime_columns' + | 'widen_time_columns'; /** True for the kinds that rewrite or rebuild existing data rather than adding to it. */ export function isInPlaceSchemaWork(kind: PendingSchemaWorkKind): boolean { - return kind === 'normalize_datetime_storage' || kind === 'widen_datetime_columns'; + return kind !== 'create_table' && kind !== 'add_columns'; } /** Ops that act on an index rather than a column — reconciled without a table rebuild. */ diff --git a/packages/plugins/driver-sql/src/sql-driver-time-canonical-storage.test.ts b/packages/plugins/driver-sql/src/sql-driver-time-canonical-storage.test.ts new file mode 100644 index 0000000000..2c0ff526ab --- /dev/null +++ b/packages/plugins/driver-sql/src/sql-driver-time-canonical-storage.test.ts @@ -0,0 +1,263 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #3994 — `Field.time` canonical storage: `HH:MM:SS`, `.fff` only when the + * milliseconds are non-zero. + * + * `Field.time` repeated the pre-#3912 `Field.datetime` pattern: `formatInput` + * never normalised it, so one SQLite column accumulated bare-time TEXT, + * full-timestamp TEXT and INTEGER epoch ms side by side. Reads looked right + * (`formatOutput` repaired), but a business-hours window filter compared the + * raw stored forms: INTEGER rows failed `>= '09:00:00'` outright + * (`INTEGER < TEXT` in SQLite's type ordering) and `'2026-…'` timestamp rows + * failed `<= '18:00:00'` lexicographically — 4 of 7 rows silently dropped, + * measured. These tests pin the three halves of the fix: canonical writes, + * canonical comparands, and the backfill + read-repair for legacy rows. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { SqlDriver } from './sql-driver.js'; +import { LegacyStorageDriver } from './legacy-datetime-storage.testkit.js'; + +const SHIFT = { + name: 'shift', + fields: { + label: { type: 'text' }, + starts_at: { type: 'time' }, + }, +} as any; + +const make = (Ctor: new (cfg: any) => T): T => + new Ctor({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }); + +// One wall clock (14:30:00.500 UTC) written in every accepted input shape, plus +// an 08:00 row that must stay OUTSIDE the business-hours window. +const WRITE_SHAPES: Array<[string, unknown]> = [ + ['s_hm', '14:30'], + ['s_hms', '14:30:00'], + ['s_ms', '14:30:00.500'], + ['s_iso', '2026-01-15T14:30:00.500Z'], + ['s_naive', '2026-01-15 14:30:00'], + ['s_date_obj', new Date(Date.UTC(2026, 0, 15, 14, 30, 0, 500))], + ['s_epoch', Date.UTC(2026, 0, 15, 14, 30, 0, 500)], + ['s_early', '08:00:00'], +]; + +describe('Field.time canonical writes (#3994)', () => { + let driver: SqlDriver; + + afterEach(async () => { + await driver.disconnect(); + }); + + it('collapses every accepted input shape to canonical TEXT — no INTEGER rows, no date prefixes', async () => { + driver = make(SqlDriver); + await driver.initObjects([SHIFT]); + for (const [id, v] of WRITE_SHAPES) { + await driver.create('shift', { id, label: id, starts_at: v }, { bypassTenantAudit: true }); + } + + const res: any = await (driver as any).knex.raw( + `select id, typeof(starts_at) as t, starts_at as v from shift order by id`, + ); + for (const row of res) { + expect(row.t).toBe('text'); + expect(row.v).toMatch(/^\d{2}:\d{2}:\d{2}(\.\d{3})?$/); + } + const byId = Object.fromEntries(res.map((r: any) => [r.id, r.v])); + // Zero milliseconds spell `HH:MM:SS`; non-zero keep exactly three digits. + expect(byId.s_hm).toBe('14:30:00'); + expect(byId.s_naive).toBe('14:30:00'); + expect(byId.s_iso).toBe('14:30:00.500'); + expect(byId.s_date_obj).toBe('14:30:00.500'); + expect(byId.s_epoch).toBe('14:30:00.500'); + }); + + it('the business-hours window matches every 14:30 row and excludes the 08:00 row (the #3994 F1 repro)', async () => { + driver = make(SqlDriver); + await driver.initObjects([SHIFT]); + for (const [id, v] of WRITE_SHAPES) { + await driver.create('shift', { id, label: id, starts_at: v }, { bypassTenantAudit: true }); + } + + const hits = await driver.find('shift', { + where: { starts_at: { $gte: '09:00:00', $lte: '18:00:00' } }, + orderBy: [{ field: 'id', order: 'asc' }], + }); + expect(hits.map((r: any) => r.id)).toEqual( + ['s_date_obj', 's_epoch', 's_hm', 's_hms', 's_iso', 's_ms', 's_naive'], + ); + }); + + it('equality cannot be split by input precision: HH:MM writes match an HH:MM:SS comparand', async () => { + driver = make(SqlDriver); + await driver.initObjects([SHIFT]); + await driver.create('shift', { id: 'a', label: 'a', starts_at: '14:30' }, { bypassTenantAudit: true }); + await driver.create('shift', { id: 'b', label: 'b', starts_at: '14:30:00' }, { bypassTenantAudit: true }); + + const hits = await driver.find('shift', { where: { starts_at: '14:30:00' } }); + expect(hits.map((r: any) => r.id).sort()).toEqual(['a', 'b']); + // And the comparand is canonicalised too — the minutes-only spelling + // matches the same two rows. + const hits2 = await driver.find('shift', { where: { starts_at: '14:30' } }); + expect(hits2.map((r: any) => r.id).sort()).toEqual(['a', 'b']); + }); + + it('ORDER BY sorts chronologically across mixed written shapes', async () => { + driver = make(SqlDriver); + await driver.initObjects([SHIFT]); + for (const [id, v] of WRITE_SHAPES) { + await driver.create('shift', { id, label: id, starts_at: v }, { bypassTenantAudit: true }); + } + const rows = await driver.find('shift', { orderBy: [{ field: 'starts_at', order: 'asc' }] }); + // 08:00 first; the `.500` rows after their `14:30:00` flat siblings + // (`.` sorts below every digit, so lexicographic == chronological). + expect((rows[0] as any).id).toBe('s_early'); + const times = rows.map((r: any) => r.starts_at); + expect(times).toEqual([...times].sort()); + }); + + it('distinct() presents canonically and collapses equal wall clocks (#3994 F6)', async () => { + driver = make(SqlDriver); + await driver.initObjects([SHIFT]); + for (const [id, v] of WRITE_SHAPES) { + await driver.create('shift', { id, label: id, starts_at: v }, { bypassTenantAudit: true }); + } + const values = await driver.distinct('shift', 'starts_at'); + expect(values.sort()).toEqual(['08:00:00', '14:30:00', '14:30:00.500']); + }); + + it('passes uninterpretable values through unchanged rather than rewriting them', async () => { + driver = make(SqlDriver); + await driver.initObjects([SHIFT]); + // '25:00' is not a wall clock; the driver must not invent one. + await driver.create('shift', { id: 'junk', label: 'j', starts_at: '25:00' }, { bypassTenantAudit: true }); + const res: any = await (driver as any).knex.raw(`select starts_at as v from shift where id = 'junk'`); + expect(res[0].v).toBe('25:00'); + }); +}); + +describe('Field.time legacy storage: backfill and read-side repair (#3994)', () => { + let driver: LegacyStorageDriver; + + afterEach(async () => { + await driver.disconnect(); + }); + + /** The mixed shapes an un-migrated column really holds. All 14:30 UTC except `t4`. */ + async function seedLegacy(d: LegacyStorageDriver) { + await d.initObjects([SHIFT]); + await d.seedLegacyTimeRows('shift', 'starts_at', [ + { id: 't1', label: 'a', starts_at: Date.UTC(2026, 0, 15, 14, 30, 0, 500) }, // INTEGER epoch + { id: 't2', label: 'b', starts_at: '2026-01-15T14:30:00.500Z' }, // full ISO TEXT + { id: 't3', label: 'c', starts_at: '2026-01-15 14:30:00' }, // naive timestamp TEXT + { id: 't4', label: 'd', starts_at: '08:00' }, // minutes-only TEXT + { id: 't5', label: 'e', starts_at: '14:30:00' }, // already canonical + ]); + } + + it('window filters and distinct() are correct BEFORE any migration, via the repair expression', async () => { + driver = make(LegacyStorageDriver); + await seedLegacy(driver); + + const hits = await driver.find('shift', { + where: { starts_at: { $gte: '09:00:00', $lte: '18:00:00' } }, + orderBy: [{ field: 'id', order: 'asc' }], + }); + expect(hits.map((r: any) => r.id)).toEqual(['t1', 't2', 't3', 't5']); + + const values = await driver.distinct('shift', 'starts_at'); + expect(values.sort()).toEqual(['08:00:00', '14:30:00', '14:30:00.500']); + }); + + it('a fresh boot over the existing table backfills every row to canonical text', async () => { + driver = make(LegacyStorageDriver); + await seedLegacy(driver); + + // Second sync over the SAME database — the state of a real upgrade boot. + await driver.initObjects([SHIFT]); + + const stored = await driver.storedForms('shift', 'starts_at'); + expect(stored.map((s) => [s.id, s.type, s.value])).toEqual([ + ['t1', 'text', '14:30:00.500'], + ['t2', 'text', '14:30:00.500'], + ['t3', 'text', '14:30:00'], + ['t4', 'text', '08:00:00'], + ['t5', 'text', '14:30:00'], + ]); + + // Converged storage means the plain indexable comparison now works. + const hits = await driver.find('shift', { + where: { starts_at: { $gte: '09:00:00', $lte: '18:00:00' } }, + orderBy: [{ field: 'id', order: 'asc' }], + }); + expect(hits.map((r: any) => r.id)).toEqual(['t1', 't2', 't3', 't5']); + }); + + it('the backfill is idempotent and leaves junk it cannot parse untouched', async () => { + driver = make(LegacyStorageDriver); + await driver.initObjects([SHIFT]); + await driver.seedLegacyTimeRows('shift', 'starts_at', [ + { id: 'j1', label: 'x', starts_at: 'not-a-time' }, + { id: 'j2', label: 'y', starts_at: '14:30' }, + ]); + + await driver.initObjects([SHIFT]); + const once = await driver.storedForms('shift', 'starts_at'); + await driver.initObjects([SHIFT]); + const twice = await driver.storedForms('shift', 'starts_at'); + + expect(once).toEqual(twice); + const byId = Object.fromEntries(once.map((s) => [s.id, s.value])); + expect(byId.j1).toBe('not-a-time'); // preserved, not NULLed or rewritten + expect(byId.j2).toBe('14:30:00'); + }); + + it('temporalFilterValue / temporalFilterColumnSql cover time fields (the #3979 contract pair)', async () => { + driver = make(LegacyStorageDriver); + await seedLegacy(driver); + + // Comparand side: every shape folds to the canonical time-of-day. + expect(driver.temporalFilterValue('shift', 'starts_at', '2026-01-15T14:30:00.500Z')).toBe('14:30:00.500'); + expect(driver.temporalFilterValue('shift', 'starts_at', '14:30')).toBe('14:30:00'); + // Column side: an un-migrated column is wrapped… + const wrapped = driver.temporalFilterColumnSql('shift', 'starts_at', '"shift"."starts_at"'); + expect(wrapped).toContain('typeof'); + expect(wrapped).toContain('%H:%M:%f'); + // …and a converged one is returned verbatim. + await driver.initObjects([SHIFT]); + expect(driver.temporalFilterColumnSql('shift', 'starts_at', '"shift"."starts_at"')).toBe('"shift"."starts_at"'); + }); +}); + +describe('os migrate plan lists the time convergence (#3994, #3954 pattern)', () => { + let driver: LegacyStorageDriver; + + afterEach(async () => { + await driver.disconnect(); + }); + + it('reports normalize_time_storage with columns and row count, without performing it', async () => { + driver = make(LegacyStorageDriver); + await driver.initObjects([SHIFT]); + await driver.seedLegacyTimeRows('shift', 'starts_at', [ + { id: 'p1', label: 'a', starts_at: Date.UTC(2026, 0, 15, 14, 30, 0) }, + { id: 'p2', label: 'b', starts_at: '2026-01-15 14:30:00' }, + { id: 'p3', label: 'c', starts_at: '14:30:00' }, // already canonical — not counted + ]); + const before = await driver.storedForms('shift', 'starts_at'); + + driver.setDeferredDdl(true); + await driver.initObjects([SHIFT]); + const pending = await driver.previewDeferredSchemaWork(); + + const converge = pending.filter((p) => p.kind === 'normalize_time_storage'); + expect(converge).toHaveLength(1); + expect(converge[0].table).toBe('shift'); + expect(converge[0].columns).toEqual(['starts_at']); + expect(converge[0].rows).toBe(2); + + // plan measures, apply changes — previewing must not have run the backfill. + expect(await driver.storedForms('shift', 'starts_at')).toEqual(before); + }); +}); diff --git a/packages/plugins/driver-sql/src/sql-driver-time-live-dialects.test.ts b/packages/plugins/driver-sql/src/sql-driver-time-live-dialects.test.ts new file mode 100644 index 0000000000..8b95541003 --- /dev/null +++ b/packages/plugins/driver-sql/src/sql-driver-time-live-dialects.test.ts @@ -0,0 +1,215 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #3994 — `Field.time` against LIVE Postgres and MySQL servers, ideally + * configured with a non-UTC timezone (the CI temporal-conformance job runs PG + * at `Asia/Shanghai`, MySQL at `+08:00`, with the Node process at + * `America/New_York`). + * + * What these pin, measured broken before the fix: + * - A full-ISO string bound to a native TIME column failed the STATEMENT on + * both dialects (`invalid input syntax for type time` / `Incorrect time + * value`) — the same payload SQLite accepted, so dev passed and prod 500ed. + * - A JS `Date` bound on pg was serialised in the PROCESS's local timezone + * (`14:30Z` became `09:30:00.500-05:00` on a New-York host), so the stored + * wall clock depended on the host's TZ. + * - MySQL's bare `TIME` is zero-precision and ROUNDS `'…00.500'` → `…01`. + * - A `defaultValue: 'NOW()'` time column resolved in the server's (PG) or + * inserting session's (MySQL) timezone — three clocks across three + * dialects for the same instant. + * + * Skipped without `OS_TEST_POSTGRES_URL` / `OS_TEST_MYSQL_URL`. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { SqlDriver } from '../src/index.js'; + +const PG_URL = process.env.OS_TEST_POSTGRES_URL; +const MY_URL = process.env.OS_TEST_MYSQL_URL; +const TABLE = 'os3994_probe'; + +const SHAPE = { + name: TABLE, + fields: { + label: { type: 'string' }, + starts_at: { type: 'time' }, + auto_at: { type: 'time', defaultValue: 'NOW()' }, + }, +} as any; + +/** One wall clock (14:30:00.500 UTC), every accepted input shape. */ +const WRITES: Array<[string, unknown, string]> = [ + ['w_hm', '14:30', '14:30:00'], + ['w_hms', '14:30:00', '14:30:00'], + ['w_ms', '14:30:00.500', '14:30:00.500'], + ['w_iso', '2026-01-15T14:30:00.500Z', '14:30:00.500'], + ['w_naive', '2026-01-15 14:30:00', '14:30:00'], + ['w_date', new Date(Date.UTC(2026, 0, 15, 14, 30, 0, 500)), '14:30:00.500'], + ['w_epoch', Date.UTC(2026, 0, 15, 14, 30, 0, 500), '14:30:00.500'], +]; + +/** + * Minutes-of-day distance between a presented `HH:MM:SS[.fff]` and UTC now, + * shortest way around the clock face — so a server-zone leak (±8h here) or a + * process-zone leak (−4/−5h) reads as hundreds of minutes, never a rounding + * artefact. + */ +function minutesOffUtc(presented: string): number { + const m = /^(\d{2}):(\d{2})/.exec(presented); + if (!m) return Number.NaN; + const now = new Date(); + const got = Number(m[1]) * 60 + Number(m[2]); + const utc = now.getUTCHours() * 60 + now.getUTCMinutes(); + const diff = Math.abs(got - utc); + return Math.min(diff, 1440 - diff); +} + +function suite(dialect: 'pg' | 'mysql', url: string | undefined) { + describe.skipIf(!url)(`Field.time on live ${dialect} (#3994)`, () => { + let driver: SqlDriver; + + beforeEach(async () => { + driver = new SqlDriver( + dialect === 'pg' + ? { client: 'pg', connection: url } + : { client: 'mysql2', connection: url }, + ); + await driver.execute(`drop table if exists ${TABLE}`).catch(() => {}); + await driver.initObjects([SHAPE]); + }); + + afterEach(async () => { + await driver.execute(`drop table if exists ${TABLE}`).catch(() => {}); + await driver.disconnect(); + }); + + it('accepts every input shape — full ISO used to fail the statement outright', async () => { + for (const [id, v] of WRITES) { + await driver.create(TABLE, { id, label: id, starts_at: v }, { bypassTenantAudit: true }); + } + for (const [id, , presented] of WRITES) { + const row: any = await driver.findOne(TABLE, id, { bypassTenantAudit: true }); + expect(row.starts_at, id).toBe(presented); + } + }); + + it('a bound Date stores its UTC wall clock, independent of the process timezone', async () => { + // On a UTC process this is trivially true; the CI job runs + // TZ=America/New_York, where the pre-fix pg driver stored 09:30. + await driver.create( + TABLE, + { id: 'd', label: 'd', starts_at: new Date(Date.UTC(2026, 0, 15, 14, 30, 0, 500)) }, + { bypassTenantAudit: true }, + ); + const row: any = await driver.findOne(TABLE, 'd', { bypassTenantAudit: true }); + expect(row.starts_at).toBe('14:30:00.500'); + }); + + it('keeps milliseconds — no zero-precision rounding to the next second', async () => { + // MySQL's bare TIME would ROUND '14:30:00.500' up to 14:30:01. + await driver.create(TABLE, { id: 'ms', label: 'ms', starts_at: '14:30:00.500' }, { bypassTenantAudit: true }); + const row: any = await driver.findOne(TABLE, 'ms', { bypassTenantAudit: true }); + expect(row.starts_at).toBe('14:30:00.500'); + }); + + it('the business-hours window matches every 14:30 row (the #3994 F1 repro)', async () => { + for (const [id, v] of WRITES) { + await driver.create(TABLE, { id, label: id, starts_at: v }, { bypassTenantAudit: true }); + } + await driver.create(TABLE, { id: 'early', label: 'e', starts_at: '08:00:00' }, { bypassTenantAudit: true }); + + const hits = await driver.find(TABLE, { + where: { starts_at: { $gte: '09:00:00', $lte: '18:00:00' } }, + orderBy: [{ field: 'id', order: 'asc' }], + }); + expect(hits.map((r: any) => r.id)).toEqual(WRITES.map(([id]) => id).sort()); + }); + + it("a NOW()-default time column records the UTC time-of-day, not the server's or session's", async () => { + await driver.create(TABLE, { id: 'now', label: 'n' }, { bypassTenantAudit: true }); + const row: any = await driver.findOne(TABLE, 'now', { bypassTenantAudit: true }); + // A leak of the +08:00 server zone is ~480 minutes; of the -04:00/-05:00 + // process zone, ~240-300. Genuine clock skew is seconds. + expect(minutesOffUtc(String(row.auto_at))).toBeLessThan(5); + }); + + it('distinct() presents exactly what find() presents', async () => { + await driver.create(TABLE, { id: 'a', label: 'a', starts_at: '14:30' }, { bypassTenantAudit: true }); + await driver.create(TABLE, { id: 'b', label: 'b', starts_at: '14:30:00' }, { bypassTenantAudit: true }); + await driver.create(TABLE, { id: 'c', label: 'c', starts_at: '14:30:00.500' }, { bypassTenantAudit: true }); + const values = await driver.distinct(TABLE, 'starts_at'); + expect(values.sort()).toEqual(['14:30:00', '14:30:00.500']); + }); + }); +} + +suite('pg', PG_URL); +suite('mysql', MY_URL); + +describe.skipIf(!MY_URL)('MySQL TIME → TIME(3) widening (#3994)', () => { + const LEGACY = 'os3994_legacy'; + let driver: SqlDriver; + + beforeEach(async () => { + // Build the table the way a pre-#3994 build did: a bare TIME column. + const legacy = new SqlDriver({ client: 'mysql2', connection: MY_URL }); + await legacy.execute(`drop table if exists ${LEGACY}`); + await legacy.execute( + `create table ${LEGACY} ( + id varchar(255) not null primary key, + label varchar(255) null, + starts_at time null + )`, + ); + await legacy.execute(`insert into ${LEGACY} (id, label, starts_at) values (?, ?, ?)`, [ + 'old', 'old', '14:30:00', + ]); + await legacy.disconnect(); + + driver = new SqlDriver({ client: 'mysql2', connection: MY_URL }); + }); + + afterEach(async () => { + await driver.execute(`drop table if exists ${LEGACY}`).catch(() => {}); + await driver.disconnect(); + }); + + const LEGACY_SHAPE = { name: LEGACY, fields: { label: { type: 'string' }, starts_at: { type: 'time' } } }; + + it('widens at schema sync without moving the stored wall clock, then keeps milliseconds', async () => { + await driver.initObjects([LEGACY_SHAPE]); + + const rows: any = await driver.execute( + `select column_type from information_schema.columns + where table_schema = database() and table_name = ? and column_name = 'starts_at'`, + [LEGACY], + ); + const list = Array.isArray(rows) && Array.isArray(rows[0]) ? rows[0] : rows; + const colType = String((list[0] as any).COLUMN_TYPE ?? (list[0] as any).column_type).toLowerCase(); + expect(colType).toBe('time(3)'); + + const old: any = await driver.findOne(LEGACY, 'old', { bypassTenantAudit: true }); + expect(old.starts_at).toBe('14:30:00'); // the wall clock must not move + + await driver.create(LEGACY, { id: 'ms', label: 'm', starts_at: '14:30:00.500' }, { bypassTenantAudit: true }); + const ms: any = await driver.findOne(LEGACY, 'ms', { bypassTenantAudit: true }); + expect(ms.starts_at).toBe('14:30:00.500'); // pre-widen this ROUNDED to 14:30:01 + }); + + it('is idempotent, and os migrate plan reports it as widen_time_columns first', async () => { + // Plan: reported, not performed. + driver.setDeferredDdl(true); + await driver.initObjects([LEGACY_SHAPE]); + const pending = await driver.previewDeferredSchemaWork(); + const widen = pending.filter((p) => p.kind === 'widen_time_columns'); + expect(widen).toHaveLength(1); + expect(widen[0].columns).toEqual(['starts_at']); + expect(widen[0].rows).toBe(1); + + // Apply: performed, then nothing left. + await driver.flushDeferredSchemaDdl(); + driver.setDeferredDdl(true); + await driver.initObjects([LEGACY_SHAPE]); + expect((await driver.previewDeferredSchemaWork()).filter((p) => p.kind === 'widen_time_columns')).toHaveLength(0); + }); +}); diff --git a/packages/plugins/driver-sql/src/sql-driver-time-of-day.test.ts b/packages/plugins/driver-sql/src/sql-driver-time-of-day.test.ts index f47de5f0a1..457833608b 100644 --- a/packages/plugins/driver-sql/src/sql-driver-time-of-day.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-time-of-day.test.ts @@ -1,24 +1,25 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * Read-side time-of-day normalization for `Field.time` on SQLite. + * `Field.time` canonical presentation (#2004, #3994). * - * `Field.time` is a wall-clock time-of-day, not an instant (#2004). A - * `defaultValue: 'NOW()'` time column historically took the full - * `CURRENT_TIMESTAMP` default, so a defaulted row read back a full - * `'YYYY-MM-DD HH:MM:SS'` timestamp instead of a time-of-day. `formatOutput` now - * repairs such legacy/raw rows to just the time portion (`toTimeOnly`), while - * leaving a value already stored as a bare time-of-day untouched — read-only, so - * no write/read asymmetry is introduced and the field-zoo round-trip - * (`f_time: '14:30:00'`, #2022) is unaffected. + * `Field.time` is a wall-clock time-of-day, not an instant. Since #3994 the + * driver stores, filters and presents ONE canonical shape — `HH:MM:SS`, with a + * `.fff` suffix only when the milliseconds are non-zero — via the same + * `canonicalTimeOfDay` on all three paths. On read that transparently repairs + * legacy rows (full-timestamp text from the old `CURRENT_TIMESTAMP` default, + * epoch ms from a bound `Date`) with no data migration; `HH:MM:SS` writes + * round-trip identically (the field-zoo `f_time` contract, #2022), while a + * minutes-only `HH:MM` gains its `:00` so equality filters and `distinct()` + * cannot split one wall clock into several presented values. */ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { SqlDriver } from '../src/index.js'; -const TIME_OF_DAY = /^\d{2}:\d{2}(:\d{2}(\.\d+)?)?$/; +const TIME_OF_DAY = /^\d{2}:\d{2}:\d{2}(\.\d{3})?$/; -describe('Field.time read normalization (time-of-day, SQLite)', () => { +describe('Field.time canonical presentation (time-of-day, SQLite)', () => { let driver: SqlDriver; let raw: any; @@ -58,11 +59,20 @@ describe('Field.time read normalization (time-of-day, SQLite)', () => { expect(row.starts_at).toBe('14:30:00.500'); }); - it('leaves a bare time-of-day untouched (field-zoo parity — no write/read asymmetry)', async () => { - for (const [id, v] of [['a', '14:30'], ['b', '14:30:00'], ['c', '09:05:30']] as const) { - await driver.create('shift', { id, label: id, starts_at: v }, { bypassTenantAudit: true }); + it('round-trips HH:MM:SS identically (field-zoo parity, #2022) and completes HH:MM', async () => { + // `HH:MM:SS[.fff]` IS the canonical shape — written, stored and presented + // byte-identically. A minutes-only `HH:MM` is the same wall clock as its + // `HH:MM:00` spelling and must canonicalise to it (#3994): leaving both + // forms in one column made `=` filters and `distinct()` treat them as two. + for (const [id, written, presented] of [ + ['a', '14:30', '14:30:00'], + ['b', '14:30:00', '14:30:00'], + ['c', '09:05:30', '09:05:30'], + ['d', '09:05:30.250', '09:05:30.250'], + ] as const) { + await driver.create('shift', { id, label: id, starts_at: written }, { bypassTenantAudit: true }); const row: any = await driver.findOne('shift', id, { bypassTenantAudit: true }); - expect(row.starts_at).toBe(v); // unchanged — round-trips identically + expect(row.starts_at).toBe(presented); } }); @@ -80,7 +90,7 @@ describe('Field.time read normalization (time-of-day, SQLite)', () => { const rows = await driver.find('shift', { orderBy: [{ field: 'id', order: 'asc' }] }); const byId = Object.fromEntries(rows.map((r: any) => [r.id, r])); expect(byId.l1.starts_at).toBe('08:15:00'); // legacy full-timestamp repaired - expect(byId.l2.starts_at).toBe('08:15:00'); // bare time-of-day preserved + expect(byId.l2.starts_at).toBe('08:15:00'); // canonical write round-trips }); it('leaves null untouched', async () => { diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index 2b4e31c4ab..6b1534c824 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -300,6 +300,81 @@ function mysqlDatetimeLiteral(canonical: unknown): unknown { return m ? `${m[1]} ${m[2]}` : canonical; } +/** + * The CANONICAL form of a `Field.time` value: a timezone-naive wall-clock + * time-of-day — `HH:MM:SS`, with a `.fff` millisecond suffix only when the + * milliseconds are non-zero (#3994). + * + * `Field.time` is a time-of-day, not an instant (#2004), so unlike + * {@link canonicalUtcDatetime} there is no zone marker — but the same + * write-unnormalised / repair-on-read drift produced the same broken window + * filters as #3912: a bound `Date` stored INTEGER epoch ms on SQLite (sorts + * before every TEXT row), a full ISO string stored as text beginning `'2026-…'` + * (sorts after every bare time-of-day), and `09:00 <= t <= 18:00` silently + * dropped both. This function is applied on write + * ({@link SqlDriver.formatInput}), to filter comparands + * ({@link SqlDriver.coerceFilterValue}) and on read + * ({@link SqlDriver.toTimeOnly}), so both sides of every comparison — and the + * presented value — are one shape. + * + * Why THIS form: + * - `.` sorts below every digit, so lexicographic order is chronological + * order even with the variable-width suffix (`'14:30:00.100' < + * '14:30:01'`), and a SQLite TEXT column range-compares through an index. + * - Deterministic per time-of-day: `'14:30'` and `'14:30:00'` are the same + * wall clock and canonicalise identically, so equality filters and + * `distinct()` cannot split one time into several values. + * - The zero-millisecond spelling is `HH:MM:SS` — the shape every dialect's + * native TIME emits and the field-zoo round-trip (#2022) already asserts — + * so converged common-case data never changes presentation. + * - Every dialect parses it: SQLite stores the text verbatim, Postgres + * `time` and MySQL `TIME(3)` both accept `HH:MM:SS[.fff]` literals — which + * the full-ISO spelling is precisely NOT (measured: `invalid input syntax + * for type time` on PG 16, `Incorrect time value` on MariaDB 10.11). + * + * A `Date` / epoch-ms / full-timestamp string folds to its **UTC** time-of-day + * (ADR-0053): the platform's instants are UTC everywhere else, and it matches + * what the SQLite read repair and `nowColumnDefault` already produced — + * crucially it does NOT depend on the Node process's local timezone, which is + * exactly what binding a raw `Date` to a Postgres TIME column did (pg + * serialised `14:30Z` as `09:30-05:00` on an America/New_York host). Fractions + * beyond milliseconds are truncated, matching `Date` resolution. + * + * Total: `null`/`undefined`, empty strings, out-of-range wall clocks (`'25:00'`) + * and unparseable junk pass through untouched — a value the driver cannot + * interpret is never silently rewritten. + */ +function canonicalTimeOfDay(value: unknown): unknown { + if (value == null) return value; + if (typeof value === 'string') { + const s = value.trim(); + if (s === '') return value; + const m = /^(\d{2}):(\d{2})(?::(\d{2})(?:\.(\d+))?)?$/.exec(s); + if (m) { + const [, hh, mm, ss = '00', frac] = m; + if (Number(hh) > 23 || Number(mm) > 59 || Number(ss) > 59) return value; + const ms = frac ? `${frac}000`.slice(0, 3) : '000'; + return ms === '000' ? `${hh}:${mm}:${ss}` : `${hh}:${mm}:${ss}.${ms}`; + } + } + // Everything that is not a bare time-of-day — `Date`, epoch ms, full ISO or + // zone-naive timestamp strings — is an instant: delegate its interpretation + // to the ONE function that owns instants, then keep the UTC time-of-day. + const instant = canonicalUtcDatetime(value); + if (typeof instant === 'string' && /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$/.test(instant)) { + const time = instant.slice(11, 23); + return time.endsWith('.000') ? time.slice(0, 8) : time; + } + return value; +} + +/** + * How many times {@link SqlDriver.sqliteCanonicalTimeSql} spells its column + * reference — the binding count a caller must supply per use of the expression. + * (1 `typeof` + 3 per `strftime` CASE branch pair × 2 + 1 `coalesce` fallback.) + */ +const SQLITE_TIME_EXPR_REFS = 8; + // ── Introspection Types ────────────────────────────────────────────────────── export interface IntrospectedColumn { @@ -347,7 +422,7 @@ export interface IntrospectedSchema { * read paths that bypass `formatOutput` (`aggregate`, `distinct`) name the rule * per column instead. See {@link SqlDriver.readPresentationKind}. */ -export type ReadPresentationKind = 'datetime' | 'date' | 'boolean' | 'number'; +export type ReadPresentationKind = 'datetime' | 'date' | 'time' | 'boolean' | 'number'; export type SqlDriverConfig = Knex.Config & { schemaMode?: SchemaMode; @@ -449,6 +524,13 @@ export class SqlDriver implements IDataDriver { */ protected canonicalDatetimeFields: Record> = {}; protected timeFields: Record> = {}; + /** + * The `Field.time` twin of {@link canonicalDatetimeFields} (#3994): columns + * known to hold only canonical `HH:MM:SS[.fff]` text — backfilled by + * {@link backfillCanonicalTimes} or created empty in this process. Read by + * {@link needsLegacyTimeRepair} to drop the repair expression. + */ + protected canonicalTimeFields: Record> = {}; /** * Federation read path (ADR-0015). For external objects whose physical * remote table differs from the object name, these map between the two so @@ -2637,8 +2719,12 @@ 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); + // #3994: the `Field.time` twin of the line above. + await this.backfillCanonicalTimes(tableName, exists); // #3942: the MySQL twin — widen legacy `TIMESTAMP` columns to `DATETIME(3)`. if (exists) await this.migrateMysqlDatetimeColumns(tableName, obj.fields ?? {}); + // #3994: widen legacy MySQL `TIME` columns to `TIME(3)`. + if (exists) await this.migrateMysqlTimeColumns(tableName, obj.fields ?? {}); } // Pre-create the auto_number counter table now, while we hold a fresh pooled @@ -2731,6 +2817,64 @@ export class SqlDriver implements IDataDriver { } } + /** + * Converge one table's `Field.time` columns on the canonical time-of-day text + * form (#3994) — the `Field.time` twin of {@link backfillCanonicalDatetimes}, + * built the same way for the same reasons. + * + * SQLite only: Postgres/MySQL store a native TIME, so their rows are already + * one shape. ONE `UPDATE` per column whose SET expression IS + * {@link sqliteCanonicalTimeSql} — the very expression the read paths use — + * with the null-safe, type-aware `IS NOT` guard as the whole `WHERE`. It + * converts INTEGER/REAL epoch ms, full-timestamp text (ISO or zone-naive) and + * under-specified `HH:MM` in one pass; canonical rows compare equal and cost + * nothing; unparseable values fall through the expression's `coalesce` + * unchanged and are left alone. + * + * What it CANNOT repair, exactly like the datetime backfill: a wall clock the + * old write path never recorded correctly. An epoch row folds to its UTC + * time-of-day — the same answer reads have always given for it. + * + * Failures are logged and swallowed: the column stays un-marked, the read and + * filter paths keep their repair expression, and queries stay correct (just + * unindexed). A migration must never take boot down. + */ + protected async backfillCanonicalTimes(table: string, tableExisted: boolean): Promise { + const fields = this.timeFields[table]; + if (!this.isSqlite || !fields || fields.size === 0) return; + + const clean = (this.canonicalTimeFields[table] ??= new Set()); + if (!tableExisted) { + for (const field of fields) clean.add(field); + return; + } + + const canonical = this.sqliteCanonicalTimeSql('??'); + const exprBindings = (field: string) => Array(SQLITE_TIME_EXPR_REFS).fill(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 time-of-day storage (#3994) for ${table}.${field}`, + { rowsConverted: converted }, + ); + } + clean.add(field); + } catch (err) { + this.logger.warn( + `[sql-driver] could not canonicalise time storage for ${table}.${field}; ` + + `queries stay correct via the read-side repair`, + { error: err instanceof Error ? err.message : String(err) }, + ); + } + } + } + /** * The `Field.datetime` (and audit) columns of `table` that MySQL still stores * as a legacy `TIMESTAMP`, with the nullability each must keep. @@ -2824,6 +2968,86 @@ export class SqlDriver implements IDataDriver { } } + /** + * The declared `Field.time` columns of `table` that MySQL still stores as a + * zero-precision `TIME`, with the nullability each must keep. Shared by + * {@link migrateMysqlTimeColumns} and {@link previewTimeConvergence} — the + * plan and the migration are the same set by construction (#3954 pattern). + */ + protected async legacyMysqlTimeColumns( + table: string, + fields: Record, + ): Promise> { + if (!this.isMysql) return []; + const candidates = new Set(); + for (const [name, field] of Object.entries(fields)) { + if ((field?.type ?? 'string') === 'time' && !field?.multiple) candidates.add(name); + } + if (candidates.size === 0) return []; + + 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 = 'time' + and coalesce(datetime_precision, 0) = 0`, + [table], + ); + const rows: any[] = Array.isArray(res) ? res[0] : (res?.rows ?? []); + return 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)); + } + + /** + * Widen a table's legacy MySQL `TIME` columns to `TIME(3)` (#3994) — the + * `Field.time` twin of {@link migrateMysqlDatetimeColumns}. + * + * A zero-precision `TIME` does not truncate a fractional literal — it ROUNDS + * it, so the canonical `'14:30:00.500'` would land as `14:30:01`: the write + * path would be changing the wall clock it was asked to store. `TIME(3)` + * keeps the milliseconds instead, matching the canonical form's resolution + * and the `DATETIME(3)` precedent (#3942). + * + * Failures are logged and swallowed for the usual reason; the only cost of a + * `TIME(0)` column that could not be widened is second-rounding of fractional + * writes — which is today's behaviour. + */ + protected async migrateMysqlTimeColumns( + table: string, + fields: Record, + ): Promise { + if (!this.isMysql) return; + try { + const legacy = await this.legacyMysqlTimeColumns(table, fields); + if (legacy.length === 0) return; + + for (const col of legacy) { + // MODIFY drops a default it does not restate. A `defaultValue: 'NOW()'` + // column gets the canonical UTC expression default (`nowColumnDefault`); + // its legacy `current_timestamp()` default read the SESSION's zone, so + // dropping-and-replacing it is a fix, not collateral. + const isNowDefault = isNowDefaultValue(fields[col.name]?.defaultValue); + const defaultClause = isNowDefault ? ' default (cast(utc_timestamp(3) as time(3)))' : ''; + await this.knex.raw( + `alter table ?? modify column ?? time(3) ${col.nullable ? 'null' : 'not null'}${defaultClause}`, + [table, col.name], + ); + } + this.logger.info?.( + `[sql-driver] widened MySQL TIME → TIME(3) (#3994) on ${table}`, + { columns: legacy.map((c) => c.name) }, + ); + } catch (err) { + this.logger.warn( + `[sql-driver] could not widen MySQL time columns on ${table}; ` + + `fractional-second writes keep rounding to whole seconds`, + { error: err instanceof Error ? err.message : String(err) }, + ); + } + } + // ── Deferred schema DDL (#3917) ──────────────────────────────────────────── /** @@ -2880,6 +3104,7 @@ export class SqlDriver implements IDataDriver { out.push({ table: tableName, kind: 'add_columns', columns: missing }); } out.push(...(await this.previewDatetimeConvergence(tableName, obj.fields ?? {}, existing))); + out.push(...(await this.previewTimeConvergence(tableName, obj.fields ?? {}, existing))); } out.sort((a, b) => a.table.localeCompare(b.table) || a.kind.localeCompare(b.kind)); return out; @@ -2946,6 +3171,53 @@ export class SqlDriver implements IDataDriver { } } + /** + * The `Field.time` storage-convergence work {@link backfillCanonicalTimes} and + * {@link migrateMysqlTimeColumns} would do for `table` — measured, not + * performed. The time twin of {@link previewDatetimeConvergence}, with the + * same probe-reuses-the-migration's-predicate construction and the same + * swallow-to-`[]` failure policy. + */ + protected async previewTimeConvergence( + table: string, + fields: Record, + existingColumns: Set, + ): Promise { + try { + if (this.isSqlite) { + const declared = [...(this.timeFields[table] ?? [])].filter((c) => existingColumns.has(c)); + if (declared.length === 0) return []; + const canonical = this.sqliteCanonicalTimeSql('??'); + const columns: string[] = []; + let rows = 0; + for (const field of declared) { + const res: any = await this.knex.raw( + `select count(*) as n from ?? where ?? is not null and ?? is not ${canonical}`, + [table, field, field, ...Array(SQLITE_TIME_EXPR_REFS).fill(field)], + ); + const n = Number((Array.isArray(res) ? res[0] : res)?.n ?? 0); + if (n > 0) { columns.push(field); rows += n; } + } + return columns.length === 0 + ? [] + : [{ table, kind: 'normalize_time_storage', columns, rows }]; + } + + if (this.isMysql) { + const legacy = await this.legacyMysqlTimeColumns(table, fields); + if (legacy.length === 0) return []; + const res: any = await this.knex.raw(`select count(*) as n from ??`, [table]); + const counted = Array.isArray(res?.[0]) ? res[0] : (res?.rows ?? res ?? []); + const rows = Number((counted[0] as any)?.n ?? (counted as any)?.n ?? 0); + return [{ table, kind: 'widen_time_columns', columns: legacy.map((c) => c.name), rows }]; + } + + return []; + } catch { + return []; + } + } + /** * Run the deferred sync and disarm the deferral. Returns the work that was * outstanding (captured before the DDL ran, so the caller can report what it @@ -3901,32 +4173,18 @@ export class SqlDriver implements IDataDriver { } /** - * Read-side repair for a `Field.time` value to its wall-clock time-of-day - * (`Field.time` is a tz-naive time-of-day, not an instant — #2004). This is a - * deliberately NARROW, read-only normalization (no write/filter counterpart): - * it only strips a leading `YYYY-MM-DD` date — exactly what a legacy - * `defaultValue: 'NOW()'` column took when the default was still the full - * `CURRENT_TIMESTAMP` (or a full ISO datetime that leaked into the column) — - * and any trailing zone, leaving the time portion. A value that is ALREADY a - * bare time-of-day (`HH:MM[:SS[.fff]]`, with or without `Z`/offset) is returned - * untouched, so the common case never changes and no write/read asymmetry is - * introduced. A `Date`/epoch-ms (defensive — a Date bound to a time column) - * maps to its UTC time-of-day. `null`/unrecognised shapes pass through. + * Present a `Field.time` value as its canonical wall-clock time-of-day + * (`HH:MM:SS[.fff]` — {@link canonicalTimeOfDay}). Shared by the filter + * (`coerceFilterValue`), write (`formatInput`) and read (`formatOutput`, + * `presentReadValue`) paths, exactly like {@link toDateOnly} for `Field.date` + * — one definition of what a time *is* on all three, which is the #3994 fix. + * On read it transparently repairs legacy rows (full-timestamp text from the + * old `CURRENT_TIMESTAMP` default, epoch ms from a bound `Date`) with no data + * migration, and re-pads a dialect's trimmed fraction (Postgres returns + * `.5` for the stored `.500`). */ protected toTimeOnly(value: any): any { - if (value == null) return value; - if (value instanceof Date) { - return Number.isNaN(value.getTime()) ? value : value.toISOString().slice(11, 19); - } - if (typeof value === 'number' && Number.isFinite(value)) { - const d = new Date(value); - return Number.isNaN(d.getTime()) ? value : d.toISOString().slice(11, 19); - } - if (typeof value !== 'string') return value; - // Legacy full date+time → keep just the time-of-day (strip date + any zone). - // A bare time-of-day is left exactly as stored. - const m = /^\d{4}-\d{2}-\d{2}[ T](\d{2}:\d{2}(?::\d{2}(?:\.\d+)?)?)(?:[Zz]|[+-]\d{2}:?\d{2})?$/.exec(value.trim()); - return m ? m[1] : value; + return canonicalTimeOfDay(value); } /** @@ -3956,7 +4214,11 @@ export class SqlDriver implements IDataDriver { const kind = this.temporalFieldKind(table, field); if (!kind) return value; - return kind === 'datetime' ? this.storageDatetimeValue(value) : this.toDateOnly(value); + if (kind === 'datetime') return this.storageDatetimeValue(value); + // `Field.time` (#3994) takes the same treatment for the same reason: the + // comparand must be the canonical time-of-day text the column stores. + if (kind === 'time') return canonicalTimeOfDay(value); + return this.toDateOnly(value); } /** @@ -4028,17 +4290,59 @@ export class SqlDriver implements IDataDriver { ); } + /** + * The `Field.time` twin of {@link needsLegacyDatetimeRepair} (#3994): might + * this SQLite `Field.time` column still hold pre-canonical values and + * therefore need {@link sqliteCanonicalTimeSql} wrapped around it? + */ + protected needsLegacyTimeRepair(table: string | null | undefined, field: string): boolean { + if (!table || !this.isSqlite) return false; + if (this.timeFields[table]?.has(field) !== true) return false; + return this.canonicalTimeFields[table]?.has(field) !== true; + } + + /** + * Read a possibly-legacy SQLite `Field.time` column as canonical time-of-day + * text — the SQL twin of {@link canonicalTimeOfDay}, for rows written before + * the convention existed and not yet backfilled. + * + * `strftime` parses every legacy text shape in one call — bare `HH:MM`, + * full-ISO, zone-naive `CURRENT_TIMESTAMP` output — and the `typeof()` + * dispatch converts epoch INTEGER/REAL through `'unixepoch'`, exactly as in + * {@link sqliteCanonicalDatetimeSql}. The extra `like '%.000'` CASE trims the + * zero-millisecond suffix `%f` always emits, so the SQL spelling of + * "canonical" is byte-identical to the JS one — the property the backfill's + * `IS NOT` guard and the plan's row count both lean on. + * + * `coalesce(…, col)` preserves uninterpretable junk, matching + * `canonicalTimeOfDay`'s totality. The column reference appears + * {@link SQLITE_TIME_EXPR_REFS} times; callers bind accordingly. + */ + protected sqliteCanonicalTimeSql(columnSql: string): string { + const canonText = (args: string) => + `case when strftime('%H:%M:%f', ${args}) like '%.000' ` + + `then strftime('%H:%M:%S', ${args}) ` + + `else strftime('%H:%M:%f', ${args}) end`; + return ( + `(case when typeof(${columnSql}) in ('integer','real') ` + + `then ${canonText(`${columnSql}/1000.0, 'unixepoch'`)} ` + + `else coalesce(${canonText(columnSql)}, ${columnSql}) end)` + ); + } + /** * Which temporal presentation rule, if any, a declared field takes — - * `null` for everything that is not a `Field.datetime` / `Field.date`. + * `null` for everything that is not a `Field.datetime` / `Field.date` / + * `Field.time`. */ protected temporalFieldKind( table: string | null | undefined, field: string, - ): 'datetime' | 'date' | null { + ): 'datetime' | 'date' | 'time' | null { if (!table) return null; if (this.datetimeFields[table]?.has(field)) return 'datetime'; if (this.dateFields[table]?.has(field)) return 'date'; + if (this.timeFields[table]?.has(field)) return 'time'; return null; } @@ -4081,6 +4385,11 @@ export class SqlDriver implements IDataDriver { switch (kind) { case 'date': return this.toDateOnly(value); + case 'time': + // Every dialect, like `date`: canonicalising also re-pads the fraction + // Postgres trims (`.5` → `.500`), so `distinct()`/`aggregate()` present + // exactly what `find()` presents (#3994, the F6 gap of the #3849 fix). + return this.toTimeOnly(value); case 'datetime': return this.isSqlite ? normalizeSqliteDatetimeOutput(value) : value; case 'boolean': @@ -4159,11 +4468,19 @@ export class SqlDriver implements IDataDriver { field: string, column: string, ): { sql: string; bindings: any[] } | null { - if (!this.needsLegacyDatetimeRepair(table, field)) return null; - return { - sql: this.sqliteCanonicalDatetimeSql('??'), - bindings: [column, column, column, column], - }; + if (this.needsLegacyDatetimeRepair(table, field)) { + return { + sql: this.sqliteCanonicalDatetimeSql('??'), + bindings: [column, column, column, column], + }; + } + if (this.needsLegacyTimeRepair(table, field)) { + return { + sql: this.sqliteCanonicalTimeSql('??'), + bindings: Array(SQLITE_TIME_EXPR_REFS).fill(column), + }; + } + return null; } /** @@ -4266,8 +4583,13 @@ 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.needsLegacyDatetimeRepair(objectName, field)) return columnSql; - return this.sqliteCanonicalDatetimeSql(columnSql); + if (this.needsLegacyDatetimeRepair(objectName, field)) { + return this.sqliteCanonicalDatetimeSql(columnSql); + } + if (this.needsLegacyTimeRepair(objectName, field)) { + return this.sqliteCanonicalTimeSql(columnSql); + } + return columnSql; } protected applyFilters(builder: Knex.QueryBuilder, filters: any) { @@ -4759,10 +5081,34 @@ export class SqlDriver implements IDataDriver { * reads are uniform without a schema migration. */ protected nowColumnDefault(type: string): Knex.Raw { - if (!this.isSqlite) return this.knex.fn.now(); + if (!this.isSqlite) { + // A `time` column deserves the same reasoning on the native dialects + // (#3994): `knex.fn.now()` compiles to CURRENT_TIMESTAMP, which a TIME + // column resolves in the SERVER's timezone on Postgres and the INSERTING + // session's timezone on MySQL — measured as three different wall clocks + // for one instant across the three dialects. Pin the default to the UTC + // time-of-day, matching what the driver's own writes and the SQLite + // branch below produce. (MySQL 8.0 additionally REJECTS a plain + // CURRENT_TIMESTAMP default on a TIME column — only the parenthesised + // expression form, MySQL 8.0.13+/MariaDB 10.2+, is legal there at all.) + if (type === 'time') { + if (this.isMysql) return this.knex.raw('(cast(utc_timestamp(3) as time(3)))'); + if (this.isPostgres) return this.knex.raw("(timezone('utc', now())::time(3))"); + } + return this.knex.fn.now(); + } switch (type) { case 'date': return this.knex.raw("(strftime('%Y-%m-%d', 'now'))"); - case 'time': return this.knex.raw("(strftime('%H:%M:%f', 'now'))"); + // The CASE trims a zero-millisecond `.000` so a defaulted row is + // byte-canonical ({@link canonicalTimeOfDay}) — `%f` alone would store + // `'01:55:08.000'` once in a thousand inserts, and that row would then + // miss an equality filter against the canonical `'01:55:08'`. SQLite + // fixes `'now'` per statement, so the three calls cannot straddle a + // millisecond boundary. + case 'time': return this.knex.raw( + "(case when strftime('%H:%M:%f', 'now') like '%.000' " + + "then strftime('%H:%M:%S', 'now') else strftime('%H:%M:%f', 'now') end)", + ); // datetime (and any non-temporal field that opts into NOW()): canonical instant. default: return this.knex.raw("(strftime('%Y-%m-%dT%H:%M:%fZ', 'now'))"); } @@ -4854,7 +5200,12 @@ export class SqlDriver implements IDataDriver { col = this.isMysql ? table.datetime(name, { precision: 3 }) : table.timestamp(name); break; case 'time': - col = table.time(name); + // MySQL's bare `TIME` is zero-precision and ROUNDS a fractional literal + // (`'14:30:00.500'` → `14:30:01`), so the canonical form's milliseconds + // would change the stored wall clock. `TIME(3)` keeps them — the + // `DATETIME(3)` precedent (#3942) applied to time-of-day (#3994). Knex's + // `time()` takes no precision, hence the explicit type. + col = this.isMysql ? table.specificType(name, 'time(3)') : table.time(name); break; // `user` is a lookup specialized to sys_user (ADR: lookup → sys_user). Same // physical storage as any lookup: a string column holding the related row id @@ -5083,6 +5434,28 @@ export class SqlDriver implements IDataDriver { } } + // #3994: a `Field.time` is a wall-clock time-of-day. Collapse every accepted + // input shape (bare `HH:MM[:SS[.fff]]`, JS `Date`, epoch number, full ISO or + // zone-naive timestamp) to the canonical `HH:MM:SS[.fff]` before it hits the + // wire — the write half of the same fix `coerceFilterValue` applies to + // comparands. On SQLite this ends the mixed TEXT/INTEGER storage that broke + // window filters and ORDER BY; on Postgres/MySQL it turns shapes the native + // TIME type rejects outright (full ISO — measured failing on both) or + // resolves against the process's local timezone (a bound `Date` on pg) into + // the one literal every dialect parses the same way. + const timeFields = this.timeFields[object]; + if (timeFields && timeFields.size > 0 && copy && typeof copy === 'object') { + for (const field of timeFields) { + const v = copy[field]; + if (v == null) continue; + const normalized = canonicalTimeOfDay(v); + if (normalized !== v) { + if (!copied) { copy = { ...copy }; copied = true; } + copy[field] = normalized; + } + } + } + // JSON field serialisation: PostgreSQL native jsonb columns require // valid JSON for ALL values (strings, numbers, booleans, objects). // SQLite stores JSON as plain TEXT so only objects/arrays need @@ -5227,13 +5600,13 @@ export class SqlDriver implements IDataDriver { } } - // Present `Field.time` as a wall-clock time-of-day (#2004), repairing a - // legacy row stored as a full timestamp — what a `defaultValue: 'NOW()'` - // column took when the SQLite default was still the full `CURRENT_TIMESTAMP` - // — to just its time portion. A value already stored as a bare time-of-day - // is left untouched, so this is read-only and asymmetry-free. Runs for every - // dialect (a native TIME column already returns a time-of-day → no-op). See - // `toTimeOnly`. + // Present `Field.time` as the canonical wall-clock time-of-day (#2004, + // #3994) — the same `canonicalTimeOfDay` the write and filter paths apply, + // so storage, comparand and presentation are one shape. On read this + // transparently repairs legacy rows (full-timestamp text from the old + // `CURRENT_TIMESTAMP` default, epoch ms from a bound `Date`) with no data + // migration, and re-pads the fraction Postgres trims (`.5` → `.500`). Runs + // for every dialect. See `toTimeOnly`. const timeFields = this.timeFields[object]; if (timeFields && timeFields.size > 0) { for (const field of timeFields) { diff --git a/packages/spec/src/contracts/data-driver.ts b/packages/spec/src/contracts/data-driver.ts index 9db000c45f..da49489406 100644 --- a/packages/spec/src/contracts/data-driver.ts +++ b/packages/spec/src/contracts/data-driver.ts @@ -132,7 +132,8 @@ export interface IDataDriver { // =========================================================================== // // A driver is the single source of truth for how a `Field.date` / - // `Field.datetime` value is physically stored on its dialect. Any surface + // `Field.datetime` / `Field.time` value is physically stored on its + // dialect. Any surface // that builds queries OUTSIDE the driver's own find()/filter path — the // analytics native-SQL strategy today, any future raw-query strategy — must // route its temporal comparands AND its column references through these two @@ -152,7 +153,8 @@ export interface IDataDriver { /** * Coerce a filter comparand to the on-disk storage form of `field` on * `objectName` — e.g. an ISO instant for a canonical-text datetime column, - * `YYYY-MM-DD` text for a `Field.date`, a dialect-spelled datetime literal + * `YYYY-MM-DD` text for a `Field.date`, canonical `HH:MM:SS[.fff]` text + * for a `Field.time` (#3994), a dialect-spelled datetime literal * where the dialect cannot parse ISO-8601. Non-temporal fields and * uninterpretable values are returned unchanged. */