diff --git a/.changeset/29137-delete-many-names-refused-id.md b/.changeset/29137-delete-many-names-refused-id.md new file mode 100644 index 00000000000..36003139811 --- /dev/null +++ b/.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. diff --git a/packages/metadata-protocol/src/protocol-batch-data.ts b/packages/metadata-protocol/src/protocol-batch-data.ts index af954474d51..7edb6a8e66e 100644 --- a/packages/metadata-protocol/src/protocol-batch-data.ts +++ b/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, @@ -934,7 +953,7 @@ 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`, ); @@ -942,6 +961,22 @@ export async function deleteManyData( 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 diff --git a/packages/metadata-protocol/src/protocol.delete-many.test.ts b/packages/metadata-protocol/src/protocol.delete-many.test.ts index 8f5b08c6045..8825d0d2e64 100644 --- a/packages/metadata-protocol/src/protocol.delete-many.test.ts +++ b/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); diff --git a/packages/rest/src/rest-delete-many-ingress.test.ts b/packages/rest/src/rest-delete-many-ingress.test.ts index 1ed0a6666d5..a8e5f48051a 100644 --- a/packages/rest/src/rest-delete-many-ingress.test.ts +++ b/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(); + }); +});