fix(metadata-protocol): deleteMany names the id entry it refuses

.changeset/29137-delete-many-names-refused-id.md

@@ -0,0 +1,12 @@
+---
+'@objectstack/metadata-protocol': patch
+---
+
+fix(metadata-protocol): `deleteMany` names the id entry it refuses
+
+Clause-②: no
+Metadata-protocol: none
+
+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.
+
+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

@@ -920,6 +920,25 @@ export function buildUpdateManyResponse(
     } as BatchUpdateResponse;
 }
 
+/**
+ * The `fields[]` row naming one `deleteMany` id entry that is not a record id.
+ * `field` is the dotted index path (`ids.N`) the REST ingress's
+ * `zodIssuesToFields` uses for the same position; `code` is a member of the
+ * ADR-0114 field-level catalog — `required` for an empty string, `invalid_type`
+ * for anything that is not a string or number (null, an object, a boolean).
+ */
+function deleteManyIdEntryField(
+    id: unknown,
+    index: number,
+): { field: string; code: 'required' | 'invalid_type'; message: string } {
+    const field = `ids.${index}`;
+    if (id === '') {
+        return { field, code: 'required', message: `${field}: the entry is empty, and a record id is required for delete` };
+    }
+    const received = id === null ? 'null' : Array.isArray(id) ? 'array' : typeof id;
+    return { field, code: 'invalid_type', message: `${field}: expected a record id (a non-empty string or a number), received ${received}` };
+}
+
 /** The body of `ObjectStackProtocolImplementation#deleteManyData`; the method's docblock there is its contract. */
 export async function deleteManyData(
     this: Pick<BatchDataHost, 'assertObjectRegistered' | 'buildDeleteManyResponse' | 'buildRolledBackBatchResponse' | 'runAtomicBatch' | 'runDeleteManyLoop'>,
@@ -934,14 +953,30 @@ export async function deleteManyData(
     const isScalarId = (v: unknown) =>
         (typeof v === 'string' && v.length > 0) || typeof v === 'number' || typeof v === 'bigint';
     const ids = request.ids as unknown;
-    if (!Array.isArray(ids) || ids.some((id) => !isScalarId(id))) {
+    if (!Array.isArray(ids)) {
         const err: any = new Error(
             `deleteMany on '${object}' requires 'ids' to be an array of record ids`,
         );
         err.code = 'VALIDATION_FAILED';
         err.status = 400;
         throw err;
     }
+    // An ARRAY with a bad entry is a different fault from "not an array", and
+    // the answer names the entry: one `fields[]` row per offending index,
+    // spelled the way the REST ingress already answers `[null]`
+    // (`{field: 'ids.0', code: 'invalid_type'}` from `zodIssuesToFields`), so
+    // a client keying on `fields` reads one shape whichever layer refused. An
+    // empty string is not a wrong TYPE — it is a missing id, the same fault
+    // the batch door's `rowRequiredIdError` names — so it answers `required`.
+    const idFields = ids.flatMap((id, index) => (isScalarId(id) ? [] : [deleteManyIdEntryField(id, index)]));
+    if (idFields.length > 0) {
+        const more = idFields.length > 1 ? ` (${idFields.length} entries refused, each named in fields)` : '';
+        const err: any = new Error(`deleteMany on '${object}' refuses ${idFields[0].message}${more}`);
+        err.code = 'VALIDATION_FAILED';
+        err.status = 400;
+        err.fields = idFields;
+        throw err;
+    }
 
     // [#4620] `atomic` here was the same fake-atomic `batchData` carried
     // before ADR-0119 D4 — it only `break`-ed the loop, so every row deleted

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

@@ -108,6 +108,61 @@ describe('deleteManyData — the id set is the contract (#3897)', () => {
     expect(del).not.toHaveBeenCalled();
   });
 
+  // A refusal of an ARRAY names the entry at fault in `fields[]`, in the
+  // `{field: 'ids.N', code}` shape the REST ingress gives `[null]`. A non-array
+  // keeps the array sentence and names no entry, because there is none.
+  async function refusal(ids: unknown) {
+    const { p, del } = makeProtocol();
+    const err: any = await p.deleteManyData({ object: 'invoice', ids } as any).then(
+      () => { throw new Error('expected a refusal'); },
+      (e: unknown) => e,
+    );
+    expect(del).not.toHaveBeenCalled();
+    expect(err).toMatchObject({ code: 'VALIDATION_FAILED', status: 400 });
+    return err;
+  }
+
+  it('an empty-string entry is refused as a missing id, named by index', async () => {
+    const err = await refusal(['']);
+    expect(err.fields).toEqual([
+      { field: 'ids.0', code: 'required', message: expect.stringContaining('empty') },
+    ]);
+    expect(err.message).toContain('ids.0');
+    expect(err.message).toContain('empty');
+  });
+
+  it('names the empty entry after a good one, not the first index', async () => {
+    const err = await refusal(['a', '']);
+    expect(err.fields.map((f: any) => [f.field, f.code])).toEqual([['ids.1', 'required']]);
+  });
+
+  it('an operator object is still refused, and now named as the wrong type', async () => {
+    const err = await refusal([{ $ne: null }]);
+    expect(err.fields.map((f: any) => [f.field, f.code])).toEqual([['ids.0', 'invalid_type']]);
+  });
+
+  it('a null entry answers the same pair the REST ingress gives it', async () => {
+    const err = await refusal([null]);
+    expect(err.fields.map((f: any) => [f.field, f.code])).toEqual([['ids.0', 'invalid_type']]);
+  });
+
+  it('names every offending entry', async () => {
+    const err = await refusal(['', 'a', null]);
+    expect(err.fields.map((f: any) => [f.field, f.code])).toEqual([['ids.0', 'required'], ['ids.2', 'invalid_type']]);
+  });
+
+  it('a non-array keeps the array sentence and names no entry', async () => {
+    const err = await refusal('a');
+    expect(err.message).toBe("deleteMany on 'invoice' requires 'ids' to be an array of record ids");
+    expect(err.fields).toBeUndefined();
+  });
+
+  it('a whitespace-only id is not refused here: it reaches the row loop', async () => {
+    const { p, del } = makeProtocol();
+    await p.deleteManyData({ object: 'invoice', ids: ['  '] } as any);
+    expect(del).toHaveBeenCalledTimes(1);
+  });
+
   it('an empty id list deletes nothing', async () => {
     const { p, del } = makeProtocol();
     const res: any = await p.deleteManyData({ object: 'invoice', ids: [] } as any);

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

@@ -10,6 +10,7 @@
 
 import { describe, it, expect, vi } from 'vitest';
 import { RestServer } from './rest-server';
+import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol';
 
 const DELETE_MANY = '/api/v1/data/:object/deleteMany';
 
@@ -128,3 +129,44 @@ describe('POST /data/:object/deleteMany — ingress validation (#3897)', () => {
     expect(deleteManyData).not.toHaveBeenCalled();
   });
 });
+
+// The ingress lets a well-typed `[""]` through (it is an array of strings), so
+// the protocol's own scalar-id check is the layer that refuses it. Measured
+// here through the real protocol so the wire answer is the one a client reads:
+// the entry at fault is named in `fields[]`, in the pair the ingress itself
+// gives `[null]`.
+describe('POST /data/:object/deleteMany — a refused entry is named on the wire', () => {
+  function setupReal() {
+    const del = vi.fn(async () => true);
+    const engine: any = {
+      registry: { getObject: (n: string) => (n === 'invoice' ? { name: 'invoice', fields: {} } : undefined) },
+      delete: del,
+    };
+    const protocol: any = new ObjectStackProtocolImplementation(engine);
+    protocol.getDiscovery = vi.fn().mockResolvedValue({ version: 'v0', routes: { data: '', metadata: '', ui: '', auth: '/auth' } });
+    protocol.getMetaItems = vi.fn().mockResolvedValue([{ name: 'invoice' }]);
+    const rest = new RestServer(createMockServer() as any, protocol, { api: { requireAuth: false } } as any);
+    (rest as any).resolveExecCtx = async () => ({ userId: 'test-user' });
+    rest.registerRoutes();
+    return { rest, del };
+  }
+
+  it('an empty-string id answers 400 naming ids.1 as required', async () => {
+    const { rest, del } = setupReal();
+    const res = await post(rest, { ids: ['a', ''] });
+
+    expect(res.statusCode).toBe(400);
+    expect(res.body.code).toBe('VALIDATION_FAILED');
+    expect(res.body.fields.map((f: any) => [f.field, f.code])).toEqual([['ids.1', 'required']]);
+    expect(del).not.toHaveBeenCalled();
+  });
+
+  it('a null id keeps the ingress answer naming ids.0 as invalid_type', async () => {
+    const { rest, del } = setupReal();
+    const res = await post(rest, { ids: [null] });
+
+    expect(res.statusCode).toBe(400);
+    expect(res.body.fields.map((f: any) => [f.field, f.code])).toEqual([['ids.0', 'invalid_type']]);
+    expect(del).not.toHaveBeenCalled();
+  });
+});