diff --git a/.changeset/rest-discovery-version-producer.md b/.changeset/rest-discovery-version-producer.md new file mode 100644 index 0000000000..faaf150451 --- /dev/null +++ b/.changeset/rest-discovery-version-producer.md @@ -0,0 +1,53 @@ +--- +'@objectstack/rest': patch +--- + +`GET /api/v1/discovery` reports the serving artifact's version instead of the +URL path segment the caller just typed + +`registerDiscoveryEndpoints` called the producer and overwrote the answer one +line later: + +```ts +const discovery = await protocol.getDiscovery(); + +// Override discovery information with actual server configuration +discovery.version = this.config.api.version; +``` + +`config.api.version` is the **API version identifier**, not an artifact +identity. `normalizeConfig()` defaults it to `'v1'`, `packages/spec`'s +`plugin-rest-api.zod.ts` describes it as "API version identifier", and the same +value builds the mount — `getApiBasePath()` returns +``api.apiPath ?? `${api.basePath}/${api.version}` `` → `/api/v1`. So on every +REST-served host, `GET /api/v1/discovery` answered `version: "v1"`: the segment +the caller had already typed to reach the endpoint, on every build of every +release, forever. + +`DiscoverySchema` declares `version` under **System Identity**, grouped with +`name` and `environment` — the "what server is this" question. The #10993 +ruling settled that reading and #11235/#11242 reaffirmed it. The override is +now gone and the producer's derived value reaches the wire. + +**What changes on the wire.** `version` on this endpoint was `"v1"` and is now +the value `getDiscovery()` derives: `OS_RUNTIME_VERSION` when a deployment or +build pipeline stamps one, else the resolved `@objectstack/metadata-protocol` +package version, else `"unknown"`. That is the same stamp `/health` and the +runtime dispatcher's own `/discovery` already read, so the two discovery +producers now give one answer rather than two dialects of one field. Before +#11297 this override masked two producers that genuinely disagreed (`'1.0.0'` +vs `'1.0'`); after it, it was overwriting a value that already agreed. + +**The API-version fact is not lost.** Every entry in the same document's +`routes` is prefixed with the mounted base path, which is built from +`api.version` — recoverable from the same response, in the field that means it. +No schema change, no new field: the accept set and the public surface are +unchanged, and `api.version` still does its real job of building the mount. + +Pinned in `packages/rest/src/discovery-schema-conformance.test.ts`, which drives +the **real** producer through the **real** handler. The assertions pin +provenance, never a literal version string — a stamp injected by the test must +appear on the wire, and the served value must equal what the producer answers +when called directly, including on a server configured with a different +`api.version` (where `routes.data` is asserted to still carry that segment). A +pin spelling a literal would rot at the next release. diff --git a/packages/rest/src/discovery-schema-conformance.test.ts b/packages/rest/src/discovery-schema-conformance.test.ts index ab7d333395..4309f15ece 100644 --- a/packages/rest/src/discovery-schema-conformance.test.ts +++ b/packages/rest/src/discovery-schema-conformance.test.ts @@ -4,9 +4,10 @@ // matters most, because this is the shape a browser client actually receives. // // It is a COMPOSED shape: `getDiscovery()` (metadata-protocol) builds the base, -// then `registerDiscoveryEndpoints` overrides `version` and `routes`, ANDs +// then `registerDiscoveryEndpoints` overrides `routes`, ANDs // `capabilities.transactionalBatch` with its own `api.enableBatch`, and attaches -// `scoping`. Neither producer alone could be checked against the schema and be +// `scoping`. It no longer overrides `version` — see the #11292 suite at the +// bottom, which pins the served value's PROVENANCE. Neither producer alone could be checked against the schema and be // meaningful here, so this test drives the REAL protocol implementation through // the REAL handler rather than the `createMockProtocol()` double the other // rest tests use — a mock would only prove the mock's shape conforms. @@ -71,7 +72,7 @@ function createMockServer() { * deployment) or the environment-scoped `/api/v1/environments/:environmentId` * one, which is the only mount that can report `scoped: true`. */ -function discoveryHandler(opts: { scoped?: boolean } = {}) { +function buildDiscovery(opts: { scoped?: boolean; apiVersion?: string } = {}) { const engine = { registry: { getObject: (_n: string) => undefined, @@ -82,18 +83,30 @@ function discoveryHandler(opts: { scoped?: boolean } = {}) { const config: any = { api: { requireAuth: false, + ...(opts.apiVersion ? { version: opts.apiVersion } : {}), ...(opts.scoped ? { enableProjectScoping: true, projectResolution: 'auto' } : {}), }, }; const rest = new RestServer(createMockServer() as any, protocol as any, config); rest.registerRoutes(); + // The mounted segment is `api.version`'s job (`getApiBasePath()`), which is + // exactly why it is not the identity answer — see the #11292 suite below. + const version = opts.apiVersion ?? 'v1'; const path = opts.scoped - ? '/api/v1/environments/:environmentId/discovery' - : '/api/v1/discovery'; + ? `/api/${version}/environments/:environmentId/discovery` + : `/api/${version}/discovery`; const entry = rest.getRouteManager().get('GET', path); if (!entry) throw new Error(`discovery route not registered at ${path}`); - return entry.handler as (req: any, res: any) => Promise; + return { + handler: entry.handler as (req: any, res: any) => Promise, + /** The REAL producer this server composes over — the provenance the wire answer must track. */ + protocol, + }; +} + +function discoveryHandler(opts: { scoped?: boolean; apiVersion?: string } = {}) { + return buildDiscovery(opts).handler; } async function invoke( @@ -318,4 +331,87 @@ describe('[#4828] the REST /discovery live shape conforms to DiscoverySchema', ( ).toBeUndefined(); }); }); + + // ═══════════════════════════════════════════════════════════════════════════ + // [#11292] `version` is the PRODUCER's — provenance, pinned without a literal + // ═══════════════════════════════════════════════════════════════════════════ + // + // This seam used to run `discovery.version = this.config.api.version` one + // line after calling the producer, so the wire answer was the MOUNTED PATH + // SEGMENT (`'v1'` by default) — the string the caller had just typed to reach + // the endpoint. `DiscoverySchema` declares `version` under "System Identity" + // next to `name` and `environment`, and the #10993 ruling (reaffirmed by + // #11235/#11242) settled that as the SERVING ARTIFACT's version. + // + // Every assertion below pins PROVENANCE, never a literal version string: the + // wire answer is compared against the producer's own answer, or against a + // stamp this test injects. A pin spelling `'1.0.0'` would rot at the next + // release and would re-create the class #11295 is filed against. + describe("[#11292] the served `version` is the producer's, not the mounted API version", () => { + it('tracks the producer when the deployment stamps OS_RUNTIME_VERSION', async () => { + // `resolveDiscoveryVersion()` reads the stamp LIVE (documented in + // `metadata-protocol/src/discovery-version.ts`), so a value injected here + // must appear on the wire. This is the provenance assertion: the sentinel + // exists nowhere in the REST layer, so it can only have come through + // `getDiscovery()`. + const old = process.env.OS_RUNTIME_VERSION; + process.env.OS_RUNTIME_VERSION = '9.9.9-provenance-sentinel'; + try { + const body = await invoke(discoveryHandler()); + + expect(body.version).toBe('9.9.9-provenance-sentinel'); + expect(DiscoverySchema.safeParse(body).success).toBe(true); + } finally { + if (old === undefined) delete process.env.OS_RUNTIME_VERSION; + else process.env.OS_RUNTIME_VERSION = old; + } + }); + + it('agrees with the producer called directly, whatever the producer derives', async () => { + // No stamp: the producer falls through to its own package version. The + // assertion still names no literal — it asks only that the two answers + // are the SAME answer, which is the whole content of "the REST seam does + // not rewrite this field". + const { handler, protocol } = buildDiscovery(); + + const body = await invoke(handler); + const direct: any = await (protocol as any).getDiscovery(); + + expect(body.version).toBe(direct.version); + expect(typeof body.version).toBe('string'); + expect(body.version.length).toBeGreaterThan(0); + }); + + it('answers the producer even when `api.version` is set to something else', async () => { + // The sharp edge, stated as a measurement. `api.version` still does its + // real job — it builds the mount — and that job is visible in the SAME + // document, on `routes`. What it no longer does is answer the identity + // question. + const { handler, protocol } = buildDiscovery({ apiVersion: 'v9' }); + + const body = await invoke(handler); + const direct: any = await (protocol as any).getDiscovery(); + + // Anti-vacuity: `api.version` really is 'v9' on this server, and really + // does drive the mounted path. Without this, the assertion below could + // pass on a server where the option was silently ignored. + expect(body.routes.data).toBe('/api/v9/data'); + + expect(body.version).toBe(direct.version); + expect(body.version).not.toBe('v9'); + }); + + it('does not answer the mounted segment on the scoped mount either', async () => { + // The scoped mount runs the same closure but resolves a per-request + // protocol, so it is a second path to the same field. + const { handler, protocol } = buildDiscovery({ scoped: true, apiVersion: 'v9' }); + + const body = await invoke(handler, { environmentId: 'env_alpha' }); + const direct: any = await (protocol as any).getDiscovery(); + + expect(body.routes.data).toBe('/api/v9/environments/env_alpha/data'); + expect(body.version).toBe(direct.version); + expect(body.version).not.toBe('v9'); + }); + }); }); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 20b8f858eb..35dfa4aef1 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -3215,8 +3215,32 @@ export class RestServer { const protocol = await this.resolveProtocol(environmentId, req); const discovery = await protocol.getDiscovery(); - // Override discovery information with actual server configuration - discovery.version = this.config.api.version; + // [#11292] `version` is the PRODUCER's, and is deliberately + // NOT overwritten here. `DiscoverySchema` declares the field + // under "System Identity", grouped with `name` and + // `environment` — the "what server is this" question, settled + // by the #10993 ruling and reaffirmed by #11235/#11242. + // + // This line used to read `discovery.version = + // this.config.api.version`, which is a different fact + // entirely: `normalizeConfig()` defaults it to `'v1'` and the + // SAME value builds the mounted path (`${basePath}/${version}` + // → `/api/v1`), so `GET /api/v1/discovery` answered with the + // path segment the caller had just typed to get there. On the + // one producer most clients actually hit, the identity field + // carried no identity. + // + // It also masked the producer. `getDiscovery()` derives the + // value from `OS_RUNTIME_VERSION` (#11235) — the same stamp + // `/health` and the runtime dispatcher's own `/discovery` + // read (#10993/#11242) — so after #11297 this overwrote a + // value that already AGREED with the other producer, turning + // one answer back into two dialects of one field. + // + // The API-version fact is not lost: every entry in `routes` + // below is prefixed with the mounted base path, which is + // built from `api.version`. It is recoverable from the same + // document, in the field that means it. // Substitute the resolved environmentId into the advertised routes so // clients can consume them verbatim (e.g. /api/v1/environments/abc/data).