fix(service-analytics): refuse an unrecognised compareTo.kind instead of answering a previous-period window under a 200

.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: <anything else> }` | 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.

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<unknown>): Promise<Refusal | undefined> {
+  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<AnalyticsResult> => {
+      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');
+  });
+});

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.
  */