Skip to content

Commit 761fef4

Browse files
fix(metadata-protocol): deleteMany names the id entry it refuses (#29198)
<!-- claude-projects-attribution: {"github_login":"os-steve"} --> _Requested by **Steve** · [project thread](https://claude.ai/code/project/chan_01FdL1XeUyvVNxLreh3F4Qi9?thread=cmsg_01FdL1XeUyvVNxLreh3F4Qi9PYTuLPa8oWyz3qTsijRhsG)_ Fixes #29137 Clause-②: no Metadata-protocol: none ## 维护者速读 - 改了什么:批量删除(deleteMany)的 ids 里有空字符串、null 或对象时,仍然 400 拒收,但 fields 现在点名出错的那一项(ids.N),空字符串报 required,其余报 invalid_type。 - 为什么改:原来不论哪种都只回一句「ids 必须是记录 id 数组」且 fields 为空,ids 明明是数组,调用方找不到是哪一项出错。 - 风险与回滚:只改拒收时的句子和 fields,拒收与放行的范围不变;回滚即 revert 本 PR。 - 先看:packages/metadata-protocol/src/protocol-batch-data.ts 的 deleteManyData 拒收分支和 deleteManyIdEntryField。 - 两分钟验证:pnpm --filter @objectstack/metadata-protocol exec vitest run src/protocol.delete-many.test.ts,应全部通过。 - 需要你做的:无,席位落地。 ## What changes `deleteManyData` (the bulk delete door of `@objectstack/metadata-protocol`) used one check, with one sentence and no `fields`, for two different faults: `ids` is not an array, and `ids` is an array with an entry that is not a record id. It now splits them: - `ids` not an array: unchanged, the sentence "requires 'ids' to be an array of record ids" and no `fields`. - `ids` an array with bad entries: still `400 VALIDATION_FAILED`, now with one `fields[]` row per offending entry, `{ field: 'ids.N', code, message }`, the shape the REST ingress already gives `[null]` (`zodIssuesToFields`). An empty string answers `required` (the same fault the batch door's `rowRequiredIdError` calls "Record id is required"), anything else that is not a string or number (null, an operator object, a boolean) answers `invalid_type`. The sentence names the first offending entry, and says how many were refused when more than one. What is refused does not move: the fail-closed rule for non-scalar entries is unchanged, and a whitespace-only id still reaches the row loop and reports `RECORD_NOT_FOUND` for its row. No new error code (the codes are existing members of the ADR-0114 field-level catalog), no `packages/spec` edit. ## Tests - `packages/metadata-protocol/src/protocol.delete-many.test.ts`: new cases for `['']`, `['a', '']`, `[{"$ne":null}]` (still refused, now named), `[null]` (same pair as the ingress), several bad entries named at once, a non-array (old sentence, no `fields`), and `[' ']` (still reaches the row loop). - `packages/rest/src/rest-delete-many-ingress.test.ts`: a new block through the real protocol at the REST door: `{"ids":["a",""]}` answers 400 with `fields` naming `ids.1` as `required`, and `[null]` keeps the ingress answer `ids.0` / `invalid_type`. - No existing test was edited. ## Verification Base `fea2dbb7`, head `9e074bf5`. <details><summary>Base reading (REST door in-process, real protocol, fake engine)</summary> ``` {"ids":[""]} 400 "requires 'ids' to be an array of record ids" fields: [] {"ids":["a",""]} 400 "requires 'ids' to be an array of record ids" fields: [] {"ids":[{"$ne":null}]} 400 "Invalid deleteMany request" fields: [ids.0 invalid_type] (ingress) {"ids":"a"} 400 "Invalid deleteMany request" fields: [ids invalid_type] (ingress) {"ids":[null]} 400 "Invalid deleteMany request" fields: [ids.0 invalid_type] (ingress) {"ids":[" "]} 200 row RECORD_NOT_FOUND ``` In-process (protocol called directly) every one of the first five got the array sentence and no `fields`. Premise holds. </details> <details><summary>Suites</summary> - `pnpm --filter @objectstack/metadata-protocol test`: Test Files 297 passed, 4 skipped; Tests 22069 passed, 26 skipped. `typecheck`: exit 0. - REST deleteMany data-door tests (the 17 `packages/rest/src/*.test.ts` files that mention deleteMany): Test Files 17 passed; Tests 1049 passed. - `pnpm exec eslint --no-inline-config` on the three touched source/test files: exit 0. </details> <details><summary>Ablation (two legs, each rebuilt, restored, git diff HEAD empty)</summary> - Leg 1, drop `err.fields = idFields`: 5 protocol cases red (`expected undefined to deeply equal [ { field: 'ids.0', ... } ]`) and the REST case red (`expected [] to deeply equal [ [ 'ids.1', 'required' ] ]`). - Leg 2, accept an empty string as an id: 3 protocol cases red and the REST case red (`expected 200 to be 400`). - Restored through `ablation-replace.mjs --restore` (blob equals HEAD), rebuilt, `ablation-dist-preflight --absent` passed. </details> <details><summary>Gates</summary> `dispatch-gates --commands` derived 60 commands; 58 exited 0. `check-plugin-teardown-shape --self-test` exited 3 on a shallow clone, then 0 after fetching its pinned fixture commit. `check:dual-build-cjs-loads` exited 3 (PREREQUISITE NOT MET: needs the whole workspace built): NOT MEASURED locally, CI runs it. `--ran` reconciliation: 60 derived, 60 run, 0 UNRUN. </details> ## Acceptance notes - Choice of `required` for an empty string (rather than `min_length`, which a `z.string().min(1)` ingress would spell) follows the batch door's "Record id is required" for the same fault. Flagged to the seat as an open question. --- _Generated by [Claude Code](https://claude.ai/code/session_01ADHkmQJyGvWVm7dbChPMZ7)_ Co-authored-by: Claude <noreply@anthropic.com>
1 parent 47ee57f commit 761fef4

4 files changed

Lines changed: 145 additions & 1 deletion

File tree

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
'@objectstack/metadata-protocol': patch
3+
---
4+
5+
fix(metadata-protocol): `deleteMany` names the id entry it refuses
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
10+
A `deleteMany` whose `ids` is an array but holds an entry that is not a record id is still refused with `400 VALIDATION_FAILED`, and the answer now says which entry. Each offending entry gets a `fields[]` row `{ field: 'ids.N', code, message }`, the same shape the REST ingress already gives `[null]`: an empty string answers `required` (the entry is empty, and a record id is required for delete), and any other non-id (null, an operator object such as `{"$ne":null}`, a boolean) answers `invalid_type`. The sentence names the first offending entry. Before, every such request got the sentence "requires 'ids' to be an array of record ids" and an empty `fields[]`, which named the wrong problem.
11+
12+
A non-array `ids` keeps that sentence and names no entry. What is refused does not change: a whitespace-only id still reaches the row loop and reports `RECORD_NOT_FOUND` for its row.

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

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -920,6 +920,25 @@ export function buildUpdateManyResponse(
920920
} as BatchUpdateResponse;
921921
}
922922

923+
/**
924+
* The `fields[]` row naming one `deleteMany` id entry that is not a record id.
925+
* `field` is the dotted index path (`ids.N`) the REST ingress's
926+
* `zodIssuesToFields` uses for the same position; `code` is a member of the
927+
* ADR-0114 field-level catalog — `required` for an empty string, `invalid_type`
928+
* for anything that is not a string or number (null, an object, a boolean).
929+
*/
930+
function deleteManyIdEntryField(
931+
id: unknown,
932+
index: number,
933+
): { field: string; code: 'required' | 'invalid_type'; message: string } {
934+
const field = `ids.${index}`;
935+
if (id === '') {
936+
return { field, code: 'required', message: `${field}: the entry is empty, and a record id is required for delete` };
937+
}
938+
const received = id === null ? 'null' : Array.isArray(id) ? 'array' : typeof id;
939+
return { field, code: 'invalid_type', message: `${field}: expected a record id (a non-empty string or a number), received ${received}` };
940+
}
941+
923942
/** The body of `ObjectStackProtocolImplementation#deleteManyData`; the method's docblock there is its contract. */
924943
export async function deleteManyData(
925944
this: Pick<BatchDataHost, 'assertObjectRegistered' | 'buildDeleteManyResponse' | 'buildRolledBackBatchResponse' | 'runAtomicBatch' | 'runDeleteManyLoop'>,
@@ -934,14 +953,30 @@ export async function deleteManyData(
934953
const isScalarId = (v: unknown) =>
935954
(typeof v === 'string' && v.length > 0) || typeof v === 'number' || typeof v === 'bigint';
936955
const ids = request.ids as unknown;
937-
if (!Array.isArray(ids) || ids.some((id) => !isScalarId(id))) {
956+
if (!Array.isArray(ids)) {
938957
const err: any = new Error(
939958
`deleteMany on '${object}' requires 'ids' to be an array of record ids`,
940959
);
941960
err.code = 'VALIDATION_FAILED';
942961
err.status = 400;
943962
throw err;
944963
}
964+
// An ARRAY with a bad entry is a different fault from "not an array", and
965+
// the answer names the entry: one `fields[]` row per offending index,
966+
// spelled the way the REST ingress already answers `[null]`
967+
// (`{field: 'ids.0', code: 'invalid_type'}` from `zodIssuesToFields`), so
968+
// a client keying on `fields` reads one shape whichever layer refused. An
969+
// empty string is not a wrong TYPE — it is a missing id, the same fault
970+
// the batch door's `rowRequiredIdError` names — so it answers `required`.
971+
const idFields = ids.flatMap((id, index) => (isScalarId(id) ? [] : [deleteManyIdEntryField(id, index)]));
972+
if (idFields.length > 0) {
973+
const more = idFields.length > 1 ? ` (${idFields.length} entries refused, each named in fields)` : '';
974+
const err: any = new Error(`deleteMany on '${object}' refuses ${idFields[0].message}${more}`);
975+
err.code = 'VALIDATION_FAILED';
976+
err.status = 400;
977+
err.fields = idFields;
978+
throw err;
979+
}
945980

946981
// [#4620] `atomic` here was the same fake-atomic `batchData` carried
947982
// before ADR-0119 D4 — it only `break`-ed the loop, so every row deleted

‎packages/metadata-protocol/src/protocol.delete-many.test.ts‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,61 @@ describe('deleteManyData — the id set is the contract (#3897)', () => {
108108
expect(del).not.toHaveBeenCalled();
109109
});
110110

111+
// A refusal of an ARRAY names the entry at fault in `fields[]`, in the
112+
// `{field: 'ids.N', code}` shape the REST ingress gives `[null]`. A non-array
113+
// keeps the array sentence and names no entry, because there is none.
114+
async function refusal(ids: unknown) {
115+
const { p, del } = makeProtocol();
116+
const err: any = await p.deleteManyData({ object: 'invoice', ids } as any).then(
117+
() => { throw new Error('expected a refusal'); },
118+
(e: unknown) => e,
119+
);
120+
expect(del).not.toHaveBeenCalled();
121+
expect(err).toMatchObject({ code: 'VALIDATION_FAILED', status: 400 });
122+
return err;
123+
}
124+
125+
it('an empty-string entry is refused as a missing id, named by index', async () => {
126+
const err = await refusal(['']);
127+
expect(err.fields).toEqual([
128+
{ field: 'ids.0', code: 'required', message: expect.stringContaining('empty') },
129+
]);
130+
expect(err.message).toContain('ids.0');
131+
expect(err.message).toContain('empty');
132+
});
133+
134+
it('names the empty entry after a good one, not the first index', async () => {
135+
const err = await refusal(['a', '']);
136+
expect(err.fields.map((f: any) => [f.field, f.code])).toEqual([['ids.1', 'required']]);
137+
});
138+
139+
it('an operator object is still refused, and now named as the wrong type', async () => {
140+
const err = await refusal([{ $ne: null }]);
141+
expect(err.fields.map((f: any) => [f.field, f.code])).toEqual([['ids.0', 'invalid_type']]);
142+
});
143+
144+
it('a null entry answers the same pair the REST ingress gives it', async () => {
145+
const err = await refusal([null]);
146+
expect(err.fields.map((f: any) => [f.field, f.code])).toEqual([['ids.0', 'invalid_type']]);
147+
});
148+
149+
it('names every offending entry', async () => {
150+
const err = await refusal(['', 'a', null]);
151+
expect(err.fields.map((f: any) => [f.field, f.code])).toEqual([['ids.0', 'required'], ['ids.2', 'invalid_type']]);
152+
});
153+
154+
it('a non-array keeps the array sentence and names no entry', async () => {
155+
const err = await refusal('a');
156+
expect(err.message).toBe("deleteMany on 'invoice' requires 'ids' to be an array of record ids");
157+
expect(err.fields).toBeUndefined();
158+
});
159+
160+
it('a whitespace-only id is not refused here: it reaches the row loop', async () => {
161+
const { p, del } = makeProtocol();
162+
await p.deleteManyData({ object: 'invoice', ids: [' '] } as any);
163+
expect(del).toHaveBeenCalledTimes(1);
164+
});
165+
111166
it('an empty id list deletes nothing', async () => {
112167
const { p, del } = makeProtocol();
113168
const res: any = await p.deleteManyData({ object: 'invoice', ids: [] } as any);

‎packages/rest/src/rest-delete-many-ingress.test.ts‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
import { describe, it, expect, vi } from 'vitest';
1212
import { RestServer } from './rest-server';
13+
import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol';
1314

1415
const DELETE_MANY = '/api/v1/data/:object/deleteMany';
1516

@@ -128,3 +129,44 @@ describe('POST /data/:object/deleteMany — ingress validation (#3897)', () => {
128129
expect(deleteManyData).not.toHaveBeenCalled();
129130
});
130131
});
132+
133+
// The ingress lets a well-typed `[""]` through (it is an array of strings), so
134+
// the protocol's own scalar-id check is the layer that refuses it. Measured
135+
// here through the real protocol so the wire answer is the one a client reads:
136+
// the entry at fault is named in `fields[]`, in the pair the ingress itself
137+
// gives `[null]`.
138+
describe('POST /data/:object/deleteMany — a refused entry is named on the wire', () => {
139+
function setupReal() {
140+
const del = vi.fn(async () => true);
141+
const engine: any = {
142+
registry: { getObject: (n: string) => (n === 'invoice' ? { name: 'invoice', fields: {} } : undefined) },
143+
delete: del,
144+
};
145+
const protocol: any = new ObjectStackProtocolImplementation(engine);
146+
protocol.getDiscovery = vi.fn().mockResolvedValue({ version: 'v0', routes: { data: '', metadata: '', ui: '', auth: '/auth' } });
147+
protocol.getMetaItems = vi.fn().mockResolvedValue([{ name: 'invoice' }]);
148+
const rest = new RestServer(createMockServer() as any, protocol, { api: { requireAuth: false } } as any);
149+
(rest as any).resolveExecCtx = async () => ({ userId: 'test-user' });
150+
rest.registerRoutes();
151+
return { rest, del };
152+
}
153+
154+
it('an empty-string id answers 400 naming ids.1 as required', async () => {
155+
const { rest, del } = setupReal();
156+
const res = await post(rest, { ids: ['a', ''] });
157+
158+
expect(res.statusCode).toBe(400);
159+
expect(res.body.code).toBe('VALIDATION_FAILED');
160+
expect(res.body.fields.map((f: any) => [f.field, f.code])).toEqual([['ids.1', 'required']]);
161+
expect(del).not.toHaveBeenCalled();
162+
});
163+
164+
it('a null id keeps the ingress answer naming ids.0 as invalid_type', async () => {
165+
const { rest, del } = setupReal();
166+
const res = await post(rest, { ids: [null] });
167+
168+
expect(res.statusCode).toBe(400);
169+
expect(res.body.fields.map((f: any) => [f.field, f.code])).toEqual([['ids.0', 'invalid_type']]);
170+
expect(del).not.toHaveBeenCalled();
171+
});
172+
});

0 commit comments

Comments
 (0)