fix(runtime): a repeated ?version= on GET /packages/:id answers 400 VALIDATION_ERROR from the one shared rule, and @objectstack/rest publishes it (#17672)

.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=<installed>` 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.

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';

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.
  */
 
 /**

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);
         });
     });
 });