From 643d437803a8792b61f0aebd42081d983b529711 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 05:26:06 +0000 Subject: [PATCH 1/3] fix(driver-mongodb): refuse retired array_agg / string_agg instead of lowering them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `buildAccumulator` carried `case 'array_agg'` and `case 'string_agg'` arms (both lowering to `$push`) plus a matching `string_agg` join in `postProcessAggregation`. Both names left `AggregationFunction` at #6188 under ADR-0049 enforce-or-remove; `driver-sql` and `driver-turso` have refused them as class-1 undeclared names ever since. This face was the only one still answering them, so one query got a 400 on two backends and a `$push` array on the third. Both now answer INVALID_QUERY/400 — answer-for-answer parity with both SQL faces. They are named explicitly rather than left to fall through: the `default` arm still answers `{ $sum: ... }`, so a bare deletion would turn a visibly-wrong array into an arithmetically plausible number, which is the defect #12818 is fixing in that arm. The `converts string_agg arrays to joined strings` pin is INVERTED IN PLACE, not re-baselined or deleted; a positive control walking `AggregationFunction.options` pins that the narrowing did not go too wide. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LZbWd2jNV1FErXTPSS4Dry --- .../mongodb-retired-agg-arms-refused.md | 45 ++++++++ .../src/mongodb-aggregation.test.ts | 66 +++++++++++- .../driver-mongodb/src/mongodb-aggregation.ts | 102 +++++++++++++++--- .../src/mongodb-pipeline-evaluator.testkit.ts | 8 +- 4 files changed, 200 insertions(+), 21 deletions(-) create mode 100644 .changeset/mongodb-retired-agg-arms-refused.md diff --git a/.changeset/mongodb-retired-agg-arms-refused.md b/.changeset/mongodb-retired-agg-arms-refused.md new file mode 100644 index 0000000000..3af798c404 --- /dev/null +++ b/.changeset/mongodb-retired-agg-arms-refused.md @@ -0,0 +1,45 @@ +--- +"@objectstack/driver-mongodb": patch +--- + +fix(driver-mongodb): refuse the retired `array_agg` / `string_agg` instead of lowering them (#13075) + +`buildAccumulator` still carried `case 'array_agg'` and `case 'string_agg'` +arms — both lowering to `$push` — plus a matching `string_agg` join in +`postProcessAggregation`. Both names left `AggregationFunction` at **#6188** +under ADR-0049 enforce-or-remove, and both SQL faces have refused them as +class-1 undeclared names ever since (`driver-sql`'s `refuseAggregateFunction`, +`driver-turso`'s `RemoteTransport`, each `INVALID_QUERY` / **400**). +`driver-mongodb` was the only face still answering them, so **one query got a +400 on two backends and a `$push` array on the third** — the local/remote fork +#5907 exists to prevent, one vocabulary later. + +Why this face kept them when `objectql`'s in-memory fallback deleted its arms +for the same two names at #6188: that fallback switches on the **enum type**, so +`case 'array_agg'` there stopped type-checking the moment the value left the +enum. `AggregationInput.function` here is a bare `string` — the driver's own +`aggregate` reads aggregations through an `any` cast — so these arms compiled +fine and survived the retirement unnoticed. + +Both names now answer `INVALID_QUERY` / **400**, answer-for-answer parity with +both SQL faces. They are named explicitly rather than left to fall through, +because falling through is not currently safe: `buildAccumulator`'s `default` +arm answers `{ $sum: … }`, so deleting the arms alone would turn a visibly-wrong +ARRAY into an arithmetically PLAUSIBLE NUMBER — strictly the worse failure, and +the very defect #12818 is fixing in that arm. Naming them is correct whichever +order the two land in, and after #12818 lands the arm still draws the +distinction `AggregationFunction`'s own error map draws: a caller who bypassed +the parse door is told the name was **removed**, not merely unrecognised. + +The retirement prescription itself is not restated here — it lives once, on the +enum's error map in `@objectstack/spec`, and a copy in the driver would be a +second wording of one vocabulary with nothing keeping the two in step. + +**Graded `patch`, deliberately.** No correct query's answer moves: all six +declared functions are byte-identically unchanged, pinned by a positive control +in the same suite that walks `AggregationFunction.options`. `AggregationNodeSchema` +already rejects both spellings at the parse door, so the only callers whose +behaviour changes are ones reaching the exported builder or the driver's +`aggregate` directly — and they were reading a value the protocol has no name +for. Nothing to migrate: read the rows with an ordinary `fields` query and shape +them in the caller, or model the roll-up as a stored field. diff --git a/packages/drivers/driver-mongodb/src/mongodb-aggregation.test.ts b/packages/drivers/driver-mongodb/src/mongodb-aggregation.test.ts index e51c4a3966..f05fed9d5e 100644 --- a/packages/drivers/driver-mongodb/src/mongodb-aggregation.test.ts +++ b/packages/drivers/driver-mongodb/src/mongodb-aggregation.test.ts @@ -1,6 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect } from 'vitest'; +import { AggregationFunction } from '@objectstack/spec/data'; import { buildAggregationPipeline, postProcessAggregation } from './mongodb-aggregation.js'; /** @@ -118,12 +119,21 @@ describe('MongoDB Aggregation Pipeline Builder', () => { expect(processed[1].unique_customers).toBe(2); }); - it('converts string_agg arrays to joined strings', () => { + // [#13075] INVERTED IN PLACE. This case read `converts string_agg arrays to + // joined strings` and asserted `'Alice, Bob, Charlie'`. That assertion is + // FALSIFIED, not re-baselined: `string_agg` left `AggregationFunction` at + // #6188 (ADR-0049 enforce-or-remove), `buildAccumulator` now refuses the + // name outright, and the join limb in `postProcessAggregation` went with + // it. What the case pins now is the OTHER half of that deletion — the limb + // is gone, so an array reaching this function under a `string_agg` alias is + // handed back untouched rather than quietly reshaped. The refusal itself is + // pinned below, at the door that can still be reached. + it('no longer joins a string_agg array — the limb went with the retired name', () => { const results = [{ names: ['Alice', 'Bob', 'Charlie'] }]; const processed = postProcessAggregation(results, [ { function: 'string_agg', field: 'name', alias: 'names' }, ]); - expect(processed[0].names).toBe('Alice, Bob, Charlie'); + expect(processed[0].names).toEqual(['Alice', 'Bob', 'Charlie']); }); it('passes through results with no special processing needed', () => { @@ -134,4 +144,56 @@ describe('MongoDB Aggregation Pipeline Builder', () => { expect(processed).toEqual(results); }); }); + + /** + * [#13075] `array_agg` / `string_agg` are REFUSED, where this face lowered + * both until now — the divergence this card closed. `AggregationFunction` + * declares six functions; #6188 removed these two under ADR-0049 + * enforce-or-remove, and `driver-sql` and `driver-turso` have refused them as + * class-1 undeclared names ever since. This face kept answering them, so ONE + * query got a 400 on two backends and a `$push` array on the third. + * + * The envelope is asserted, not the throw: `code` and `status` are the + * contract (ADR-0112), and a bare `toThrow()` would pass just as well against + * a driver throwing a naked `Error` — which is precisely the #1116/#1117 gap + * the two-class refusal (#5907) exists to close. + */ + describe('[#13075] retired aggregate functions', () => { + for (const func of ['array_agg', 'string_agg'] as const) { + it(`refuses ${func} with INVALID_QUERY/400, the class-1 answer both SQL faces give`, () => { + let thrown: (Error & { code?: string; status?: number }) | undefined; + try { + buildAggregationPipeline({ + aggregations: [{ function: func, field: 'name', alias: 'out' }], + }); + } catch (e) { + thrown = e as Error & { code?: string; status?: number }; + } + expect(thrown, `${func} must be refused, not lowered`).toBeDefined(); + // Class 1 (#5907): the protocol no longer HAS this name. Distinct from + // NOT_IMPLEMENTED/501, which says the backend cannot lower a name the + // spec still declares. + expect(thrown!.code).toBe('INVALID_QUERY'); + expect(thrown!.status).toBe(400); + // The wording IS the contract here: a caller who bypassed the parse + // door has no other way to learn the name was RETIRED rather than + // merely misspelled. + expect(thrown!.message).toContain('was REMOVED'); + expect(thrown!.message).toContain('#6188'); + }); + } + + it('still lowers every function AggregationFunction declares', () => { + // The other half of the narrowing: exactly the declared six survive, so a + // refusal that grew too wide fails here rather than in a dashboard. + for (const func of AggregationFunction.options) { + expect( + () => buildAggregationPipeline({ + aggregations: [{ function: func, field: 'amount', alias: 'out' }], + }), + `${func} is declared and must still lower`, + ).not.toThrow(); + } + }); + }); }); diff --git a/packages/drivers/driver-mongodb/src/mongodb-aggregation.ts b/packages/drivers/driver-mongodb/src/mongodb-aggregation.ts index c40300b0c8..b21ced0ecc 100644 --- a/packages/drivers/driver-mongodb/src/mongodb-aggregation.ts +++ b/packages/drivers/driver-mongodb/src/mongodb-aggregation.ts @@ -9,6 +9,7 @@ import type { Document } from 'mongodb'; import { StandardErrorCode } from '@objectstack/spec/api'; +import { AggregationFunction } from '@objectstack/spec/data'; import type { DateGranularityValue, GroupByNode } from '@objectstack/spec/data'; import { translateFilter } from './mongodb-filter.js'; import type { TemporalFieldKindResolver } from './mongodb-temporal.js'; @@ -528,6 +529,72 @@ function numericAggregandExpr(path: string): Document { return { $cond: [{ $eq: [{ $type: path }, 'bool'] }, { $cond: [path, 1, 0] }, path] }; } +/** + * [#13075] `array_agg` / `string_agg` — names the Query Protocol RETIRED. + * + * Both left `AggregationFunction` at #6188 under ADR-0049 enforce-or-remove: no + * SQL backend ever compiled either, and `string_agg` never had one shape to + * lower to (the delimiter is a second argument in PostgreSQL, a `SEPARATOR` + * clause in MySQL and a differently-named function in SQL Server). This face + * kept lowering both anyway, so ONE query answered `400` on `driver-sql` and + * `driver-turso` and a `$push` array here — the local/remote fork #5907 exists + * to prevent, one vocabulary later. + * + * ## Why 400 and not 501 — the #5907 classification + * + * `driver-sql` sorts every refused aggregate into two classes and these two are + * class 1: `refuseAggregateFunction` asks whether the spec still DECLARES the + * name, and answers `INVALID_QUERY`/400 when it does not. "The protocol has no + * such function" is a different fact from "this backend cannot lower it" + * (`NOT_IMPLEMENTED`/501, the class {@link refusePerAggregationFilter} and + * {@link refuseDateBucketedGroupBy} answer in) and deserves the different + * answer. `driver-turso`'s `RemoteTransport` carries the same note verbatim. + * So this refusal is answer-for-answer parity with both SQL faces. + * + * ## Why these two are NAMED here rather than left to the `default` arm + * + * `objectql`'s in-memory fallback deleted its arms for these two outright at + * #6188 and let them fall through, which it could do safely because its switch + * is over the ENUM TYPE — `case 'array_agg'` there does not type-check, which + * is exactly why that face could not keep them by accident and this one could. + * `AggregationInput.function` is a bare `string` (the driver's own `aggregate` + * reads aggregations through an `any` cast), so the arms here compiled fine and + * survived the retirement unnoticed. + * + * Falling through is ALSO not currently safe here: this builder's `default` arm + * answers `{ $sum: … }`, so deleting these two arms without naming them would + * turn a visibly-wrong ARRAY into an arithmetically PLAUSIBLE NUMBER — strictly + * the worse failure, and the very defect #12818 is fixing in that arm. Naming + * them is correct whichever order the two land in: before #12818's fix it is + * the only thing standing between these names and a silent sum, and after it + * the two agree on the answer while this arm keeps telling a caller that the + * name was REMOVED rather than merely unrecognised — the same distinction + * `AggregationFunction`'s own error map draws, and for the same reason (telling + * the author of `arry_agg` that their value "was removed" would misinform). + * + * The prescription itself is deliberately NOT restated here. It lives once, on + * the enum's error map in `@objectstack/spec`, where the parse door hands it to + * every caller who arrives through a spec-valid request; a copy in this file + * would be a second wording of one vocabulary with nothing keeping the two in + * step. This message names where it is and what to do instead in one line. + */ +function refuseRetiredAggregateFunction(func: string): never { + const err = new Error( + `Aggregate function "${func}" was REMOVED from @objectstack/spec ` + + `AggregationFunction at #6188 (ADR-0049 enforce-or-remove) and is not lowered by this ` + + `backend (driver-mongodb). Declared now: ${AggregationFunction.options.join(', ')}. ` + + `This answers INVALID_QUERY/400 rather than NOT_IMPLEMENTED/501 because the protocol no ` + + `longer has this name at all, which is a different fact from a capability gap in the ` + + `backend (#5907) — the same answer \`driver-sql\` and \`driver-turso\` give it. There is no ` + + `replacement in the query vocabulary: read the rows with an ordinary \`fields\` query and ` + + `shape them in the caller, or model the roll-up as a stored field. Parsing the query ` + + `through AggregationNodeSchema reports this with the full retirement prescription.`, + ) as Error & { code?: string; status?: number }; + err.code = StandardErrorCode.enum.INVALID_QUERY; + err.status = 400; + throw err; +} + /** * Build a single MongoDB accumulator expression from an aggregation descriptor. */ @@ -573,12 +640,14 @@ function buildAccumulator(agg: AggregationInput): Document { // on that side rather than in this expression. return { $addToSet: fieldRef ?? null }; + // [#13075] REFUSED, where this face used to LOWER both: `array_agg` to a + // `$push` and `string_agg` to a `$push` plus a join in + // {@link postProcessAggregation}. Both names left `AggregationFunction` at + // #6188; see {@link refuseRetiredAggregateFunction} for why they are named + // here rather than left to fall through. case 'array_agg': - return { $push: fieldRef ?? '$$ROOT' }; - case 'string_agg': - // Collect into array; caller can post-process with $reduce - return { $push: fieldRef ?? '' }; + refuseRetiredAggregateFunction(agg.function); default: return { $sum: fieldRef ?? 0 }; @@ -588,8 +657,18 @@ function buildAccumulator(agg: AggregationInput): Document { /** * Post-process aggregation results. * - * Handles count_distinct conversion ($addToSet → count) and - * string_agg conversion ($push → joined string). + * Handles count_distinct conversion ($addToSet -> count). + * + * ## [#13075] The `string_agg` join is GONE + * + * This function also joined a `string_agg` alias's `$push` array into a + * delimited string. `string_agg` left `AggregationFunction` at #6188, and + * {@link buildAccumulator} now refuses the name outright, so no pipeline this + * builder emits can produce the array that limb existed to reshape — it was + * reachable only for a caller hand-feeding `postProcessAggregation` a result + * set the builder could not have built. Deleted rather than left unreachable, + * the reason `objectql`'s in-memory fallback gives for the same deletion: dead + * arms are how a retired vocabulary comes back by accident. * * ## [#6814] Why the null exclusion is HERE * @@ -628,11 +707,7 @@ export function postProcessAggregation( .filter((a) => a.function === 'count_distinct') .map((a) => a.alias); - const stringAggFields = aggregations - .filter((a) => a.function === 'string_agg') - .map((a) => a.alias); - - if (countDistinctFields.length === 0 && stringAggFields.length === 0) { + if (countDistinctFields.length === 0) { return results; } @@ -645,11 +720,6 @@ export function postProcessAggregation( processed[field] = processed[field].filter((v: unknown) => v != null).length; } } - for (const field of stringAggFields) { - if (Array.isArray(processed[field])) { - processed[field] = processed[field].join(', '); - } - } return processed; }); } diff --git a/packages/drivers/driver-mongodb/src/mongodb-pipeline-evaluator.testkit.ts b/packages/drivers/driver-mongodb/src/mongodb-pipeline-evaluator.testkit.ts index a268c29696..0678ca1403 100644 --- a/packages/drivers/driver-mongodb/src/mongodb-pipeline-evaluator.testkit.ts +++ b/packages/drivers/driver-mongodb/src/mongodb-pipeline-evaluator.testkit.ts @@ -258,9 +258,11 @@ function truthy(v: unknown): boolean { * Evaluate an aggregation EXPRESSION against one document. * * Models only the operators this builder emits. `$$`-prefixed system variables - * are refused rather than guessed at — `$$ROOT` reaches the `array_agg` arm, and - * an evaluator that quietly resolved it would be asserting a semantics nobody - * checked. + * are refused rather than guessed at: an evaluator that quietly resolved one + * would be asserting a semantics nobody checked. `$$ROOT` used to arrive here + * from the builder's fieldless `array_agg` arm, which #13075 deleted with the + * retired name; the refusal stays because it is about system variables in + * general, not about that one arm. */ export function evalExpr(doc: Doc, expr: unknown): unknown { if (typeof expr === 'string' && expr.startsWith('$')) { From 44b1e837206d3e3e659e61293120f8e7407acaa6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 09:48:05 +0000 Subject: [PATCH 2/3] fix(driver-mongodb): reconcile the retired-aggregate refusal with #13076's landed refusal Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LZbWd2jNV1FErXTPSS4Dry --- .../driver-mongodb/src/mongodb-aggregation.ts | 36 ++++--- ...db-unrecognised-aggregate-function.test.ts | 97 ++++++++++++++----- 2 files changed, 98 insertions(+), 35 deletions(-) diff --git a/packages/drivers/driver-mongodb/src/mongodb-aggregation.ts b/packages/drivers/driver-mongodb/src/mongodb-aggregation.ts index 47ab3bd565..fc601da367 100644 --- a/packages/drivers/driver-mongodb/src/mongodb-aggregation.ts +++ b/packages/drivers/driver-mongodb/src/mongodb-aggregation.ts @@ -25,9 +25,14 @@ import type { TemporalFieldKindResolver } from './mongodb-temporal.js'; * no value at this driver's own call site, and this module is EXPORTED, so a * caller hands the builder whatever string it likes. A door that cannot be * reached by the values it governs looks shut and is not. The narrowing would - * additionally delete the `array_agg` / `string_agg` arms below as a side - * effect (both left the enum at #6188), which is a second accept-face change - * and a separate card. See {@link LOWERED_HERE}. + * additionally have deleted the `array_agg` / `string_agg` arms below as a side + * effect (both left the enum at #6188), which was a second accept-face change + * and a separate card: #13075, now LANDED. It did not land as a deletion — + * those two arms are NAMED and refused ({@link refuseRetiredAggregateFunction}), + * because this switch's `default` answers a `$sum` and falling through would + * have turned a visibly-wrong array into a plausible number. The reasoning + * above is unchanged by it: `function` is still a bare `string`, and the + * enforcement is still at the lowering site. See {@link LOWERED_HERE}. */ export interface AggregationInput { function: string; @@ -381,17 +386,24 @@ function malformedGroupByError(node: unknown): Error { * develops the day after it is typed — the note over `driver-memory`'s * `SUPPORTED_FIELD_OPERATORS` (#5345), applied to the aggregate vocabulary. * - * ⚠️ Two entries are NOT declared by `AggregationFunction`: `array_agg` and - * `string_agg` left the enum at #6188 (ADR-0049 enforce-or-remove) and both SQL - * faces refuse them as undeclared names today, while this face still lowers - * them. That divergence is real, it PRE-DATES this change, and closing it is a - * second accept-face narrowing with its own changeset — filed as #13075 rather - * than ridden in here. What it must not do is leak into a refusal: the messages - * offer {@link LOWERED_AND_DECLARED}, because a remedy naming a retired - * spelling is a remedy `AggregationNodeSchema` rejects at the protocol door. + * ⚠️ This roster CARRIED two entries `AggregationFunction` does not declare — + * `array_agg` and `string_agg`, which left the enum at #6188 (ADR-0049 + * enforce-or-remove) while this face went on lowering them, so one query + * answered 400 on `driver-sql` and `driver-turso` and a `$push` array here. + * #13075 CLOSED that divergence: {@link buildAccumulator} names both arms and + * refuses them ({@link refuseRetiredAggregateFunction}), so they are off this + * roster too — a roster that named what the switch no longer lowers would be a + * lie, and it is what the refusal messages read. + * + * The reason they were kept OUT of those messages stands, and is why + * {@link LOWERED_AND_DECLARED} still filters rather than being collapsed into + * this constant: a remedy naming a retired spelling is a remedy + * `AggregationNodeSchema` rejects at the protocol door. The two sets are equal + * TODAY; the filter is what keeps that true of the next name this face lowers + * ahead of the enum, instead of only of those two. */ const LOWERED_HERE: readonly string[] = [ - 'count', 'sum', 'avg', 'min', 'max', 'count_distinct', 'array_agg', 'string_agg', + 'count', 'sum', 'avg', 'min', 'max', 'count_distinct', ]; /** diff --git a/packages/drivers/driver-mongodb/src/mongodb-unrecognised-aggregate-function.test.ts b/packages/drivers/driver-mongodb/src/mongodb-unrecognised-aggregate-function.test.ts index 41b0a46f8c..72f5ce9c10 100644 --- a/packages/drivers/driver-mongodb/src/mongodb-unrecognised-aggregate-function.test.ts +++ b/packages/drivers/driver-mongodb/src/mongodb-unrecognised-aggregate-function.test.ts @@ -61,10 +61,13 @@ * — and NOT on `expected undefined to be 'INVALID_QUERY'`, because the un-fixed * builder does not throw anonymously, it answers. (That is the opposite * direction from `driver-sql`'s ablation of the same class, where every failure - * was an absent `code`.) The controls — the six declared functions and the two - * retired ones this face still lowers, and the numbers they compute — must stay - * GREEN, pinning that the change moved what happens to UNRECOGNISED names and - * nothing else. + * was an absent `code`.) The controls — the six declared functions and the + * numbers they compute — must stay GREEN, pinning that the change moved what + * happens to UNRECOGNISED names and nothing else. (As written for #12818 that + * control set ALSO named `array_agg` / `string_agg`, "the two retired ones this + * face still lowers". #13075 closed that divergence, so they are controls no + * longer: they are refusals, pinned below in the block that used to record the + * lowering.) * * Measured: recorded on the PR. */ @@ -132,8 +135,12 @@ function value(fn: string, field?: string): unknown { describe('[#12818] an UNDECLARED aggregate function answers INVALID_QUERY / 400', () => { // `median` is the card's own repro. The rest are what a SQL-fluent author // reaches for and `AggregationFunction` does not declare — the same roster - // `driver-sql` refuses, minus the two this face still lowers (pinned as a - // divergence of its own further down, rather than quietly omitted). + // `driver-sql` refuses, minus `array_agg` / `string_agg`. Those two are + // refused here as well since #13075 — but by `refuseRetiredAggregateFunction`, + // whose first sentence says the name "was REMOVED" rather than "is not a + // declared aggregate function", so they would fail the `startsWith` below. + // They are pinned in their own block further down, rather than quietly + // omitted from this one. const UNDECLARED = ['median', 'stddev', 'percentile_cont', 'group_concat', 'variance']; for (const fn of UNDECLARED) { @@ -211,7 +218,18 @@ describe('[#12818] class 2 is EMPTY — every declared function lowers here', () * exported, so it is restated here as the population the cases below drive — * and the cases are what hold the restatement honest. */ - const LOWERED = ['count', 'sum', 'avg', 'min', 'max', 'count_distinct', 'array_agg', 'string_agg']; + const LOWERED = ['count', 'sum', 'avg', 'min', 'max', 'count_distinct']; + + /** + * [#13075] The two names that LEFT the roster above, kept rather than + * dropped. `array_agg` and `string_agg` sat on it because this face lowered + * them; it refuses both now, so naming them as lowered would be the lie the + * roster exists to prevent. But two names simply disappearing from a list is + * exactly the shape a silent re-baseline takes, and from `main` it is + * indistinguishable from the divergence reopening — so they move here and the + * roster case below asserts the fact that replaced the one they used to pin. + */ + const RETIRED_AND_REFUSED = ['array_agg', 'string_agg']; it('the declared-but-unlowered set is EMPTY', () => { expect([...AggregationFunction.options].filter((f) => !LOWERED.includes(f))).toEqual([]); @@ -237,6 +255,17 @@ describe('[#12818] class 2 is EMPTY — every declared function lowers here', () `${fn} is on the roster and must lower`, ).not.toThrow(); } + // [#13075] …and every name that left the roster really refuses. This half + // is what makes the shrink above a measured change rather than a quiet one: + // if `buildAccumulator` lowered either name again, the roster would be back + // to advertising a lowering the switch does not perform, and nothing but + // this loop would say so. The envelope is read, never a bare `toThrow()` — + // `refusalOf` already fails loudly when a pipeline comes back instead. + for (const fn of RETIRED_AND_REFUSED) { + const err = refusalOf(fn); + expect(err.code, `${fn} left the roster at #13075 and must refuse`).toBe('INVALID_QUERY'); + expect(err.status).toBe(400); + } }); it('the two refusal sentences remain distinguishable', () => { @@ -250,27 +279,49 @@ describe('[#12818] class 2 is EMPTY — every declared function lowers here', () }); }); -// ── The divergence this card does NOT close, pinned so it cannot be mistaken ─ +// ── The divergence this file recorded, CLOSED by #13075 ───────────────────── -describe('[#12818] `array_agg` / `string_agg` still lower here — recorded, not fixed', () => { +describe('[#12818 → #13075] `array_agg` / `string_agg` are REFUSED here — the recorded divergence is CLOSED', () => { /** * #6188 retired both from `AggregationFunction` (ADR-0049 enforce-or-remove); - * `driver-sql` and `driver-turso` therefore refuse them today as UNDECLARED - * names (400), while this face still lowers them to `$push`. That divergence - * PRE-DATES this card and closing it is a second accept-face narrowing with - * its own changeset, so it is filed as #13075 rather than ridden in here. + * `driver-sql` and `driver-turso` have refused them as UNDECLARED names ever + * since, while this face went on lowering them to `$push` — so one query + * answered 400 on two backends and an array on the third. #12818 could not + * close that (a second accept-face narrowing, its own changeset), so it + * RECORDED the divergence here as two cases asserting the lowering, for one + * reason: without them, the absence of `array_agg` from the UNDECLARED roster + * above reads as an oversight in this file instead of a measured property of + * this driver. * - * It is pinned rather than left silent for one reason: without these cases, - * the absence of `array_agg` from the UNDECLARED roster above reads as an - * oversight in this file instead of a measured property of this driver. + * #13075 closed it, and those two cases are INVERTED IN PLACE — not deleted, + * not re-baselined. A pin whose fact a later card falsifies is the one kind + * of test that must not quietly vanish: from `main`, its disappearance and + * the divergence silently REOPENING look identical. Each case now asserts the + * refusal that replaced the lowering it used to record. + * + * ⚠️ Class 1 / 400, but NOT the message the roster above asserts. + * `refuseRetiredAggregateFunction` says the name "was REMOVED" at #6188, + * which is a different fact from "is not a declared aggregate function" — + * telling the author of `arry_agg` their value was removed would misinform, + * and telling the author of `array_agg` the protocol never had the name would + * too. Both producers are kept for that reason, the same distinction + * `AggregationFunction`'s own error map draws. The envelope is what is + * asserted — `code` and `status` (ADR-0112) — never a bare `toThrow()`, for + * the reason this file's head note gives. */ - it('lowers `array_agg` rather than refusing it (unlike both SQL faces)', () => { - expect(value('array_agg', 'score')).toEqual([10, 20, 30, 40, 50, 60]); - }); - - it('lowers `string_agg` rather than refusing it (unlike both SQL faces)', () => { - expect(value('string_agg', 'score')).toEqual([10, 20, 30, 40, 50, 60]); - }); + for (const fn of ['array_agg', 'string_agg'] as const) { + it(`refuses \`${fn}\` rather than lowering it — the answer both SQL faces already gave`, () => { + const err = refusalOf(fn); + expect(err.code).toBe('INVALID_QUERY'); + expect(err.status).toBe(400); + // The RETIRED wording: the caller learns the name left the vocabulary, + // and when. These two positive readings are also the control for the + // negative one below — an empty message could not satisfy them. + expect(err.message).toContain('was REMOVED'); + expect(err.message).toContain('#6188'); + expect(err.message.startsWith(UNDECLARED_SENTENCE(fn))).toBe(false); + }); + } it('neither is a member of the declared vocabulary', () => { expect(AggregationFunction.options).not.toContain('array_agg'); From 42ea291f5121626b9ad476c14cf488d613968b0f Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 09:59:34 +0000 Subject: [PATCH 3/3] =?UTF-8?q?docs(changeset):=20the=20#13076=20arm=20lan?= =?UTF-8?q?ded=20=E2=80=94=20correct=20the=20tense,=20no=20claim=20changed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LZbWd2jNV1FErXTPSS4Dry --- .changeset/mongodb-retired-agg-arms-refused.md | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/.changeset/mongodb-retired-agg-arms-refused.md b/.changeset/mongodb-retired-agg-arms-refused.md index 3af798c404..dd1cdeb4a4 100644 --- a/.changeset/mongodb-retired-agg-arms-refused.md +++ b/.changeset/mongodb-retired-agg-arms-refused.md @@ -22,14 +22,17 @@ enum. `AggregationInput.function` here is a bare `string` — the driver's own fine and survived the retirement unnoticed. Both names now answer `INVALID_QUERY` / **400**, answer-for-answer parity with -both SQL faces. They are named explicitly rather than left to fall through, -because falling through is not currently safe: `buildAccumulator`'s `default` -arm answers `{ $sum: … }`, so deleting the arms alone would turn a visibly-wrong -ARRAY into an arithmetically PLAUSIBLE NUMBER — strictly the worse failure, and -the very defect #12818 is fixing in that arm. Naming them is correct whichever -order the two land in, and after #12818 lands the arm still draws the +both SQL faces. They are named explicitly rather than left to fall through. +When this change was written, falling through was not safe at all: +`buildAccumulator`'s `default` arm answered `{ $sum: … }`, so deleting the arms +alone would have turned a visibly-wrong ARRAY into an arithmetically PLAUSIBLE +NUMBER — strictly the worse failure, and exactly the defect #13076 has since +fixed in that arm (#12818). Naming them was correct whichever order the two +landed in, and now that #13076 is on `main` the named arm still draws the distinction `AggregationFunction`'s own error map draws: a caller who bypassed -the parse door is told the name was **removed**, not merely unrecognised. +the parse door is told the name was **removed** at #6188, which is a different +fact from `default`'s "is not a declared aggregate function". Both producers are +kept for that reason. The retirement prescription itself is not restated here — it lives once, on the enum's error map in `@objectstack/spec`, and a copy in the driver would be a