From 275c3ecf3847e63a9de414a3b97b575f91100de2 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Tue, 28 Jul 2026 20:06:56 +0800 Subject: [PATCH] fix(objectql,driver-sql)!: a group key is the column's value, in the shape `find()` presents it (#3849) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three code paths produce a `groupBy` key and no two agreed: qty (number) won (boolean) find() 3 number true boolean aggregate() pushed down 3 number 0/1 number in-memory fallback '3' string 'false'/'true' string Two independent causes. `applyInMemoryAggregation` ran every key through `String()`, which the pushed-down path never did. And the pushed-down path returns raw builder output: #3797 taught it to present temporal columns the way `formatOutput` does on a `find()` row, but not the boolean and numeric repairs, so a SQLite boolean — no native type, stored 0/1 — surfaced as an integer from `aggregate()` and a real boolean from `find()`. `engine.aggregate` picks between the two aggregate paths per query, so the same column changed shape with no change to the data or the query. The measures were always right, which is why nobody noticed. What broke was downstream code probing a raw `Map` keyed by the value's own type — `Map` lookup is SameValueZero, so '1' never finds 1: select-option labels fell back to raw values, lookup labels missed whenever the referenced object had a numeric id (routine for external objects), and cross-object rebucketing filed every row under RESTRICTED_BUCKET — one bar, correct grand total, no error. A boolean dimension drilled from the in-memory path also sent { won: 'true' } to SQLite, which an INTEGER column can never match. - `applyInMemoryAggregation` emits the value verbatim; its rows are find() output, so passing through is what makes the key equal the column's read shape. - The internal bucket id is type-preserving, so 1 and '1', true and 'true' stay distinct groups. BigInt is encoded explicitly — JSON.stringify throws on it, and a value that bucketed fine under String() must not start crashing. - `SqlDriver.aggregate`/`.distinct` present group keys and min/max with the same rules `formatOutput` applies, generalizing #3797 to boolean and numeric columns. The protected helpers are renamed to match (temporalFieldKind → readPresentationKind, presentTemporalValue → presentReadValue, presentTemporalColumns → presentReadColumns) and the kind union is exported. Date-bucketed groupBy is unaffected: both sides already produce canonical string labels, and #3839 pinned their empty bucket. Gate: group-key-read-shape-parity.test.ts measures both aggregate paths against find() for number, boolean and text columns on driver-sql and driver-sqlite-wasm, asserting the runtime TYPE — folding through String() is the reflex that hid this and would pass against the bug. Each half was confirmed to redden the gate on its own. Co-Authored-By: Claude --- .changeset/group-key-read-shape.md | 92 +++++++++++ .../src/in-memory-aggregation.test.ts | 63 ++++++++ .../objectql/src/in-memory-aggregation.ts | 46 ++++-- packages/plugins/driver-sql/src/sql-driver.ts | 131 ++++++++++----- .../test/group-key-read-shape-parity.test.ts | 152 ++++++++++++++++++ 5 files changed, 435 insertions(+), 49 deletions(-) create mode 100644 .changeset/group-key-read-shape.md create mode 100644 packages/qa/dogfood/test/group-key-read-shape-parity.test.ts diff --git a/.changeset/group-key-read-shape.md b/.changeset/group-key-read-shape.md new file mode 100644 index 0000000000..211da037f4 --- /dev/null +++ b/.changeset/group-key-read-shape.md @@ -0,0 +1,92 @@ +--- +"@objectstack/objectql": minor +"@objectstack/driver-sql": minor +--- + +fix(objectql,driver-sql)!: a group key is the column's value, in the shape `find()` presents it (#3849) + +`groupBy: ['qty']` now returns `3`, not `'3'`. `groupBy: ['won']` returns `true` / +`false`, not `'true'` / `'false'` on one path and `1` / `0` on the other. A bucket +key is a column value, so there is one right answer for what it looks like — +whatever that column looks like on a `find()` row — and all three paths that +produce one now give it. + +### What was wrong + +Three code paths produce a group key, and no two of them agreed: + +| | `qty` (number) | `won` (boolean) | +|---|---|---| +| `find()` | `3` number | `true` boolean | +| `aggregate()` pushed down | `3` number | `0` / `1` **number** | +| in-memory fallback | `'3'` **string** | `'false'` / `'true'` **string** | + +Two independent causes: + +- `applyInMemoryAggregation` ran every key through `String()`. The pushed-down + path never did. +- The pushed-down path returns raw builder output. #3797 taught it to present + temporal columns the way `formatOutput` does on a `find()` row, but not the + boolean and numeric repairs — so a SQLite boolean, which has no native type and + is stored as `0`/`1`, surfaced as an integer from `aggregate()` and as a real + boolean from `find()`. + +`engine.aggregate` chooses between the two aggregate paths per query — by whether +the driver aggregates natively, whether it advertises the requested granularity, +and whether the reference timezone is UTC — so the same column changed shape with +no change to the data or the query. + +### Why it mattered + +The measures were always right, which is why this went unnoticed. What broke was +downstream code that probes a raw `Map` keyed by the value's own type. `Map` +lookup is SameValueZero, so `'1'` never finds `1`: + +- **Select-option labels** (`dimension-labels.ts`) — the label table is keyed by + the option's own `value`. A numeric option value never matched a stringified + key, so the chart rendered the raw stored value instead of its label. +- **Lookup / master-detail labels** — the id → record-name table is built by an + inner query that always pushes down (raw ids), then probed with the outer + query's keys, which may be in-memory (stringified). With a numeric primary key + — routine for external/federated objects — every label missed. +- **Cross-object rebucketing** (`cross-object-rebucket.ts`) — the FK → attribute + map is built and probed the same way, and a miss is not a fallback but + `RESTRICTED_BUCKET`. A numeric FK filed **every row** under `'(restricted)'`: + one bar, correct grand total, no error. +- **Drill-through** — the raw dimension value goes into the drill filter + verbatim, so a boolean dimension drilled from the in-memory path sent + `{ won: 'true' }` to SQLite, whose INTEGER column cannot equal the text + `'true'`. Zero rows. + +### What changed + +- `applyInMemoryAggregation` (`@objectstack/objectql`) emits the value verbatim. + Its rows come straight from `driver.find()`, so passing the value through is + what makes the key equal the column's own read shape. +- The internal composite bucket id is now type-preserving, so `1` and `'1'`, + `true` and `'true'` stay distinct groups rather than merging on the way in. + BigInt is encoded explicitly — `JSON.stringify` throws on it, and a value that + used to bucket under `String()` must not start crashing the aggregate. +- `SqlDriver.aggregate` / `.distinct` (`@objectstack/driver-sql`) present group + keys and `min`/`max` results with the same rules `formatOutput` applies on a + `find()` row, generalizing the #3797 temporal fix to boolean and numeric + columns. The `protected` helpers behind it are renamed accordingly + (`temporalFieldKind` → `readPresentationKind`, `presentTemporalValue` → + `presentReadValue`, `presentTemporalColumns` → `presentReadColumns`) and the + kind union is exported as `ReadPresentationKind`. + +Date-bucketed `groupBy` items are unaffected: `bucketDateValue` and the dialect +bucket expressions both produce canonical string labels, and #3839 already pinned +their empty bucket. + +### Gate + +`packages/qa/dogfood/test/group-key-read-shape-parity.test.ts` measures both +aggregate paths against `find()` for a number, boolean and text column, on +`driver-sql` and `driver-sqlite-wasm`. It asserts the runtime TYPE, not just the +value — folding both sides through `String()` is the reflex that hid this in the +first place and would make the check pass against the bug it exists to catch. + +Each half was confirmed to fail the gate on its own: reverting only the +in-memory change reddens the number and boolean cases, reverting only the driver +change reddens the boolean cases with `0` against `false`. diff --git a/packages/objectql/src/in-memory-aggregation.test.ts b/packages/objectql/src/in-memory-aggregation.test.ts index 992263bbd7..50bf4e8884 100644 --- a/packages/objectql/src/in-memory-aggregation.test.ts +++ b/packages/objectql/src/in-memory-aggregation.test.ts @@ -115,6 +115,69 @@ describe('applyInMemoryAggregation', () => { expect(out.find((r) => r.stage === null)!.total).toBe(10); expect(out.find((r) => r.stage === 'null')!.total).toBe(5); }); + + // #3849 — these rows ARE `driver.find()` output, so a key that is not the + // value verbatim is a key that disagrees with every other read of the column. + // This used to `String()` everything, which the pushed-down path never did. + it('keys a non-empty bucket with the value verbatim, not a string of it', () => { + const dataset = [ + { qty: 3, won: true, amount: 1 }, + { qty: 3, won: false, amount: 2 }, + { qty: 7, won: false, amount: 4 }, + ]; + const agg = [{ function: 'sum' as const, field: 'amount', alias: 'total' }]; + + const byQty = applyInMemoryAggregation(dataset, { groupBy: ['qty'], aggregations: agg }); + expect(byQty.map((r) => r.qty).sort()).toEqual([3, 7]); + expect(byQty.find((r) => r.qty === 3)!.total).toBe(3); + + const byWon = applyInMemoryAggregation(dataset, { groupBy: ['won'], aggregations: agg }); + expect(byWon.map((r) => r.won).sort()).toEqual([false, true]); + expect(byWon.find((r) => r.won === false)!.total).toBe(6); + }); + + // The bucket id is built from the key, so preserving the key's type is only + // half of it — the id has to preserve it too, or `1` and `'1'` merge on the + // way in and the surviving key is whichever row happened to arrive first. + it('keeps values of different types in different buckets', () => { + const dataset = [ + { v: 1, amount: 1 }, + { v: '1', amount: 2 }, + { v: true, amount: 4 }, + { v: 'true', amount: 8 }, + { v: null, amount: 16 }, + { v: 'null', amount: 32 }, + ]; + const out = applyInMemoryAggregation(dataset, { + groupBy: ['v'], + aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }], + }); + expect(out).toHaveLength(6); + const total = (v: unknown) => out.find((r) => Object.is(r.v, v))!.total; + expect(total(1)).toBe(1); + expect(total('1')).toBe(2); + expect(total(true)).toBe(4); + expect(total('true')).toBe(8); + expect(total(null)).toBe(16); + expect(total('null')).toBe(32); + }); + + // `JSON.stringify` throws on a BigInt, and the id builder runs on every row of + // every grouped query — a shape that used to bucket fine under `String()` must + // not start crashing the aggregate. + it('buckets a BigInt key without throwing', () => { + const dataset = [ + { v: 9007199254740993n, amount: 1 }, + { v: 9007199254740993n, amount: 2 }, + { v: 9007199254740994n, amount: 4 }, + ]; + const out = applyInMemoryAggregation(dataset, { + groupBy: ['v'], + aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }], + }); + expect(out).toHaveLength(2); + expect(out.find((r) => r.v === 9007199254740993n)!.total).toBe(3); + }); }); describe('bucketDateValue', () => { diff --git a/packages/objectql/src/in-memory-aggregation.ts b/packages/objectql/src/in-memory-aggregation.ts index 54e34a950b..a065d2d964 100644 --- a/packages/objectql/src/in-memory-aggregation.ts +++ b/packages/objectql/src/in-memory-aggregation.ts @@ -40,6 +40,20 @@ // '(empty)', a localized "Uncategorized") and build drill filters as // `field = null`, so a sentinel leaked a raw English debug string into the UI // and turned the empty bucket's drill-through into a zero-row query. +// +// A NON-EMPTY KEY IS THE VALUE AS `find()` PRESENTS IT — no `String()` (#3849). +// The rows this function groups come straight from `driver.find()`, so passing +// the value through is what makes a bucket key equal the column's own read +// shape: a `number` stays a number, a `boolean` stays a boolean. This used to +// `String()` every key, which the pushed-down path never did, so `1` became +// `'1'` and `true` became `'true'` depending only on which path ran. +// +// That was not cosmetic. Several consumers probe a raw `Map` keyed by the +// value's own type — the select-option label table, the lookup FK → record-name +// table, the cross-object FK → attribute table — and `Map` lookup is +// SameValueZero, so `'1'` never finds `1`. A stringified key silently missed +// every entry: labels fell back to raw ids, and cross-object rebucketing filed +// every row under `'(restricted)'` while the grand total still reconciled. import { calendarPartsInTzOrUtc } from '@objectstack/core'; import type { QueryAST, GroupByNode, AggregationNode, DateGranularityValue } from '@objectstack/spec/data'; @@ -75,11 +89,11 @@ export function applyInMemoryAggregation( const fieldName = typeof g === 'string' ? g : (g.alias ?? g.field); const value = projectGroupValue(row, g, timezone); key[fieldName] = value; - // JSON-encoded so the empty bucket's `null` key cannot collide with a row - // whose value is the literal STRING `"null"` — plain interpolation renders - // both as `null` and would merge two distinct groups. This id is internal - // to the bucketing loop; only `key` is emitted. - parts.push(`${fieldName}=${JSON.stringify(value)}`); + // Type-preserving, because the key no longer is: `1` and `'1'`, `true` and + // `'true'`, `null` and `'null'` are all distinct groups and must not merge + // just because they interpolate the same. This id is internal to the + // bucketing loop; only `key` is emitted. + parts.push(`${fieldName}=${bucketIdPart(value)}`); } const id = parts.join('\u0001'); let bucket = buckets.get(id); @@ -98,15 +112,29 @@ export function applyInMemoryAggregation( return out; } -function projectGroupValue(row: any, g: GroupByNode, timezone?: string): string | null { +/** + * Stable, TYPE-PRESERVING encoding of one group value, for the internal bucket + * id only. `JSON.stringify` gives that for every shape a column can hold — + * `1` → `1`, `'1'` → `"1"`, `true` → `true`, `null` → `null` — except the two + * it refuses, which are handled explicitly so a value that used to bucket fine + * under `String()` cannot start throwing mid-aggregate. + */ +function bucketIdPart(v: unknown): string { + if (typeof v === 'bigint') return `bigint:${v}`; // JSON.stringify throws on these + return JSON.stringify(v) ?? `${typeof v}:${String(v)}`; // undefined / symbol / function +} + +function projectGroupValue(row: any, g: GroupByNode, timezone?: string): unknown { const field = typeof g === 'string' ? g : g.field; const v = row?.[field]; if (typeof g !== 'string' && g.dateGranularity) { return bucketDateValue(v, g.dateGranularity, timezone); } - // `null`, not a sentinel string — same key the pushed-down SQL gives a NULL - // group column (#3839). See the empty-bucket note at the top of this file. - return v == null ? null : String(v); + // The value as `driver.find()` presented it — these rows ARE find() output, so + // passing it through is what makes the bucket key equal the column's own read + // shape (#3849). `undefined` (absent field) folds into the empty bucket's + // `null` (#3839). See both notes at the top of this file. + return v ?? null; } function aggregateBucket(rows: any[], aggregations: AggregationNode[]): Record { diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index c564f1c237..6e4be246bd 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -243,6 +243,14 @@ export interface IntrospectedSchema { * all schema-mutating DDL. Defaults to `'managed'` when omitted, preserving * legacy behaviour. */ +/** + * How a stored column value must be reshaped to become the value a `find()` row + * presents. One entry per rule `formatOutput` applies to a scalar column; the + * 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 SqlDriverConfig = Knex.Config & { schemaMode?: SchemaMode; /** @@ -1668,14 +1676,15 @@ export class SqlDriver implements IDataDriver { // GROUP BY bucket expression (#3773) and the result presentation (#3797). const table = this.coercionKey(builder); - // Result columns that carry a raw temporal VALUE, keyed by the column name - // the caller will read. Collected while the statement is built because that - // is the only point where a column name and its meaning are both known: a - // `min()` lands under its alias (never under the field name), and a - // date-BUCKETED column lands under the field name while holding a label - // (`'2026-01'`), not an instant. Matching on names after the fact gets both - // of those backwards. See {@link presentTemporalColumns}. - const temporalOutput = new Map(); + // Result columns that carry a raw column VALUE (rather than a count/total + // derived from one), keyed by the column name the caller will read. + // Collected while the statement is built because that is the only point + // where a column name and its meaning are both known: a `min()` lands under + // its alias (never under the field name), and a date-BUCKETED column lands + // under the field name while holding a label (`'2026-01'`), not a value. + // Matching on names after the fact gets both of those backwards. + // See {@link presentReadColumns}. + const presentedOutput = new Map(); if (query.groupBy) { // groupBy items may be plain strings ('region') or structured objects @@ -1686,8 +1695,8 @@ export class SqlDriver implements IDataDriver { if (typeof g === 'string') { builder.groupBy(g); builder.select(g); - const kind = this.temporalFieldKind(table, g); - if (kind) temporalOutput.set(g, kind); + const kind = this.readPresentationKind(table, g); + if (kind) presentedOutput.set(g, kind); } else if (g && typeof g === 'object' && g.field) { if (g.dateGranularity) { const bucket = this.buildDateBucketExpr(g.field, g.dateGranularity as any, table); @@ -1702,8 +1711,8 @@ export class SqlDriver implements IDataDriver { } else { builder.groupBy(g.field); builder.select(g.field); - const kind = this.temporalFieldKind(table, g.field); - if (kind) temporalOutput.set(g.field, kind); + const kind = this.readPresentationKind(table, g.field); + if (kind) presentedOutput.set(g.field, kind); } } } @@ -1724,13 +1733,14 @@ export class SqlDriver implements IDataDriver { } // `min`/`max` are the only supported functions that hand back a value // OF the column rather than a count/total derived from it, so they are - // the only ones whose result is still an instant. `alias` is required - // by `AggregationNodeSchema`; the unaliased branch below lands under a - // dialect-dependent column name (`max("closed_at")` on SQLite, `max` on - // Postgres) and is defensive only, so it is deliberately not tracked. + // the only ones whose result still needs the column's presentation. + // `alias` is required by `AggregationNodeSchema`; the unaliased branch + // below lands under a dialect-dependent column name + // (`max("closed_at")` on SQLite, `max` on Postgres) and is defensive + // only, so it is deliberately not tracked. if ((funcName === 'min' || funcName === 'max') && agg.field) { - const kind = this.temporalFieldKind(table, agg.field); - if (kind) temporalOutput.set(agg.alias, kind); + const kind = this.readPresentationKind(table, agg.field); + if (kind) presentedOutput.set(agg.alias, kind); } } else { if (fieldExpr === '*') { @@ -1743,7 +1753,7 @@ export class SqlDriver implements IDataDriver { } const rows = await builder; - return this.presentTemporalColumns(rows, temporalOutput); + return this.presentReadColumns(rows, presentedOutput); } // =================================== @@ -1761,15 +1771,15 @@ export class SqlDriver implements IDataDriver { const results = await builder; const values = results.map((row: any) => row[field]); - // Same presentation `find()` gives the column (#3797) — a caller listing a - // datetime's values should not get epoch integers here and ISO strings - // there. Re-deduplicate afterwards: SQL `DISTINCT` compares STORED values, - // and one SQLite `Field.datetime` column holds both INTEGER epoch ms and - // ISO TEXT, so two rows recording the same instant survive as two rows and - // then collapse to the same presented value. - const kind = this.temporalFieldKind(this.coercionKey(builder), field); + // Same presentation `find()` gives the column (#3797, #3849) — a caller + // listing a datetime's values should not get epoch integers here and ISO + // strings there, nor `0`/`1` for a boolean. Re-deduplicate afterwards: SQL + // `DISTINCT` compares STORED values, and one SQLite `Field.datetime` column + // holds both INTEGER epoch ms and ISO TEXT, so two rows recording the same + // instant survive as two rows and then collapse to the same presented value. + const kind = this.readPresentationKind(this.coercionKey(builder), field); if (!kind) return values; - return [...new Set(values.map((v: any) => this.presentTemporalValue(kind, v)))]; + return [...new Set(values.map((v: any) => this.presentReadValue(kind, v)))]; } // =================================== @@ -3532,23 +3542,64 @@ export class SqlDriver implements IDataDriver { } /** - * Present one temporal value exactly the way `formatOutput` presents it on a - * `find()` row, for the read paths that return raw builder output instead - * (`aggregate`, `distinct` — #3797). + * Which read-presentation rule, if any, a declared field takes — the same + * question {@link formatOutput} answers implicitly while walking a `find()` + * row, asked one field at a time so the paths that return raw builder output + * can ask it too. `null` means the stored form already IS the presented form. + * + * The boolean / numeric rules are SQLite-only because `formatOutput` gates + * them that way: SQLite is the dialect without a native boolean, and the + * numeric repair only exists for legacy TEXT-affinity columns. + */ + protected readPresentationKind( + table: string | null | undefined, + field: string, + ): ReadPresentationKind | null { + if (!table) return null; + const temporal = this.temporalFieldKind(table, field); + if (temporal) return temporal; + if (!this.isSqlite) return null; + if (this.booleanFields[table]?.includes(field)) return 'boolean'; + if (this.numericFields[table]?.includes(field)) return 'number'; + return null; + } + + /** + * Present one value exactly the way `formatOutput` presents it on a `find()` + * row, for the read paths that return raw builder output instead + * (`aggregate`, `distinct` — #3797 for instants, #3849 for scalars). * - * The dialect gating mirrors `formatOutput`: the `Field.datetime` repair is - * SQLite-only (it is the one dialect where storage ≠ presentation), while the - * `Field.date` → `YYYY-MM-DD` collapse runs everywhere. + * The dialect gating mirrors `formatOutput`: the `Field.datetime` repair and + * the boolean / numeric coercions are SQLite-only (it is the one dialect where + * storage ≠ presentation), while the `Field.date` → `YYYY-MM-DD` collapse runs + * everywhere. {@link readPresentationKind} does the SQLite gating for the + * scalar kinds, so by the time one arrives here the dialect is settled. */ - protected presentTemporalValue(kind: 'datetime' | 'date', value: any): any { + protected presentReadValue(kind: ReadPresentationKind, value: any): any { if (value == null) return value; - if (kind === 'date') return this.toDateOnly(value); - return this.isSqlite ? normalizeSqliteDatetimeOutput(value) : value; + switch (kind) { + case 'date': + return this.toDateOnly(value); + case 'datetime': + return this.isSqlite ? normalizeSqliteDatetimeOutput(value) : value; + case 'boolean': + return Boolean(value); + case 'number': { + // Only strings are repaired, exactly as in `formatOutput`: a fresh + // REAL/INTEGER column already yields a number, and genuinely + // non-numeric legacy junk is left intact rather than turned into NaN. + if (typeof value === 'string' && value.trim() !== '') { + const n = Number(value); + if (!Number.isNaN(n)) return n; + } + return value; + } + } } /** - * Apply {@link presentTemporalValue} to the result columns a caller of - * `aggregate()` will read as instants. + * Apply {@link presentReadValue} to the result columns a caller of + * `aggregate()` will read as column VALUES — group keys, and `min`/`max`. * * Which columns those are cannot be recovered from the rows — the driver has * to be told, because the mapping from column name to meaning is only @@ -3556,12 +3607,12 @@ export class SqlDriver implements IDataDriver { * alias; a date-BUCKETED column lands under the field name but holds a label). * Rows are mutated in place, as `formatOutput` does. */ - protected presentTemporalColumns(rows: any, columns: Map): any { + protected presentReadColumns(rows: any, columns: Map): any { if (columns.size === 0 || !Array.isArray(rows)) return rows; for (const row of rows) { if (!row || typeof row !== 'object') continue; for (const [column, kind] of columns) { - if (row[column] !== undefined) row[column] = this.presentTemporalValue(kind, row[column]); + if (row[column] !== undefined) row[column] = this.presentReadValue(kind, row[column]); } } return rows; diff --git a/packages/qa/dogfood/test/group-key-read-shape-parity.test.ts b/packages/qa/dogfood/test/group-key-read-shape-parity.test.ts new file mode 100644 index 0000000000..d0b9356bd7 --- /dev/null +++ b/packages/qa/dogfood/test/group-key-read-shape-parity.test.ts @@ -0,0 +1,152 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// Group-key read-shape parity (#3849). +// +// A `groupBy` key is a COLUMN VALUE, so there is only one right answer for what +// it looks like: whatever that column looks like on a `find()` row. Three code +// paths produce it and all three must agree — +// +// find() → `formatOutput`, the canonical read shape +// aggregate() pushed down → raw builder output + per-column presentation +// applyInMemoryAggregation() → passes through find()'s own output +// +// — because `engine.aggregate` picks between the latter two per query (does the +// driver aggregate natively? does it advertise the requested granularity? is the +// reference timezone UTC?), and `find()` is what every non-aggregated read of +// the same column returns. +// +// They did not agree. The in-memory path `String()`-coerced every key (`3` → +// `'3'`, `true` → `'true'`), and the pushed-down path skipped the boolean and +// numeric repairs `formatOutput` applies, so a SQLite boolean surfaced as `0`/`1` +// there and `'false'`/`'true'` here. Three paths, three answers, same column. +// +// The damage was silent because every measure still reconciled. What broke was +// downstream code that probes a raw `Map` keyed by the value's own type — the +// select-option label table, the lookup FK → record-name table, the cross-object +// FK → attribute table. `Map` lookup is SameValueZero, so `'1'` never finds `1`: +// labels fell back to raw ids, and cross-object rebucketing filed every row +// under `'(restricted)'` while the totals stayed correct. +// +// This asserts the TYPE as well as the value. Folding both sides through +// `String()` — the reflex when comparing bucket labels — is exactly what hid +// this, and would make the check pass against the bug it exists to catch. + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { applyInMemoryAggregation } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { SqliteWasmDriver } from '@objectstack/driver-sqlite-wasm'; + +const TABLE = 'deal'; + +const DRIVERS = [ + { + name: 'driver-sql (better-sqlite3 :memory:)', + make: () => + new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }) as any, + }, + { + name: 'driver-sqlite-wasm (:memory:)', + make: () => new SqliteWasmDriver({ filename: ':memory:' }) as any, + }, +]; + +/** + * Every column whose stored form differs from its presented form on SQLite, plus + * a text control. `text` is the shape that always agreed — if it ever fails, the + * harness itself is wrong, not the driver. + */ +const COLUMNS = ['qty', 'won', 'stage'] as const; + +/** `value` pairs, sorted. Carrying the TYPE is the whole point. */ +function shape(rows: any[], field: string): string[] { + return rows + .map((r) => `${JSON.stringify(r[field]) ?? 'undefined'}<${r[field] === null ? 'null' : typeof r[field]}>`) + .sort(); +} + +describe.each(DRIVERS)('group-key read-shape parity: $name', ({ make }) => { + let driver: any; + let rows: any[]; + + beforeEach(async () => { + driver = make(); + await driver.initObjects([ + { + name: TABLE, + fields: { + qty: { type: 'number' }, + won: { type: 'boolean' }, + stage: { type: 'text' }, + amount: { type: 'number' }, + }, + }, + ]); + const opts = { bypassTenantAudit: true }; + // Two rows share `qty`/`stage` and differ on `won`, so every column produces + // both a merged and a split bucket — a key that silently changed shape would + // otherwise be able to hide behind a one-row-per-bucket fixture. + await driver.create(TABLE, { id: 'a', qty: 3, won: true, stage: 'won', amount: 1 }, opts); + await driver.create(TABLE, { id: 'b', qty: 3, won: false, stage: 'won', amount: 2 }, opts); + await driver.create(TABLE, { id: 'c', qty: 7, won: false, stage: 'lost', amount: 4 }, opts); + // What `engine.aggregate` feeds the in-memory fallback, and the canonical + // presentation both aggregate paths are measured against. + rows = await driver.find(TABLE, { object: TABLE }); + }); + + afterEach(async () => { + await driver?.disconnect?.(); + }); + + it.each(COLUMNS)("groups by '%s' identically on both paths, in find()'s shape", async (field) => { + const ast = { + object: TABLE, + groupBy: [field], + aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }], + }; + + const pushedDown = await driver.aggregate(TABLE, ast); + const inMemory = applyInMemoryAggregation(rows, ast as never); + + // The two aggregate paths agree with each other… + expect(shape(pushedDown, field)).toEqual(shape(inMemory, field)); + // …and with the shape the same column takes on a plain read. Deduplicated, + // because find() returns one entry per ROW and aggregate one per BUCKET. + const fromFind = [...new Set(shape(rows, field))].sort(); + expect([...new Set(shape(pushedDown, field))].sort()).toEqual(fromFind); + + // Totals were never the broken part; assert them so a "fix" that lost rows + // while making the keys agree cannot pass. + const total = (rs: any[]) => rs.reduce((a, r) => a + Number(r.total), 0); + expect(total(pushedDown)).toBe(7); + expect(total(inMemory)).toBe(7); + }); + + // Spelled out separately from the parametrised case above: these are the two + // concrete shapes #3849 was filed about, and naming them makes a regression + // report readable without decoding a `shape()` string. + it('keeps a number key a number and a boolean key a boolean', async () => { + const by = async (field: string) => { + const ast = { + object: TABLE, + groupBy: [field], + aggregations: [{ function: 'count', alias: 'n' }], + }; + return { + sql: (await driver.aggregate(TABLE, ast)).map((r: any) => r[field]).sort(), + mem: applyInMemoryAggregation(rows, ast as never).map((r: any) => r[field]).sort(), + }; + }; + + const qty = await by('qty'); + expect(qty.sql).toEqual([3, 7]); + expect(qty.mem).toEqual([3, 7]); + + const won = await by('won'); + expect(won.sql).toEqual([false, true]); + expect(won.mem).toEqual([false, true]); + }); +});