diff --git a/.changeset/reports-save-input-contract-at-the-door.md b/.changeset/reports-save-input-contract-at-the-door.md new file mode 100644 index 0000000000..e779eb052a --- /dev/null +++ b/.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. + + diff --git a/packages/client/src/client.test.ts b/packages/client/src/client.test.ts index e2d0314f5b..72ca94c7fd 100644 --- a/packages/client/src/client.test.ts +++ b/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 () => { diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index 8f8dc38d3f..073f14e578 100644 --- a/packages/client/src/index.ts +++ b/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 => { + /** + * 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 => { const res = await this.fetch(`${this.baseUrl}/api/v1/reports`, { method: 'POST', body: JSON.stringify(report ?? {}), diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 8ac62c8093..7b977048b1 100644 --- a/packages/rest/src/rest-server.ts +++ b/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) { diff --git a/packages/rest/src/rest.test.ts b/packages/rest/src/rest.test.ts index b9bd0287c8..3d9edb7cd3 100644 --- a/packages/rest/src/rest.test.ts +++ b/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 }));