diff --git a/.changeset/analytics-compareto-kind-refusal.md b/.changeset/analytics-compareto-kind-refusal.md new file mode 100644 index 0000000000..a47684f9ae --- /dev/null +++ b/.changeset/analytics-compareto-kind-refusal.md @@ -0,0 +1,54 @@ +--- +"@objectstack/service-analytics": patch +--- + +fix(service-analytics): an unrecognised `compareTo.kind` is refused, not answered with a previous-period window under a 200 (#17550) + +`shiftRange` had one branch and a fall-through — `previousYear` was named, and +**everything else** landed in the `previousPeriod` arm. No `default`, no +exhaustiveness check. So `compareTo: { kind: 'previousQuarter' }` came back as a +previous-period comparison under an ordinary **200**, and the caller was told +nothing. The wrong answer is a comparison **window**: a number a dashboard +renders and a person reads as fact, with no status, header or field in the +response to distinguish it from a real answer. + +`DatasetCompareTo.kind` has only ever declared two values +(`'previousPeriod' | 'previousYear'`), but `DatasetSelection` is a TypeScript +interface with no Zod schema anywhere, and `/analytics/dataset/query`'s door +parses only the seven members the selection shares with `AnalyticsQuery` — +`compareTo` is one of the four it projects away before its parse, and the route +forwards the caller's selection to the service untouched. So `kind` was checked +by `tsc` inside this repo and by nothing at all on the wire. + +## FROM → TO + +| Input | Was | Now | +|:--|:--|:--| +| `compareTo: { kind: 'previousPeriod' }` | the equal-length window before | **unchanged** | +| `compareTo: { kind: 'previousYear' }` | the same window one year back | **unchanged** | +| `compareTo: { kind: }` | a previous-period window, **200** | `DATASET_INVALID` / **400**, naming the value received and both legal ones | + +The fix is to name one of the two declared windows, or drop `compareTo` — which +is what the refusal says. No accept set widens, no new error code is minted: the +refusal is the fourth member of the `datasetInvalidError` family +`resolveCompareDimension` already raises three times for the same document, so it +arrives at the route through the envelope that route already classifies on. + +## Why this is a `patch` + +It pulls behaviour back onto the contract the type has always declared, rather +than narrowing past it: every input `DatasetCompareTo` permits returns +byte-identical windows, pinned by a control in the same change. What flips from +200 to 400 is input the declared contract never permitted. The reachable-today +population for that input was measured on the tree — the dashboard authoring path +is already doored (`DashboardWidgetSchema` parses the widget's `kind` as a +`z.enum`, so a third kind cannot arrive through a parsed widget), and no producer +in this repository sends a third value. What is not enumerable from here is a +consumer outside it calling the published `shiftRange` export, or posting a +hand-rolled body to the dataset route; for those, the refusal replaces a wrong +answer with a located one. + +`alignedCompareBucketKey` reads the same two-valued `kind` and deliberately gains +no refusal of its own: it is not on the package's public surface, and its only +caller runs `shiftRange` first — both pinned, so exporting it turns the pin red +rather than silently reopening this defect. diff --git a/packages/services/service-analytics/src/__tests__/dataset-compare-kind-refusal.test.ts b/packages/services/service-analytics/src/__tests__/dataset-compare-kind-refusal.test.ts new file mode 100644 index 0000000000..7b21098f72 --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/dataset-compare-kind-refusal.test.ts @@ -0,0 +1,229 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * An unrecognised `compareTo.kind` is REFUSED, not answered with a + * previous-period window under a 200. + * + * ## What was wrong + * + * `shiftRange` had one branch and a fall-through: `previousYear` was named, and + * EVERYTHING else — including a value the declared type says is impossible — + * landed in the `previousPeriod` arm. There was no `default` and no + * exhaustiveness check, so `compareTo: { kind: 'previousQuarter' }` came back as + * a previous-period comparison under an ordinary **200**. Nothing in the + * response distinguished it from a real answer, and the wrong answer is a + * comparison WINDOW — a number a dashboard renders and a person reads as fact. + * + * ## Why the declared type did not save it + * + * `DatasetCompareTo.kind` is declared as the closed pair + * `'previousPeriod' | 'previousYear'` (`spec/contracts/analytics-service.ts`), + * but `DatasetSelection` is a TypeScript interface with **no Zod schema anywhere + * in the repo**, and `/analytics/dataset/query`'s door parses only the seven + * members `DatasetSelection` shares with `AnalyticsQuery` — `compareTo` is one of + * the four it projects away before the parse, and the route forwards the + * caller's selection to the service untouched. So `kind` is checked by `tsc` + * inside this repo and by nothing at all on the wire. The dashboard AUTHORING + * path is doored (`dashboard.zod.ts` parses the widget's `kind` as a + * `z.enum`), which is why both halves are pinned here: the compile-time one that + * already held, and the runtime one that did not. + * + * ## What is pinned + * + * - the refusal itself, in the ADR-0112 envelope the route classifies on + * (`DATASET_INVALID` / 400) — the fourth member of `resolveCompareDimension`'s + * family, not a new error vocabulary; + * - its message discipline: what was received, the two legal values, what to do; + * - the CONTROL that the refusal did not widen — both declared kinds return + * exactly the windows they returned before; + * - that the refusal reaches the executor seam, and reaches it BEFORE the + * comparison pass is issued; + * - ⭐ the surface asymmetry that makes ONE refusal site sufficient: + * `alignedCompareBucketKey` branches on the same two-valued `kind` with the + * same shape, and needs no refusal of its own because it is not on the + * package's public surface and its only caller runs `shiftRange` first. That + * is a claim about `src/index.ts`, so it is pinned against `src/index.ts` + * rather than asserted in prose — export that function and this file goes red. + * + * Every runtime assertion here was run RED first against the pre-fix + * `shiftRange` (the fall-through returned a window, so each refusal case came + * back as a successful previous-period answer). + */ + +import { describe, it, expect, vi } from 'vitest'; +import { DatasetSchema } from '@objectstack/spec/ui'; +import type { AnalyticsQuery, AnalyticsResult, IAnalyticsService } from '@objectstack/spec/contracts'; +import { compileDataset } from '../dataset-compiler.js'; +import { DatasetExecutor, shiftRange } from '../dataset-executor.js'; + +/** The ADR-0112 fields `rest-server.ts`'s catch classifies a thrown error on. */ +interface Refusal extends Error { + code?: unknown; + status?: unknown; +} + +/** Run `thunk` and hand back the error it threw, if any. */ +async function refusalFrom(thunk: () => unknown | Promise): Promise { + try { + await thunk(); + return undefined; + } catch (e) { + return e as Refusal; + } +} + +const WINDOW: [string, string] = ['2026-01-01', '2026-01-31']; + +/** The window `previousPeriod` made of {@link WINDOW} before this change, and still does. */ +const PREVIOUS_PERIOD: [string, string] = ['2025-12-01', '2025-12-31']; +/** And what `previousYear` made of it. */ +const PREVIOUS_YEAR: [string, string] = ['2025-01-01', '2025-01-31']; + +/** + * Three spellings an unparsed wire body can carry in `kind`: a plausible fourth + * window name, the retired string form, and a non-string. The declared type + * permits none of them; nothing on the wire stops any of them. + */ +const UNRECOGNISED: unknown[] = ['previousQuarter', 'previous_period', 7]; + +/** Calls the published export the way an unparsed body reaches it. */ +const shiftWith = (kind: unknown) => shiftRange(WINDOW, kind as never); + +/** + * A trend dataset: `close_date` is a GRID dimension AND bucketed, which is the + * #6007 shape — the only shape that reaches `alignedCompareBucketKey` at all. + */ +const dataset = DatasetSchema.parse({ + name: 'sales_trend', + label: 'Sales Trend', + object: 'opportunity', + dimensions: [ + { name: 'close_date', field: 'close_date', type: 'date', label: 'Close Date', dateGranularity: 'month' }, + ], + measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount', label: 'Revenue' }], +}); + +/** An analytics service that records every query the executor issues. */ +function recordingService(): { svc: IAnalyticsService; seen: AnalyticsQuery[] } { + const seen: AnalyticsQuery[] = []; + const svc: IAnalyticsService = { + query: vi.fn(async (q: AnalyticsQuery): Promise => { + seen.push(q); + return { rows: [{ close_date: '2026-01', revenue: 100 }], fields: [] }; + }), + getMeta: async () => [], + }; + return { svc, seen }; +} + +/** The trend selection, with whatever `kind` the case is about. */ +const selectionWith = (kind: unknown) => ({ + dimensions: ['close_date'], + measures: ['revenue'], + timeDimensions: [{ dimension: 'close_date', dateRange: WINDOW, granularity: 'month' }], + compareTo: { kind }, +}); + +/** Did the executor ask the service for the SHIFTED window? */ +const sawShiftedPass = (seen: AnalyticsQuery[]) => + seen.some((q) => JSON.stringify(q.timeDimensions ?? []).includes('2025-12')); + +// ───────────────────────────────────────────────────────────────────────────── + +describe('shiftRange refuses an unrecognised compareTo.kind', () => { + for (const kind of UNRECOGNISED) { + it(`${JSON.stringify(kind)} → DATASET_INVALID / 400, the envelope the route reads`, async () => { + const err = await refusalFrom(() => shiftWith(kind)); + expect(err, 'an unrecognised kind was ANSWERED — the fall-through is back').toBeInstanceOf(Error); + // Read exactly as `rest-server.ts`'s catch reads them: code + 4xx status. + expect(err?.code).toBe('DATASET_INVALID'); + expect(err?.status).toBe(400); + }); + } + + it('says what it received, what the two legal windows are, and what to do', async () => { + const err = await refusalFrom(() => shiftWith('previousQuarter')); + const msg = String(err?.message); + // ① what arrived — so the caller can find it in the body they sent. + expect(msg).toContain('"previousQuarter"'); + // ② the whole legal set, both members, spelled as an author would write them. + expect(msg).toContain("'previousPeriod'"); + expect(msg).toContain("'previousYear'"); + // ③ the fix, and the fact that it IS this key's value that is wrong. + expect(msg).toContain('compareTo.kind'); + expect(msg).toContain('drop compareTo'); + }); + + it('CONTROL — both declared kinds still return the windows they always did', () => { + expect(shiftRange(WINDOW, 'previousPeriod')).toEqual(PREVIOUS_PERIOD); + expect(shiftRange(WINDOW, 'previousYear')).toEqual(PREVIOUS_YEAR); + // A window whose length is not a month, so the previousPeriod arithmetic is + // exercised rather than only the month-aligned happy case. + expect(shiftRange(['2026-03-10', '2026-03-12'], 'previousPeriod')).toEqual(['2026-03-07', '2026-03-09']); + }); + + it('and `tsc` still owns the compile-time half', () => { + // @ts-expect-error — 'previousQuarter' is not a `DatasetCompareTo['kind']`. + // This assertion is about the EXPECTED ERROR, not the throw: the runtime + // refusal above exists for the wire, where no schema parses `kind` at all. + expect(() => shiftRange(WINDOW, 'previousQuarter')).toThrow(); + }); +}); + +describe('the refusal reaches the executor seam, BEFORE the comparison pass', () => { + it('DatasetExecutor.execute() refuses the selection in the same envelope', async () => { + const { svc } = recordingService(); + const err = await refusalFrom(() => + new DatasetExecutor(svc).execute(compileDataset(dataset), selectionWith('previousQuarter') as never), + ); + expect(err).toBeInstanceOf(Error); + expect(err?.code).toBe('DATASET_INVALID'); + expect(err?.status).toBe(400); + }); + + it('the SHIFTED pass is never issued — so the second `kind` reader is never reached', async () => { + const { svc, seen } = recordingService(); + await refusalFrom(() => + new DatasetExecutor(svc).execute(compileDataset(dataset), selectionWith('previousQuarter') as never), + ); + // `runCompare` reads `kind` twice: into `shiftRange`, and (for this exact + // grid-and-bucketed shape) into `alignedCompareBucketKey` over the shifted + // pass's ROWS. The shift is what produces the second call's own argument, so + // refusing in `shiftRange` means no shifted query, no shifted rows, and + // nothing for the second reader to mis-align. + expect(sawShiftedPass(seen), 'the comparison window was queried despite the refusal').toBe(false); + }); + + it('CONTROL — the same selection with a legal kind DOES issue the shifted pass', async () => { + const { svc, seen } = recordingService(); + const res = await new DatasetExecutor(svc).execute( + compileDataset(dataset), + selectionWith('previousPeriod') as never, + ); + expect(sawShiftedPass(seen), 'the control never reached the comparison pass either').toBe(true); + expect(res.rows.length).toBeGreaterThan(0); + }); +}); + +describe('one refusal site is enough — the surface asymmetry that makes it so', () => { + /** + * `shiftRange` owes a refusal because an external caller can reach it without + * passing through `runCompare`. `alignedCompareBucketKey` does not, because no + * external caller can reach it at all: the barrel names its exports one by one + * (no `export *`) and the manifest maps only `.`, so the function's sole caller + * is `runCompare`, which runs `shiftRange` first. + * + * ⚠️ Adding it to the barrel makes that reasoning false. This case is what + * turns that into a red test instead of a silent reopening of the defect. + */ + it('publishes shiftRange and NOT alignedCompareBucketKey', async () => { + const barrel = await import('../index.js'); + const published = Object.keys(barrel); + // The firing control: the same probe, on the export that IS published. + expect(published, 'the barrel probe itself is broken').toContain('shiftRange'); + expect( + published, + 'alignedCompareBucketKey is now published — it needs its own `kind` verdict, see its docblock', + ).not.toContain('alignedCompareBucketKey'); + }); +}); diff --git a/packages/services/service-analytics/src/dataset-executor.ts b/packages/services/service-analytics/src/dataset-executor.ts index 7204a307f2..6fd5b3c64b 100644 --- a/packages/services/service-analytics/src/dataset-executor.ts +++ b/packages/services/service-analytics/src/dataset-executor.ts @@ -564,19 +564,62 @@ function resolveCompareDimension(selection: DatasetSelection): string { ); } -/** Compute the comparison window for a [start,end] range. */ +/** + * Compute the comparison window for a [start,end] range. + * + * ## Why an unrecognised `kind` is REFUSED here, not fallen through + * + * `kind` is declared as the closed pair `'previousPeriod' | 'previousYear'` + * (`DatasetCompareTo`, `spec/contracts/analytics-service.ts`) and enforced on + * the wire by NOTHING: `DatasetSelection` has no Zod schema anywhere, and + * `/analytics/dataset/query`'s own door projects `compareTo` away before its + * parse and forwards the caller's selection untouched (`analytics-selection-door` + * says so in its header). So a body carrying `compareTo: { kind: 'previousQuarter' }` + * reaches this function with its declared type unenforced. + * + * The shape this function used to have — one `if` for `previousYear`, then the + * previousPeriod arm as a FALL-THROUGH — answered that body with a + * previous-period window under an ordinary 200. The caller is told nothing, a + * dashboard renders a window nobody asked for as fact, and no status, header or + * field in the response distinguishes it from a real answer. That is the + * silently-wrong answer the refusal families in this package exist to replace, + * one layer down. + * + * So it is the FOURTH member of {@link resolveCompareDimension}'s family — + * `DATASET_INVALID` / 400, a verdict about the SELECTION, the caller fixes the + * request — and it is spelled as an exhaustive `switch` so that a third kind + * added to the contract fails to COMPILE here (`const exhaustive: never`) + * instead of arriving as a silent previous-period answer a second time. + * + * ⛔ This is deliberately the ONLY site in this module that judges `kind`. + * {@link alignedCompareBucketKey} branches on the same two-valued `kind` and + * carries no refusal of its own — its docblock states why that is sufficient + * rather than an omission. + */ export function shiftRange(range: [string, string], kind: CompareTo['kind']): [string, string] { const [start, end] = range; - if (kind === 'previousYear') { - return [shiftYear(start, -1), shiftYear(end, -1)]; + switch (kind) { + case 'previousYear': + return [shiftYear(start, -1), shiftYear(end, -1)]; + case 'previousPeriod': { + // The equal-length window ending the day before `start`. + const startMs = parseUTC(start); + const endMs = parseUTC(end); + const lengthDays = Math.round((endMs - startMs) / DAY_MS) + 1; + const prevEndMs = startMs - DAY_MS; + const prevStartMs = prevEndMs - (lengthDays - 1) * DAY_MS; + return [toISODate(prevStartMs), toISODate(prevEndMs)]; + } + default: { + const exhaustive: never = kind; + throw datasetInvalidError( + `[dataset-executor] compareTo.kind ${JSON.stringify(exhaustive)} is not a comparison window ` + + 'this executor implements. The two it runs are \'previousPeriod\' (the equal-length window ' + + 'ending the day before this one starts) and \'previousYear\' (the same window one calendar ' + + 'year back). Name one of those, or drop compareTo.', + ); + } } - // previousPeriod — the equal-length window ending the day before `start`. - const startMs = parseUTC(start); - const endMs = parseUTC(end); - const lengthDays = Math.round((endMs - startMs) / DAY_MS) + 1; - const prevEndMs = startMs - DAY_MS; - const prevStartMs = prevEndMs - (lengthDays - 1) * DAY_MS; - return [toISODate(prevStartMs), toISODate(prevEndMs)]; } // ── compareTo bucket alignment (#6007) ─────────────────────────────────────── @@ -719,6 +762,33 @@ export function bucketKeyAtOrdinal(ordinal: number, granularity: DateGranularity * for a plausible-looking wrong one; it keeps its own key and appends, as it * did before. * + * ## Why this carries no `kind` refusal of its own + * + * It branches on the same two-valued `kind` with the same shape {@link shiftRange} + * used to have — `previousYear` named, the previousPeriod arm taken by everything + * else — so the obvious reading is that it needs the same refusal. It does not, + * for three reasons, and the first two are PINNED rather than asserted + * (`__tests__/dataset-compare-kind-refusal.test.ts`): + * + * - it is **not on the package's public surface**. `src/index.ts` re-exports + * `shiftRange` and not this function, and the manifest maps only `.`, so no + * consumer outside this package can call it at all — the asymmetry that makes + * `shiftRange` owe a refusal and this function not; + * - its **only caller is {@link DatasetExecutor.runCompare}**, which calls + * `shiftRange` unconditionally — and needs its result to build the very + * `shiftedRange` argument passed here — BEFORE the row map that reaches this. + * So there is no path on which this function sees a `kind` that `shiftRange` + * did not already accept; a refusal here would be unreachable code; + * - a throw would **contradict the posture stated above**. Every uncertainty this + * function knows about answers `null` — "leave this row's key alone" — and it + * runs inside a per-row map, where failing the whole request is a different + * contract from the one this docblock commits to. + * + * ⚠️ Export this function, or give it a second caller that does not go through + * `shiftRange` first, and the pin goes red: that is the moment the `kind` + * judgment has to be made here too, and the red is what makes it a decision + * instead of a silent regression. + * * @param currentRange - the selection's own window for the anchor dimension. * @param shiftedRange - what {@link shiftRange} made of it. */