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 // ===========================================================================