From 410700aa4f1db35bddb6a951f89cc51b38f44544 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 19:47:38 +0000 Subject: [PATCH] =?UTF-8?q?fix(runtime):=20callData=20=E7=9A=84=20ObjectQL?= =?UTF-8?q?=20=E5=85=9C=E5=BA=95=E5=AF=B9=E3=80=8C=E8=AE=B0=E5=BD=95?= =?UTF-8?q?=E4=B8=8D=E5=AD=98=E5=9C=A8=E3=80=8D=E7=BB=9F=E4=B8=80=E7=AD=94?= =?UTF-8?q?=20404=20RECORD=5FNOT=5FFOUND=20(#5138)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `callData` 是「protocol 优先,ObjectQL 兜底」,而兜底分支对同一个事实 (id 指向的记录不存在)给了三种互不一致的答案:get 回 null(/data 包成 200 {data:null})、update 抛不带 .status 的裸 Error(两个 dispatcher 出口 都兜底 500)、delete 无存在性检查直接删并回 200 {deleted:true}。 protocol 路径自 #4435 起三个动词已经都答 404 RECORD_NOT_FOUND(先测后判, 实测确认,故不改动它),所以同一个请求的答案取决于调用方看不见的东西: 部署有没有注册 protocol 槽。三个兜底分支现在抛同一个信封。 信封不重新拼写:`recordNotFoundError` 从 @objectstack/metadata-protocol 导出、由兜底导入,一个构造点,两条路径无法再漂移。 delete 的存在性检查用 find 探测而非读 ql.delete 的返回值:IDataDriver.delete 声明 Promise 所以 protocol 能读它,但 IDataEngine.delete 声明 Promise,引擎把驱动结果穿过 hook 链返回 opCtx.result —— 对它测 `=== false` 是读一个契约没有承诺的信号,且失败方向正是本单要修的方向。 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_016FNvXhtSdnEGEfLEsMmvxh --- .changeset/calldata-record-not-found-unify.md | 51 +++ packages/metadata-protocol/src/index.ts | 4 + packages/metadata-protocol/src/protocol.ts | 13 +- ...ction-execution-calldata-not-found.test.ts | 362 ++++++++++++++++++ packages/runtime/src/action-execution.ts | 51 ++- 5 files changed, 478 insertions(+), 3 deletions(-) create mode 100644 .changeset/calldata-record-not-found-unify.md create mode 100644 packages/runtime/src/action-execution-calldata-not-found.test.ts diff --git a/.changeset/calldata-record-not-found-unify.md b/.changeset/calldata-record-not-found-unify.md new file mode 100644 index 0000000000..c57a877664 --- /dev/null +++ b/.changeset/calldata-record-not-found-unify.md @@ -0,0 +1,51 @@ +--- +"@objectstack/runtime": patch +"@objectstack/metadata-protocol": patch +--- + +fix(runtime): `callData`'s ObjectQL fallback answers a missing record id with 404 `RECORD_NOT_FOUND` (#5138) + +`callData` (the data bridge behind `/data`, the MCP bridge and the declarative +endpoint executor) is protocol-first with an ObjectQL fallback. The fallback +gave **three different answers to one fact** — that `id` names no row: + +| verb | before | on the wire | +|---|---|---| +| `get` | `return … : null` | `200 { data: null }` | +| `update` | `throw new Error('[ObjectStack] Not Found')` — no `.status` | **500** | +| `delete` | no existence check at all | `200 { deleted: true }` | + +The protocol path has answered `404 RECORD_NOT_FOUND` on all three verbs since +#4435 (re-asserted for the batch path by #5088), so the answer to the same +request depended on something no caller can see: whether the deployment +registered the `protocol` slot (`MetadataPlugin` / `@objectstack/metadata-protocol`). +All three fallback branches now throw the SAME envelope the protocol throws. + +Two of these were actively harmful. `update` reported a caller mistake as an +internal fault — every dispatcher exit reads `.status` → `.statusCode` → 500, so +a 4xx fact entered error reporting and alerting as a 5xx. `delete` reported +success for a row that never existed, which is the hardest class to notice: an +integrator reading `200` records the cleanup as done. + +The envelope is not re-spelled. `recordNotFoundError` is now exported from +`@objectstack/metadata-protocol` and imported by the fallback, so there is one +construction point and the two paths behind one `callData` cannot drift apart +again. + +**Upgrade note.** If you run an assembly WITHOUT the metadata-protocol plugin +(lean hosts, and the MCP multi-env path that threads a raw driver), these three +calls change their answer for a missing id — from `200`/`200`/`500` to `404 +{ code: 'RECORD_NOT_FOUND', message: 'Record not found in ' }`. +Deployments that DO register the protocol slot are unaffected: they already +answered `404` and this release does not touch that path. A client that +branched on `data === null` from `GET /data/:object/:id` should branch on the +`404` instead; a client that treated `DELETE` as idempotent should treat `404` +as "already gone". Declarative endpoints (`object_operation`) inherit the same +answer, since they reuse `/data`'s delegation. + +`delete`'s existence check is a `find` probe, not a read of what `ql.delete` +returned: `IDataDriver.delete` declares `Promise< boolean >` and the protocol +can read it, but `IDataEngine.delete` declares `Promise< any >` and the engine +returns its driver's result through the hook chain — testing that for `false` +would be reading a signal the contract does not promise, and it fails in the +direction this fixes. diff --git a/packages/metadata-protocol/src/index.ts b/packages/metadata-protocol/src/index.ts index 4f4c0d1546..6f354ce8eb 100644 --- a/packages/metadata-protocol/src/index.ts +++ b/packages/metadata-protocol/src/index.ts @@ -1,6 +1,10 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. export { ObjectStackProtocolImplementation, ConcurrentUpdateError, normalizeViewMetadata, graftNormalizedOperators, stripReadDecorations } from './protocol.js'; +// [#5138] The 404 envelope every single-record path answers, exported so the +// ObjectQL FALLBACK in `@objectstack/runtime`'s `callData` builds the SAME one +// instead of minting a second not-found shape. See `recordNotFoundError`. +export { recordNotFoundError } from './protocol.js'; export { createMetadataProtocolPlugin, assembleMetadataProtocol } from './plugin.js'; export type { MetadataProtocolPluginOptions } from './plugin.js'; export type { UninstallCleanup, UninstallCleanupOutcome } from './protocol.js'; diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index f79fe0fc8c..e77793a61f 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -340,8 +340,19 @@ function resolveOverlaySchema(type: string, _item: unknown): z.ZodTypeAny | null * params, #4190 stopped dropping filters) — a write that touched zero rows * reporting 200 is that shape one level up, on the verb where it costs the * most. + * + * [#5138] EXPORTED, for the same "cannot disagree about it" reason one layer + * out. `@objectstack/runtime`'s `callData` is protocol-first with an ObjectQL + * FALLBACK, and the fallback had reinvented this fact three incompatible ways + * (`get` → `null`, `update` → a bare `Error` with no status ⇒ 500, `delete` → + * no check at all ⇒ `200 { deleted: true }` for a row that never existed). It + * now calls THIS function, so the two paths behind one `callData` answer a + * missing id identically — which is the only reason a caller may stop caring + * which of them served it. Re-spelling the envelope there would have been a + * second not-found envelope; `RECORD_NOT_FOUND` (#5088) is the one this repo + * has. */ -function recordNotFoundError(object: string, id: string | number): Error { +export function recordNotFoundError(object: string, id: string | number): Error { const err = new Error(`Record ${id} not found in ${object}`) as Error & { code?: string; status?: number; diff --git a/packages/runtime/src/action-execution-calldata-not-found.test.ts b/packages/runtime/src/action-execution-calldata-not-found.test.ts new file mode 100644 index 0000000000..32e126e3af --- /dev/null +++ b/packages/runtime/src/action-execution-calldata-not-found.test.ts @@ -0,0 +1,362 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #5138 — `callData` answers "this id names no row" with ONE fact. + * + * `callData` is protocol-first with an ObjectQL fallback, and the fallback gave + * three different answers to the single fact that `params.id` matches nothing. + * Measured on `main` @ `c11369013` before the fix, same harnesses as below: + * + * ``` + * FALLBACK get : resolved null → /data 200 {data:null} + * FALLBACK update: rejected Error('[ObjectStack] Not Found') → no .status ⇒ 500 + * (code: undefined, status: undefined) + * FALLBACK delete: resolved { object, id, deleted: true } → 200, row never existed + * PROTOCOL get : rejected RECORD_NOT_FOUND / 404 + * PROTOCOL update: rejected RECORD_NOT_FOUND / 404 + * PROTOCOL delete: rejected RECORD_NOT_FOUND / 404 + * ``` + * + * So the protocol path was ALREADY the canonical answer on all three verbs + * (#4435, re-asserted for the batch path by #5088) and needed no change — only + * pinning. The whole defect was the fallback, where the same GET answered 200 + * or 404 depending on nothing the caller can see: whether the deployment + * registered the `protocol` slot. + * + * The fix imports `recordNotFoundError` from `@objectstack/metadata-protocol` + * rather than re-spelling it, so there is one construction point for the + * envelope and the two paths behind one `callData` cannot drift apart again. + * + * The suite is organised around that: the fallback's three verbs (was-red + * cases), the protocol's three (pins), and a field-for-field identity assertion + * across the two — which is the claim a caller actually depends on. Then the + * two consuming faces the issue names: `/data` through the REAL `HttpDispatcher`, + * and the declarative endpoint executor (#5092 / PR #5136) driven with the REAL + * `callData` bound, which is where the status a client receives is decided. + */ + +import { describe, it, expect } from 'vitest'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { ApiEndpointSchema } from '@objectstack/spec/api'; +import type { ApiEndpoint } from '@objectstack/spec/api'; + +import { callData, type ActionExecutionDeps } from './action-execution.js'; +import { HttpDispatcher, type HttpProtocolContext } from './http-dispatcher.js'; +import { buildEndpointExecutionContext, executeEndpointTarget } from './endpoint-executor.js'; + +const EC = { userId: 'u1', isSystem: false, positions: [], permissions: [] } as any; +/** [#5155] Every service lookup resolves off the REQUEST's kernel. */ +const REQ = { request: {} } as HttpProtocolContext; +const SCHEMA = { name: 'task', fields: { title: { name: 'title', type: 'text' } } }; +const GHOST = 'definitely_not_a_row'; + +// --------------------------------------------------------------------------- +// Harnesses — the SAME row set behind both paths, so "exists" means one thing +// --------------------------------------------------------------------------- + +function rows() { + return new Map([['r1', { id: 'r1', title: 'one' }]]); +} + +/** + * ObjectQL-only: no `protocol` slot, so every verb takes the fallback. This is + * the combination the issue names — a deployment WITHOUT `MetadataPlugin` + * (`@objectstack/metadata-protocol`, which is what registers the slot). With it + * installed the fallback is unreachable, which is why nothing caught this. + */ +function fallbackHarness(store = rows()) { + const deleted: string[] = []; + const ql: any = { + // `registry` is what `HttpDispatcher.getObjectQLService` requires before + // it will hand the service to `callData` — not decoration. + registry: { getObject: (n: string) => (n === 'task' ? SCHEMA : undefined) }, + find: async (_o: string, bag: any) => { + const hit = store.get(String(bag?.where?.id)); + return hit ? [hit] : []; + }, + update: async (_o: string, data: any, opts: any) => { + const id = String(opts?.where?.id); + if (!store.has(id)) return null; + store.set(id, { ...store.get(id), ...data }); + return store.get(id); + }, + delete: async (_o: string, opts: any) => { + const id = String(opts?.where?.id); + deleted.push(id); + return store.delete(id); + }, + }; + const services: Record = { + metadata: { getObject: async () => ({ name: 'task', fields: {} }) }, + objectql: ql, + }; + const deps: ActionExecutionDeps = { + resolveService: (async (_c: HttpProtocolContext, name: string) => services[name]) as any, + getObjectQL: async () => ql, + }; + return { deps, store, deleted, ql, services }; +} + +/** Protocol-first, with the REAL `@objectstack/metadata-protocol` occupant. */ +function protocolHarness(store = rows()) { + const engine: any = { + registry: { getObject: (n: string) => (n === 'task' ? SCHEMA : undefined) }, + findOne: async (_o: string, opts: any) => store.get(String(opts?.where?.id)) ?? null, + find: async () => [...store.values()], + update: async (_o: string, data: any, opts: any) => { + const id = String(opts?.where?.id); + if (!store.has(id)) return null; + store.set(id, { ...store.get(id), ...data }); + return store.get(id); + }, + delete: async (_o: string, opts: any) => store.delete(String(opts?.where?.id)), + }; + const services: Record = { + metadata: { getObject: async () => ({ name: 'task', fields: {} }) }, + protocol: new ObjectStackProtocolImplementation(engine), + objectql: engine, + }; + const deps: ActionExecutionDeps = { + resolveService: (async (_c: HttpProtocolContext, name: string) => services[name]) as any, + getObjectQL: async () => engine, + }; + return { deps, store, services }; +} + +const verb = { + get: (id: string) => ['get', { object: 'task', id }] as const, + update: (id: string) => ['update', { object: 'task', id, data: { title: 'x' } }] as const, + delete: (id: string) => ['delete', { object: 'task', id }] as const, +}; +type Verb = keyof typeof verb; +const VERBS: Verb[] = ['get', 'update', 'delete']; + +const run = (deps: ActionExecutionDeps, v: Verb, id: string) => { + const [action, params] = verb[v](id); + return callData(deps, REQ, action, params, undefined, undefined, EC); +}; + +/** Capture the rejection as plain data so the two paths can be compared. */ +async function rejection(p: Promise) { + let caught: any; + let resolved: unknown; + let didResolve = false; + try { + resolved = await p; + didResolve = true; + } catch (e) { + caught = e; + } + expect( + didResolve, + `expected a RECORD_NOT_FOUND rejection, but the call RESOLVED with ${JSON.stringify(resolved)}`, + ).toBe(false); + return { code: caught?.code, status: caught?.status, message: caught?.message, object: caught?.object }; +} + +/** The #5088/#4435 envelope, spelled once. */ +const notFoundEnvelope = (id: string) => ({ + code: 'RECORD_NOT_FOUND', + status: 404, + message: `Record ${id} not found in task`, + object: 'task', +}); + +// --------------------------------------------------------------------------- +// The fallback — the three answers that were three different facts +// --------------------------------------------------------------------------- + +describe('the ObjectQL fallback answers a missing id with RECORD_NOT_FOUND (#5138)', () => { + it.each(VERBS)('%s rejects with the #5088 envelope — code, status, message, object', async (v) => { + const h = fallbackHarness(); + expect(await rejection(run(h.deps, v, GHOST))).toEqual(notFoundEnvelope(GHOST)); + }, 60_000); + + it('the 404 carries a `status`, not a `statusCode` — the property every exit reads FIRST', async () => { + // `update` used to throw a bare `Error`, so all three dispatcher exits + // (`HttpDispatcher.errorFromThrown`, `dispatcher-plugin`'s + // `errorResponseBase`, `endpoint-executor`'s `errorAnswer`) fell to + // their 500 default and a caller mistake entered error reporting as an + // internal fault. They read `.status` → `.statusCode` → 500. + const h = fallbackHarness(); + let caught: any; + try { await run(h.deps, 'update', GHOST); } catch (e) { caught = e; } + expect(caught.status).toBe(404); + expect(caught).toBeInstanceOf(Error); + // Nothing here should look like the old bare throw. + expect(caught.message).not.toBe('[ObjectStack] Not Found'); + }, 60_000); + + it('delete refuses BEFORE touching the store — no delete is issued for a row that is not there', async () => { + // The old branch called `ql.delete` unconditionally and answered + // `{ deleted: true }`. A 404 that still issued the write would be the + // same lie with a different status code. + const h = fallbackHarness(); + await rejection(run(h.deps, 'delete', GHOST)); + expect(h.deleted).toEqual([]); + }, 60_000); + + it.each(VERBS)('%s is unchanged for an id that DOES exist', async (v) => { + const h = fallbackHarness(); + await expect(run(h.deps, v, 'r1')).resolves.toBeTruthy(); + }, 60_000); + + it('a real delete still deletes, and still answers `deleted: true`', async () => { + const h = fallbackHarness(); + const out: any = await run(h.deps, 'delete', 'r1'); + expect(out).toEqual({ object: 'task', id: 'r1', deleted: true }); + expect(h.deleted).toEqual(['r1']); + expect(h.store.has('r1')).toBe(false); + }, 60_000); + + it('a real update still returns the merged record', async () => { + const h = fallbackHarness(); + const out: any = await run(h.deps, 'update', 'r1'); + expect(out).toEqual({ object: 'task', id: 'r1', record: { id: 'r1', title: 'x' } }); + }, 60_000); +}); + +// --------------------------------------------------------------------------- +// The protocol path — already canonical, pinned so it stays that way +// --------------------------------------------------------------------------- + +describe('the protocol path answers the same missing id the same way (#4435/#5088)', () => { + it.each(VERBS)('%s rejects with the #5088 envelope', async (v) => { + const h = protocolHarness(); + expect(await rejection(run(h.deps, v, GHOST))).toEqual(notFoundEnvelope(GHOST)); + }, 60_000); +}); + +// --------------------------------------------------------------------------- +// The claim a caller depends on +// --------------------------------------------------------------------------- + +describe('one `callData`, one answer — the two paths are field-for-field identical', () => { + it.each(VERBS)('%s: fallback envelope === protocol envelope', async (v) => { + const withoutProtocol = await rejection(run(fallbackHarness().deps, v, GHOST)); + const withProtocol = await rejection(run(protocolHarness().deps, v, GHOST)); + expect(withoutProtocol).toEqual(withProtocol); + }, 60_000); + + it('and the envelope is built ONCE — the fallback imports the protocol’s factory', async () => { + // If either side ever re-spells the shape, the three assertions above + // can be kept green by editing a literal in this file. This one cannot: + // it compares the fallback's throw against the exported factory itself. + const { recordNotFoundError } = await import('@objectstack/metadata-protocol'); + const reference = recordNotFoundError('task', GHOST) as any; + const actual = await rejection(run(fallbackHarness().deps, 'get', GHOST)); + expect(actual).toEqual({ + code: reference.code, + status: reference.status, + message: reference.message, + object: reference.object, + }); + }, 60_000); +}); + +// --------------------------------------------------------------------------- +// Consuming face 1 — `/data` through the REAL dispatcher +// --------------------------------------------------------------------------- + +/** + * A kernel with an `objectql` occupant and NO `protocol` one — the combination + * that reaches the fallback. `auth` answers a session so the request gets past + * the anonymous-deny gate and reaches the branch under test. + */ +function dispatcherOverFallback() { + const h = fallbackHarness(); + const resolve = (name: string) => + name === 'objectql' ? h.ql + : name === 'metadata' ? h.services.metadata + : name === 'auth' ? { api: { getSession: async () => ({ user: { id: 'u1' } }) } } + : undefined; + const kernel: any = { getService: resolve, getServiceAsync: async (n: string) => resolve(n) }; + return { dispatcher: new HttpDispatcher(kernel), deleted: h.deleted }; +} + +describe('/data through the real HttpDispatcher inherits the one answer (#5138)', () => { + const cases: Array<[string, string, any]> = [ + ['GET', `/data/task/${GHOST}`, undefined], + ['PATCH', `/data/task/${GHOST}`, { title: 'x' }], + ['DELETE', `/data/task/${GHOST}`, undefined], + ]; + + it.each(cases)('%s %s rejects with 404 RECORD_NOT_FOUND', async (method, path, body) => { + const { dispatcher } = dispatcherOverFallback(); + // `dispatch()` re-throws everything but a permission denial; the two + // mounts that serve this domain (`dispatcher-plugin`'s catch → + // `errorResponseBase`, and `HttpDispatcher.errorFromThrown`) both read + // `.status` first, which is what turns this into a 404 on the wire — + // pinned for those exits by `domains/error-passthrough.test.ts`. + await expect( + dispatcher.dispatch(method, path, body, {}, { request: {} } as HttpProtocolContext), + ).rejects.toMatchObject({ status: 404, code: 'RECORD_NOT_FOUND' }); + }, 60_000); + + it('DELETE no longer answers a 200 success for a row that never existed', async () => { + // The was-red case in its most consequential form: this used to RESOLVE + // `{ handled: true, response: { status: 200, ... deleted: true } }`. + const { dispatcher, deleted } = dispatcherOverFallback(); + const result = dispatcher.dispatch('DELETE', `/data/task/${GHOST}`, undefined, {}, { request: {} } as HttpProtocolContext); + await expect(result).rejects.toBeTruthy(); + expect(deleted).toEqual([]); + }, 60_000); + + it('GET on a row that exists still answers 200 with the record', async () => { + const { dispatcher } = dispatcherOverFallback(); + const res: any = await dispatcher.dispatch('GET', '/data/task/r1', undefined, {}, { request: {} } as HttpProtocolContext); + expect(res.handled).toBe(true); + expect(res.response.status).toBe(200); + }, 60_000); +}); + +// --------------------------------------------------------------------------- +// Consuming face 2 — the declarative endpoint executor (#5092 / PR #5136) +// --------------------------------------------------------------------------- + +/** + * The executor reuses `/data`'s delegation byte for byte, so it inherits this + * unification for free — and unlike `dispatch()` it BUILDS the HTTP answer, so + * this is where the status a client receives is asserted directly. + */ +function declaredEndpoint(operation: 'get' | 'update' | 'delete'): ApiEndpoint { + return ApiEndpointSchema.parse({ + name: 'task_by_id', + path: '/api/v1/apps/showcase/task', + method: operation === 'get' ? 'GET' : operation === 'update' ? 'PATCH' : 'DELETE', + type: 'object_operation', + target: 'task', + objectParams: { object: 'task', operation }, + }); +} + +describe('a declared endpoint answers 404 RECORD_NOT_FOUND on the wire (#5092/#5136)', () => { + it.each(['get', 'update', 'delete'] as const)('%s → status 404, code RECORD_NOT_FOUND', async (operation) => { + const h = fallbackHarness(); + const ctx = buildEndpointExecutionContext({ + request: { + method: 'GET', + path: '/api/v1/apps/showcase/task', + query: { id: GHOST }, + headers: {}, + ...(operation === 'update' ? { body: { title: 'x' } } : {}), + }, + match: { endpoint: declaredEndpoint(operation), params: {} }, + executionContext: EC, + }); + const answer = await executeEndpointTarget(ctx, { + // The REAL `callData`, bound the way `dispatcher-plugin` binds it. + callData: (action, params, driver, scope, ec) => + callData(h.deps, REQ, action, params, driver, scope, ec), + }); + + expect(answer.status).toBe(404); + const error = (answer.body as any).error; + expect(error.code).toBe('RECORD_NOT_FOUND'); + expect(error.httpStatus).toBe(404); + // A 4xx message is a deliberate business answer and reaches the caller + // intact — the 5xx leak sanitiser must not have touched it. + expect(error.message).toBe(`Record ${GHOST} not found in task`); + expect(h.deleted).toEqual([]); + }, 60_000); +}); diff --git a/packages/runtime/src/action-execution.ts b/packages/runtime/src/action-execution.ts index 4e23305a20..081d1d0e6d 100644 --- a/packages/runtime/src/action-execution.ts +++ b/packages/runtime/src/action-execution.ts @@ -19,6 +19,13 @@ import { validateActionParams, type ResolvedActionParam } from '@objectstack/spe import type { ExecutionContext } from '@objectstack/spec/kernel'; import type { IObjectQLEngine, ServiceSlotContract, ServiceSlotContracts } from '@objectstack/spec/contracts'; import { checkApiExposure } from './api-exposure.js'; +// [#5138] The ONE 404 envelope a single-record path answers. Imported rather +// than re-spelled so `callData`'s ObjectQL fallback and the protocol service it +// falls back FROM cannot disagree about what "this id names no row" looks like. +// A pure factory — no service resolution — so importing it costs the fallback +// nothing on an assembly where the protocol plugin is absent, which is exactly +// when the fallback runs. +import { recordNotFoundError } from '@objectstack/metadata-protocol'; import { actorUserFromExecutionContext, resolveActorDisplayName } from './security/actor-user.js'; import type { HttpProtocolContext } from './http-dispatcher.js'; import { @@ -165,7 +172,14 @@ export async function callData(deps: ActionExecutionDeps, if (all && (all as any).value) all = (all as any).value; if (!all) all = []; const match = (all as any[]).find((i: any) => i.id === params.id); - return match ? { object: params.object, id: params.id, record: match } : null; + // [#5138] Was `: null` — a miss resolved, and `/data` wrapped it as + // `200 { data: null }`. The protocol path this falls back from has + // answered `404 RECORD_NOT_FOUND` since #4435, so the same GET + // answered 200 or 404 depending only on whether the deployment + // registered the protocol slot — a difference the caller cannot see + // and never asked for. + if (!match) throw recordNotFoundError(params.object, params.id); + return { object: params.object, id: params.id, record: match }; } throw { statusCode: 503, message: 'Data service not available' }; } @@ -179,7 +193,15 @@ export async function callData(deps: ActionExecutionDeps, if (all && (all as any).value) all = (all as any).value; if (!all) all = []; const existing = (all as any[]).find((i: any) => i.id === params.id); - if (!existing) throw new Error('[ObjectStack] Not Found'); + // [#5138] Was `throw new Error('[ObjectStack] Not Found')`. That + // error carried neither `.status` nor `.statusCode`, so BOTH + // dispatcher exits fell through to their 500 fallback + // (`HttpDispatcher.errorFromThrown`, `dispatcher-plugin`'s + // `errorResponseBase`, and the endpoint executor's `errorAnswer` + // all read `.status` → `.statusCode` → 500). A caller mistake was + // reported as an internal fault and taken to the error reporter + // with it. + if (!existing) throw recordNotFoundError(params.object, params.id); await ql.update(params.object, params.data, findOpts({ where: { id: params.id } })); return { object: params.object, id: params.id, record: { ...existing, ...params.data } }; } @@ -191,6 +213,31 @@ export async function callData(deps: ActionExecutionDeps, return await protocol.deleteData({ object: params.object, id: params.id, ...(scopeId ? { environmentId: scopeId } : {}), context: executionContext }); } if (ql && typeof ql.delete === 'function') { + // [#5138] There was NO existence check here: the delete ran and the + // answer was `200 { deleted: true }` for any string in the path, so + // a typo'd id, an already-deleted row and a real deletion were + // indistinguishable — the exact shape #4435 removed from the + // protocol's `deleteData`, still live on the path that stands in + // for it. The "assume it worked" answer is the worst of the three + // this fallback gave, because an integrator reading 200 records the + // cleanup as done. + // + // The existence PROBE is a `find`, not a read of what `ql.delete` + // returned. `deleteData` can read its result because `IDataDriver. + // delete` declares `Promise` ("true if deleted, false if + // not found"); `ql` here is the ObjectQL ENGINE (or, on the MCP + // multi-env path, a raw driver), and `IDataEngine.delete` declares + // `Promise` — the engine passes its driver's result through + // the hook chain and returns `opCtx.result`. Testing that for + // `=== false` would be reading a signal the contract does not + // promise, which fails silently in the direction this issue is + // about: back to reporting a delete that removed nothing. The probe + // is the same one the sibling `get`/`update` fallbacks already run. + let all = await ql.find(params.object, findOpts({ where: { id: params.id }, limit: 1 })); + if (all && (all as any).value) all = (all as any).value; + if (!all) all = []; + const existing = (all as any[]).find((i: any) => i.id === params.id); + if (!existing) throw recordNotFoundError(params.object, params.id); await ql.delete(params.object, findOpts({ where: { id: params.id } })); return { object: params.object, id: params.id, deleted: true }; }