diff --git a/.changeset/17672-repeated-version-400-reachability.md b/.changeset/17672-repeated-version-400-reachability.md new file mode 100644 index 0000000000..de972866db --- /dev/null +++ b/.changeset/17672-repeated-version-400-reachability.md @@ -0,0 +1,68 @@ +--- +'@objectstack/rest': minor +'@objectstack/runtime': patch +--- + +fix(runtime): a repeated `?version=` on `GET /packages/:id` is refused `400 VALIDATION_ERROR` in the repo's one message, and `@objectstack/rest` publishes the rule that owns it (#17672) + +`GET /api/v1/packages/:id?version=a&version=b` answered **`404`**, with a second +sentence written at that door. This repo already had a landed answer for exactly +that condition on exactly that route — `400 VALIDATION_ERROR` in the ADR-0112 +nested body (#6307) — and one implementation of it, `refuseRepeatedQueryParams` +/ `repeatedQueryParamMessage` in `packages/rest/src/query-multiplicity.ts`, +whose header is the authority on the rule. + +Driven before the change, one host, three refusals: + +``` +GET /packages/com.acme.crm?version=a&version=b -> 404 RESOURCE_NOT_FOUND +GET /packages/com.acme.crm?version=99.0.0 -> 404 RESOURCE_NOT_FOUND +GET /packages/com.absent.pkg?version=99.0.0 -> 404 RESOURCE_NOT_FOUND +``` + +A client branching on the answer could not tell "your request named the +parameter twice" from the two genuine not-founds. After: + +``` +GET /packages/com.acme.crm?version=a&version=b -> 400 VALIDATION_ERROR +GET /packages/com.acme.crm?version=99.0.0 -> 404 RESOURCE_NOT_FOUND +GET /packages/com.absent.pkg?version=99.0.0 -> 404 RESOURCE_NOT_FOUND +``` + +The body is the dispatcher's declared envelope — +`{ success: false, error: { code: 'VALIDATION_ERROR', message, httpStatus: 400 } }` +— with `VALIDATION_ERROR` derived by `buildApiError` from +`standardErrorCodeForHttpStatus(400)`, the standard catalog's member for 400. +⛔ Nothing in `packages/spec` moves. + +**What was actually blocking this was reachability, not judgement.** +`@objectstack/rest` declares exactly one export subpath and that module was not +on it, so #17668 could neither call the rule nor (correctly) copy it, and +shipped the `404` with its own sentence instead. The barrel now publishes +`repeatedQueryParamMessage` and `refuseRepeatedQueryParams`, and the dispatcher +domain calls the message function — so the sentence a caller is told for a +repeated parameter is the same one on every door that carries the rule, ⛔ never +a second copy that drifts. + +⚠️ The two published symbols are not interchangeable across a package boundary, +and the barrel entry says so. `repeatedQueryParamMessage` is the portable half: +a pure function of two primitives. `refuseRepeatedQueryParams` writes the bare +ADR-0112 body onto a `res`, which suits handlers of that shape and ⛔ not a +runtime dispatcher domain — measured, its body fails that surface's +`BaseResponseSchema` with `success is missing, must be a boolean`. + +**Not a breaking change, measured rather than assumed.** The `404` it replaces +was introduced by #17668 (`1a25f4a8d`), which is not an ancestor of +`@objectstack/runtime@17.4.0` (exit 1; two control commits from that tag's own +history answer exit 0 on the same predicate, in a checkout +`--is-shallow-repository` reports `false`). It has never been published, so no +released consumer can have branched on it. Everything else about the door is +unchanged: `?version=` and `?version=latest` still serve the +installed row, an absent version and an unknown id still answer `404`, and a +one-element array is still one occurrence. + +Also corrected, on the module that owns the rule: its header said the +dispatcher's `/packages` domain "reads no `version`" — load-bearing prose, +since it is part of why the rule needs only one home. That stopped being true +when #17668 landed. The paragraph now states what is true, which is that the one +home did not move and now serves two doors. diff --git a/packages/rest/src/index.ts b/packages/rest/src/index.ts index a88ece9c64..714ca8d902 100644 --- a/packages/rest/src/index.ts +++ b/packages/rest/src/index.ts @@ -47,3 +47,24 @@ export { coerceRow } from './import-coerce.js'; export type { CoerceContext, RefResolver } from './import-coerce.js'; export { buildFieldMetaMap } from './export-format.js'; export type { ExportFieldMeta } from './export-format.js'; + +// Query-parameter MULTIPLICITY — the repo's ONE rule for a single-valued +// parameter supplied more than once (#6307 / #6877), published so the doors +// OUTSIDE this package can answer it with that one implementation instead of a +// second copy that drifts (#17672). `query-multiplicity.ts`'s header is the +// authority on the rule; what belongs here is which half travels. +// +// `repeatedQueryParamMessage` is the portable half and the one the dispatcher's +// `/packages` domain calls: it is a pure function of two primitives, so a +// caller in any package gets the same sentence and no transport assumptions +// ride along with it. +// +// ⚠️ `refuseRepeatedQueryParams` is the `res`-shaped gate, for a consumer that +// has a response object to write — this package's own handlers, and any sibling +// mounting handlers of that shape. It is NOT usable from a runtime dispatcher +// domain: the body it writes is the bare ADR-0112 `{ error: { code, message } }` +// and that surface's envelope needs the `success` / `httpStatus` siblings +// `@objectstack/runtime`'s `buildApiError` adds (measured on #17672 — the gate's +// body fails `BaseResponseSchema` with `success is missing, must be a boolean`). +// ⛔ A dispatcher domain takes the message and builds its own body. +export { refuseRepeatedQueryParams, repeatedQueryParamMessage } from './query-multiplicity.js'; diff --git a/packages/rest/src/query-multiplicity.ts b/packages/rest/src/query-multiplicity.ts index 875e3238bf..e6887a5557 100644 --- a/packages/rest/src/query-multiplicity.ts +++ b/packages/rest/src/query-multiplicity.ts @@ -58,9 +58,28 @@ import { RPC_QUERY_ALIAS_SLOTS } from '@objectstack/spec/data'; * #6307 landed the first copy of this rule in `package-routes.ts`, on the * `?version=` of that registrar's package read/delete routes. Those routes are * gone (#14503 — the dispatcher's `/packages` domain is their single - * implementation, and it reads no `version`), so the rule now has one home: - * here, for the `rest-server.ts` read points — ONE rule and one message, not - * a second implementation that drifts. + * implementation), so the rule has one home: here. + * + * That domain DOES read `?version=`: #17668 taught `GET /packages/:id` to + * honour it. An earlier version of this paragraph said it read none, which + * stopped being true the day that landed and left this module understating its + * own scope (#17672). A repeated occurrence there is refused with + * {@link repeatedQueryParamMessage} from here, so the home neither moved nor + * split: the rule serves TWO doors — the `rest-server.ts` read points through + * {@link refuseRepeatedQueryParams}, and that dispatcher domain through the + * message function alone — ONE rule and one message, not a second + * implementation that drifts. + * + * ⚠️ Why the dispatcher domain takes only the message: the two doors write + * their bodies through different builders. {@link refuseRepeatedQueryParams} + * puts the ADR-0112 body on `res` itself, which is right for the handlers in + * this package; a dispatcher domain RETURNS `{ handled, response }` and every + * error body on that surface is built by `@objectstack/runtime`'s + * `buildApiError`, whose envelope carries the `success` / `httpStatus` siblings + * this one does not — measured on #17672: the body written below fails that + * surface's `BaseResponseSchema` with `success is missing, must be a boolean`. + * So the message is the portable half and the gate is not: ⛔ a dispatcher + * domain calls {@link repeatedQueryParamMessage}, never this gate. */ /** diff --git a/packages/runtime/src/domains/packages-get-version-scope.test.ts b/packages/runtime/src/domains/packages-get-version-scope.test.ts index 9318d9748c..4cbd50798a 100644 --- a/packages/runtime/src/domains/packages-get-version-scope.test.ts +++ b/packages/runtime/src/domains/packages-get-version-scope.test.ts @@ -32,14 +32,28 @@ * * §3 pins the two requests that mean "the installed row" — no parameter and * `?version=latest` — as ONE request, which is the contract the deleted - * handler published (`requested.value || 'latest'`). §4 pins the repeated - * parameter as not-a-silent-success: `?version=a&version=b` carries two - * conflicting intents, and the answer names what it saw rather than choosing. - * ⚠️ §4 asserts the DEFECT CLASS is closed (no `200` with the installed row), - * deliberately not the exact status, because the repo's one rule for a repeated - * single-valued parameter answers `400 VALIDATION_ERROR` and is unreachable - * from this package today — see `readRequestedVersion`'s header in - * `packages.ts`. So this pin stays green when that rule lands here. + * handler published (`requested.value || 'latest'`). + * + * §4 pins the repeated parameter. ⚠️ [#17672] **This section's pin was changed + * deliberately.** As written for #17416 it asserted only that the DEFECT CLASS + * was closed (`status` is not `200`, no installed row rides out) and explicitly + * NOT the status, because the repo's one rule for a repeated single-valued + * parameter answers `400 VALIDATION_ERROR` and was then reachable from nowhere + * outside `@objectstack/rest` — so the door shipped a `404` and this pin was + * written loose enough to survive the eventual fix. ⛔ That `404` was never + * this door's contract: it was the interim answer of an unreachable rule, and + * #17672 filed it because it made a request-shape error indistinguishable from + * the two genuine not-founds §1 pins. The rule is reachable now + * (`@objectstack/rest` publishes `repeatedQueryParamMessage`), so §4 pins the + * END state — the status, the `VALIDATION_ERROR` code, the ADR-0112 nested + * body, and the message BY DERIVATION from the shared function rather than as a + * literal, so a caller is told the same sentence here as on every other door + * that carries the rule. + * + * §5 is the card's actual acceptance criterion, which neither §1 nor §4 states + * on its own: the three refusals this door can give are mutually + * distinguishable by `status` + `error.code`, so a client branching on them can + * finally tell "your request named the parameter twice" from "not found". * * ## The harness * @@ -51,6 +65,13 @@ import { describe, it, expect } from 'vitest'; import { SchemaRegistry } from '@objectstack/objectql'; +// [#17672] The SHARED rule's message, from the module that owns it. §4 asserts +// the wire text by DERIVATION from this function — ⛔ never as a literal, which +// would go on passing while the door answered a sentence of its own that +// happened to match the day it was written. The control that makes the +// derivation falsifiable is the ablation recorded in the PR: change the +// sentence here and this door's answer moves with it. +import { repeatedQueryParamMessage } from '@objectstack/rest'; import { HttpDispatcher } from '../http-dispatcher.js'; const PKG = 'com.acme.crm'; @@ -121,6 +142,9 @@ describe('#17416 GET /packages/:id — ?version= scopes the read', () => { // The discriminating field, not the status alone. expect(scoped.status).toBe(404); + // [#17672] A GENUINE not-found, and it stays one: this is the half + // of the card that must SURVIVE the repeated-parameter fix. + expect(scoped.body?.error?.code).toBe('RESOURCE_NOT_FOUND'); expect(scoped.body?.error?.message).toContain(ABSENT); expect(scoped.body?.error?.message).toContain(INSTALLED); // ⛔ No package row rode out on the refusal. @@ -147,6 +171,26 @@ describe('#17416 GET /packages/:id — ?version= scopes the read', () => { // ⛔ The id 404 is NOT re-worded by the version scope: a package that // is not here cannot be "at the wrong version". expect(r.response?.body?.error?.message).toBe(`Package 'com.absent.pkg' not found`); + // [#17672] The second genuine not-found, pinned on its code too — + // and this pair is a PUBLISHED sentence, not only an internal one: + // `content/docs/kernel/contracts/metadata-service.mdx`'s route table + // says of `GET /api/v1/packages/:id` that «a missing id answers + // `404 RESOURCE_NOT_FOUND`, message `Package 'ID' not found`». Both + // halves are asserted here, so moving either turns this red instead + // of silently falsifying that page. + expect(r.response?.body?.error?.code).toBe('RESOURCE_NOT_FOUND'); + }); + + it('an unknown id wins over a repeated ?version= — the #17416 ordering, unchanged', async () => { + // [#17672] The multiplicity check sits AFTER the id lookup, where + // #17416 put the version scope. This card moved the STATUS of a + // refusal, ⛔ not the order of two refusals — so an id this registry + // does not hold keeps answering `not found` with a repeated + // parameter riding along, and that is pinned rather than incidental. + const dispatcher = make(); + const r = await dispatcher.handlePackages('/com.absent.pkg', 'GET', undefined, { version: ['a', 'b'] }, reader()); + expect(r.response?.status).toBe(404); + expect(r.response?.body?.error?.message).toBe(`Package 'com.absent.pkg' not found`); }); }); @@ -180,14 +224,47 @@ describe('#17416 GET /packages/:id — ?version= scopes the read', () => { }); }); - describe('§4 a repeated ?version= is not resolved silently', () => { + describe('§4 a repeated ?version= is refused 400 VALIDATION_ERROR, in the shared rule’s words', () => { it('two conflicting values are not answered 200 with the installed row', async () => { const r = await get({ version: [ABSENT, INSTALLED] }); - // The defect class: a success carrying a row the caller did not ask for. + // The defect class #17416 closed: a success carrying a row the + // caller did not ask for. Kept as its own assertion — the status + // pin below is a stronger claim, and this one is the reason. expect(r.status).not.toBe(200); expect(r.body?.data).toBeUndefined(); - // It says what it saw rather than choosing one of the two. - expect(r.body?.error?.message).toContain('supplied 2 times'); + }); + + it('[#17672] answers 400 VALIDATION_ERROR — a request-shape error, not a not-found', async () => { + const r = await get({ version: [ABSENT, INSTALLED] }); + // ⚠️ The interim answer was `404`. See this file's header: that was + // the answer of an unreachable rule, never this door's contract. + expect(r.status).toBe(400); + // ADR-0112 NESTED body, and the standard catalog's member for 400 — + // derived by `buildApiError` from the status, so nothing in + // `packages/spec` moved for it. It is also the answer a PUBLISHED + // page already documented for this exact condition: + // `content/docs/api/client-sdk.mdx`'s error table gives + // `VALIDATION_ERROR` / 400 for «The request was refused before any + // record was validated — a repeated query parameter, …». This door + // contradicted that page for as long as it answered `404`. + expect(r.body?.error?.code).toBe('VALIDATION_ERROR'); + expect(r.body?.error?.httpStatus).toBe(400); + expect(r.body?.success).toBe(false); + expect(r.body?.data).toBeUndefined(); + }); + + it('[#17672] the sentence is the SHARED one, by derivation — not a local copy that matches', async () => { + const r = await get({ version: [ABSENT, INSTALLED] }); + // ⛔ Not a literal. This is the whole point of the card: one rule, + // one message. Computed from `@objectstack/rest`'s function, so the + // day that sentence changes, this door's answer changes with it — + // and a door that grew a second sentence of its own turns this red. + expect(r.body?.error?.message).toBe(repeatedQueryParamMessage('version', 2)); + // The count is the door's own reading, not a constant in the + // message: three occurrences say three. + const three = await get({ version: ['a', 'b', 'c'] }); + expect(three.body?.error?.message).toBe(repeatedQueryParamMessage('version', 3)); + expect(three.status).toBe(400); }); it('ONE occurrence encoded as a one-element array is one occurrence', async () => { @@ -202,6 +279,38 @@ describe('#17416 GET /packages/:id — ?version= scopes the read', () => { const r = await get({ version: [ABSENT] }); expect(r.status).toBe(404); expect(r.body?.error?.message).toContain(ABSENT); + // [#17672] Still a genuine not-found — the unwrapping rule means + // one occurrence is one occurrence, so this is NOT a shape error. + expect(r.body?.error?.code).toBe('RESOURCE_NOT_FOUND'); + }); + }); + + describe('§5 [#17672] the three refusals are mutually distinguishable', () => { + it('a client branching on status + code can tell shape-error from not-found', async () => { + // The card's acceptance criterion, stated as one reading. Before + // this fix all three were `404` / `RESOURCE_NOT_FOUND` — the whole + // defect, and the reason §1's two pins alone did not catch it. + const host = make(); + const repeated = await read(host, { version: [ABSENT, INSTALLED] }); + const wrongVersion = await read(host, { version: ABSENT }); + const unknownId = await (async () => { + const r = await host.handlePackages('/com.absent.pkg', 'GET', undefined, {}, reader()); + return { status: r.response?.status ?? 200, body: r.response?.body }; + })(); + + const seen = [repeated, wrongVersion, unknownId] + .map((r) => `${r.status} ${r.body?.error?.code}`); + expect(seen).toEqual([ + '400 VALIDATION_ERROR', + '404 RESOURCE_NOT_FOUND', + '404 RESOURCE_NOT_FOUND', + ]); + // The request-shape error is separated from BOTH not-founds, which + // is the distinction the card asked for. The two not-founds remain + // one class on purpose — they differ by message, and §1 pins that. + expect(seen[0]).not.toBe(seen[1]); + expect(seen[0]).not.toBe(seen[2]); + expect(wrongVersion.body?.error?.message).not.toBe(unknownId.body?.error?.message); }); }); }); diff --git a/packages/runtime/src/domains/packages.ts b/packages/runtime/src/domains/packages.ts index 55742fe2e8..d0390d2d73 100644 --- a/packages/runtime/src/domains/packages.ts +++ b/packages/runtime/src/domains/packages.ts @@ -71,6 +71,17 @@ import { organizationIdForMetaWrite } from '@objectstack/metadata-core'; // that already call it — the dataset query in `rest-server.ts`, the cold-boot // flow bind in `service-automation`, and `saveMetaItem`'s verbatim persist. import { stripReadDecorations } from '@objectstack/spec/kernel'; +// [#17672] The repo's ONE message for a single-valued query parameter supplied +// more than once, from the module whose header is the authority on the rule +// (`packages/rest/src/query-multiplicity.ts`). Imported, never restated: this +// door carried its own sentence for the identical condition, which is the drift +// that module's header forbids — and what stopped #17668 from importing it was +// reachability alone, which this card closes. +// ⛔ The gate beside it (`refuseRepeatedQueryParams`) is NOT what a dispatcher +// domain calls: it writes the bare `{ error: { code, message } }` body onto a +// `res`, and every error body on this surface is `deps.error`'s. See the +// `@objectstack/rest` barrel entry that publishes the pair. +import { repeatedQueryParamMessage } from '@objectstack/rest'; import { setPackageDisabled } from '../package-state-store.js'; import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js'; import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry.js'; @@ -566,16 +577,39 @@ function withWritableVerdict' not found`, whether or not a repeated `?version=` rode along + * — this card moved the status of a refusal, ⛔ not the order of two refusals. * * A one-element array is one occurrence encoded differently by an adapter and * is unwrapped, per that same rule's stated semantics. @@ -596,12 +630,6 @@ function readRequestedVersion(raw: unknown): RequestedVersion { return { kind: 'exact', value: raw }; } -/** The one sentence this door answers a repeated `?version=` with. */ -function repeatedVersionMessage(id: string, count: number): string { - return `Package '${id}' — the "version" query parameter was supplied ${count} times, ` - + 'so this read names no single version. Supply it at most once.'; -} - /** * The version the registry's row for a package IS (#17416). * @@ -1317,9 +1345,13 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin // here can be at the wrong version. const requested = readRequestedVersion(query?.version); if (requested.kind === 'repeated') { + // [#17672] A request-shape error, answered `400` in the shared + // rule's own words — ⛔ never a `404`, which says "not found" + // about a package that is right here. `deps.error` derives + // `VALIDATION_ERROR` from the status; see `readRequestedVersion`. return { handled: true, - response: deps.error(repeatedVersionMessage(id, requested.count), 404), + response: deps.error(repeatedQueryParamMessage('version', requested.count), 400), }; } if (requested.kind === 'exact') {