From 5bbfa88d9d485b68cfbc7c0c42eee55f897952f6 Mon Sep 17 00:00:00 2001 From: Andrii Kohut Date: Tue, 18 Aug 2026 12:09:42 +0200 Subject: [PATCH] fix: describe the whole API in openapi.json, not the read half MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The document was generated from READ_ROUTES, so it advertised seven GET endpoints out of eighteen. Ingestion, the JUnit path, OTLP traces, artifact presigning, the run summary, the quality gate, CODEOWNERS, notification routing and quarantine were all invisible to anything generating a client, and nothing in the document said a fraction was on offer. It now comes from REST_ENDPOINTS — the same table the human reference is built from — so there is one description of the API and two renderings of it, rather than two descriptions that can disagree. Request bodies come from the zod schemas the endpoints already validate against, inlined rather than named: a name puts the schema under `definitions` and leaves a $ref pointing where OpenAPI does not look, which a generated client cannot resolve. Four guards, because a specification that drifts is worse than none — it is wrong with authority. Every documented endpoint must appear; every endpoint taking a body must describe one; the liveness probe must not demand a token, since a document that does teaches people to ignore its security blocks; and every path parameter a URL names must be declared, or no generated client can build that URL. --- .changeset/complete-openapi.md | 5 + apps/api/package.json | 4 +- apps/api/src/__tests__/api-surface.test.ts | 56 ++++++++- apps/api/src/__tests__/rest.test.ts | 20 +++- apps/api/src/rest.ts | 129 +++++++++++++++------ pnpm-lock.yaml | 6 + 6 files changed, 181 insertions(+), 39 deletions(-) create mode 100644 .changeset/complete-openapi.md diff --git a/.changeset/complete-openapi.md b/.changeset/complete-openapi.md new file mode 100644 index 0000000..e810b0e --- /dev/null +++ b/.changeset/complete-openapi.md @@ -0,0 +1,5 @@ +--- +'@flakemetry/contracts': patch +--- + +`/openapi.json` now describes the whole API rather than the read half. It was generated from the read-route table, so ingestion, the quality gate, artifact presigning and quarantine were invisible to anything generating a client — with nothing in the document to say a client was seeing a fraction of it. Request bodies are now included, generated from the same zod schemas the endpoints validate against. diff --git a/apps/api/package.json b/apps/api/package.json index d48f89e..4b2582c 100644 --- a/apps/api/package.json +++ b/apps/api/package.json @@ -36,12 +36,14 @@ "@flakemetry/contracts": "workspace:*", "@flakemetry/db": "workspace:*", "@flakemetry/queries": "workspace:*", + "@flakemetry/storage": "workspace:*", "@opentelemetry/api": "^1.9.0", "@opentelemetry/exporter-metrics-otlp-http": "^0.221.0", "@opentelemetry/resources": "^2.10.0", "@opentelemetry/sdk-metrics": "^2.10.0", "@trpc/server": "^11.0.0", "fastify": "^5.11.0", - "@flakemetry/storage": "workspace:*" + "zod": "^3.25.76", + "zod-to-json-schema": "^3.24.1" } } diff --git a/apps/api/src/__tests__/api-surface.test.ts b/apps/api/src/__tests__/api-surface.test.ts index cdf9cd1..2cb15f9 100644 --- a/apps/api/src/__tests__/api-surface.test.ts +++ b/apps/api/src/__tests__/api-surface.test.ts @@ -7,7 +7,7 @@ import type { PrismaClient } from '@flakemetry/db' import { afterAll, beforeAll, describe, expect, it } from 'vitest' import { buildApp } from '../app' -import { READ_ROUTES } from '../rest' +import { openApiDocument, READ_ROUTES } from '../rest' import { appRouter } from '../trpc/router' const app = buildApp({ prisma: {} as unknown as PrismaClient, store: null }) @@ -69,3 +69,57 @@ describe('documented API surface', () => { expect(documented).toEqual(exposed) }) }) + +describe('the machine-readable description covers the whole surface', () => { + const document = openApiDocument('0.0.0') as { + paths: Record> + } + + it('describes every documented endpoint, not only the readable half', () => { + // It used to be generated from READ_ROUTES, so ingestion, the quality gate and + // quarantine were invisible to anything generating a client — with nothing in the + // document to say a client was seeing a fraction of the API. + const missing = REST_ENDPOINTS.filter((endpoint) => { + const path = endpoint.path.replace(/:(\w+)/g, '{$1}') + return !document.paths[path]?.[endpoint.method.toLowerCase()] + }).map((endpoint) => `${endpoint.method} ${endpoint.path}`) + + expect(missing).toEqual([]) + }) + + it('describes the body of every endpoint that takes one', () => { + for (const endpoint of REST_ENDPOINTS.filter((candidate) => candidate.request)) { + const path = endpoint.path.replace(/:(\w+)/g, '{$1}') + const operation = document.paths[path]?.[endpoint.method.toLowerCase()] + expect(operation?.requestBody, `${endpoint.path} has no requestBody`).toBeDefined() + } + }) + + it('leaves the unauthenticated endpoints unauthenticated', () => { + const health = document.paths['/health']?.get + const ingest = document.paths['/v1/ingest']?.post + + // A document that demands a token for the liveness probe teaches people to ignore its + // security blocks entirely. + expect(health?.security).toEqual([]) + expect(ingest?.security).not.toEqual([]) + }) + + it('declares every path parameter it names', () => { + const undeclared: string[] = [] + for (const [path, operations] of Object.entries(document.paths)) { + const named = [...path.matchAll(/\{(\w+)\}/g)].map((match) => match[1]) + for (const [method, operation] of Object.entries(operations)) { + const params = ((operation as { parameters?: { name: string; in: string }[] }).parameters ?? + []) as { name: string; in: string }[] + for (const name of named) { + if (!params.some((param) => param.name === name && param.in === 'path')) { + undeclared.push(`${method.toUpperCase()} ${path} → ${name}`) + } + } + } + } + // A path with an undeclared parameter is a URL no generated client can build. + expect(undeclared).toEqual([]) + }) +}) diff --git a/apps/api/src/__tests__/rest.test.ts b/apps/api/src/__tests__/rest.test.ts index b3dc1c7..6200693 100644 --- a/apps/api/src/__tests__/rest.test.ts +++ b/apps/api/src/__tests__/rest.test.ts @@ -1,5 +1,6 @@ import { randomUUID } from 'node:crypto' +import { REST_ENDPOINTS } from '@flakemetry/contracts' import { generateToken, hashToken, PrismaClient } from '@flakemetry/db' import type { FastifyInstance } from 'fastify' import { afterAll, beforeEach, describe, expect, it } from 'vitest' @@ -27,10 +28,23 @@ describe('openapi document', () => { it('describes every route that is registered', () => { const document = openApiDocument('0.1.0') as { paths: Record } - // Generated from the same table that registers the routes, so a spec that disagrees with - // the implementation is not expressible. - expect(Object.keys(document.paths)).toHaveLength(READ_ROUTES.length) + // Generated from REST_ENDPOINTS — the same table the human reference is built from — + // so a spec that disagrees with the implementation is not expressible. It covers the + // whole surface, not only the read routes, which is why the count is checked against + // the documented endpoints rather than against READ_ROUTES. + const documented = new Set( + REST_ENDPOINTS.map((endpoint) => endpoint.path.replace(/:(\w+)/g, '{$1}')), + ) + expect(Object.keys(document.paths).sort()).toEqual([...documented].sort()) expect(document.paths['/v1/runs/{runId}']).toBeDefined() + expect(document.paths['/v1/ingest']).toBeDefined() + }) + + it('still registers every read route it describes', () => { + expect(READ_ROUTES.length).toBeGreaterThan(0) + for (const route of READ_ROUTES) { + expect(REST_ENDPOINTS.some((endpoint) => endpoint.path === route.path)).toBe(true) + } }) }) diff --git a/apps/api/src/rest.ts b/apps/api/src/rest.ts index 2857202..3060b18 100644 --- a/apps/api/src/rest.ts +++ b/apps/api/src/rest.ts @@ -3,6 +3,9 @@ import { createGzip } from 'node:zlib' import { flakyBoardInputSchema, + REST_ENDPOINTS, + type RestAuth, + type RestEndpoint, runsListInputSchema, testGetInputSchema, testHealthInputSchema, @@ -22,6 +25,7 @@ import { } from '@flakemetry/queries' import { type ObjectStore, projectArtifactPrefix } from '@flakemetry/storage' import type { FastifyInstance, FastifyReply, FastifyRequest } from 'fastify' +import { zodToJsonSchema } from 'zod-to-json-schema' import { type AuthenticatedProject, authenticateProject, hasScope } from './auth' import type { RateLimiter } from './rate-limit' @@ -218,40 +222,98 @@ export const READ_ROUTES: ReadRoute[] = [ }, ] +const SCOPE_NOTE: Record = { + none: null, + 'ingest-token': 'Needs a token carrying the "ingest" scope.', + 'read-token': 'Needs a token carrying the "read" scope.', + 'quarantine-token': 'Needs a token carrying the "quarantine" scope.', + 'any-token': 'Needs a project token carrying either the "ingest" or the "read" scope.', +} + +const LEADING_STATUS = /^`(\d{3})`/ + +const successStatus = (response: string): number => { + const match = LEADING_STATUS.exec(response.trim()) + return match ? Number(match[1]) : 200 +} + +const readRouteByPath = new Map(READ_ROUTES.map((route) => [route.path, route])) + +const parametersFor = (endpoint: RestEndpoint): unknown[] => { + const route = readRouteByPath.get(endpoint.path) + const declared = [ + ...(route?.params ?? []).map((param) => ({ ...param, in: 'path', required: true })), + ...(route?.query ?? []).map((param) => ({ ...param, in: 'query', required: false })), + ] + if (declared.length > 0) { + return declared.map((param) => ({ + name: param.name, + in: param.in, + required: param.required, + description: param.description, + schema: { type: 'string' }, + })) + } + + // Paths carrying a segment that no route table describes still have to declare it, or + // the document is not a valid description of a URL a client can build. + return [...endpoint.path.matchAll(/:(\w+)/g)].map((match) => ({ + name: match[1], + in: 'path', + required: true, + schema: { type: 'string' }, + })) +} + +/** + * Generated from `REST_ENDPOINTS` — the same table the human reference is built from — so + * the machine-readable description covers the whole surface rather than the read half. + * It previously came from `READ_ROUTES`, which meant ingestion, the quality gate and + * quarantine were invisible to anything generating a client, with nothing to say so. + */ export const openApiDocument = (version: string): Record => { - const paths: Record = {} + const paths: Record> = {} + + for (const endpoint of REST_ENDPOINTS) { + const path = endpoint.path.replace(/:(\w+)/g, '{$1}') + const route = readRouteByPath.get(endpoint.path) + const scopeNote = SCOPE_NOTE[endpoint.auth] + + const responses: Record = { + [successStatus(endpoint.response)]: { + description: endpoint.response, + ...(route?.produces ? { content: { [route.produces]: {} } } : {}), + }, + } + if (endpoint.auth !== 'none') { + responses[401] = { description: 'Missing or invalid token' } + responses[403] = { description: 'The token does not carry the required scope' } + responses[429] = { description: 'Rate limited' } + } - for (const route of READ_ROUTES) { - const path = route.path.replace(/:(\w+)/g, '{$1}') paths[path] = { - get: { - summary: route.summary, - security: [{ bearerAuth: [] }], - parameters: [ - ...(route.params ?? []).map((param) => ({ - name: param.name, - in: 'path', - required: true, - description: param.description, - schema: { type: 'string' }, - })), - ...(route.query ?? []).map((param) => ({ - name: param.name, - in: 'query', - required: false, - description: param.description, - schema: { type: 'string' }, - })), - ], - responses: { - 200: route.produces - ? { description: 'Success', content: { [route.produces]: {} } } - : { description: 'Success' }, - 401: { description: 'Missing or invalid token' }, - 403: { description: 'The token does not carry the read scope' }, - 404: { description: 'Not found' }, - 429: { description: 'Rate limited' }, - }, + ...paths[path], + [endpoint.method.toLowerCase()]: { + summary: endpoint.summary, + ...(scopeNote ? { description: scopeNote } : {}), + security: endpoint.auth === 'none' ? [] : [{ bearerAuth: [] }], + parameters: parametersFor(endpoint), + ...(endpoint.request + ? { + requestBody: { + required: true, + content: { + 'application/json': { + // Inlined rather than named: a `name` puts the schema under + // `definitions` and leaves a $ref pointing where OpenAPI does not + // look, so a generated client cannot resolve it. + schema: zodToJsonSchema(endpoint.request.schema, { $refStrategy: 'none' }), + }, + }, + }, + } + : {}), + responses, }, } } @@ -259,17 +321,16 @@ export const openApiDocument = (version: string): Record => { return { openapi: '3.1.0', info: { - title: 'Flakemetry read API', + title: 'Flakemetry API', version, description: - 'Read-only access to a single project. Authorise with a token carrying the "read" scope; an ingest token will not do, so a credential handed to a script cannot forge test data.', + 'Ingestion, read and quarantine for a single project. Scopes are separate on purpose: a credential handed to a script cannot forge test data, one that reads cannot silence a failing test, and the one in every CI job carries neither.', }, components: { securitySchemes: { bearerAuth: { type: 'http', scheme: 'bearer', description: 'A project token' }, }, }, - security: [{ bearerAuth: [] }], paths, } } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index bfa4009..ccadee6 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -70,6 +70,12 @@ importers: fastify: specifier: ^5.11.0 version: 5.11.2 + zod: + specifier: ^3.25.76 + version: 3.25.76 + zod-to-json-schema: + specifier: ^3.24.1 + version: 3.25.2(zod@3.25.76) devDependencies: '@flakemetry/eslint-config': specifier: workspace:*