From 7bd43f5d35ccdb5dc0c3e2aebf008385523988eb Mon Sep 17 00:00:00 2001 From: os-justin Date: Tue, 8 Sep 2026 16:03:54 +0000 Subject: [PATCH 1/2] fix(data-objectstack): aggregate()'s spec-shape branch refuses the analytics branch's keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The spec-shape branch builds its request from exactly four keys — `groupBy`, `aggregations`, `where`, `limit` — and reads nothing else. `filter`, `field` and `function` are the analytics branch's own parameters and were neither read, nor refused, nor warned about on this branch: they were simply absent from the body posted to `POST /data/:object/query`. That is worse than the `where` half objectui#6825 ruled on. `field` + `function` are the analytics branch's whole measure and this branch takes a measure only out of `aggregations`, so the legacy shape `{ field, function, groupBy, filter }` whose `groupBy` happened to be an ARRAY produced a query with a `groupBy` and no `aggregations` at all — a grouping with no measure — and with the author's filter gone too. The chart rendered, the numbers were wrong, and there was nothing to look at. Apply #6825's ruling (option A: refuse at the producer, never degrade quietly) to the rest of the same branch. `AnalyticsKeysOnSpecShapeError` carries the `INVALID_FILTER` / 400 pair its siblings carry, names each offending key and what its spec-shape equivalent is, states which `looksLikeSpecShape` disjunct selected the branch, and says outright when the query would have had no measure. Scoped to those three keys and to non-nullish values on purpose: refusing every unrecognised key would break legitimate traffic, and a key spread in as `undefined` carries nothing to drop — which is how both in-tree producers build their params. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S --- ...864-aggregate-spec-shape-analytics-keys.md | 50 +++ packages/data-objectstack/README.md | 51 ++- ...ggregate-spec-shape-analytics-keys.test.ts | 376 ++++++++++++++++++ .../src/aggregate-spec-shape-where.test.ts | 28 +- packages/data-objectstack/src/index.ts | 167 ++++++++ 5 files changed, 667 insertions(+), 5 deletions(-) create mode 100644 .changeset/6864-aggregate-spec-shape-analytics-keys.md create mode 100644 packages/data-objectstack/src/aggregate-spec-shape-analytics-keys.test.ts diff --git a/.changeset/6864-aggregate-spec-shape-analytics-keys.md b/.changeset/6864-aggregate-spec-shape-analytics-keys.md new file mode 100644 index 0000000000..d129da605e --- /dev/null +++ b/.changeset/6864-aggregate-spec-shape-analytics-keys.md @@ -0,0 +1,50 @@ +--- +'@object-ui/data-objectstack': minor +--- + +`aggregate()`'s spec-shape branch now REFUSES the analytics branch's `filter` / +`field` / `function` instead of dropping them (objectui#6864, extending the +maintainer ruling of 2026-08-30 on objectui#6825 — option A, refuse at the +producer). + +**Breaking for callers that were already broken, so read this if you call +`aggregate()` with an ARRAY `groupBy`.** The spec-shape branch — entered when +`params` carries an array `groupBy`, an array `aggregations`, or any `where` +key — builds its request from exactly four keys: `groupBy`, `aggregations`, +`where`, `limit`. `filter`, `field` and `function` are the OTHER branch's +parameters, and they were neither read, nor refused, nor warned about: they +were simply absent from the body that went to `POST /data/:object/query`. + +**Why that was worse than the `where` half #6825 fixed.** `field` + `function` +are the analytics branch's whole measure, and this branch takes a measure only +out of `aggregations`. So the legacy shape `{ field, function, groupBy, filter }` +whose `groupBy` happened to be an ARRAY produced a query carrying a `groupBy` +and **no aggregations at all** — a grouping with no measure — with the author's +filter gone as well. The chart rendered, the numbers were wrong, and there was +nothing on screen or on the wire to look at. + +**What now throws that previously went through.** A spec-shape call carrying a +non-nullish `filter`, `field` or `function` throws the new +`AnalyticsKeysOnSpecShapeError`. It carries the `INVALID_FILTER` / 400 pair its +siblings carry (so `isMalformedFilterError()` recognises it and a failed widget +renders "this filter is malformed" rather than "check your connection"), plus +`keys` — the offending key names — and `received`, what each one carried. The +message names each key, says what its spec-shape equivalent is, states which +`looksLikeSpecShape` disjunct put the call on this branch, and says outright +when the resulting query would have had no measure. Nothing is sent to the +server, so no unfiltered numbers come back. + +**What is deliberately NOT refused.** The legacy analytics shape (a STRING +`groupBy`) is untouched and still lowers `filter` and still fuses +`field` + `function` into its measure — the refusal lives inside the spec-shape +branch only. A key that is present but nullish (`filter: undefined`) carries +nothing to drop and passes, so params built by spreading possibly-absent +authored values keep working. And keys outside those three (`orderBy`, a future +spec key, any host extra) are not refused: the gate names three keys, and only +those. + +**Migration.** Pick one shape per call. Spec-shape: `{ groupBy: GroupByNode[], +aggregations: AggregationNode[], where?, limit? }`, with `where` already lowered. +Analytics: `{ field, function, groupBy: string, filter? }`, which lowers `filter` +for you. `AnalyticsKeysOnSpecShapeError` is exported from +`@object-ui/data-objectstack`. diff --git a/packages/data-objectstack/README.md b/packages/data-objectstack/README.md index 116e313fdc..a67fccba06 100644 --- a/packages/data-objectstack/README.md +++ b/packages/data-objectstack/README.md @@ -278,6 +278,45 @@ Two shapes are deliberately NOT refused, because the receiving door accepts them: a `FilterCondition` object (`{ stage: 'won' }` — what `QuerySchema.where` declares), and an empty array (`[]` means "no filter"). +#### The two shapes are alternatives, not a mixture + +The spec-shape branch reads exactly four keys — `groupBy`, `aggregations`, +`where`, `limit` — and `filter` / `field` / `function` are the **analytics** +branch's own parameters. Until objectui#6864 they were neither read nor +reported on the spec-shape branch: they simply were not in the body that went +out. Since #6864 they are refused, by name: + +```typescript +import type { ObjectStackAdapter } from '@object-ui/data-objectstack'; + +declare const dataSource: ObjectStackAdapter; + +// ⛔ throws AnalyticsKeysOnSpecShapeError — an ARRAY `groupBy` selects the +// spec-shape branch, and these three keys would have vanished from the query: +// no filter, and no measure at all. +await dataSource.aggregate('opportunity', { + field: 'amount', + function: 'sum', + groupBy: ['stage'], + filter: [{ field: 'stage', operator: 'equals', value: 'won' }], +}); + +// ✅ the analytics shape, unchanged — a STRING `groupBy` keeps this call on the +// analytics branch, where all three keys are read and `filter` is lowered for you +await dataSource.aggregate('opportunity', { + field: 'amount', + function: 'sum', + groupBy: 'stage', + filter: [{ field: 'stage', operator: 'equals', value: 'won' }], +}); +``` + +Pick one shape per call. A key that is present but nullish (`filter: undefined`) +carries nothing to drop and is not refused, so params built by spreading +possibly-absent authored values keep working. Keys outside those three are not +refused either — the gate names `filter`, `field` and `function`, and only +those. + ### Sorting ```typescript @@ -434,7 +473,12 @@ import { // the spec's filter-AST gate rejects (400 // INVALID_FILTER). See "aggregate({ where }) does // NOT lower" above. - isMalformedFilterError, // Recognises BOTH of the two above, and the server's + AnalyticsKeysOnSpecShapeError, // aggregate()'s spec-shape branch was handed the + // ANALYTICS branch's keys (`filter` / `field` / + // `function`), which it does not read (400 + // INVALID_FILTER). See "the two shapes are + // alternatives, not a mixture" above. + isMalformedFilterError, // Recognises ALL THREE of the above, and the server's // own version of the same refusal. } from '@object-ui/data-objectstack'; ``` @@ -511,8 +555,9 @@ All errors include unique error codes for programmatic handling: - `CONNECTION_ERROR` - Connection/network error - `AUTHENTICATION_ERROR` - Authentication failure - `VALIDATION_ERROR` - Data validation error -- `INVALID_FILTER` - A filter the adapter refuses to send (`MalformedFilterError`, - `UnloweredAggregateWhereError`); matches the data API's own code for the same refusal +- `INVALID_FILTER` - A request the adapter refuses to send (`MalformedFilterError`, + `UnloweredAggregateWhereError`, `AnalyticsKeysOnSpecShapeError`); matches the data + API's own code for the same refusal - `UNSUPPORTED_OPERATION` - Unsupported operation - `NOT_FOUND` - Resource not found - `UNKNOWN_ERROR` - Unknown error diff --git a/packages/data-objectstack/src/aggregate-spec-shape-analytics-keys.test.ts b/packages/data-objectstack/src/aggregate-spec-shape-analytics-keys.test.ts new file mode 100644 index 0000000000..dad55c45a2 --- /dev/null +++ b/packages/data-objectstack/src/aggregate-spec-shape-analytics-keys.test.ts @@ -0,0 +1,376 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * `aggregate()`'s SPEC-SHAPE branch refuses the ANALYTICS branch's own keys + * instead of dropping them (objectui#6864). + * + * WHY THIS FILE EXISTS. The spec-shape branch builds its `queryAst` from four + * keys — `groupBy`, `aggregations`, `where`, `limit` — and reads nothing else. + * `filter`, `field` and `function` are the OTHER branch's parameters, and until + * this card they were neither read, nor refused, nor warned about: they simply + * were not in the body that went to `POST /data/:object/query`. + * + * ⭐ WHY THAT IS WORSE THAN THE `where` HALF (#6825, whose ruling this extends). + * `field` + `function` are the analytics branch's WHOLE measure, and the + * spec-shape branch takes a measure only from `aggregations`. So the legacy + * shape `{ field, function, groupBy, filter }` whose `groupBy` happens to be an + * ARRAY posted a `groupBy` with NO `aggregations` at all — a grouping with no + * measure — and with the author's `filter` gone as well. The chart rendered, the + * numbers were wrong, and there was nothing on screen or on the wire to look at. + * + * THE PRODUCER CHAIN IS IN-TREE, measured on 2026-09-08 (the census behind + * #6825 read this differently on 2026-08-30, and the tree has moved). + * `ObjectChart.runAggregate` gates its spec-shape call on the STRUCTURED node + * shape — `gb && typeof gb === 'object' && !Array.isArray(gb)` — so an ARRAY + * `aggregate.groupBy` falls through to its LEGACY call, `{ field, function, + * groupBy, filter }`, and `Array.isArray(params.groupBy)` lands that call on + * the spec-shape branch anyway. `ObjectMetricWidget.computeOne` forwards + * `aggregate.groupBy || '_all'` into the same legacy shape. Both read authored + * widget metadata across an `any` seam (`isObjectProvider`'s `aggregate?: any`, + * `ds: any`), so no type refuses the array. What remains unmeasured is an + * authored array `groupBy` in metadata: still zero in this tree. + * + * ⭐ WHAT THE PINS BELOW ASSERT, AND WHY IT IS NOT "IT THREW". This branch + * ALREADY refuses one thing — an unlowered `where` (#6825) — with the same + * `INVALID_FILTER` / 400 envelope. An envelope-only pin would therefore pass on + * a refusal that came from the OTHER gate, and would pass on an implementation + * strictly worse than the bug (one that refuses every unrecognised key). So + * every refusal row asserts the REASON: the error class, the exact `keys` set, + * and the message naming that key and not the others. + * + * ⛔ Three things this deliberately does NOT do: + * - it does not ROUTE the legacy shape back to the analytics branch (the + * tolerant-consumer direction #6825 refused, and impossible anyway: that + * branch posts `dimensions: [params.groupBy]`, so an array would nest); + * - it does not widen `AggregateParams` (`@object-ui/types`), a separate + * contract question; + * - it does not refuse keys OUTSIDE the analytics set, nor nullish ones. + */ + +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { parseFilterAST } from '@objectstack/spec/data'; +import { + ObjectStackAdapter, + clearSharedDiscoveryCache, + isMalformedFilterError, + AnalyticsKeysOnSpecShapeError, + UnloweredAggregateWhereError, +} from './index'; + +/** Rows the spec-shape door answers with, so nothing degrades to a fallback. */ +const QUERY_ROWS = { success: true, data: { object: 'opportunity', records: [{ stage: 'won', n: 2 }], total: 1 } }; +/** Rows the analytics door answers with, carrying the measure it was asked for. */ +const ANALYTICS_ROWS = { rows: [{ stage: 'won', amount_sum: 150 }] }; + +/** + * A fetch mock that keeps the three doors apart — the same harness + * `aggregate-spec-shape-where.test.ts` uses, so "nothing was sent" can be + * asserted against ALL of them and not just the one a test happens to watch. + */ +function makeAdapter() { + const specShapeBodies: any[] = []; + const analyticsBodies: any[] = []; + const urls: string[] = []; + const fetchImpl = vi.fn(async (url: any, init?: any) => { + const u = String(url); + urls.push(u); + if (u.includes('/api/v1/discovery')) { + return { + ok: true, status: 200, statusText: 'OK', + json: async () => ({ success: true, data: { version: 'v1', routes: {} } }), + } as any; + } + if (u.includes('/api/v1/data/') && u.endsWith('/query')) { + specShapeBodies.push(init?.body ? JSON.parse(String(init.body)) : undefined); + return { ok: true, status: 200, statusText: 'OK', json: async () => QUERY_ROWS } as any; + } + if (u.includes('/api/v1/analytics/query')) { + analyticsBodies.push(init?.body ? JSON.parse(String(init.body)) : undefined); + return { ok: true, status: 200, statusText: 'OK', json: async () => ANALYTICS_ROWS } as any; + } + return { + ok: true, status: 200, statusText: 'OK', + json: async () => ({ success: true, data: { object: 'opportunity', records: [], total: 0 } }), + } as any; + }); + const adapter = new ObjectStackAdapter({ + baseUrl: 'http://localhost:3000', token: 't', autoReconnect: false, fetch: fetchImpl as any, + }); + return { adapter, specShapeBodies, analyticsBodies, urls }; +} + +/** Which door a call came out of. `null` = it never reached the wire at all. */ +type Door = 'spec-shape' | 'analytics' | 'find-fallback' | null; + +function doorOf(urls: string[]): Door { + if (urls.some((u) => u.includes('/api/v1/data/') && u.endsWith('/query'))) return 'spec-shape'; + if (urls.some((u) => u.includes('/api/v1/analytics/query'))) return 'analytics'; + if (urls.some((u) => u.includes('/api/v1/data/opportunity'))) return 'find-fallback'; + return null; +} + +/** Run `aggregate()` and report the door it came out of plus what it posted. */ +async function run(params: unknown) { + const { adapter, specShapeBodies, analyticsBodies, urls } = makeAdapter(); + let rows: any = null; + const error = await adapter + .aggregate('opportunity', params as any) + .then((r) => { rows = r; return null; }, (e) => e); + return { door: doorOf(urls), specShapeBodies, analyticsBodies, urls, error, rows }; +} + +/** + * `looksLikeSpecShape`, transcribed from `aggregate()` — the same transcription + * `aggregate-spec-shape-where.test.ts` keeps, for the same reason: it lets a + * test state that two `params` take the same branch even when neither reaches + * the wire, and the observed controls below bring it down if it ever drifts. + */ +function branchSelectors(params: any): [boolean, boolean, boolean] { + return [ + params != null && Array.isArray(params.groupBy), + params != null && Array.isArray(params.aggregations), + params != null && params.where !== undefined, + ]; +} + +/** Spec-shape params, analytics-key-free — the shape that must keep working. */ +const SPEC_SHAPE = { + groupBy: ['stage'], + aggregations: [{ function: 'count', field: 'id', alias: 'n' }], +}; + +/** A properly-lowered `where`: the AST the spec's own gate accepts. */ +const AST_WHERE = ['stage', '=', 'won']; +/** Authoring sugar the #6825 gate refuses — used here only to pin precedence. */ +const RULE_WHERE = [{ field: 'stage', operator: 'equals', value: 'won' }]; + +/** + * The exact params `ObjectChart.runAggregate` builds for an authored ARRAY + * `aggregate.groupBy` — its structured gate excludes arrays, so this legacy + * call is what an array produces. Transcribed, not imported: this file pins the + * adapter's contract, and `@object-ui/plugin-charts` is not one of its deps. + */ +const OBJECT_CHART_LEGACY_CALL_WITH_ARRAY_GROUPBY = { + field: 'amount', + function: 'sum', + groupBy: ['stage'], + filter: [{ field: 'stage', operator: 'equals', value: 'won' }], +}; + +describe('the analytics branch keys are refused on the spec-shape branch, by name', () => { + beforeEach(() => clearSharedDiscoveryCache()); + + it('refuses all three at once and names each of them', async () => { + const r = await run(OBJECT_CHART_LEGACY_CALL_WITH_ARRAY_GROUPBY); + expect(r.error).toBeInstanceOf(AnalyticsKeysOnSpecShapeError); + // ⭐ The REASON, not the envelope: exactly which keys were wrong. + expect(r.error.keys).toEqual(['filter', 'field', 'function']); + expect(r.error.resource).toBe('opportunity'); + expect(r.error.received).toEqual({ + filter: OBJECT_CHART_LEGACY_CALL_WITH_ARRAY_GROUPBY.filter, + field: 'amount', + function: 'sum', + }); + const msg = String(r.error.message); + expect(msg).toContain('`filter`'); + expect(msg).toContain('`field`'); + expect(msg).toContain('`function`'); + // what arrived, verbatim, so the producer is identifiable from a log + expect(msg).toContain(JSON.stringify(OBJECT_CHART_LEGACY_CALL_WITH_ARRAY_GROUPBY.filter)); + expect(msg).toContain('field=' + JSON.stringify('amount')); + expect(msg).toContain('function=' + JSON.stringify('sum')); + // why this call took this branch at all + expect(msg).toContain('`groupBy` is an array'); + // and that no numbers were invented on the way out + expect(msg).toContain('Nothing was sent to the server'); + }); + + it('sends NOTHING — not to the spec-shape door, not to analytics, not to the find() fallback', async () => { + // The failure this replaces ended in wrong numbers on a rendered chart. A + // refusal that quietly degraded to `aggregateViaFind` would reproduce it, + // so all three doors are asserted, not just the one. + const r = await run(OBJECT_CHART_LEGACY_CALL_WITH_ARRAY_GROUPBY); + expect(r.door).toBeNull(); + expect(r.specShapeBodies).toHaveLength(0); + expect(r.analyticsBodies).toHaveLength(0); + expect(r.urls.every((u) => u.includes('/api/v1/discovery'))).toBe(true); + }); + + it('carries the INVALID_FILTER / 400 envelope its siblings carry', async () => { + const r = await run(OBJECT_CHART_LEGACY_CALL_WITH_ARRAY_GROUPBY); + expect(r.error.code).toBe('INVALID_FILTER'); + expect(r.error.httpStatus).toBe(400); + expect(isMalformedFilterError(r.error)).toBe(true); + expect(r.error.name).toBe('AnalyticsKeysOnSpecShapeError'); + }); + + // ⭐ One row per key. Each asserts the message names ITS key and NOT the + // others, so a refusal that fired for a different reason cannot pass here. + const PER_KEY: Array<[string, Record, string[], string[]]> = [ + ['filter alone', { ...SPEC_SHAPE, filter: RULE_WHERE }, ['filter'], ['field', 'function']], + ['field alone', { ...SPEC_SHAPE, field: 'amount' }, ['field'], ['filter', 'function']], + ['function alone', { ...SPEC_SHAPE, function: 'sum' }, ['function'], ['filter', 'field']], + ['the measure pair', { ...SPEC_SHAPE, field: 'amount', function: 'sum' }, ['field', 'function'], ['filter']], + ]; + + for (const [name, params, expected, absent] of PER_KEY) { + it(`refuses ${name} and reports exactly that key set`, async () => { + const r = await run(params); + expect(r.error).toBeInstanceOf(AnalyticsKeysOnSpecShapeError); + // ⛔ NOT the pre-existing #6825 gate: these params carry no `where` at all, + // so a throw from `assertSpecShapeWhereIsFilterAst` would be the + // wrong-reason refusal an envelope-only pin cannot tell apart. + expect(r.error).not.toBeInstanceOf(UnloweredAggregateWhereError); + expect(r.error.keys).toEqual(expected); + const msg = String(r.error.message); + for (const key of expected) expect(msg).toContain(`\`${key}\``); + for (const key of absent) expect(msg).not.toContain(`\`${key}\``); + expect(r.door).toBeNull(); + }); + } + + it('names the missing measure when there is no `aggregations` either', async () => { + // ⭐ The half that is worse than #6825: `groupBy` survives, the measure does + // not, and the query that would have gone out groups nothing. + const r = await run(OBJECT_CHART_LEGACY_CALL_WITH_ARRAY_GROUPBY); + expect(String(r.error.message)).toContain('NO MEASURE'); + }); + + it('does NOT claim a missing measure when `aggregations` is present', async () => { + // The same refusal, one key different: the diagnosis must track the params, + // not be boilerplate stapled to every message. + const r = await run({ ...SPEC_SHAPE, filter: RULE_WHERE }); + expect(r.error).toBeInstanceOf(AnalyticsKeysOnSpecShapeError); + expect(String(r.error.message)).not.toContain('NO MEASURE'); + }); + + it('the refused params take the SAME branch as an observed spec-shape control', async () => { + // The branch-selection proof for an input that never reaches the wire: the + // control below is observed landing on the spec-shape door, and the refused + // params agree with it on every disjunct `looksLikeSpecShape` reads. + const control = SPEC_SHAPE; + const refused = { ...SPEC_SHAPE, filter: RULE_WHERE }; + expect(branchSelectors(refused)).toEqual(branchSelectors(control)); + expect(branchSelectors(control)).toEqual([true, true, false]); + expect((await run(control)).door).toBe('spec-shape'); + }); + + it('an array `groupBy` alone is enough — no `aggregations`, no `where`', async () => { + // This is the card's reachable shape: only `Array.isArray(groupBy)` selects + // the branch, and the whole legacy payload behind it used to disappear. + const params = { field: 'amount', function: 'sum', groupBy: ['stage'], filter: RULE_WHERE }; + expect(branchSelectors(params)).toEqual([true, false, false]); + const r = await run(params); + expect(r.error).toBeInstanceOf(AnalyticsKeysOnSpecShapeError); + expect(r.error.keys).toEqual(['filter', 'field', 'function']); + }); +}); + +describe('precedence with the #6825 gate is stated, not accidental', () => { + beforeEach(() => clearSharedDiscoveryCache()); + + it('an unlowered `where` still answers UnloweredAggregateWhereError, even alongside analytics keys', async () => { + // The addition is strictly additive: no input that already refused changes + // which error it gets. The new gate runs after the `where` gate for exactly + // this reason. + const r = await run({ ...SPEC_SHAPE, where: RULE_WHERE, filter: RULE_WHERE, field: 'amount' }); + expect(r.error).toBeInstanceOf(UnloweredAggregateWhereError); + expect(r.error).not.toBeInstanceOf(AnalyticsKeysOnSpecShapeError); + expect(r.door).toBeNull(); + }); + + it('a LOWERED `where` alongside analytics keys answers the new refusal', async () => { + // …and once the `where` gate has nothing to say, the analytics keys are + // what is left to report. + const r = await run({ ...SPEC_SHAPE, where: AST_WHERE, filter: RULE_WHERE }); + expect(r.error).toBeInstanceOf(AnalyticsKeysOnSpecShapeError); + expect(r.error.keys).toEqual(['filter']); + expect(String(r.error.message)).toContain('a `where` key is present'); + expect(r.door).toBeNull(); + }); +}); + +describe('NON-REGRESSION: the refusal is not too broad', () => { + beforeEach(() => clearSharedDiscoveryCache()); + + it('the LEGACY shape still succeeds and still lowers all three of its keys', async () => { + // ⭐ The axis derived from the plausible WRONG FIX. A refusal that caught + // the legacy path would satisfy "the spec-shape branch now refuses + // `filter`" and break every working chart in the product. A string + // `groupBy` is not an array, so this call never reaches the spec-shape + // branch — and all three keys must still do their jobs on the analytics + // wire: `filter` lowered to a FilterCondition, `field` + `function` fused + // into the measure, `groupBy` into the dimension. + const params = { function: 'sum', field: 'amount', groupBy: 'stage', filter: RULE_WHERE }; + expect(branchSelectors(params)).toEqual([false, false, false]); + const r = await run(params); + expect(r.error).toBeNull(); + expect(r.door).toBe('analytics'); + expect(r.analyticsBodies[0].where).toEqual(parseFilterAST(['stage', '=', 'won'])); + expect(r.analyticsBodies[0].measures).toEqual(['amount_sum']); + expect(r.analyticsBodies[0].dimensions).toEqual(['stage']); + // …and the rows come back keyed by the column the convention promises. + expect(r.rows).toEqual([{ stage: 'won', amount: 150 }]); + }); + + it('the legacy single-bucket shape (groupBy "_all") still succeeds', async () => { + // `ObjectMetricWidget` and the `element:number` renderer both build this. + const r = await run({ function: 'sum', field: 'amount', groupBy: '_all', filter: RULE_WHERE }); + expect(r.error).toBeNull(); + expect(r.door).toBe('analytics'); + expect(r.analyticsBodies[0].dimensions).toEqual([]); + }); + + it('the STRUCTURED spec-shape call ObjectChart builds still reaches the wire untouched', async () => { + // The one in-tree producer that legitimately builds spec-shape params + // (`runAggregate`'s structured branch, for `{ field, dateGranularity }` + // grouping). It carries no analytics key, so the new gate must be invisible + // to it — transcribed from that call site. + const params = { + groupBy: [{ field: 'closed_at', dateGranularity: 'day' }], + aggregations: [{ function: 'count', alias: 'count' }], + where: AST_WHERE, + }; + const r = await run(params); + expect(r.error).toBeNull(); + expect(r.door).toBe('spec-shape'); + expect(r.specShapeBodies[0]).toEqual(params); + }); + + it('a key that is present but NULLISH carries nothing to drop, so it passes', async () => { + // Both in-tree producers spread possibly-absent authored values + // (`filter: filterForRun`, `field: schema.aggregate.field`), so `in` would + // refuse calls that lose nothing at all. + const r = await run({ ...SPEC_SHAPE, filter: undefined, field: undefined, function: undefined }); + expect(r.error).toBeNull(); + expect(r.door).toBe('spec-shape'); + expect(r.specShapeBodies[0]).toEqual(SPEC_SHAPE); + + const withNulls = await run({ ...SPEC_SHAPE, filter: null, field: null, function: null }); + expect(withNulls.error).toBeNull(); + expect(withNulls.door).toBe('spec-shape'); + }); + + it('keys OUTSIDE the analytics set are not refused', async () => { + // ⛔ The strictly-worse implementation: refusing every unrecognised key + // would pass a naive "spec-shape refuses `filter`" pin while breaking any + // caller carrying an extra. The gate names three keys and only three. + const r = await run({ ...SPEC_SHAPE, orderBy: [{ field: 'stage' }], somethingElse: 1, limit: 5 }); + expect(r.error).toBeNull(); + expect(r.door).toBe('spec-shape'); + // still only the four keys this branch reads reach the wire + expect(r.specShapeBodies[0]).toEqual({ ...SPEC_SHAPE, limit: 5 }); + }); + + it('a clean spec-shape call is byte-identical to what it posted before', async () => { + const r = await run({ ...SPEC_SHAPE, where: AST_WHERE, limit: 5 }); + expect(r.error).toBeNull(); + expect(r.specShapeBodies[0]).toEqual({ ...SPEC_SHAPE, where: AST_WHERE, limit: 5 }); + }); +}); diff --git a/packages/data-objectstack/src/aggregate-spec-shape-where.test.ts b/packages/data-objectstack/src/aggregate-spec-shape-where.test.ts index c1c8380720..93e2b7ff31 100644 --- a/packages/data-objectstack/src/aggregate-spec-shape-where.test.ts +++ b/packages/data-objectstack/src/aggregate-spec-shape-where.test.ts @@ -45,6 +45,12 @@ * - it does not touch `AggregateParams` (`packages/types`), which is carded * separately. * + * ⚠️ SINCE objectui#6864, the same branch also refuses the ANALYTICS branch’s + * own keys — `filter`, `field`, `function` — which it reads not at all and used + * to drop in silence. That is the same ruling applied to the rest of the branch, + * pinned in `aggregate-spec-shape-analytics-keys.test.ts`; the rows here vary only + * `where` and are unaffected, except the one that names #6864 inline. + * * ⭐ WHICH BRANCH RAN IS ASSERTED, NOT ASSUMED. The two branches post to * different endpoints, so the wire proves the branch for anything that reaches * it. A refusal reaches nothing, so for the refusing inputs the branch is @@ -62,6 +68,7 @@ import { clearSharedDiscoveryCache, isMalformedFilterError, UnloweredAggregateWhereError, + AnalyticsKeysOnSpecShapeError, } from './index'; /** Rows the spec-shape door answers with, so nothing degrades to a fallback. */ @@ -190,9 +197,26 @@ describe('the spec-shape branch is the branch under test — asserted on the wir // Same legacy params as the row above; only the KEY NAME changes. This is // the asymmetry the card reported, and it is what makes the branch // selection provable for params that never reach the wire. + // + // ⚠️ WHAT THIS ROW USED TO ASSERT, AND WHY IT MOVED (objectui#6864). It + // used to observe the flipped call landing on the spec-shape door with only + // its `where` in the body — which is to say, it pinned the very drop #6864 + // reported: this call's `field` and `function` never reached the wire and + // nothing said so. Since #6864 the branch REFUSES them, applying #6825’s own + // ruling (refuse at the producer, never degrade quietly) to the keys it does + // not read. The flip is still what is being proven, and it is proven at + // least as well: only the spec-shape branch has this gate, so the refusal + // could not have come from the analytics branch. const r = await run({ function: 'sum', field: 'amount', groupBy: '_all', where: AST_WHERE }); - expect(r.door).toBe('spec-shape'); - expect(r.specShapeBodies[0].where).toEqual(AST_WHERE); + expect(r.error).toBeInstanceOf(AnalyticsKeysOnSpecShapeError); + expect(r.error.keys).toEqual(['field', 'function']); + expect(r.door).toBeNull(); + // …and with those two analytics keys gone, the same flipped params post the + // `where` verbatim, exactly as this row observed before. + const clean = await run({ groupBy: '_all', where: AST_WHERE }); + expect(clean.error).toBeNull(); + expect(clean.door).toBe('spec-shape'); + expect(clean.specShapeBodies[0].where).toEqual(AST_WHERE); }); }); diff --git a/packages/data-objectstack/src/index.ts b/packages/data-objectstack/src/index.ts index f30fb4fef8..a7092b7e31 100644 --- a/packages/data-objectstack/src/index.ts +++ b/packages/data-objectstack/src/index.ts @@ -270,6 +270,160 @@ function assertSpecShapeWhereIsFilterAst(where: unknown, resource: string): void throw new UnloweredAggregateWhereError(where, resource); } +/** + * The keys `aggregate()`'s ANALYTICS branch reads and its SPEC-SHAPE branch does + * not: `filter`, `field`, `function`. + * + * Written out once, and read by both the refusal below and its message, so the + * set a caller is told about cannot drift from the set that is checked. + * + * ⛔ Deliberately NOT "every key the spec-shape branch does not read". A refusal + * scoped that widely would reject `orderBy`, a future spec key, or any harmless + * extra a host happens to carry, and it would satisfy a naive "the spec-shape + * branch refuses `filter`" test while breaking traffic nobody complained about. + * These three are named one by one because they are the OTHER branch's own + * parameters: their presence says the params were built for the analytics wire + * and arrived on the spec-shape one, which is a producer defect with a name. + */ +const ANALYTICS_ONLY_AGGREGATE_KEYS = ['filter', 'field', 'function'] as const; + +type AnalyticsOnlyAggregateKey = (typeof ANALYTICS_ONLY_AGGREGATE_KEYS)[number]; + +/** What each refused key means, and where its spec-shape equivalent lives. */ +const ANALYTICS_ONLY_KEY_ADVICE: Record = { + filter: + '`filter` is the ANALYTICS branch\'s filter and is lowered there through ' + + '`parseFilterAST`; this branch\'s filter is `where` (`QuerySchema.where`, ' + + '@objectstack/spec data/query.zod.ts) and is posted VERBATIM — rename it to ' + + '`where` and lower it in the producer.', + field: + '`field` is half of the analytics branch\'s `${field}_${function}` measure; ' + + 'this branch\'s measure is `aggregations`, an AggregationNode[] such as ' + + "[{ function: 'sum', field: 'amount', alias: 'amount_sum' }].", + function: + '`function` is the other half of that measure pair and belongs inside an ' + + '`aggregations` node, not beside `groupBy`.', +}; + +/** Which disjunct(s) of `looksLikeSpecShape` put this call on the branch. */ +function specShapeSelectorReasons(params: any): string[] { + const reasons: string[] = []; + if (Array.isArray(params?.groupBy)) reasons.push('`groupBy` is an array'); + if (Array.isArray(params?.aggregations)) reasons.push('`aggregations` is an array'); + if (params?.where !== undefined) reasons.push('a `where` key is present'); + return reasons; +} + +/** + * Analytics-branch params — `filter`, `field`, `function` — that reached + * `aggregate()`'s SPEC-SHAPE branch, which reads none of them. + * + * WHY A REFUSAL AND NOT A DROP (objectui#6864). This applies the maintainer + * ruling of 2026-08-30 on objectui#6825 — option A, REFUSE at the producer — to + * the rest of the same branch. That ruling's reason was that a shape the spec's + * own gate would reject is off-contract at the PRODUCER, so the adapter says no + * instead of degrading leniently; the reason lands harder here, because a + * silent drop is precisely the disposition it refused for `where`. Leaving one + * key on this branch refusing while three others vanish without a word would be + * harder to explain than the old behaviour. + * + * ⭐ AND THE DROP HERE IS WORSE THAN THE `where` HALF. `filter` is not the only + * casualty: `field` and `function` are the analytics branch's whole measure, and + * the spec-shape branch copies a measure only out of `aggregations`. So the + * legacy shape `{ field, function, groupBy, filter }` whose `groupBy` happens to + * be an ARRAY produced a `queryAst` carrying a `groupBy` and NO `aggregations` + * at all — a grouping with no measure — with the author's predicate gone too. + * The chart still rendered, the numbers were wrong, and there was nothing to + * look at. + * + * HOW A LEGACY CALLER GETS HERE, measured in this repo on 2026-09-08. + * `Array.isArray(params.groupBy)` is one of `looksLikeSpecShape`'s three + * disjuncts, and `ObjectChart`'s `runAggregate` gates only on the STRUCTURED + * node shape (`gb && typeof gb === 'object' && !Array.isArray(gb)`), so an + * ARRAY `aggregate.groupBy` falls through to its legacy call — `{ field, + * function, groupBy, filter }` — and that call lands here. + * `ObjectMetricWidget.computeOne` forwards `aggregate.groupBy` the same way. + * Both take the value from authored widget metadata across an `any` seam + * (`isObjectProvider`'s `aggregate?: any`, `ds: any`), so the type system + * cannot refuse it and this runtime refusal is what an author gets instead. + * + * ⛔ WHAT THIS DELIBERATELY DOES NOT DO. It does not ROUTE the legacy shape back + * to the analytics branch: that is the tolerant-consumer direction AGENTS.md + * §0.1 exists to stop, it is the option #6825 refused for `where`, and it could + * not work anyway — the analytics branch posts `dimensions: [params.groupBy]`, + * so an array `groupBy` would go out nested. It does not widen `AggregateParams` + * (`@object-ui/types`), which is a separate contract question. And it does not + * refuse a key that is present but nullish: `undefined`/`null` carries nothing + * for the branch to drop, and both `ObjectChart` and `ObjectMetricWidget` build + * their params by spreading possibly-absent authored values. + * + * Carries the `INVALID_FILTER` / 400 pair its siblings {@link MalformedFilterError} + * and {@link UnloweredAggregateWhereError} carry, so `isMalformedFilterError()` + * recognises it and a failed widget renders "this filter is malformed" rather + * than "check your connection" (objectui#3066). One branch, one envelope: a + * caller can catch `aggregate()`'s producer-side refusals with one predicate. + */ +export class AnalyticsKeysOnSpecShapeError extends Error { + readonly code = 'INVALID_FILTER'; + readonly httpStatus = 400; + /** The offending keys, in the order {@link ANALYTICS_ONLY_AGGREGATE_KEYS} lists them. */ + readonly keys: readonly AnalyticsOnlyAggregateKey[]; + /** What each offending key carried, so a producer is identifiable from a log. */ + readonly received: Record; + /** The object `aggregate()` was called for. */ + readonly resource: string; + constructor( + keys: readonly AnalyticsOnlyAggregateKey[], + params: any, + resource: string, + ) { + const received: Record = {}; + for (const key of keys) received[key] = params?.[key]; + const named = keys.map((k) => `\`${k}\``).join(', '); + const shown = keys + .map((k) => `${k}=${JSON.stringify(params?.[k]) ?? String(params?.[k])}`) + .join(', '); + const why = specShapeSelectorReasons(params).join(' and '); + const noMeasure = !Array.isArray(params?.aggregations) + ? ' Because no `aggregations` was supplied either, the query this would ' + + 'have built is a grouping with NO MEASURE — the chart renders and its ' + + 'numbers mean nothing.' + : ''; + super( + `aggregate('${resource}'): the spec-shape branch received the analytics ` + + `branch's ${named} — ${shown} — and reads none of them. This call took the ` + + `spec-shape branch because ${why}; the branch posts only \`groupBy\`, ` + + '`aggregations`, `where` and `limit` to POST /data/:object/query, so those ' + + `keys would have been dropped in silence.${noMeasure} ` + + keys.map((k) => ANALYTICS_ONLY_KEY_ADVICE[k]).join(' ') + + ' The two shapes are alternatives, not a mixture: the analytics branch ' + + '(`{ field, function, groupBy, filter }`, which lowers its filter for you) ' + + 'is reached only when NONE of an array `groupBy`, an array `aggregations` ' + + 'or a `where` key is present. Nothing was sent to the server, so no ' + + 'unfiltered numbers came back.', + ); + this.name = 'AnalyticsKeysOnSpecShapeError'; + this.keys = keys; + this.received = received; + this.resource = resource; + } +} + +/** + * Refuse the analytics branch's own keys when they reach the spec-shape branch. + * + * Presence is `!= null` on purpose, not `in`: a params object that spreads an + * absent authored value (`filter: filterForRun` where nothing was authored) + * carries the key with `undefined`, and there is nothing there to drop. Both + * in-tree producers build their params exactly that way, so keying on `in` + * would refuse calls that lose nothing. + */ +function assertNoAnalyticsKeysOnSpecShape(params: any, resource: string): void { + const present = ANALYTICS_ONLY_AGGREGATE_KEYS.filter((key) => params?.[key] != null); + if (present.length === 0) return; + throw new AnalyticsKeysOnSpecShapeError(present, params, resource); +} + /** * An ARRAY `filter` that reached `aggregate()`'s ANALYTICS branch and that the * protocol's lowering sink cannot turn into a `FilterCondition` — an infix join @@ -5403,6 +5557,19 @@ export class ObjectStackAdapter implements DataSource { assertSpecShapeWhereIsFilterAst(params.where, resource); queryAst.where = params.where; } + // The other half of the same ruling — objectui#6864. `where` above is the + // key this branch DOES read and refuses when unlowered; `filter`, `field` + // and `function` are the analytics branch's keys, which this branch reads + // not at all and used to drop without a word. Same disposition, applied to + // the rest of the branch, so it has ONE answer for off-contract params + // rather than two. + // + // ORDER IS DELIBERATE: this runs AFTER the `where` gate, so no input that + // already refused changes which error it gets — the addition is strictly + // additive over #6825's behaviour. Both refusals are producer-side and + // nothing has been sent at this point either way. + assertNoAnalyticsKeysOnSpecShape(params, resource); + if (typeof params.limit === 'number') queryAst.limit = params.limit; const result: any = await this.client.data.query(resource, queryAst as any); // client.data.query returns { object, records, total, hasMore } From c84d46240264f3dac08040cb1a4f6fd972a96685 Mon Sep 17 00:00:00 2001 From: os-justin Date: Tue, 8 Sep 2026 16:20:04 +0000 Subject: [PATCH 2/2] chore(scripts): re-key the doc-example ledger row that this diff shifted `check:doc-example-types` keys its declared-failure ledger by `FILE:LINE symbol`. Inserting the new refusal moved `createObjectStackAdapter`'s `@example` block from `index.ts:6156` to `:6323`, so the row went stale and the same pre-existing `process`-is-undeclared failure came back as an UNDECLARED FAILURE. Only the line number in the key changes; the codes, the reason and the row count are untouched. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S --- scripts/check-doc-example-types.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/check-doc-example-types.mjs b/scripts/check-doc-example-types.mjs index c71311fdff..f68bc003a2 100644 --- a/scripts/check-doc-example-types.mjs +++ b/scripts/check-doc-example-types.mjs @@ -642,7 +642,7 @@ export const UNGATED_EXAMPLES = { reason: 'usage fragment: references `MetadataCache`, `fetchSchemaFromServer`, which the example never declares', }, - 'packages/data-objectstack/src/index.ts:6156 createObjectStackAdapter': { + 'packages/data-objectstack/src/index.ts:6323 createObjectStackAdapter': { card: null, codes: [2591], reason: