State SaveReportInput's requirements at the reports.save door

.changeset/reports-save-input-contract-at-the-door.md

@@ -0,0 +1,46 @@
+---
+"@objectstack/client": minor
+"@objectstack/rest": minor
+---
+
+fix(client,rest): state `SaveReportInput`'s requirements at the `reports.save` door (#11926)
+
+**BREAKING** accept-set narrowing on `POST /api/v1/reports` and on the
+`client.reports.save` parameter type, shipped as `minor` under the repo's
+launch-window convention for breaking changes.
+
+`IReportService.saveReport` takes a `SaveReportInput`, on which `name`, `object`
+and `query` are all required. Nothing on the path said so. The SDK method
+declared its parameter `any`, and the route forwarded `req.body ?? {}` straight
+through, so the requirement held only as far as each reports implementation
+chose to re-derive it privately — the bundled `@objectstack/plugin-reports` does
+re-derive all three, but a third-party implementation need not, and a caller
+could not tell which one it was talking to. This is the ADR-0078
+declared-but-unenforced shape arriving at an authoring surface: the producer
+accepted off-spec input and handed it to a service that requires more.
+
+Both halves now state the contract:
+
+- **`client.reports.save(report)`** takes `SaveReportInput` instead of `any`.
+ Omitting `query` (or `name`, or `object`) is now a compile error at the call
+ site rather than a surprise from whichever implementation is mounted. The SDK
+ remains a transport and adds no runtime validation — it is not a second
+ validator.
+- **`POST /api/v1/reports`** refuses a body missing any of the three required
+ keys, and a `query` that is not a `ReportQuery` envelope (a scalar or an
+ array), with `400` / `VALIDATION_FAILED` — the same envelope the route
+ already produced for a service-raised validation error (ADR-0112). A
+ JavaScript or `curl` caller that never sees the TypeScript type is refused
+ too. The refusal is ordered **after** the existing `501` for an unmounted
+ reports service: "no reports service on this deployment" is a deployment fact
+ and outranks anything about the body. An empty `query: {}` stays legal —
+ every field on `ReportQuery` is optional — and is pinned as such.
+
+**Migration.** A caller that omitted `query` was already relying on
+implementation-specific behaviour; supply the `ReportQuery` envelope the report
+should run (`{}` for "no filters"). Callers already sending a complete
+definition are unaffected, and the bundled reports implementation already
+refused all three omissions, so no deployment running it changes behaviour —
+only the layer that produces the refusal moves, from the service to the door.
+
+<!-- adr-0087: not-required (no-migration-prescription) A validity narrowing over keys that already exist and are already required by the service contract: no key is removed, renamed or re-shaped, so there is no tombstone and nothing mechanical for `objectstack migrate meta` to rewrite. `SaveReportInput` is a cross-package TS contract with no spec schema and no stored-metadata form, so no authored artifact at rest carries the affected shape. What a query-less report definition was *meant* to query is authoring intent no migration entry can supply on an upgrader's behalf; the 400 at the door is the channel that reaches the author, naming the missing keys. Mirrors the disposition of the #11519 and #11842 accept-set narrowings. -->

packages/client/src/client.test.ts

