diff --git a/.changeset/comparison-operator-string-comparand.md b/.changeset/comparison-operator-string-comparand.md new file mode 100644 index 0000000000..8d7d960429 --- /dev/null +++ b/.changeset/comparison-operator-string-comparand.md @@ -0,0 +1,50 @@ +--- +"@objectstack/spec": minor +--- + +fix(spec): `$gt`/`$gte`/`$lt`/`$lte` accept the ISO string the platform itself produces (#5685) + +The four ordering-comparison slots declared `number | Date | FieldReference` — +and the platform's own producers put a **string** in them and nothing else. The +declaration did not merely under-describe reality, it contradicted it: + +- `resolveFilterTokens` (`@objectstack/core`) is the evaluator for the `{token}` + grammar and **every** branch returns a string — `asYmd(…)` for a calendar day, + `.toISOString()` for the sub-day tokens. Its own module example is exactly + this shape: `{ close_date: { $gte: '{current_year_start}' } }` becomes + `{ close_date: { $gte: '2026-01-01' } }`. +- `date-macros.zod.ts` states the same rule from the other end: "the DRIVER only + ever sees ISO date / timestamp strings, never `{tokens}`". +- Three first-party callers send strings today — `lifecycle-service`'s retention + cutoffs, `plugin-email`'s outbox sweep, and `plugin-auth`'s better-auth + adapter. + +An author — an AI author in particular — reading `number | Date` concluded that +a date window must be a `Date` object or an epoch number, which is the one form +the date-macro path can never hand them. + +**This is additive and declaration-side only.** No producer, caller or driver +changed. Every evaluation surface already compared strings: `driver-sql` binds +`>`/`>=`/`<`/`<=`, and `formula`'s `matchesFilter` and `driver-memory`'s matcher +fall through to the JS operators. Filters that validated before still validate. + +Widened in all three places this contract is spelled: `ComparisonOperatorSchema` +(documentation), `FieldOperatorsSchema` (the copy `NormalizedFilterSchema` +validates against and `FieldOperators` is inferred from), and the `Filter` +TypeScript helper — where `T` is known, so it stays type-precise: a `Date` field +now also takes the resolver's ISO string, a `string` field (a `Field.time` +`'09:00'`, an autonumber code) is orderable instead of collapsing to `never`, +and a `number` field stays numbers-only. + +**The comparand form the contract guarantees** is the ISO/clock one — an ISO +calendar day (`YYYY-MM-DD`), a UTC ISO-8601 instant, or a wall-clock time of day +(`HH:MM[:SS[.fff]]`). Those are ASCII and fixed-width, so lexicographic order IS +chronological order and every backend agrees. The union is a bare `string` +rather than an ISO refinement because this schema is field-agnostic (it never +sees which column the operator applies to) and because an ISO refinement would +reject `Field.time`'s declared `HH:MM` form, which `SqlDriver.temporalFilterValue` +canonicalises in the comparand position. Ordering **non-temporal** text is +therefore permitted but not promised: the order is the backend collation's +(byte-wise on SQLite, the database locale on Postgres, UTF-16 code units in the +JS matchers), and those coincide only for ASCII. The `.describe()` on each slot +says so. diff --git a/packages/objectql/src/engine-filter-array-lowering.test.ts b/packages/objectql/src/engine-filter-array-lowering.test.ts index 7382910561..c573f5be20 100644 --- a/packages/objectql/src/engine-filter-array-lowering.test.ts +++ b/packages/objectql/src/engine-filter-array-lowering.test.ts @@ -517,10 +517,12 @@ describe('Door 2 lowers FilterArray to FilterCondition before the driver (#5158) it('a scalar on a NON-collection operator is untouched', async () => { await engine.find('deal', asFilterArrayQuery([['stage', '!=', 'won']])); expect(lastWhere()).toEqual({ stage: { $ne: 'won' } }); - // String bounds on a range comparison stay legal — `FieldOperatorsSchema` - // declares `$gt` as number|Date|FieldReference, but ISO strings are what the - // showcase apps send and every backend accepts. This gate enforces the - // three list declarations, not the whole schema. + // String bounds on a range comparison stay legal, and since #5685 the + // declaration agrees: `FieldOperatorsSchema` now declares `$gt` as + // number|Date|string|FieldReference, matching the ISO strings the showcase + // apps send and every backend accepts. This gate still enforces only the + // three list declarations, not the whole schema — for cost, not because the + // schema disagrees. await engine.find('deal', asFilterArrayQuery([['stage', '>', '2026-01-01']])); expect(lastWhere()).toEqual({ stage: { $gt: '2026-01-01' } }); }); diff --git a/packages/objectql/src/filter-comparand-shape.ts b/packages/objectql/src/filter-comparand-shape.ts index 38b8578aad..29a828f1f6 100644 --- a/packages/objectql/src/filter-comparand-shape.ts +++ b/packages/objectql/src/filter-comparand-shape.ts @@ -210,14 +210,20 @@ function malformedRangeComparandError( * * Read-only and allocation-free on the overwhelmingly common path (a filter * with no list operator walks its own keys and returns). Runs on every engine - * read and write, so it stays a walk rather than a schema parse: - * `FieldOperatorsSchema` cannot be used as the gate directly because it is - * stricter than the runtime in ways the runtime deliberately allows — `$gt` is - * declared `number | Date | FieldReference`, while `['created_at', '>', - * '2026-01-01']` lowers to a STRING bound that every backend accepts and that - * the showcase apps rely on. Enforcing the whole schema here would refuse - * working queries; this gate enforces the three declarations that the drivers - * genuinely cannot agree on. + * read and write, so it stays a walk rather than a schema parse — that cost is + * now the whole reason, and this gate deliberately enforces only the three + * list declarations the drivers genuinely cannot agree on. + * + * [#5685] This paragraph used to carry a second reason: that + * `FieldOperatorsSchema` was "stricter than the runtime in ways the runtime + * deliberately allows", because `$gt` was declared `number | Date | + * FieldReference` while `['created_at', '>', '2026-01-01']` lowers to a STRING + * bound that every backend accepts and the showcase apps rely on. That was a + * real mismatch and it is **fixed at the source** rather than tolerated here: + * the four ordering slots now declare `string` too, so the observation that + * motivated this note no longer describes the schema. It is recorded rather + * than deleted because this file's workaround is part of the evidence that + * closed #5685 — the schema, not the runtime, was the wrong side. */ export function assertListComparandShapes( object: string, diff --git a/packages/spec/src/data/filter.test.ts b/packages/spec/src/data/filter.test.ts index 6a2b8cf4cd..df72150189 100644 --- a/packages/spec/src/data/filter.test.ts +++ b/packages/spec/src/data/filter.test.ts @@ -59,6 +59,95 @@ describe('ComparisonOperatorSchema', () => { expect(() => ComparisonOperatorSchema.parse({ $lt: date })).not.toThrow(); expect(() => ComparisonOperatorSchema.parse({ $lte: date })).not.toThrow(); }); + + // ========================================================================== + // #5685 — the four slots accept the STRING the platform itself produces. + // + // Before this was pinned the union was `number | Date | FieldReference`, so + // every shape below threw — including the date-macro resolver's own + // documented output. These are the exact spellings the producers emit; see + // `ComparisonOperatorSchema`'s docblock for why the union is a bare `string` + // rather than an ISO refinement. + // ========================================================================== + + describe('string comparands (#5685)', () => { + const OPS = ['$gt', '$gte', '$lt', '$lte'] as const; + + /** `resolveFilterTokens` returns `asYmd(...)` for every calendar-day token. */ + it('accepts the calendar-day string a date macro resolves to', () => { + for (const op of OPS) { + expect(() => ComparisonOperatorSchema.parse({ [op]: '2026-01-01' })).not.toThrow(); + } + }); + + /** + * The sub-day tokens (`{now}`, `{N_hours_ago}`) and all three first-party + * callers — `lifecycle-service`, `plugin-email`'s outbox sweep — emit a + * full `.toISOString()`. + */ + it('accepts the full ISO instant the sub-day tokens and the sweeps emit', () => { + const cutoff = new Date('2026-08-08T04:32:56.000Z').toISOString(); + for (const op of OPS) { + expect(() => ComparisonOperatorSchema.parse({ [op]: cutoff })).not.toThrow(); + } + }); + + /** + * A `Field.time` comparand is `HH:MM[:SS[.fff]]` and is declared + * NOT `Date.parse`-able (`field-value.zod.ts`, CLOCK_TIME_TYPES); the SQL + * driver canonicalises exactly this in the comparand position (#3979). + * This case is why the union is not narrowed to an ISO date/date-time shape. + */ + it('accepts a wall-clock time-of-day, which an ISO refinement would reject', () => { + for (const op of OPS) { + expect(() => ComparisonOperatorSchema.parse({ [op]: '09:00' })).not.toThrow(); + expect(() => ComparisonOperatorSchema.parse({ [op]: '14:30:00.500' })).not.toThrow(); + } + }); + + /** + * Non-temporal text ordering rides along with the widening. Pinned as an + * ADMITTED shape rather than a promised ordering — the docblock says the + * ORDER is the backend collation's, and this schema is field-agnostic so it + * cannot tell a code column from a date one. + */ + it('admits non-temporal text (order is the backend collation\'s, not this contract\'s)', () => { + for (const op of OPS) { + expect(() => ComparisonOperatorSchema.parse({ [op]: 'M' })).not.toThrow(); + } + }); + + it('still accepts numbers, Dates and field references — widening is additive', () => { + for (const op of OPS) { + expect(() => ComparisonOperatorSchema.parse({ [op]: 42 })).not.toThrow(); + expect(() => ComparisonOperatorSchema.parse({ [op]: new Date('2026-01-01') })).not.toThrow(); + expect(() => ComparisonOperatorSchema.parse({ [op]: { $field: 'other.col' } })).not.toThrow(); + } + }); + + it('still rejects a comparand that is orderable at no backend', () => { + for (const op of OPS) { + expect(() => ComparisonOperatorSchema.parse({ [op]: true })).toThrow(); + expect(() => ComparisonOperatorSchema.parse({ [op]: { nope: 1 } })).toThrow(); + } + }); + + /** + * The documentation copy above and the ENFORCED copy must not drift: it is + * `FieldOperatorsSchema` that `NormalizedFilterSchema` validates against and + * that the exported `FieldOperators` type is inferred from, so widening only + * the documented one would have left the reachable surface still rejecting + * the platform's own output. + */ + it('is matched by the enforced copy — FieldOperatorsSchema and the normalized AST', () => { + for (const op of OPS) { + expect(() => FieldOperatorsSchema.parse({ [op]: '2026-01-01' })).not.toThrow(); + } + expect(() => NormalizedFilterSchema.parse({ + $and: [{ close_date: { $gte: '2026-01-01' } }, { created_at: { $lt: '2026-08-08T04:32:56.000Z' } }], + })).not.toThrow(); + }); + }); }); // ============================================================================ @@ -439,6 +528,34 @@ describe('TypeScript Type System', () => { expect(filter).toBeDefined(); }); + /** + * #5685 — the TYPED half of the same contract. `Filter` knows `T`, so it + * stays type-precise where `ComparisonOperatorSchema` cannot; what it must + * NOT do is reject the string the platform's own date-macro resolver hands + * the author. These assignments are checked by `pnpm typecheck`, not by the + * runtime expectation below — vitest never typechecks, so reverting + * `filter.zod.ts` leaves this test GREEN under vitest and RED under `tsc`. + * Measured on the reverted schema, both errors land in this block: + * `$gte: '2026-01-01'` -> TS2322 Type 'string' is not assignable to type 'Date' + * `$gte: '09:00'` -> TS2322 Type 'string' is not assignable to type 'undefined' + * (the second reads `undefined`, not `never`: the old guard's `never` meets + * the slot's own `?`, and an optional `never` IS `undefined`.) + */ + it('accepts an ISO string on a Date field and orders string fields (#5685)', () => { + interface Deal { + close_date: Date; // resolved date macro arrives as 'YYYY-MM-DD' + shift_start: string; // Field.time — 'HH:MM[:SS[.fff]]' + amount: number; + } + + const resolvedMacro: Filter = { close_date: { $gte: '2026-01-01' } }; + const stillTakesDate: Filter = { close_date: { $lt: new Date('2026-01-01') } }; + const clockTime: Filter = { shift_start: { $gte: '09:00' } }; + const numeric: Filter = { amount: { $gt: 1000 } }; + + expect([resolvedMacro, stillTakesDate, clockTime, numeric]).toHaveLength(4); + }); + it('should support logical operators', () => { interface Task { title: string; diff --git a/packages/spec/src/data/filter.zod.ts b/packages/spec/src/data/filter.zod.ts index 26d9977d33..da7db126c9 100644 --- a/packages/spec/src/data/filter.zod.ts +++ b/packages/spec/src/data/filter.zod.ts @@ -86,21 +86,128 @@ export const EqualityOperatorSchema = lazySchema(() => z.object({ })); /** - * Comparison operators for numeric and date comparisons. - * Supported data types: Number, Date + * The comparand contract shared by `$gt` / `$gte` / `$lt` / `$lte` (#5685). + * + * Module-private on purpose: it is documentation attached to four slots, not an + * authorable surface of its own, so it stays out of the exported API surface. + * The reasoning behind every sentence is in {@link ComparisonOperatorSchema}'s + * docblock. + */ +const ORDERING_COMPARAND_DESCRIPTION = + 'Comparand is a number, a Date, a string, or a { $field } reference. ' + + 'STRING is the form the platform itself produces: the date-macro resolver ' + + 'returns only strings ("{current_year_start}" -> "2026-01-01"), and the ' + + 'guaranteed spellings are an ISO calendar day (YYYY-MM-DD), a UTC ISO-8601 ' + + 'instant, or a wall-clock time of day (HH:MM[:SS[.fff]]) for a Field.time ' + + 'column. Those are ASCII and fixed-width, so lexicographic order IS ' + + 'chronological order and every backend agrees; the driver reconciles the ' + + 'comparand with the column (a bare calendar day used as an upper bound ' + + 'becomes the half-open next-day boundary). Ordering NON-temporal text is ' + + 'permitted but NOT promised: the order is the backend collation\'s ' + + '(byte-wise on SQLite, the database locale on Postgres, UTF-16 code units ' + + 'in the JS matchers), and those coincide only for ASCII.'; + +/** + * Ordering-comparison operators. + * + * Supported comparand types: **Number, Date, ISO/clock STRING, FieldReference**. + * + * ## Why `string` is in the union (#5685) + * + * Until this was written down the four slots read `number | Date | + * FieldReference` — and the platform's own producers put a STRING in them and + * nothing else. The declaration did not merely under-describe reality, it + * contradicted it: + * + * - `resolveFilterTokens` (`@objectstack/core`, `filter-tokens.ts`) is the + * evaluator for the `{token}` grammar, and **every** branch returns a string + * — `asYmd(…)` for a calendar day, `.toISOString()` for the sub-day tokens. + * Its own module example is exactly this shape: + * `{ close_date: { $gte: '{current_year_start}' } }` → + * `{ close_date: { $gte: '2026-01-01' } }`. + * - `date-macros.zod.ts` states the same rule from the other end: "the DRIVER + * only ever sees ISO date / timestamp strings, never `{tokens}`". + * - Three first-party callers send strings today — + * `lifecycle-service.ts` (`{ created_at: { $lt: keepCutoff } }`, a + * `.toISOString()`), `plugin-email`'s `outbox-sweep.ts` (same shape), and + * `plugin-auth`'s better-auth adapter, which lowers a `gt`/`gte`/`lt`/`lte` + * clause with the producer's own untyped `condition.value`. + * + * The mismatch was already COSTING something, and the receipt is in the tree: + * `@objectstack/objectql`'s `filter-comparand-shape.ts` (#5869) had to state, + * as its reason for not using this schema as its gate, that the schema is + * "stricter than the runtime in ways the runtime deliberately allows — `$gt` is + * declared `number | Date | FieldReference`, while `['created_at', '>', + * '2026-01-01']` lowers to a STRING bound that every backend accepts and that + * **the showcase apps rely on**". A second package building around this + * declaration, and writing down that it is wrong, is the measurement that says + * the pull is real rather than hypothetical. + * + * An author — an AI author in particular — reading `number | Date` concluded + * that a date window must be a `Date` object or an epoch number, which is the + * one form the platform's own date-macro path can never hand them. This is the + * declaration aligning to a contract the rest of the stack already keeps, not + * a new capability: every evaluation surface ALREADY compares strings + * (`driver-sql` binds `>`/`>=`/`<`/`<=`, `formula`'s `matchesFilter` and + * `driver-memory`'s matcher fall through to the JS operators). + * + * ## Why a BARE string, and not an ISO-shaped refinement (#5685 rider ①) + * + * A tempting narrowing is "accept only an ISO date / date-time string". It was + * measured and rejected, for three reasons: + * + * 1. **This schema is field-AGNOSTIC.** It never sees which column the operator + * is applied to, so any value-shape refinement here is a guess about the + * column. Comparand-vs-column correctness is a field-TYPED judgement and it + * already has an owner: `SqlDriver.coerceFilterValue` dispatches on + * `temporalFieldKind` — `storageDatetimeValue` for `datetime`, `toDateOnly` + * for `date`, `canonicalTimeOfDay` for `time`, passthrough otherwise. + * 2. **An ISO refinement would reject a form this platform DECLARES.** + * `field-value.zod.ts`'s `CLOCK_TIME_TYPES` defines a `Field.time` value as + * `HH:MM[:SS[.fff]]` and says in as many words that it is "not + * `Date.parse`-able". `SqlDriver.temporalFilterValue` canonicalises exactly + * that in the COMPARAND position (`'14:30'` → `'14:30:00'`, the #3979 + * contract pair). A `$gte: '09:00'` on a `time` column is a supported + * comparison an ISO refinement would refuse. + * 3. **date-only and full-timestamp are already reconciled by the driver**, so + * narrowing buys no safety there. A bare `YYYY-MM-DD` anchors to midnight + * UTC for a lower bound and is rewritten to the half-open + * `< next-day-midnight` for an upper bound (`calendarDayUpperBoundRewrite`, + * the #3777 convention). + * + * ## What widening ADMITS, stated plainly + * + * `string` also admits ordering comparisons on NON-temporal text columns + * (`{ code: { $gt: 'M' } }`). That is real SQL and every backend answers it — + * but **the ORDER is the backend's, not this contract's**: `driver-sql` binds a + * plain `>` decided by the dialect's collation (byte-wise on SQLite, the + * database locale on Postgres, the column collation on MySQL), while `formula` + * and `driver-memory` use the JS operators, i.e. UTF-16 code-unit order. Those + * answers coincide for ASCII and diverge outside it — the same split + * {@link StringOperatorSchema} had to rule on for case sensitivity. + * + * **The comparand form this contract guarantees is therefore the ISO/clock one** + * — `YYYY-MM-DD`, a UTC ISO-8601 instant, or `HH:MM[:SS[.fff]]`. All three are + * ASCII and fixed-width, so lexicographic order IS chronological order and every + * backend agrees. Ordering arbitrary natural-language text is permitted, not + * promised: it is the collation's answer, and it may differ per backend. */ export const ComparisonOperatorSchema = lazySchema(() => z.object({ /** Greater than - SQL: > | MongoDB: $gt */ - $gt: z.union([z.number(), z.date(), FieldReferenceSchema]).optional(), - + $gt: z.union([z.number(), z.date(), z.string(), FieldReferenceSchema]).optional() + .describe(`Greater than. ${ORDERING_COMPARAND_DESCRIPTION}`), + /** Greater than or equal to - SQL: >= | MongoDB: $gte */ - $gte: z.union([z.number(), z.date(), FieldReferenceSchema]).optional(), - + $gte: z.union([z.number(), z.date(), z.string(), FieldReferenceSchema]).optional() + .describe(`Greater than or equal to. ${ORDERING_COMPARAND_DESCRIPTION}`), + /** Less than - SQL: < | MongoDB: $lt */ - $lt: z.union([z.number(), z.date(), FieldReferenceSchema]).optional(), - + $lt: z.union([z.number(), z.date(), z.string(), FieldReferenceSchema]).optional() + .describe(`Less than. ${ORDERING_COMPARAND_DESCRIPTION}`), + /** Less than or equal to - SQL: <= | MongoDB: $lte */ - $lte: z.union([z.number(), z.date(), FieldReferenceSchema]).optional(), + $lte: z.union([z.number(), z.date(), z.string(), FieldReferenceSchema]).optional() + .describe(`Less than or equal to. ${ORDERING_COMPARAND_DESCRIPTION}`), })); // ============================================================================ @@ -263,11 +370,16 @@ export const FieldOperatorsSchema = lazySchema(() => z.object({ $eq: z.any().optional(), $ne: z.any().optional(), - // Comparison (numeric/date) - $gt: z.union([z.number(), z.date(), FieldReferenceSchema]).optional(), - $gte: z.union([z.number(), z.date(), FieldReferenceSchema]).optional(), - $lt: z.union([z.number(), z.date(), FieldReferenceSchema]).optional(), - $lte: z.union([z.number(), z.date(), FieldReferenceSchema]).optional(), + // Ordering. `string` is in the union for the reason {@link ComparisonOperatorSchema} + // gives at length (#5685): the date-macro resolver and all three first-party + // callers produce ISO/clock STRINGS in these slots and nothing else. This copy + // is the ENFORCED one — `NormalizedFilterSchema` validates against it and the + // exported `FieldOperators` is inferred from it — so it must not drift from the + // documentation copy above. + $gt: z.union([z.number(), z.date(), z.string(), FieldReferenceSchema]).optional(), + $gte: z.union([z.number(), z.date(), z.string(), FieldReferenceSchema]).optional(), + $lt: z.union([z.number(), z.date(), z.string(), FieldReferenceSchema]).optional(), + $lte: z.union([z.number(), z.date(), z.string(), FieldReferenceSchema]).optional(), // Set & Range $in: z.array(z.any()).optional(), @@ -464,10 +576,20 @@ export type Filter = { | { $eq?: T[K]; $ne?: T[K]; - $gt?: T[K] extends number | Date ? T[K] : never; - $gte?: T[K] extends number | Date ? T[K] : never; - $lt?: T[K] extends number | Date ? T[K] : never; - $lte?: T[K] extends number | Date ? T[K] : never; + // Ordering (#5685). The TYPED half of what {@link ComparisonOperatorSchema} + // declares — and unlike that field-agnostic schema, `T` is known here, so + // this stays type-precise instead of admitting `string` everywhere: + // - a `Date` field also takes the ISO STRING the date-macro resolver + // produces (`{ close_date: { $gte: '2026-01-01' } }`), which the old + // `T[K] extends number | Date ? T[K]` guard rejected outright; + // - a `string` field (a `Field.time` `'09:00'`, an autonumber code) is + // orderable at every backend, where the old guard collapsed it to + // `never` and made the operator unwritable; + // - a `number` field stays numbers-only — nothing here wants `'5'`. + $gt?: T[K] extends number ? number : T[K] extends Date | string ? T[K] | string : never; + $gte?: T[K] extends number ? number : T[K] extends Date | string ? T[K] | string : never; + $lt?: T[K] extends number ? number : T[K] extends Date | string ? T[K] | string : never; + $lte?: T[K] extends number ? number : T[K] extends Date | string ? T[K] | string : never; $in?: T[K][]; $nin?: T[K][]; $between?: T[K] extends number | Date ? [T[K], T[K]] : never;