feat(drivers,spec)!: GroupByNode.alias is honoured by the three SQL faces (#6401)

.changeset/groupby-alias-multi-face.md

@@ -0,0 +1,101 @@
+---
+"@objectstack/driver-sql": minor
+"@objectstack/driver-turso": minor
+"@objectstack/driver-sqlite-wasm": minor
+"@objectstack/spec": minor
+"@objectstack/objectql": patch
+---
+
+feat(drivers,spec)!: `GroupByNode.alias` is honoured by the SQL faces — one aggregate, one column key (#6401)
+
+`GroupByNodeSchema` has declared `alias` ("Alias for the projected group
+value", defaulting to `field`) for as long as the structured `groupBy` entry has
+existed. Exactly one execution path read it. The result: the SAME query came
+back with a different result-column key depending on which path the engine
+happened to take.
+
+```ts
+groupBy: [{ field: 'closed_at', dateGranularity: 'month', alias: 'qtr' }]
+```
+
+- pushed down to a driver ⇒ rows keyed **`closed_at`**
+- run through the in-memory fallback ⇒ rows keyed **`qtr`**
+
+And the choice between them is `engine.ts`'s
+`allStructuredSupported && !tzRequiresInMemory` — a driver capability bit and a
+`timezone`, neither of which the caller can see. That is the multi-face
+consistency invariant broken in its quietest form: both answers are valid rows,
+so nothing throws and nothing looks wrong.
+
+**Resolved to ENFORCE**, and the leg was chosen by measurement rather than
+taste. ADR-0049 splits on whether the feature already exists: a *dangling*
+promise is removed, a *live* one with a missing gate is enforced. `alias` is
+live — three consumers read it and change behaviour
+(`in-memory-aggregation.ts`, `MemoryDriver.performAggregation`, and
+`chartAggregateCategoryKey`), and the publish gate *compels* it:
+`validate-react-page-props.ts` errors `REACT_CHART_AXIS_UNKNOWN` unless a
+chart's category axis is bound to `alias ?? field`, telling the author in so
+many words to "bind it to" the alias. A key the build gate makes you write is
+not a dangling promise. The count of real non-test producers is **zero**, which
+is what makes enforcing safe rather than what argues against it: no shipped
+payload changes its result keys.
+
+**What changed, on every SQL face at once** — a fix landing on one and not its
+twin is the #6203 shape, and `TursoDriver` picks its face from `url`:
+
+- **`driver-sql`** — both limbs of the structured `groupBy` branch project
+ `alias ?? field`: the date-bucket limb aliases the bucket expression to it,
+ and the plain limb emits `?? as ??` (only when the name actually moves — an
+ alias equal to the field emits no self-rename). `presentedOutput` is now keyed
+ by the OUTPUT column, matching how the aggregation branch beside it has always
+ worked; an aliased group value went unpresented before.
+- **`driver-turso` REMOTE** — the same projection, `"field" AS "alias"`. The
+ alias reaches the statement as a quoted identifier and is therefore held to
+ `assertSafeIdentifier`, exactly like `field`.
+- **`driver-sqlite-wasm`** — inherits `SqlDriver`'s compiler; covered by its own
+ conformance suite rather than by assumption.
+
+**GROUP BY still keys on the FIELD** on every face. Only the projection is
+renamed, so the buckets are unchanged. This is deliberate and pinned: SQLite
+resolves output names in `GROUP BY`, so a face that grouped by the alias would
+look correct here and diverge on a dialect that does not.
+
+`having` needed no change and now means one thing: it is applied over the
+aggregated row's own columns, so a filter on a group projection references the
+alias on every path — previously the alias on one path and the field on the
+other.
+
+**Conformance.** `AGGREGATION_CASES` (#6409) gains a `groupByAlias` axis and two
+cases. Their VALUES are an existing case verbatim — only the key moves — so they
+can fail only on the key, which is the point: every wrong answer in this area is
+a valid query returning plausible rows. `objectql`'s in-memory fallback is now
+**enrolled** as a fourth face, answering #6409's open question ②: it is the face
+the SQL three were converged onto, so the new behaviour would otherwise be
+pinned against nothing, and reaching it needs no engine at all —
+`applyInMemoryAggregation` is a pure function of rows and an AST.
+
+**Reverse verification**, predicted before running. Reverting the in-memory face
+to `g.field`: only the two alias cases move and only ONE fails — the degenerate
+`alias === field` case stays green, which is why both are in the table.
+Reverting the harness to read `c.groupBy` instead of `c.groupByAlias ?? c.groupBy`
+— the copied-neighbour mistake: everything passes on an unmodified face, a false
+GREEN, which is the failure mode that would have made the axis vacuous.
+
+**Frozen drivers (#5499), measured from source, not flipped.** `driver-memory`
+already returned `{ field, alias: node.alias ?? node.field }` and projects under
+the alias — it had independently reached the enforce answer, so it needed no
+alignment. `driver-mongodb` is a recorded DEBT row and the defect is wider than
+`alias`: `buildAggregationPipeline` types `groupBy` as `string[]` and builds
+`groupId[field] = '$' + field`, so a structured node — aliased or not — becomes
+the literal key `"[object Object]"`. It cannot take a structured `GroupByNode`
+at all; `mongodb-driver.ts` passes `(query as any).groupBy`, which is why `tsc`
+never saw it. Tracked on #6814.
+
+**Compatibility.** A caller who writes `alias` and reads the result under
+`field` on a pushdown path will now find the value under `alias` — which is what
+the key has always meant on the fallback path, and what the chart gate already
+required. Callers who never write `alias` are unaffected: the emitted SQL is
+byte-identical.
+
+<!-- adr-0087: not-required (no-migration-prescription) Nothing is retired: `GroupByNodeSchema.alias` keeps its declaration, its spelling and its type — it starts being HONOURED by three faces that parsed and ignored it. There is no tombstone to write and no authored metadata to rewrite, so there is no mechanical transform a migration could prescribe: every stack that validated before validates after, unchanged. The behaviour change is in the RESULT of a runtime query (a result-column key moves from `field` to `alias` on the pushdown path, converging on what the in-memory path and the chart publish gate already required), which the ledger has no channel for and no upgrader could apply a codemod to. The bang is on the changeset because callers who read that column by the field name must move, and the measured non-test producer count for the key is zero. -->
+

packages/drivers/driver-sql/src/sql-driver-aggregation-conformance.test.ts

@@ -80,7 +80,12 @@ const CONFORMANCE_OBJECT = {
 const astFor = (c: AggregationCase): QueryAST => ({
  object: CONFORMANCE_OBJECT.name,
  aggregations: [{ function: c.function, ...(c.field ? { field: c.field } : {}), alias: 'n' }],
- ...(c.groupBy ? { groupBy: [c.groupBy] } : {}),
+ // [#6401] A case carrying `groupByAlias` is sent as the STRUCTURED node, so
+ // what the face receives is the union member that declares `alias`. Without
+ // this the alias cases would send a bare string and pin nothing.
+ ...(c.groupBy
+ ? { groupBy: [c.groupByAlias ? { field: c.groupBy, alias: c.groupByAlias } : c.groupBy] }
+ : {}),
 });
 
 /**
@@ -89,10 +94,15 @@ const astFor = (c: AggregationCase): QueryAST => ({
  * numbers — SQLite hands `avg` back as a float and `count` as an integer, and
  * neither is the property under test.
  */
-const actualFor = (c: AggregationCase, rows: Array<Record<string, unknown>>) =>
- rows
- .map((r) => ({ group: c.groupBy ? String(r[c.groupBy]) : null, value: Number(r.n) }))
+const actualFor = (c: AggregationCase, rows: Array<Record<string, unknown>>) => {
+ // [#6401] The group value is read from the column the case SAYS it lands in —
+ // `groupByAlias ?? groupBy`. Reading `c.groupBy` unconditionally is the bug
+ // this axis exists to catch: it is green on a face that ignores the alias.
+ const groupKey = c.groupByAlias ?? c.groupBy;
+ return rows
+ .map((r) => ({ group: groupKey ? String(r[groupKey]) : null, value: Number(r.n) }))
  .sort((x, y) => String(x.group).localeCompare(String(y.group)));
+};
 
 describe('[#6409] SqlDriver — aggregate vocabulary conformance', () => {
  let driver: SqlDriver;

packages/drivers/driver-sql/src/sql-driver.ts

@@ -4103,7 +4103,8 @@ export class SqlDriver implements IDataDriver {
  // groupBy items may be plain strings ('region') or structured objects
  // ({ field: 'closed_at', dateGranularity: 'quarter' }). For structured
  // items we emit a dialect-specific bucket expression aliased as the
- // field name so the resulting row keys match in-memory bucketDateValue.
+ // projected column name so the resulting row keys match in-memory
+ // bucketDateValue — see the `outKey` note below for what that name is.
  // [#6212] The element type is `GroupByNode` — the spec's own union — so
  // the local `Array<string | { field, dateGranularity? }>` restatement is
  // gone. It had drifted from the declaration it was restating: `alias` was
@@ -4116,6 +4117,18 @@ export class SqlDriver implements IDataDriver {
  const kind = this.readPresentationKind(table, g);
  if (kind) presentedOutput.set(g, kind);
  } else if (g && typeof g === 'object' && g.field) {
+ // [#6401] The projected column is named `alias ?? field` — the rule
+ // `AggregationNodeSchema.alias` already gets a few dozen lines below,
+ // and the one `in-memory-aggregation.ts` has always applied
+ // (`g.alias ?? g.field`). This face was the half that PARSED the key
+ // and ignored it, so one aggregate came back keyed by `closed_at`
+ // under pushdown and by `qtr` under the in-memory fallback — decided
+ // by a driver capability bit and a timezone the caller cannot see
+ // (`engine.ts`'s `allStructuredSupported && !tzRequiresInMemory`
+ // fork). GROUP BY still keys on the FIELD; only the projection is
+ // renamed, so the buckets are identical and only their column name
+ // moves.
+ const outKey = g.alias ?? g.field;
  if (g.dateGranularity) {
  const bucket = this.buildDateBucketExpr(g.field, g.dateGranularity, table);
  if (!bucket) {
@@ -4129,12 +4142,18 @@ export class SqlDriver implements IDataDriver {
  );
  }
  builder.groupByRaw(bucket.sql, bucket.bindings);
- builder.select(this.knex.raw(`${bucket.sql} as ??`, [...bucket.bindings, g.field]));
+ builder.select(this.knex.raw(`${bucket.sql} as ??`, [...bucket.bindings, outKey]));
  } else {
  builder.groupBy(g.field);
- builder.select(g.field);
+ // `?? as ??` only when the name actually moves: an alias equal to
+ // the field would otherwise rewrite `select "region"` into
+ // `select "region" as "region"` on every dialect for no gain.
+ builder.select(outKey === g.field ? g.field : this.knex.raw('?? as ??', [g.field, outKey]));
+ // Keyed by the OUTPUT column, like the aggregation branch below —
+ // `presentReadColumns` matches on the name the row actually
+ // carries, so an aliased group value went unpresented before.
  const kind = this.readPresentationKind(table, g.field);
- if (kind) presentedOutput.set(g.field, kind);
+ if (kind) presentedOutput.set(outKey, kind);
  }
  }
  }

packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-aggregation-conformance.test.ts

@@ -34,13 +34,23 @@ const OBJECT = 'conformance_agg';
 const astFor = (c: AggregationCase): QueryAST => ({
  object: OBJECT,
  aggregations: [{ function: c.function, ...(c.field ? { field: c.field } : {}), alias: 'n' }],
- ...(c.groupBy ? { groupBy: [c.groupBy] } : {}),
+ // [#6401] A case carrying `groupByAlias` is sent as the STRUCTURED node, so
+ // what the face receives is the union member that declares `alias`. Without
+ // this the alias cases would send a bare string and pin nothing.
+ ...(c.groupBy
+ ? { groupBy: [c.groupByAlias ? { field: c.groupBy, alias: c.groupByAlias } : c.groupBy] }
+ : {}),
 });
 
-const actualFor = (c: AggregationCase, rows: Array<Record<string, unknown>>) =>
- rows
- .map((r) => ({ group: c.groupBy ? String(r[c.groupBy]) : null, value: Number(r.n) }))
+const actualFor = (c: AggregationCase, rows: Array<Record<string, unknown>>) => {
+ // [#6401] The group value is read from the column the case SAYS it lands in —
+ // `groupByAlias ?? groupBy`. Reading `c.groupBy` unconditionally is the bug
+ // this axis exists to catch: it is green on a face that ignores the alias.
+ const groupKey = c.groupByAlias ?? c.groupBy;
+ return rows
+ .map((r) => ({ group: groupKey ? String(r[groupKey]) : null, value: Number(r.n) }))
  .sort((x, y) => String(x.group).localeCompare(String(y.group)));
+};
 
 describe('[#6409] driver-sqlite-wasm — aggregate vocabulary conformance', () => {
  let driver: SqliteWasmDriver;

packages/drivers/driver-turso/src/remote-transport-groupby-node.test.ts

@@ -47,14 +47,21 @@
  * envelope would have been a new fork, so both faces were moved together and the
  * parity block below compares their runtime messages.
  *
- * # `alias` is deliberately NOT read
+ * # `alias` IS read — as of #6401
  *
- * `GroupByNodeSchema.alias` is honoured by the in-memory path
- * (`in-memory-aggregation.ts` projects `g.alias ?? g.field`) and ignored by
- * `SqlDriver.aggregate`. Reading it here would make this transport the only SQL
- * face that honours it — a new divergence dressed as a fix. It is ignored, in
- * step with the local face, and the pushdown/in-memory disagreement is filed
- * separately; it is not created here.
+ * This section used to read "`alias` is deliberately NOT read", and the reason
+ * it gave was sound: `GroupByNodeSchema.alias` was honoured by the in-memory
+ * path (`in-memory-aggregation.ts` projects `g.alias ?? g.field`) and ignored
+ * by `SqlDriver.aggregate`, so reading it HERE alone would have made this
+ * transport the only SQL face that did — a new divergence dressed as a fix. The
+ * disagreement was filed separately instead (#6401).
+ *
+ * That issue resolved to ENFORCE, and moved all three SQL faces in one change:
+ * `driver-sql`, this transport, and `driver-sqlite-wasm` (which inherits
+ * `SqlDriver`'s compiler). The projected column is `alias ?? field` everywhere;
+ * GROUP BY still keys on the FIELD. So the deferral is discharged rather than
+ * reversed — the condition it named ("only one face would read it") is what
+ * stopped being true.
  *
  * # Reverse verification — direction predicted BEFORE it was run, per case
  *
@@ -176,17 +183,60 @@ describe('[#6212] RemoteTransport compiles the GroupByNode union', () => {
  expect(calls[0].sql).toBe('SELECT "stage", count("stage") AS "n" FROM "deal" GROUP BY "stage"');
  });
 
- it('ignores `alias`, in step with the local face', async () => {
- // Pinned as a DELIBERATE choice, not an oversight: `SqlDriver.aggregate`
- // does not read `alias` either, so honouring it only here would make this
- // transport the one SQL face that does. See the file header.
+ it('[#6401] projects `alias` as the column name, and still GROUPS BY the field', async () => {
+ // Flipped from 'ignores `alias`, in step with the local face'. That pin
+ // was correct when written — at #6212 this transport was the only face
+ // that could have started reading the key, and doing so alone would have
+ // been a new divergence. #6401 moved all three SQL faces together, so the
+ // step it was keeping is now a step toward the alias, not away from it.
+ //
+ // The old assertion is REPLACED, not dropped: it claimed the alias never
+ // reaches the statement; the new truth is the exact statement that
+ // carries it — projection renamed, grouping untouched.
  const { t, calls } = transportWithCapturingClient();
  await t.aggregate('deal', {
  groupBy: [{ field: 'stage', alias: 'bucket' }],
  aggregations: [{ function: 'count', field: 'stage', alias: 'n' }],
  });
+ expect(calls[0].sql).toBe(
+ 'SELECT "stage" AS "bucket", count("stage") AS "n" FROM "deal" GROUP BY "stage"',
+ );
+ // ⛔ The grouping key is the FIELD. SQLite resolves output names in
+ // GROUP BY, so a face that grouped by the alias would return identical
+ // rows here and diverge on a dialect that does not.
+ expect(calls[0].sql).not.toContain('GROUP BY "bucket"');
+ });
+
+ it('[#6401] an alias equal to the field name emits no self-rename', async () => {
+ const { t, calls } = transportWithCapturingClient();
+ await t.aggregate('deal', {
+ groupBy: [{ field: 'stage', alias: 'stage' }],
+ aggregations: [{ function: 'count', field: 'stage', alias: 'n' }],
+ });
+ // Byte-identical to the string spelling above — the degenerate alias must
+ // not start rewriting statements on every dialect for no gain.
  expect(calls[0].sql).toBe('SELECT "stage", count("stage") AS "n" FROM "deal" GROUP BY "stage"');
- expect(calls[0].sql).not.toContain('bucket');
+ });
+
+ it('[#6401] refuses an unsafe identifier in `alias`, not only in `field`', async () => {
+ // The alias is caller-supplied text that now reaches the statement as a
+ // quoted identifier, so it needs the gate `field` already has. The
+ // assertion names the OFFENDING TEXT, not just the sentence (#6144): a
+ // `field` that is itself safe is what makes this case reach the alias
+ // check at all.
+ const { t, calls } = transportWithCapturingClient();
+ const err = await t
+ .aggregate('deal', {
+ groupBy: [{ field: 'stage', alias: 'bucket"; DROP TABLE deal; --' }],
+ aggregations: [{ function: 'count', alias: 'n' }],
+ })
+ .then(
+ () => { throw new Error('expected the transport to refuse an unsafe alias'); },
+ (e) => e as Error,
+ );
+ expect(err.message).toContain('unsafe identifier rejected');
+ expect(err.message).toContain('bucket"; DROP TABLE deal; --');
+ expect(calls).toEqual([]);
  });
 
  it('still refuses an unsafe identifier inside a structured entry', async () => {