Skip to content

Commit 9f5cc79

Browse files
os-zhuangclaude
andauthored
fix(metadata-protocol): a batch row's httpStatus reads the declared status, not one spelling of it (#8570) (#8633)
* fix(metadata-protocol): a batch row's httpStatus reads the declared status, not one spelling of it (#8570) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH * chore: changeset for the batch row's declared httpStatus (#8570) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH * fix(test): pass the required packageId in the #8570 approvals batch-row pin `registry.registerObject` takes (schema, packageId, …). The new integration test omitted the second argument, which the package's own `typecheck` script cannot see — its tsconfig excludes `**/*.test.ts` — while the TEST_DEBT ratchet measures tsc WITH the test layer in the program and counted the +1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 018d22c commit 9f5cc79

11 files changed

Lines changed: 1016 additions & 7 deletions
Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
---
2+
"@objectstack/metadata-protocol": patch
3+
"@objectstack/types": minor
4+
---
5+
6+
A bulk write's per-row `errors[].httpStatus` carries the status its producer declared, in any spelling (#8570)
7+
8+
`toRowApiError` set the limb from `err.status` alone, so two well-defined client
9+
refusals shipped a batch row with no status at all: objectql's `ValidationError`
10+
(a 400 recognisable by shape, which deliberately declares no `status`) and
11+
`plugin-approvals`' record lock (a 409 spelled `statusCode`). Sibling rows in the
12+
same response did carry one — `rowRequiredIdError` → 400,
13+
`recordNotFoundError` → 404 — so a caller branching on `httpStatus` to tell "fix
14+
your input" from "the server broke" got an answer for some failure rows and
15+
silence for others, with nothing saying which. Same single-spelling defect #7525
16+
fixed at the HTTP door, one layer down.
17+
18+
The limb now asks `resolveThrownHttpError` — the resolver the HTTP doors and the
19+
row's `message` limb already answer with — so a refusal declaring `.status`,
20+
`.statusCode` or the `VALIDATION_FAILED` shape reaches the row as the status it
21+
always meant. Rows whose throw declared nothing (a driver fault, a hook throwing
22+
a bare `Error`) still carry no `httpStatus`: the resolver's 500 there is the
23+
caller's fallback, not a producer's claim, and stamping it would add a field to
24+
the wire for those populations rather than restore a declared one. `code` reads
25+
the same resolution, so a row can no longer contradict itself.
26+
27+
`ThrownHttpError` gains `declaredStatus` — the resolved status minus the
28+
fallback, absent when the throw declared none. `status` is unchanged, and every
29+
boundary that answers with the status itself keeps reading it; the new field is
30+
for sinks that mirror a status onto response DATA, where a fallback would be an
31+
invention.

‎packages/metadata-protocol/src/protocol.batch-row-driver-text.test.ts‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -307,9 +307,13 @@ describe('[#8502] section 2 — the authored population survives, in all THREE d
307307

308308
expect(res.results[0].errors[0].message).toBe('title must be ≤ 4 characters (got 15)');
309309
expect(res.results[0].errors[0].code).toBe('VALIDATION_FAILED');
310-
// NO `httpStatus`: the producer declared none, and #8502 does not mint
311-
// one — that would be an ADDITION to the wire, a separate decision.
312-
expect(res.results[0].errors[0].httpStatus).toBeUndefined();
310+
// `httpStatus: 400` since #8570 — the separate decision this line used
311+
// to defer ("the producer declared no `.status`, and minting one is an
312+
// ADDITION to the wire") was taken there: the limb now reads the same
313+
// resolution this one does, so the validation SHAPE declares its 400.
314+
// The undeclared populations still gain nothing; that half is pinned in
315+
// `protocol.batch-row-http-status.test.ts` §3.
316+
expect(res.results[0].errors[0].httpStatus).toBe(400);
313317
});
314318

