From e568de087d31522d173b1b53de79a58501289d91 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 02:33:23 +0000 Subject: [PATCH 1/3] fix(driver-mongodb): refuse an unrecognised aggregate function instead of answering it as a silent SUM MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `default` arm of `buildAccumulator` answered ANY function name this driver does not lower with `{ $sum: fieldRef ?? 0 }` — a sum of the column under the caller's alias, with no error, no envelope and no log. Refuse it instead, with the two-class ADR-0112 envelope both SQL faces already answer with: INVALID_QUERY/400 for a name the Query Protocol does not declare, NOT_IMPLEMENTED/501 for a declared name this backend does not lower. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LZbWd2jNV1FErXTPSS4Dry --- .../driver-mongodb/src/mongodb-aggregation.ts | 178 +++++++++- ...db-unrecognised-aggregate-function.test.ts | 329 ++++++++++++++++++ 2 files changed, 506 insertions(+), 1 deletion(-) create mode 100644 packages/drivers/driver-mongodb/src/mongodb-unrecognised-aggregate-function.test.ts diff --git a/packages/drivers/driver-mongodb/src/mongodb-aggregation.ts b/packages/drivers/driver-mongodb/src/mongodb-aggregation.ts index c40300b0c8..536941d3ca 100644 --- a/packages/drivers/driver-mongodb/src/mongodb-aggregation.ts +++ b/packages/drivers/driver-mongodb/src/mongodb-aggregation.ts @@ -9,12 +9,25 @@ 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'; /** * Aggregation function descriptor from QueryAST. + * + * [#12818] `function` stays a bare `string`, deliberately: the enforcement is + * {@link refuseAggregateFunction} at the lowering site, not this annotation. + * Narrowing it to `AggregationFunction` was the card's other candidate remedy + * and it does not close the hole it names — `MongoDBDriver.aggregate` reads its + * aggregations through `(query as any).aggregations`, so a narrowed type meets + * 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}. */ export interface AggregationInput { function: string; @@ -352,6 +365,143 @@ function malformedGroupByError(node: unknown): Error { return err; } +/** + * [#12818] The aggregate functions {@link buildAccumulator} LOWERS into a + * `$group` accumulator — and therefore the exact population that is NOT + * refused. + * + * A ROSTER rather than `driver-sql`'s table of lowerings, because this face's + * lowerings are not one shape with a name in it: `count` branches on whether a + * `field` was given, `count_distinct` splits its work with + * {@link postProcessAggregation}, and the four arithmetic/order arms wrap + * {@link numericAggregandExpr}. So the `switch` stays the compiler and this is + * what the refusal messages read; `mongodb-unrecognised-aggregate-function.test.ts` + * holds the two equal in BOTH directions (every name here lowers, every name + * absent from here refuses), which is the drift a hand-written list otherwise + * 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 separately 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. + */ +const LOWERED_HERE: readonly string[] = [ + 'count', 'sum', 'avg', 'min', 'max', 'count_distinct', 'array_agg', 'string_agg', +]; + +/** + * [#5907] The aggregate vocabulary the Query Protocol DECLARES, read from the + * spec rather than restated — `AggregationNodeSchema.function` is this enum, so + * "declared" has exactly one definition and this driver cannot drift from it. + */ +const DECLARED_AGGREGATE_FUNCTIONS: readonly string[] = AggregationFunction.options; + +/** + * [#12818] What a refusal offers back as the remedy: lowered HERE *and* + * writable by a caller. The intersection, not {@link LOWERED_HERE} itself — + * see that constant's warning. + */ +const LOWERED_AND_DECLARED: readonly string[] = + LOWERED_HERE.filter((f) => DECLARED_AGGREGATE_FUNCTIONS.includes(f)); + +/** + * [#12818] Class 1 — a function name the Query Protocol does not declare. + * + * The caller wrote something no backend can run (`median`), so this is a + * request-shaped mistake: `INVALID_QUERY` / 400, the catalogued + * `StandardErrorCode` for "malformed query syntax" and a member of + * `@objectstack/rest`'s `isExpectedQueryRejection` list, so a client mistake + * stops being logged as an unhandled server fault. It is also the code the + * PROTOCOL DOOR already gives this condition — `metadata-protocol`'s + * `invalidQueryError` refuses "a function outside the spec enum" with exactly + * `400 INVALID_QUERY` (#4254) — so a caller who reaches this driver in-process + * gets the same wire identity as one who came through REST. + * + * The FIRST SENTENCE is shared verbatim with the twins in `driver-sql`'s + * `undeclaredAggregateFunctionError` and `driver-turso`'s `remote-transport.ts` + * (#5240 — one condition, one wording): a caller must not be able to tell which + * backend answered from the words it used. Spelled out rather than imported, + * which is what the two SQL faces do to each other; the bytes are pinned + * against those faces' literals in + * `mongodb-unrecognised-aggregate-function.test.ts`. + * + * Judged against the declared enum CASE-SENSITIVELY, which is what the enum is: + * `COUNT_DISTINCT` is not `count_distinct` (`AggregationFunction.parse('COUNT')` + * throws), so answering "declared but not implemented" for it would be false. + */ +function undeclaredAggregateFunctionError(func: string): Error { + const err = new Error( + `Aggregate function "${func}" is not a declared aggregate function. ` + + `Declared functions: ${DECLARED_AGGREGATE_FUNCTIONS.join(', ')} ` + + `(@objectstack/spec AggregationFunction). Fix the "function" key of the aggregations[] ` + + `entry — the Query Protocol has no such function, so this is a query no backend can run, ` + + `not a gap in this one (#5907). It is refused rather than accumulated: until #12818 this ` + + `builder answered any unrecognised name with a $sum of that column under the alias the ` + + `caller asked for, which is a plausible number nothing downstream can tell from an answer.`, + ) as Error & { code?: string; status?: number }; + err.code = StandardErrorCode.enum.INVALID_QUERY; + err.status = 400; + return err; +} + +/** + * [#12818] Class 2 — a DECLARED function this backend does not lower. + * + * Kept distinct from {@link undeclaredAggregateFunctionError} for the reason + * #5907 gives on the SQL faces: `count_distinct` is declared and implemented by + * several backends, so telling a dashboard author their correct query is a typo + * would be false. The line #5345 drew in `driver-memory`'s `filter-refusal.ts` + * between "the protocol has no such operator" and "the protocol has it, this + * face cannot lower it". + * + * ⚠️ **This class is EMPTY today, and the producer is kept deliberately** — + * {@link LOWERED_HERE} covers every member of `AggregationFunction`, pinned as + * a positive assertion ("the declared-but-unlowered set is empty") rather than + * left to be rediscovered. Deleting it as dead code was considered and + * rejected, exactly as on `driver-sql`: the branch is not an unenforced + * declaration, it is the CLASSIFIER that decides which of two truths a future + * name is told. Without it, the first function a later spec bump adds would be + * told the protocol has no such name — the misreport #5907 exists to prevent, + * landing precisely in the window between a spec change and a driver change. + * + * `NOT_IMPLEMENTED` / 501 from the ADR-0112 STANDARD catalog, whose own + * `HttpStatusErrorCodeMap` pairs the two — the same envelope + * {@link refuseDateBucketedGroupBy} and {@link refusePerAggregationFilter} + * already answer with, one seam over in this file. + */ +function uncompilableAggregateFunctionError(func: string): Error { + const err = new Error( + `Aggregate function "${func}" is declared but not implemented by this backend. ` + + `Lowered here: ${LOWERED_AND_DECLARED.join(', ')} (driver-mongodb). The name is spelled ` + + `correctly and @objectstack/spec AggregationFunction declares it — this is a capability gap ` + + `in the backend, not a mistake in the query, which is why it answers NOT_IMPLEMENTED/501 ` + + `rather than a 400. Aggregate with a function this backend lowers; whether the declaration ` + + `itself should stand is ADR-0049's enforce-or-remove question (#5907).`, + ) as Error & { code?: string; status?: number }; + err.code = StandardErrorCode.enum.NOT_IMPLEMENTED; + err.status = 501; + return err; +} + +/** + * [#12818] Which refusal a name this builder cannot lower deserves. + * + * Written ONCE and reached from the single `default:` arm, so "is this the + * caller's mistake or ours?" cannot be answered two ways for one query. `func` + * is the name the CALLER wrote — not a normalised form — because that is what + * the enum is judged against and what the message has to quote back. + */ +function refuseAggregateFunction(func: string): never { + throw DECLARED_AGGREGATE_FUNCTIONS.includes(func) + ? uncompilableAggregateFunctionError(func) + : undeclaredAggregateFunctionError(func); +} + /** * Build a MongoDB aggregation pipeline from QueryAST components. * @@ -581,7 +731,33 @@ function buildAccumulator(agg: AggregationInput): Document { return { $push: fieldRef ?? '' }; default: - return { $sum: fieldRef ?? 0 }; + // [#12818] REFUSED, where this arm used to `return { $sum: fieldRef ?? 0 }`. + // + // Any name this switch does not lower — a typo, a function added to the + // contract but not to this file, an unnarrowed `method` arriving from + // `StrategyContext.executeAggregate` (#12776) — was answered as a SUM of + // that column, under the alias the caller asked for. No error, no + // envelope, no log. It is the worst available answer precisely because a + // sum of a numeric column is arithmetically plausible: a dashboard tile + // renders it without complaint, so nothing downstream can notice that the + // function it asked for was never run. The `"[object Object]"` group id + // (#6850) and the null-carrying `count_distinct` (#6814) are this file's + // earlier members of the same family, and both emitted well-formed + // pipelines too. + // + // The refusal is also what makes the rest of this file consistent with + // itself: one seam over, a `groupBy` entry carrying a granularity this + // driver cannot bucket is refused rather than grouped by the raw instant + // ({@link refuseDateBucketedGroupBy}), and a per-aggregation `filter` it + // cannot lower is refused rather than accumulated unfiltered + // ({@link refusePerAggregationFilter}). Aggregation function and groupBy + // entry are the two halves of one lowering; they no longer disagree about + // what to do with a shape this driver does not model. + // + // Reached before anything is sent to the server — `buildAggregationPipeline` + // throws while assembling stages into a local array, so no partial + // pipeline executes. + return refuseAggregateFunction(agg.function); } } 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 new file mode 100644 index 0000000000..9bc0b2ec0e --- /dev/null +++ b/packages/drivers/driver-mongodb/src/mongodb-unrecognised-aggregate-function.test.ts @@ -0,0 +1,329 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#12818] An aggregate function this builder does not lower is REFUSED, with + * the wire identity that says which kind of "no" it is — where it used to be + * answered as a SUM of the column. + * + * # What was measured on `origin/main` @ `cd1348802` + * + * `buildAccumulator`'s switch ended: + * + * ```js + * default: + * return { $sum: fieldRef ?? 0 }; + * ``` + * + * so `{ function: 'median', field: 'score', alias: 'm' }` built + * `{ $group: { _id: null, m: { $sum: '$score' } } }` and, over + * `AGGREGATION_ROWS`, ANSWERED `m: 210`. No error, no envelope, no log — the + * sum of the column, under the alias the caller asked for. That is the worst + * available answer: `median` is not `sum`, but 210 is a number a dashboard tile + * renders without complaint, so nothing downstream can tell the difference + * between "your function ran" and "your function was silently replaced". + * + * It is the "answers rather than fails" family this file's history is made of: + * the `"[object Object]"` `$group._id` (#6850) and the `count_distinct` that + * kept its nulls (#6814) both emitted well-formed pipelines and returned + * plausible numbers. + * + * # ⚠️ Why every case asserts `code` AND `status`, never merely "it threw" + * + * The inverse of the trap `driver-sql`'s twin records, and it bites the other + * way round here. On that face the un-fixed driver already threw (anonymously), + * so `rejects.toThrow()` was permanently green. On THIS face the un-fixed + * builder does not throw at all — it returns a pipeline — so a bare `toThrow()` + * would catch the defect today and go blind the moment somebody replaces the + * ADR-0112 envelope with a bare `Error`, which is exactly the state #5907 found + * on the SQL faces. The envelope IS the deliverable: `mapDataError` reads + * `code`/`status`, and without them a legible client mistake reaches the caller + * as an opaque 500. + * + * # What this file does NOT establish + * + * That a real mongod agrees with any pipeline here. This fleet cannot run one: + * there is no daemon on the box and no image path, and the `mongodb-memory-server` + * download is refused by the egress proxy (#5517 — the real-server suites in + * this package have been opt-in since). Every value below is produced by + * `mongodb-pipeline-evaluator.testkit.ts`, a strict in-process reader modelled + * from the MongoDB manual. That bound is real and is not weakened by this card: + * a REFUSAL, though, is decided entirely inside `buildAggregationPipeline` + * before a single stage reaches a server, so it is one of the few claims here + * that a live catalog could not tell us more about. The positive controls are + * the half that carries the bound — they say "the evaluator computes these + * numbers from the emitted pipeline", not "MongoDB returns them". + * + * # Reverse verification — direction predicted BEFORE it was run + * + * Prediction: restore the `default: return { $sum: fieldRef ?? 0 };` arm and + * change nothing else, and every refusal case here fails on its FIRST + * assertion — `refusalOf`'s "expected a refusal, but it returned a pipeline" + * — 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. + * + * Measured: recorded on the PR. + */ + +import { describe, it, expect } from 'vitest'; +import { AggregationFunction, AGGREGATION_ROWS } from '@objectstack/spec/data'; +import { buildAggregationPipeline, type AggregationInput } from './mongodb-aggregation.js'; +import { runPipeline, type Doc } from './mongodb-pipeline-evaluator.testkit.js'; + +interface WireBearingError extends Error { + code?: string; + status?: number; +} + +const ROWS = AGGREGATION_ROWS as unknown as Doc[]; + +/** + * The first sentences, spelled out here rather than imported from the producer. + * This is the contract #5240 asks for — "one condition, one wording", so a + * caller cannot tell which backend answered from the words it used — and a test + * that read the same constant the producer reads would pass however the wording + * drifted. These bytes are the ones `driver-sql`'s + * `sql-driver-out-of-contract-aggregate-function.test.ts` and `driver-turso`'s + * `remote-transport-aggregate-function-refusal.test.ts` already spell for their + * own faces; this is the third copy, for the third face. + */ +const UNDECLARED_SENTENCE = (f: string) => + `Aggregate function "${f}" is not a declared aggregate function.`; +const UNCOMPILABLE_SENTENCE = (f: string) => + `Aggregate function "${f}" is declared but not implemented by this backend.`; + +/** + * ⚠️ No `as unknown as` cast anywhere in this file, and that is a statement + * rather than a convenience. `driver-sql`'s twin needs one because its fixtures + * are `QueryAST`s, whose `aggregations[].function` is the declared enum. + * `AggregationInput.function` is a bare `string` ON PURPOSE (see its docblock): + * this builder is exported, its own driver reads aggregations through an `any` + * cast, and `StrategyContext.executeAggregate` declares `method` as `string` + * (#12776). An off-contract name reaching here needs no cast because nothing in + * the type system is stopping it — which is the whole argument for a RUNTIME + * refusal over the narrowing the card offered as its alternative. + */ +const aggs = (...entries: AggregationInput[]): AggregationInput[] => entries; + +/** Build the pipeline for `fn`, expecting a refusal; return it. */ +function refusalOf(fn: string, field: string | null = 'score'): WireBearingError { + try { + buildAggregationPipeline({ + aggregations: aggs({ function: fn, ...(field ? { field } : {}), alias: 'm' }), + }); + } catch (err) { + return err as WireBearingError; + } + throw new Error(`expected the builder to refuse "${fn}", but it returned a pipeline`); +} + +/** Run a single whole-table aggregation through the evaluator and read its value. */ +function value(fn: string, field?: string): unknown { + const aggregations = aggs({ function: fn, ...(field ? { field } : {}), alias: 'm' }); + return runPipeline(ROWS, buildAggregationPipeline({ aggregations }))[0].m; +} + +// ── Class 1: the Query Protocol does not declare this name ────────────────── + +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). + const UNDECLARED = ['median', 'stddev', 'percentile_cont', 'group_concat', 'variance']; + + for (const fn of UNDECLARED) { + it(`refuses "${fn}"`, () => { + const err = refusalOf(fn); + expect(err.code).toBe('INVALID_QUERY'); + expect(err.status).toBe(400); + expect(err.message.startsWith(UNDECLARED_SENTENCE(fn))).toBe(true); + // The remedy is in the message: what the protocol DOES declare. + for (const declared of AggregationFunction.options) { + expect(err.message).toContain(declared); + } + // …and it must not be mistaken for the capability-gap answer. The + // positive control for this zero-hit reading is a phrase that shares no + // substring with the absent one, so "the message is empty" cannot pass + // both lines. + expect(err.message).not.toContain('capability gap'); + expect(err.message).toContain('no such function'); + }); + } + + // The case-sensitivity ruling, pinned. `AggregationFunction` is a + // case-SENSITIVE `z.enum` (`AggregationFunction.parse('COUNT')` throws), so + // `COUNT_DISTINCT` is not `count_distinct` and "declared but not implemented" + // would be false of it. Same judgement the two SQL faces make, so one query + // cannot get a 400 on one face and a 501 on another. + for (const fn of ['COUNT_DISTINCT', 'Median', 'COUNT', 'SUM']) { + it(`refuses the miscased "${fn}" as UNDECLARED, not as a capability gap`, () => { + const err = refusalOf(fn); + expect(err.code).toBe('INVALID_QUERY'); + expect(err.status).toBe(400); + expect(err.message.startsWith(UNDECLARED_SENTENCE(fn))).toBe(true); + // The caller's own spelling is quoted back — that is the actionable part. + expect(err.message).toContain(`"${fn}"`); + }); + } + + it('refuses a name with no `field` too — the SUM it used to answer was `0`', () => { + // `fieldRef ?? 0` meant a field-less unrecognised function accumulated the + // constant 0, i.e. `{ $sum: 0 }` = 0 per group. A zero is even quieter than + // a plausible sum: it reads as "no matching rows". + const err = refusalOf('median', null); + expect(err.code).toBe('INVALID_QUERY'); + expect(err.status).toBe(400); + }); + + it('refuses the SECOND entry too, not just the first', () => { + // The switch runs per aggregation; a call whose first entry is fine must + // not smuggle the second past the door. + let thrown: WireBearingError | undefined; + try { + buildAggregationPipeline({ + aggregations: aggs( + { function: 'count', alias: 'n' }, + { function: 'median', field: 'score', alias: 'm' }, + ), + groupBy: ['region'], + }); + } catch (err) { + thrown = err as WireBearingError; + } + expect(thrown, 'an unrecognised name in any position must be refused').toBeDefined(); + expect(thrown!.code).toBe('INVALID_QUERY'); + expect(thrown!.message).toContain('"median"'); + }); +}); + +// ── Class 2: declared by the protocol, not lowered by this backend ────────── + +describe('[#12818] class 2 is EMPTY — every declared function lowers here', () => { + /** + * The guard that keeps this block from silently covering nothing. It fails in + * both directions: the spec growing a function this driver does not lower, or + * this driver's roster drifting away from the enum. The lowered roster is not + * 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']; + + it('the declared-but-unlowered set is EMPTY', () => { + expect([...AggregationFunction.options].filter((f) => !LOWERED.includes(f))).toEqual([]); + }); + + it('every declared function goes through the lowering door, not the refusal door', () => { + for (const fn of AggregationFunction.options) { + expect( + () => buildAggregationPipeline({ aggregations: aggs({ function: fn, field: 'score', alias: 'm' }) }), + `${fn} must lower, not refuse`, + ).not.toThrow(); + } + }); + + it('every name on the lowered roster really lowers — the roster is not decoration', () => { + // The other direction of the same equality: a name the refusal messages + // claim is lowered here must not be refused by the switch. Without this the + // roster could name a function the switch dropped, and the message would + // advertise a lowering that does not exist. + for (const fn of LOWERED) { + expect( + () => buildAggregationPipeline({ aggregations: aggs({ function: fn, field: 'score', alias: 'm' }) }), + `${fn} is on the roster and must lower`, + ).not.toThrow(); + } + }); + + it('the two refusal sentences remain distinguishable', () => { + // The class-2 PRODUCER is deliberately kept in `mongodb-aggregation.ts` + // with nothing to produce — see its docblock: it is the classifier that + // decides which of two truths the FIRST function of a later spec bump is + // told. This case states the consequence so the unreachable branch is not + // read as an oversight, and keeps its sentence exercised. + expect(UNCOMPILABLE_SENTENCE('x')).not.toBe(UNDECLARED_SENTENCE('x')); + expect(UNCOMPILABLE_SENTENCE('x')).toContain('declared but not implemented'); + }); +}); + +// ── The divergence this card does NOT close, pinned so it cannot be mistaken ─ + +describe('[#12818] `array_agg` / `string_agg` still lower here — recorded, not fixed', () => { + /** + * #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 separately rather than ridden in here. + * + * 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. + */ + 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]); + }); + + it('neither is a member of the declared vocabulary', () => { + expect(AggregationFunction.options).not.toContain('array_agg'); + expect(AggregationFunction.options).not.toContain('string_agg'); + // Positive control for the two zero-hit readings above, sharing no + // substring with either term: the enum is non-empty and holds what it + // should. + expect(AggregationFunction.options).toContain('count_distinct'); + }); +}); + +// ── Controls: a recognised function still answers, in the same call ────────── + +describe('[#12818] the refusal did not break aggregation', () => { + /** + * The control the refusal is worthless without. "It refused" must never be + * readable as "aggregation broke", so each case below runs a RECOGNISED + * function through the same builder — and, for the numbers, through the same + * evaluator — and reads the value out. + * + * ⚠️ These numbers come from `mongodb-pipeline-evaluator.testkit.ts`, not from + * a mongod. See this file's head note. + */ + it('every declared function computes its value over the shared fixture', () => { + expect(value('count')).toBe(6); // count(*) — the rows + expect(value('count', 'stage')).toBe(4); // count(col) — non-null values + expect(value('sum', 'score')).toBe(210); + expect(value('avg', 'score')).toBe(35); + expect(value('min', 'score')).toBe(10); + expect(value('max', 'score')).toBe(60); + // `count_distinct` collects here and is SIZED by `postProcessAggregation` + // (#6814, which is where the null exclusion lives) — this is the `$addToSet` + // as the accumulator leaves it, nulls included, which is the half this + // builder owns. + expect(value('count_distinct', 'stage')).toEqual(['won', 'lost', null]); + }); + + it('a recognised function answers in a call that ALSO carries a refused one, once the bad entry is dropped', () => { + // Same shape, one entry apart: the call with `median` in it refuses (above), + // and the same call without it answers both measures. So the refusal is + // attributable to the unrecognised name and to nothing else in the query. + const aggregations = aggs( + { function: 'count', alias: 'n' }, + { function: 'sum', field: 'score', alias: 'total' }, + ); + const rows = runPipeline(ROWS, buildAggregationPipeline({ aggregations, groupBy: ['region'] })); + const byRegion = Object.fromEntries(rows.map((r) => [r.region, `${r.n}/${r.total}`])); + expect(byRegion).toEqual({ west: '4/100', east: '2/110' }); + }); + + it('grouped aggregation still answers', () => { + const aggregations = aggs({ function: 'count', field: 'id', alias: 'n' }); + const rows = runPipeline(ROWS, buildAggregationPipeline({ aggregations, groupBy: ['region'] })); + expect(rows.map((r) => `${r.region}:${r.n}`).sort()).toEqual(['east:2', 'west:4']); + }); +}); From 85a7ebef1e4f7cfa70e442a416426a6a20c21466 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 02:41:52 +0000 Subject: [PATCH 2/3] chore(changeset): driver-mongodb refuses unrecognised aggregate functions Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LZbWd2jNV1FErXTPSS4Dry --- .changeset/khaki-donuts-refuse.md | 47 +++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 .changeset/khaki-donuts-refuse.md diff --git a/.changeset/khaki-donuts-refuse.md b/.changeset/khaki-donuts-refuse.md new file mode 100644 index 0000000000..6083187428 --- /dev/null +++ b/.changeset/khaki-donuts-refuse.md @@ -0,0 +1,47 @@ +--- +'@objectstack/driver-mongodb': patch +--- + +`driver-mongodb` refuses an aggregate function it does not lower, instead of +answering it as a silent SUM (#12818). + +`buildAccumulator`'s `switch` on `agg.function` ended with +`default: return { $sum: fieldRef ?? 0 }`, so ANY name this driver does not +lower — a typo (`median`), a miscased spelling (`COUNT_DISTINCT`), a function +added to the contract but not to this file, or an unnarrowed `method` arriving +from `StrategyContext.executeAggregate` (#12776) — was answered as a **sum of +that column**, under the alias the caller asked for, with no error, no envelope +and no log. It is the worst available answer precisely because it is +arithmetically plausible: a dashboard tile renders the number without complaint, +so nothing downstream can tell "your function ran" from "your function was +silently replaced". The field-less spelling was quieter still — `{ $sum: 0 }`, +i.e. `0`, which reads as "no matching rows". + +The refusal is the two-class ADR-0112 envelope both SQL faces already answer +with (#5907), first sentence for first sentence, so one condition cannot have +two wire identities depending on which backend served it: + +- a name the Query Protocol does not declare answers `INVALID_QUERY` / **400** + and names the declared vocabulary (`@objectstack/spec AggregationFunction`); +- a DECLARED name this backend does not lower answers `NOT_IMPLEMENTED` / **501** + and names what it does lower. That class is empty today — every member of + `AggregationFunction` lowers here — and is pinned as a positive assertion, so + the day the spec grows a function this driver misses, the suite goes red + rather than quietly stopping to cover anything. + +Judged case-sensitively, which is what the enum is: `COUNT_DISTINCT` is not +`count_distinct`, and telling its author the backend has a capability gap would +be false. + +**Graded `patch`, deliberately.** No correct query's answer moves: all six +declared functions and the two retired ones this face still lowers +(`array_agg` / `string_agg`, an existing divergence from the SQL faces, recorded +and filed separately rather than closed here) are byte-identically unchanged, +pinned by controls that compute their values in the same suite. The only inputs +whose behaviour changes are ones this driver was already answering *wrongly*, so +there is no working capability being removed — the same shape, in this same +package, that #10576's per-aggregation-`filter` refusal shipped as a patch. + +Nothing to migrate. A caller that was reaching the old `default` arm was reading +a SUM in place of the function it asked for; the refusal now names the function +and the remedy. From 19f89da392db793f954003eed3c8d737a917754f Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 03:01:28 +0000 Subject: [PATCH 3/3] docs(driver-mongodb): name the filed finding (#13075) for the array_agg/string_agg divergence Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LZbWd2jNV1FErXTPSS4Dry --- .changeset/khaki-donuts-refuse.md | 2 +- packages/drivers/driver-mongodb/src/mongodb-aggregation.ts | 2 +- .../src/mongodb-unrecognised-aggregate-function.test.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.changeset/khaki-donuts-refuse.md b/.changeset/khaki-donuts-refuse.md index 6083187428..c9343a9a92 100644 --- a/.changeset/khaki-donuts-refuse.md +++ b/.changeset/khaki-donuts-refuse.md @@ -36,7 +36,7 @@ be false. **Graded `patch`, deliberately.** No correct query's answer moves: all six declared functions and the two retired ones this face still lowers (`array_agg` / `string_agg`, an existing divergence from the SQL faces, recorded -and filed separately rather than closed here) are byte-identically unchanged, +and filed as #13075 rather than closed here) are byte-identically unchanged, pinned by controls that compute their values in the same suite. The only inputs whose behaviour changes are ones this driver was already answering *wrongly*, so there is no working capability being removed — the same shape, in this same diff --git a/packages/drivers/driver-mongodb/src/mongodb-aggregation.ts b/packages/drivers/driver-mongodb/src/mongodb-aggregation.ts index 536941d3ca..10c8db7437 100644 --- a/packages/drivers/driver-mongodb/src/mongodb-aggregation.ts +++ b/packages/drivers/driver-mongodb/src/mongodb-aggregation.ts @@ -385,7 +385,7 @@ function malformedGroupByError(node: unknown): Error { * `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 separately rather + * 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. 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 9bc0b2ec0e..41b0a46f8c 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 @@ -258,7 +258,7 @@ describe('[#12818] `array_agg` / `string_agg` still lower here — recorded, not * `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 separately rather than ridden in here. + * its own changeset, so it is filed as #13075 rather than ridden in here. * * 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