@@ -375,11 +375,37 @@ describe('Reports namespace (#3587 gap closure)', () => {
 
  it('reports.save pins POST /reports', async () => {
  const { client, fetchMock } = createMockClient({ id: 'r1' });
- await client.reports.save({ name: 'Pipeline', object: 'lead' });
+ await client.reports.save({ name: 'Pipeline', object: 'lead', query: { fields: ['id'] } });
  const [url, init] = fetchMock.mock.calls[0];
  expect(String(url)).toBe('http://localhost:3000/api/v1/reports');
  expect(init.method).toBe('POST');
- expect(JSON.parse(init.body)).toEqual({ name: 'Pipeline', object: 'lead' });
+ expect(JSON.parse(init.body)).toEqual({ name: 'Pipeline', object: 'lead', query: { fields: ['id'] } });
+ });
+
+ // [#11926] The literal below is the ORIGINAL fixture of the test above,
+ // preserved verbatim rather than repaired. For as long as `reports.save`
+ // took `any` it sat there constructing an input the service contract
+ // REFUSES — `SaveReportInput.query` is required — against a mock transport
+ // that never reaches a service, so no run could ever have failed on it. It
+ // is evidence, and giving it a `query` would have silenced the evidence
+ // without closing anything. So it moves here, and the compiler asserts the
+ // refusal instead.
+ //
+ // This is a bidirectional pin, not a comment. `client.test.ts` is compiled
+ // by `tsconfig.test.json` — named by this package's `typecheck` script —
+ // and holds no `test-typecheck-debt.json` entry, so an unlisted file must
+ // have zero errors. Widen the parameter back to `any` and the directive
+ // below stops matching an error: tsc reds with TS2578, "unused
+ // '@ts-expect-error' directive". It cannot rot into a phantom check.
+ it('[#11926] reports.save refuses a query-less report at the type level', async () => {
+ const { client, fetchMock } = createMockClient({ id: 'r1' });
+ // @ts-expect-error — `query` is required by `SaveReportInput`.
+ await client.reports.save({ name: 'Pipeline', object: 'lead' });
+ // The SDK is a transport, not a second validator: the request still
+ // goes out unaltered. The refusal ON THE WIRE belongs to the route and
+ // is pinned in packages/rest/src/rest.test.ts — see
+ // 'POST /reports refuses a body the service contract requires more of'.
+ expect(JSON.parse(fetchMock.mock.calls[0][1].body)).toEqual({ name: 'Pipeline', object: 'lead' });
  });
 
  it('reports.get / delete pin /reports/:id and delete tolerates 204', async () => {

packages/client/src/index.ts

@@ -104,6 +104,7 @@ import type {
  RemoteTable,
  ReportRunResult,
  ReportSchedule,
+ SaveReportInput,
  SavedReport,
  SchemaValidationReport,
  ScreenSpec,
@@ -4383,8 +4384,17 @@ export class ObjectStackClient {
  return Array.isArray(body) ? body : (body?.data ?? []);
  },
 
- /** Create or update a saved report definition. 400 [VALIDATION_FAILED] on a bad spec. */
- save: async (report: any): Promise<SavedReport> => {
+ /**
+ * Create or update a saved report definition.
+ *
+ * [#11926] The parameter is the service contract's own `SaveReportInput`,
+ * not `any`: `name`, `object` and `query` are required, and omitting one is
+ * a compile error here rather than a surprise from whichever reports
+ * implementation the deployment mounts. The wire refusal is the route's —
+ * `POST /reports` answers 400 [VALIDATION_FAILED] for the same three keys,
+ * so a JavaScript caller that never sees this type is refused too.
+ */
+ save: async (report: SaveReportInput): Promise<SavedReport> => {
  const res = await this.fetch(`${this.baseUrl}/api/v1/reports`, {
  method: 'POST',
  body: JSON.stringify(report ?? {}),

packages/rest/src/rest-server.ts

@@ -10227,6 +10227,40 @@ export class RestServer {
  code: 'NOT_IMPLEMENTED',
  message: 'Reports service is not configured on this deployment',
  });
+ // [#11926] The door states the contract for `POST /reports` below.
+ // `IReportService.saveReport` takes a `SaveReportInput`
+ // (`packages/spec/src/contracts/report-service.ts`), on which `name`,
+ // `object` and `query` are all REQUIRED — but an HTTP body is untyped,
+ // so forwarding it unchecked handed the service a value merely CLAIMED
+ // to be a `SaveReportInput`. That left the requirement for every
+ // implementation to re-derive privately: the bundled
+ // `@objectstack/plugin-reports` does re-derive it, a third-party one
+ // need not, and a caller could not tell which one it was talking to.
+ // Refusing here makes the contract true for every implementation, in
+ // the same envelope `handleValidation` already produces (400 /
+ // VALIDATION_FAILED, ADR-0112).
+ // It THROWS rather than writing a response, and that is the design, not
+ // a detour: `handleValidation` below is this surface's single place for
+ // building a VALIDATION_FAILED body. Writing a second one here would
+ // make one route answer the same refusal in two different envelopes —
+ // and would add a non-conforming body to the `check:route-envelope`
+ // ratchet, which only ticks down. Raised before `saveReport` is called,
+ // so the door refuses rather than the service.
+ const assertSaveReportInput = (body: any): void => {
+ const missing = (['name', 'object', 'query'] as const)
+ .filter((field) => body?.[field] === undefined || body?.[field] === null);
+ if (missing.length > 0) {
+ throw new Error(
+ `VALIDATION_FAILED: ${missing.join(', ')} ${missing.length === 1 ? 'is' : 'are'} required`,
+ );
+ }
+ // `query` is a `ReportQuery` envelope — never a scalar and never a
+ // list. A string here is the shape an authoring mistake actually
+ // takes, and it reaches storage as a stringified scalar otherwise.
+ if (typeof body.query !== 'object' || Array.isArray(body.query)) {
+ throw new Error('VALIDATION_FAILED: query must be a ReportQuery object');
+ }
+ };
  const handleValidation = (res: any, err: any): boolean => {
  const msg = String(err?.message ?? err ?? '');
  if (msg.startsWith('VALIDATION_FAILED')) {
@@ -10280,6 +10314,11 @@ export class RestServer {
  const svc = await resolveService(environmentId);
  if (!svc) return respond501(res);
  try {
+ // AFTER the 501 on purpose: "no reports service is
+ // mounted" is a deployment fact and outranks anything
+ // about the body. Inside the try so the refusal reaches
+ // `handleValidation` like any other VALIDATION_FAILED.
+ assertSaveReportInput(req.body ?? {});
  const row = await svc.saveReport(req.body ?? {}, context ?? {});
  res.status(201).json(row);
  } catch (err: any) {

packages/rest/src/rest.test.ts

@@ -1605,16 +1605,80 @@ describe('RestServer', () => {
  expect(saveReport).toHaveBeenCalled();
  });
 
- it('POST /reports surfaces VALIDATION_FAILED as 400', async () => {
- const saveReport = vi.fn(async () => { throw new Error('VALIDATION_FAILED: name is required'); });
+ // [#11926] This pin's subject is the PASS-THROUGH: a VALIDATION_FAILED
+ // raised by the SERVICE is mapped to 400 by `handleValidation`. It used to
+ // drive `body: {}`, which the route now refuses at the door before the
+ // service is ever consulted — the assertions below would still have gone
+ // green, but on the door's refusal rather than the service's, pinning
+ // nothing. So the body is now one the door ACCEPTS and the double throws
+ // for a reason only a service can have, and `saveReport` is asserted to
+ // have actually been called so this cannot quietly go vacuous again.
+ it('POST /reports surfaces a service-raised VALIDATION_FAILED as 400', async () => {
+ const saveReport = vi.fn(async () => { throw new Error('VALIDATION_FAILED: format must be one of csv, json, html_table'); });
  const rest = makeRest(async () => ({ saveReport }));
  const { save } = getReportRoutes(rest);
  const res = { json: vi.fn(), status: vi.fn().mockReturnThis() };
- await save!.handler({ body: {} } as any, res as any);
+ await save!.handler({ body: { name: 'X', object: 'lead', query: {}, format: 'pdf' } } as any, res as any);
+ expect(saveReport).toHaveBeenCalled();
  expect(res.status).toHaveBeenCalledWith(400);
  expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ code: 'VALIDATION_FAILED' }));
  });
 
+ // [#11926] The door itself. `IReportService.saveReport` takes a
+ // `SaveReportInput`, on which `name`, `object` and `query` are required;
+ // the route used to forward `req.body ?? {}` unchecked, so the requirement
+ // held only as far as each implementation chose to re-derive it. These pin
+ // that the ROUTE states it — note `saveReport` is asserted NOT to have been
+ // called, which is the whole difference from the pass-through pin above.
+ it('POST /reports refuses a body the service contract requires more of', async () => {
+ const cases: Array<{ label: string; body: any }> = [
+ { label: 'no query', body: { name: 'X', object: 'lead' } },
+ { label: 'null query', body: { name: 'X', object: 'lead', query: null } },
+ { label: 'no object', body: { name: 'X', query: {} } },
+ { label: 'no name', body: { object: 'lead', query: {} } },
+ { label: 'empty body', body: {} },
+ ];
+ for (const { label, body } of cases) {
+ const saveReport = vi.fn(async (input: any) => ({ id: 'rpt_new', ...input }));
+ const rest = makeRest(async () => ({ saveReport }));
+ const { save } = getReportRoutes(rest);
+ const res = { json: vi.fn(), status: vi.fn().mockReturnThis() };
+ await save!.handler({ body } as any, res as any);
+ expect(res.status, label).toHaveBeenCalledWith(400);
+ expect(res.json, label).toHaveBeenCalledWith(expect.objectContaining({ code: 'VALIDATION_FAILED' }));
+ expect(saveReport, label).not.toHaveBeenCalled();
+ }
+ });
+
+ // [#11926] `query` is a `ReportQuery` envelope. A scalar or a list is the
+ // shape an authoring mistake actually takes, and it is present, so the
+ // required-key check above cannot catch it.
+ it('POST /reports refuses a query that is not a ReportQuery envelope', async () => {
+ for (const query of ['object=lead', 42, true, [{ field: 'id' }]]) {
+ const saveReport = vi.fn(async (input: any) => ({ id: 'rpt_new', ...input }));
+ const rest = makeRest(async () => ({ saveReport }));
+ const { save } = getReportRoutes(rest);
+ const res = { json: vi.fn(), status: vi.fn().mockReturnThis() };
+ await save!.handler({ body: { name: 'X', object: 'lead', query } } as any, res as any);
+ expect(res.status, JSON.stringify(query)).toHaveBeenCalledWith(400);
+ expect(saveReport, JSON.stringify(query)).not.toHaveBeenCalled();
+ }
+ });
+
+ // [#11926] An empty `ReportQuery` is LEGAL — every field on it is optional
+ // — so the door must not over-refuse. Without this, tightening `query` to
+ // "present and an object" could drift into "present and non-empty" with no
+ // test noticing.
+ it('POST /reports accepts an empty query object', async () => {
+ const saveReport = vi.fn(async (input: any) => ({ id: 'rpt_new', ...input }));
+ const rest = makeRest(async () => ({ saveReport }));
+ const { save } = getReportRoutes(rest);
+ const res = { json: vi.fn(), status: vi.fn().mockReturnThis() };
+ await save!.handler({ body: { name: 'X', object: 'lead', query: {} } } as any, res as any);
+ expect(saveReport).toHaveBeenCalled();
+ expect(res.status).toHaveBeenCalledWith(201);
+ });
+
  it('GET /reports/:id returns 404 when missing', async () => {
  const getReport = vi.fn(async () => null);
  const rest = makeRest(async () => ({ getReport }));