315319
it('a 4xx `statusCode` is quoted — THIS sink’s own population, met by neither sibling', async () => {
@@ -324,6 +328,9 @@ describe('[#8502] section 2 — the authored population survives, in all THREE d
324328
"RECORD_LOCKED: record 'r1' of 'leave_request' is locked while an approval is in progress",
325329
);
326330
expect(res.results[0].errors[0].code).toBe('RECORD_LOCKED');
331+
// The `statusCode` spelling reaches `httpStatus` too since #8570 — this
332+
// row is the card's second measured one.
333+
expect(res.results[0].errors[0].httpStatus).toBe(409);
327334
});
328335

329336
it('an UNDECLARED hook refusal is withheld — the measured cost of a positive list', async () => {

‎packages/metadata-protocol/src/protocol.batch-row-http-status.test.ts‎

Lines changed: 453 additions & 0 deletions
Large diffs are not rendered by default.

‎packages/metadata-protocol/src/protocol.ts‎

Lines changed: 54 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1491,16 +1491,67 @@ type BatchDataRowResult = BatchOperationResult;
14911491
* ⛔ `fallback` is REQUIRED, not defaulted. All three catches that build a row
14921492
* know their operation, and a default would let a fourth one be added that
14931493
* silently reports the wrong verb.
1494+
*
1495+
* ## `httpStatus` reads the DECLARATION, not one spelling of it (#8570)
1496+
*
1497+
* This limb read `err.status` and nothing else, so of the producers measured at
1498+
* these catches — a real `ObjectQL` over a real `SqlDriver`, driven through all
1499+
* three bulk-write loops — two well-defined client refusals shipped a row with
1500+
* no status at all:
1501+
*
1502+
* | producer | code | `.status` | `.statusCode` | validation shape | was | is |
1503+
* |---|---|---|---|---|---|---|
1504+
* | {@link rowRequiredIdError} | VALIDATION_FAILED | **400** | — | no | 400 | 400 |
1505+
* | `recordNotFoundError` (`@objectstack/core`) | RECORD_NOT_FOUND | **404** | — | no | 404 | 404 |
1506+
* | objectql `ValidationError` | VALIDATION_FAILED | — | — | **yes** | — | **400** |
1507+
* | plugin-approvals' record lock | RECORD_LOCKED | — | **409** | no | — | **409** |
1508+
* | an app hook throwing a bare `Error` | — | — | — | no | — | — |
1509+
* | driver fault (`SqliteError`, …) | SQLITE_* | — | — | no | — | — |
1510+
*
1511+
* The first two are siblings of the last four *in the same response*: a caller
1512+
* branching on `httpStatus` to tell "fix your input" from "the server broke"
1513+
* got an answer for some failure rows and nothing for others, with no signal
1514+
* saying which. The single-spelling defect is #7525's, fixed there at the HTTP
1515+
* door; this is the same defect on the row.
1516+
*
1517+
* The question is answered by {@link resolveThrownHttpError}, IMPORTED — the
1518+
* `message` limb beside it already delegates there (#8502), and a second local
1519+
* chain would be the third derivation of "what status is this throw" in one
1520+
* function. ⛔ Do not re-spell it as `status ?? statusCode ?? validation`.
1521+
*
1522+
* ## ⛔ `declaredStatus`, never `status` — the over-broad direction is real
1523+
*
1524+
* That resolver answers for EVERY throw: its `status` is 500 for a bare hook
1525+
* `Error` and for a `SqliteError`, because 500 is the caller's fallback.
1526+
* Stamping that would put `httpStatus: 500` on the last two rows of the table,
1527+
* which never carried one — an ADDITION to the wire for those populations, and
1528+
* a claim the producer never made. `declaredStatus` is the same resolution
1529+
* minus the fallback: present exactly when the throw declared a status in one
1530+
* of the three spellings, absent otherwise. So a declared refusal gains the
1531+
* status it always meant, and an undeclared fault keeps carrying none.
1532+
*
1533+
* The gate is DECLARED-ness and deliberately not the 4xx band that
1534+
* {@link clientFacingRowFailureText} uses. That limb decides disclosure of free
1535+
* text, where a 5xx must be withheld; this one decides a number the producer
1536+
* itself authored, and a row already ships `httpStatus: 503` today when the
1537+
* refusal spells `.status` — narrowing to 4xx would WITHDRAW a status the wire
1538+
* carries, which is a different decision from this one.
1539+
*
1540+
* Both limbs read the same resolution for a second reason: they must agree.
1541+
* Deriving `code` from `err.status` while `httpStatus` came from
1542+
* `declaredStatus` would mint incoherent rows — `{ code: 'INTERNAL_ERROR',
1543+
* httpStatus: 409 }` for a `statusCode`-spelled refusal whose own code the
1544+
* ledger does not know.
14941545
*/
14951546
function toRowApiError(err: any, fallback: string): ApiError {
14961547
const thrown = typeof err?.code === 'string' && ErrorCode.safeParse(err.code).success
14971548
? (err.code as ApiError['code'])
14981549
: undefined;
1499-
const status = typeof err?.status === 'number' ? err.status : undefined;
1550+
const { declaredStatus } = resolveThrownHttpError(err);
15001551
return {
1501-
code: thrown ?? (status !== undefined ? standardErrorCodeForHttpStatus(status) : 'INTERNAL_ERROR'),
1552+
code: thrown ?? (declaredStatus !== undefined ? standardErrorCodeForHttpStatus(declaredStatus) : 'INTERNAL_ERROR'),
15021553
message: clientFacingRowFailureText(err, fallback),
1503-
...(status !== undefined ? { httpStatus: status } : {}),
1554+
...(declaredStatus !== undefined ? { httpStatus: declaredStatus } : {}),
15041555
};
15051556
}
15061557

‎packages/plugins/plugin-approvals/package.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
},
2828
"devDependencies": {
2929
"@objectstack/driver-sql": "workspace:*",
30+
"@objectstack/metadata-protocol": "workspace:*",
3031
"@objectstack/objectql": "workspace:*",
3132
"@objectstack/service-automation": "workspace:*",
3233
"@objectstack/trigger-record-change": "workspace:*",
Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,179 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#8570] The record lock's refusal, as a BATCH ROW — driven through the REAL
5+
* hook, a real {@link ObjectQL} and a real sqlite driver.
6+
*
7+
* ## The row this file exists for
8+
*
9+
* Measured on the real stack while #8502 was being verified, a bulk update of a
10+
* record this plugin holds locked answered:
11+
*
12+
* ```json
13+
* { "code": "RECORD_LOCKED", "message": "RECORD_LOCKED: record 'ok1' of 'm8502_task' is locked while an approval is in progress" }
14+
* ```
15+
*
16+
* — a deliberate **409** shipping with no `httpStatus` at all, while sibling
17+
* rows of the same response carried one. `toRowApiError` read `err.status`, and
18+
* {@link lockedError} spells its refusal `statusCode`, which is the same
19+
* single-spelling defect that made `/api/v1/data` answer 500 to this very
20+
* refusal until #7525.
21+
*
22+
* ## Why the pin lives HERE
23+
*
24+
* `metadata-protocol` cannot import this plugin, and its own pins therefore
25+
* stand in for this producer with a double whose shape was measured. This file
26+
* is the half that needs no double: the error is raised by the actual
27+
* `beforeUpdate` hook `bindApprovalLockHook` binds, against an actual pending
28+
* `sys_approval_request` row, and the response row is built by the actual
29+
* `updateManyData` loop. If the hook ever re-spells its refusal — `.status`,
30+
* or a plain `Error` — this file goes red where a double would happily keep
31+
* asserting the old shape.
32+
*
33+
* The rig is `record-lock-multi-update.integration.test.ts`'s, for the same
34+
* reason it gives: the store is better-sqlite3 through `@objectstack/driver-sql`,
35+
* so the predicates are compiled and executed by the SQL builder rather than by
36+
* fixture code written by the same author as the assertion.
37+
*/
38+
39+
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
40+
import { ObjectQL } from '@objectstack/objectql';
41+
import { SqlDriver } from '@objectstack/driver-sql';
42+
import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol';
43+
import { resolveThrownHttpError } from '@objectstack/types';
44+
import { bindApprovalLockHook } from './lifecycle-hooks.js';
45+
46+
const opportunity = {
47+
name: 'opportunity',
48+
label: 'Opportunity',
49+
fields: {
50+
id: { name: 'id', type: 'text' as const, primaryKey: true },
51+
name: { name: 'name', type: 'text' as const },
52+
amount: { name: 'amount', type: 'number' as const },
53+
approval_status: { name: 'approval_status', type: 'text' as const },
54+
},
55+
};
56+
57+
/** The lock hook reads pending requests off this object. */
58+
const approvalRequest = {
59+
name: 'sys_approval_request',
60+
label: 'Approval Request',
61+
fields: {
62+
id: { name: 'id', type: 'text' as const, primaryKey: true },
63+
object_name: { name: 'object_name', type: 'text' as const },
64+
record_id: { name: 'record_id', type: 'text' as const },
65+
status: { name: 'status', type: 'text' as const },
66+
flow_run_id: { name: 'flow_run_id', type: 'text' as const },
67+
node_config_json: { name: 'node_config_json', type: 'text' as const },
68+
},
69+
};
70+
71+
describe('[#8570] a locked record\'s batch row carries the 409 the hook declared', () => {
72+
let engine: ObjectQL;
73+
let protocol: any;
74+
/** Held by a pending approval. */
75+
let lockedId: string;
76+
/** Same object, no approval — the row that must still succeed. */
77+
let freeId: string;
78+
79+
afterEach(async () => {
80+
try { await engine?.destroy(); } catch { /* noop */ }
81+
});
82+
83+
beforeEach(async () => {
84+
engine = new ObjectQL();
85+
engine.registerDriver(new SqlDriver({
86+
client: 'better-sqlite3',
87+
connection: { filename: ':memory:' },
88+
useNullAsDefault: true,
89+
}), true);
90+
await engine.init();
91+
// `packageId` is REQUIRED by `registerObject` — passed rather than elided
92+
// so this file adds no raw `tsc` error to the package's TEST_DEBT ledger,
93+
// which is measured with the test layer in the program (the package's own
94+
// `typecheck` script excludes `**/*.test.ts`, so it cannot see this).
95+
for (const o of [opportunity, approvalRequest]) {
96+
engine.registry.registerObject(o as any, 'com.objectstack.test.8570');
97+
}
98+
// Real DDL through the real path.
99+
await engine.syncSchemas();
100+
101+
lockedId = String((await engine.insert('opportunity', { name: 'Deal', amount: 100 })).id);
102+
freeId = String((await engine.insert('opportunity', { name: 'Other', amount: 100 })).id);
103+
await engine.insert('sys_approval_request', {
104+
object_name: 'opportunity',
105+
record_id: lockedId,
106+
status: 'pending',
107+
flow_run_id: 'run_1',
108+
node_config_json: JSON.stringify({ lockRecord: true, approvalStatusField: 'approval_status' }),
109+
}, { context: { isSystem: true } } as any);
110+
111+
bindApprovalLockHook(engine as any);
112+
protocol = new ObjectStackProtocolImplementation(engine as any);
113+
});
114+
115+
it('the card\'s second row, verbatim, now carrying 409', async () => {
116+
const res: any = await protocol.updateManyData({
117+
object: 'opportunity',
118+
records: [{ id: lockedId, data: { amount: 999 } }],
119+
});
120+
121+
expect(res.results[0].success).toBe(false);
122+
expect(res.results[0].errors[0]).toEqual({
123+
code: 'RECORD_LOCKED',
124+
message: `RECORD_LOCKED: record '${lockedId}' of 'opportunity' is locked while an approval is in progress`,
125+
httpStatus: 409,
126+
});
127+
128+
// The refusal was a refusal: nothing reached the store.
129+
expect((await engine.findOne('opportunity', { where: { id: lockedId } }))?.amount).toBe(100);
130+
});
131+
132+
it('the hook really declares its 409 in the `statusCode` spelling ONLY', async () => {
133+
// Non-vacuity for the row above, taken off the REAL producer rather than
134+
// asserted about it: if `lockedError` ever grew a `.status`, the row would
135+
// be green against the pre-#8570 limb too, and this file would stop
136+
// measuring anything.
137+
let thrown: any = null;
138+
try {
139+
await engine.update('opportunity', { amount: 999 }, { where: { id: lockedId } } as any);
140+
} catch (e) { thrown = e; }
141+
142+
expect(thrown).not.toBeNull();
143+
expect(thrown.code).toBe('RECORD_LOCKED');
144+
expect(thrown.statusCode).toBe(409);
145+
expect(thrown.status).toBeUndefined();
146+
expect(Object.getOwnPropertyNames(thrown)).toEqual(['stack', 'message', 'code', 'statusCode']);
147+
// The production recogniser, on the real throw — and the field the row's
148+
// limb reads, which is what separates a declared refusal from a fault.
149+
expect(resolveThrownHttpError(thrown).declaredStatus).toBe(409);
150+
});
151+
152+
it('an unlocked row in the SAME batch still succeeds and carries no error', async () => {
153+
// The asymmetry the card is about is per-row, so the mixed response is the
154+
// shape a caller actually has to reconcile.
155+
const res: any = await protocol.updateManyData({
156+
object: 'opportunity',
157+
records: [
158+
{ id: freeId, data: { amount: 555 } },
159+
{ id: lockedId, data: { amount: 999 } },
160+
],
161+
options: { continueOnError: true },
162+
});
163+
164+
expect(res.results[0].success).toBe(true);
165+
expect(res.results[0].errors).toBeUndefined();
166+
expect(res.results[1].errors[0].httpStatus).toBe(409);
167+
expect((await engine.findOne('opportunity', { where: { id: freeId } }))?.amount).toBe(555);
168+
});
169+
170+
it('the batchData upsert loop answers the same way — not just updateManyData', async () => {
171+
const res: any = await protocol.batchData({
172+
object: 'opportunity',
173+
request: { operation: 'upsert', records: [{ id: lockedId, data: { amount: 999 } }] },
174+
});
175+
176+
expect(res.results[0].errors[0].code).toBe('RECORD_LOCKED');
177+
expect(res.results[0].errors[0].httpStatus).toBe(409);
178+
});
179+
});
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
import { defineConfig } from 'vitest/config';
4+
import path from 'node:path';
5+
6+
export default defineConfig({
7+
resolve: {
8+
// [#8570] `record-lock-batch-row-status.integration.test.ts` drives the
9+
// REAL lock hook through the REAL bulk-write loops, so it imports
10+
// `@objectstack/metadata-protocol` as a value. That specifier resolves
11+
// through `exports` to `dist/` — a build artifact — which would make the
12+
// pin a verdict about build state rather than about the source in the
13+
// checkout (`pnpm check:test-source-alias`, #7668/#7778). Aliased to
14+
// source, which is that gate's prescribed fix; registering the package as
15+
// an unaliased importer is explicitly NOT (the registry is shrink-only).
16+
//
17+
// ANCHORED regex, array form: a bare string `find` matches by PREFIX, so
18+
// with a FILE replacement it would also swallow any subpath and resolve it
19+
// to `…/metadata-protocol/src/index.ts/<subpath>` — `ENOTDIR`, at run
20+
// time, from a config that reads as correct.
21+
alias: [
22+
{
23+
find: /^@objectstack\/metadata-protocol$/,
24+
replacement: path.resolve(__dirname, '../../metadata-protocol/src/index.ts'),
25+
},
26+
],
27+
},
28+
test: {
29+
globals: true,
30+
environment: 'node',
31+
include: ['src/**/*.test.ts'],
32+
},
33+
});

0 commit comments

Comments
 (0)