diff --git a/.changeset/batch-create-readonly-ingress.md b/.changeset/batch-create-readonly-ingress.md new file mode 100644 index 0000000000..634d864e7f --- /dev/null +++ b/.changeset/batch-create-readonly-ingress.md @@ -0,0 +1,39 @@ +--- +"@objectstack/rest": minor +--- + +fix(rest): a batch create goes through the same create ingress as a single create (#3835) + +`readonly` meant two different things depending on which create endpoint you +used. `POST /data/:object` runs the #3043 ingress strip, so a non-system caller +cannot seed a read-only column — the field is dropped and reported. The +cross-object transactional batch (`POST /batch`) called `ql.insert` directly and +skipped that ingress entirely, and the engine's INSERT path is +static-`readonly`-exempt **by design** (#3413, the strip lives one layer up), so +nothing enforced it: the same forged `readonly` value that was rejected on one +route was written through on the other. + +Measured on the showcase (`showcase_contact.lead_score`, `readonly: true`, same +signed-in non-system user): + +| | before | after | +|---|---|---| +| `POST /data/showcase_contact` | `lead_score = null`, reported | unchanged | +| `POST /batch` create | **`lead_score = 999` written, silent** | `null`, reported | + +The fix routes the batch's create ops through the protocol's `createData` rather +than re-implementing the strip at the REST layer. That keeps **one** create +ingress: a future change to its policy covers the batch for free, and the +carve-outs already encoded there stay intact — notably the platform-object +exemption (a `sys_`/`managedBy` object's own guard must *reject* a forged value +with 403, not have it silently swallowed) and the `isSystem` exemption. The +context passed through is the transaction context, so the insert still joins the +batch transaction and rolls back with it, and `{ $ref: }` resolution is +unaffected. + +`createData`'s `droppedFields` are folded into the batch response's per-op +`droppedFields` list (#3794), so a batch create now reports its strips the same +way an update does. + +Update ops are untouched: the engine enforces `readonly` and `readonlyWhen` on +its own update path. diff --git a/.changeset/batch-dropped-fields-observability.md b/.changeset/batch-dropped-fields-observability.md new file mode 100644 index 0000000000..9f07350bbf --- /dev/null +++ b/.changeset/batch-dropped-fields-observability.md @@ -0,0 +1,25 @@ +--- +"@objectstack/spec": minor +"@objectstack/rest": minor +--- + +fix(rest): report `droppedFields` from the cross-object batch, so a silent strip stops reading as a clean save (#3794) + +The engine strips writes to `readonly` (#2948) and `readonlyWhen`-locked (#3042) +fields and completes the write without them. Every write path already reported +which fields it dropped (#3431/#3455) — except the cross-object transactional +batch, which never wired `onFieldsDropped` at all. + +That path is the Console record form's save for a master-detail record, so it is +exactly where a *user* edits a `readonlyWhen` field: they changed it, the form +said "updated successfully", the value never moved, and nothing anywhere said +so. + +`POST /batch` responses now carry a top-level `droppedFields` list, each event +tagged with the `index` of the operation that produced it (`results` entries are +bare record echoes, with no envelope to hang a per-row list on). Omitted +entirely when nothing was dropped, so the shape stays backward-compatible; the +batch still commits either way — a strip is legal semantics, not an error. + +The Console half ships in objectui: the write-warning toast now fires on batch +saves too. diff --git a/content/docs/references/api/batch.mdx b/content/docs/references/api/batch.mdx index 514086fdb2..06016b0012 100644 --- a/content/docs/references/api/batch.mdx +++ b/content/docs/references/api/batch.mdx @@ -30,8 +30,8 @@ Industry alignment: Salesforce Bulk API, Microsoft Dynamics Bulk Operations ## TypeScript Usage ```typescript -import { BatchConfig, BatchOperationResult, BatchOperationType, BatchOptions, BatchRecord, BatchUpdateRequest, BatchUpdateResponse, CrossObjectBatchOperation, CrossObjectBatchRequest, CrossObjectBatchResponse, DeleteManyRequest, UpdateManyRequest } from '@objectstack/spec/api'; -import type { BatchConfig, BatchOperationResult, BatchOperationType, BatchOptions, BatchRecord, BatchUpdateRequest, BatchUpdateResponse, CrossObjectBatchOperation, CrossObjectBatchRequest, CrossObjectBatchResponse, DeleteManyRequest, UpdateManyRequest } from '@objectstack/spec/api'; +import { BatchConfig, BatchOperationResult, BatchOperationType, BatchOptions, BatchRecord, BatchUpdateRequest, BatchUpdateResponse, CrossObjectBatchDroppedFields, CrossObjectBatchOperation, CrossObjectBatchRequest, CrossObjectBatchResponse, DeleteManyRequest, UpdateManyRequest } from '@objectstack/spec/api'; +import type { BatchConfig, BatchOperationResult, BatchOperationType, BatchOptions, BatchRecord, BatchUpdateRequest, BatchUpdateResponse, CrossObjectBatchDroppedFields, CrossObjectBatchOperation, CrossObjectBatchRequest, CrossObjectBatchResponse, DeleteManyRequest, UpdateManyRequest } from '@objectstack/spec/api'; // Validate data const result = BatchConfig.parse(data); @@ -136,6 +136,22 @@ const result = BatchConfig.parse(data); | **results** | `{ id?: string; success: boolean; errors?: { code: string; message: string; category?: string; details?: any; … }[]; data?: Record; … }[]` | ✅ | Detailed results for each record | +--- + +## CrossObjectBatchDroppedFields + +A cross-object batch strip event: dropped fields plus the operation index + +### Properties + +| Property | Type | Required | Description | +| :--- | :--- | :--- | :--- | +| **object** | `string` | ✅ | Object the write targeted (resolved object name) | +| **fields** | `string[]` | ✅ | Caller-supplied field names the engine removed from the write payload | +| **reason** | `Enum<'readonly' \| 'readonly_when'>` | ✅ | Why the fields were dropped: static readonly (#2948) or a TRUE readonlyWhen predicate (#3042) | +| **index** | `integer` | ✅ | Index of the operation in the request `operations` array | + + --- ## CrossObjectBatchOperation @@ -171,6 +187,7 @@ const result = BatchConfig.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | | **results** | `any[]` | ✅ | Per-operation result, index-aligned with the request operations | +| **droppedFields** | `{ object: string; fields: string[]; reason: Enum<'readonly' \| 'readonly_when'>; index: integer }[]` | optional | Write-observability (#3407/#3431/#3455/#3794): caller-supplied fields the engine LEGALLY stripped from an operation before it was written — static `readonly` (#2948) or a TRUE `readonlyWhen` predicate (#3042). This endpoint is the console record form's save path (master-detail writes parent + children in one transaction), so without it the ONE surface where a user edits a `readonlyWhen` field reported plain success while the value never landed. Each event carries the `index` of its operation. Present ONLY when ≥1 field was dropped; the batch still committed without them (results/success semantics unchanged). Optional — omit-when-empty keeps the shape backward-compatible. | --- diff --git a/packages/rest/src/rest-batch-endpoint.test.ts b/packages/rest/src/rest-batch-endpoint.test.ts index e64ffb736e..49a35b0c02 100644 --- a/packages/rest/src/rest-batch-endpoint.test.ts +++ b/packages/rest/src/rest-batch-endpoint.test.ts @@ -42,13 +42,42 @@ function makeQl(overrides: Partial> return ql; } +/** + * A fake `createData` standing in for the protocol's create ingress (#3835): + * the batch's create ops go through it rather than calling `ql.insert`, so the + * #3043 static-`readonly` strip applies to a batch create exactly as it does to + * `POST /data/:object`. Delegates to the same `ql.insert` the other ops use, so + * assertions about what reached the engine still read off `ql.insert`. + */ +function makeCreateData(ql: any, opts: { readonlyFields?: string[] } = {}) { + const readonlyFields = opts.readonlyFields ?? []; + return vi.fn(async (request: any) => { + const supplied = request?.data ?? {}; + const stripped: string[] = readonlyFields.filter((f) => f in supplied); + const data = { ...supplied }; + for (const f of stripped) delete data[f]; + const record = await ql.insert(request.object, data, { context: request.context }); + return { + object: request.object, + id: record?.id, + record, + ...(stripped.length > 0 + ? { droppedFields: [{ object: request.object, fields: stripped, reason: 'readonly' }] } + : {}), + }; + }); +} + /** Build a RestServer with an optional ObjectQL provider and object metadata. */ -function buildServer(opts: { ql?: any; objects?: any[] } = {}) { +function buildServer(opts: { ql?: any; objects?: any[]; readonlyFields?: string[] } = {}) { const server = mockServer(); const protocol: any = { getDiscovery: vi.fn().mockResolvedValue({ version: 'v0', endpoints: {} }), getMetaTypes: vi.fn().mockResolvedValue([]), getMetaItems: vi.fn().mockResolvedValue(opts.objects ?? []), + createData: opts.ql + ? makeCreateData(opts.ql, { readonlyFields: opts.readonlyFields ?? [] }) + : vi.fn(), }; const objectQLProvider = opts.ql ? async () => opts.ql : undefined; const rest = new RestServer( @@ -219,6 +248,103 @@ describe('POST {basePath}/batch — cross-object transactional batch', () => { expect(ql.transaction).not.toHaveBeenCalled(); }); + // ── write observability (#3794) ─────────────────────────────────────────── + // + // The console's record form saves a master-detail record through THIS route, + // so a `readonlyWhen`-locked field edited in that form was stripped by the + // engine and the response said nothing — the user saw "updated successfully" + // over a value that never changed. Every other write path already reports its + // strips (#3431/#3455); this one now does too. + + it('surfaces engine-stripped fields as droppedFields tagged with the op index', async () => { + const ql = makeQl({ + update: vi.fn(async (_object: string, data: any, opts: any) => { + opts?.onFieldsDropped?.({ object: 'invoice', fields: ['tax_rate'], reason: 'readonly_when' }); + const { tax_rate: _stripped, ...kept } = data; + return kept; + }), + }); + const { route } = buildServer({ ql }); + const res = await post(route, { + operations: [ + { object: 'account', action: 'create', data: { name: 'Acme' } }, + { object: 'invoice', action: 'update', id: 'inv_1', data: { status: 'paid', tax_rate: 9 } }, + ], + }); + expect(res.statusCode).toBe(200); + // The batch still committed — a strip is legal semantics, not an error. + expect(res.body.results).toHaveLength(2); + expect(res.body.droppedFields).toEqual([ + { object: 'invoice', fields: ['tax_rate'], reason: 'readonly_when', index: 1 }, + ]); + }); + + it('omits droppedFields entirely when nothing was stripped', async () => { + const ql = makeQl(); + const { route } = buildServer({ ql }); + const res = await post(route, { operations: [{ object: 'account', data: { name: 'Acme' } }] }); + expect(res.statusCode).toBe(200); + expect(res.body).not.toHaveProperty('droppedFields'); + }); + + // ── create ingress parity (#3835) ───────────────────────────────────────── + // + // The engine's INSERT path is static-`readonly`-exempt by design (#3413), so + // the #3043 strip that stops a non-system caller from seeding a read-only + // column lives at the protocol's create ingress. This route used to call + // `ql.insert` directly and skip it, so `readonly` meant two different things + // depending on which create endpoint you used. + + it('routes create ops through the protocol create ingress, not ql.insert', async () => { + const ql = makeQl(); + const { route, protocol } = buildServer({ ql }); + const res = await post(route, { + operations: [{ object: 'account', action: 'create', data: { name: 'Acme' } }], + }); + expect(res.statusCode).toBe(200); + expect(protocol.createData).toHaveBeenCalledTimes(1); + const request = protocol.createData.mock.calls[0][0]; + expect(request).toMatchObject({ object: 'account', data: { name: 'Acme' } }); + // The ingress must run INSIDE this transaction — same context the other ops + // bind, or the create would commit independently of the rollback. + expect(request.context).toMatchObject({ __trx: true }); + }); + + it('strips a caller-forged readonly column on a batch create and reports it', async () => { + const ql = makeQl(); + const { route } = buildServer({ ql, readonlyFields: ['approval_status'] }); + const res = await post(route, { + operations: [ + { object: 'account', action: 'create', data: { name: 'Acme', approval_status: 'approved' } }, + ], + }); + expect(res.statusCode).toBe(200); + // Never reached the engine… + expect(ql.insert).toHaveBeenCalledWith('account', { name: 'Acme' }, expect.anything()); + // …and the caller is told, tagged with the op that dropped it. + expect(res.body.droppedFields).toEqual([ + { object: 'account', fields: ['approval_status'], reason: 'readonly', index: 0 }, + ]); + }); + + it('keeps $ref resolution working through the ingress', async () => { + // `results` now holds the ingress's echoed record rather than the raw + // engine return — a later op must still resolve `{ $ref: 0 }` to its id. + const ql = makeQl(); + const { route } = buildServer({ ql }); + const res = await post(route, { + operations: [ + { object: 'project', action: 'create', data: { name: 'Apollo' } }, + { object: 'task', action: 'create', data: { title: 'Kickoff', project: { $ref: 0 } } }, + ], + }); + expect(res.statusCode).toBe(200); + expect(res.body.results[0].id).toBe('id_1'); + expect(ql.insert).toHaveBeenLastCalledWith( + 'task', { title: 'Kickoff', project: 'id_1' }, expect.anything(), + ); + }); + // ── request validation ──────────────────────────────────────────────────── it('rejects a non-atomic request (this endpoint is always atomic)', async () => { diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index af35d91c9a..1e13be5280 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -6670,15 +6670,53 @@ export class RestServer { return result; }; + // [#3794] Write-observability on THIS surface too. The engine + // strips `readonly` / `readonlyWhen` writes silently, and every + // other write path already reports what it dropped (#3431/#3455) + // — but this one did not, and it is precisely the path the + // console's record form takes for a master-detail save. Result: + // a user edited a `readonlyWhen`-locked field, got "updated + // successfully", and the value never changed with nothing said + // (#3794 problem 2). Each event is tagged with its operation + // index, since `results` entries are bare record echoes with no + // envelope to hang a per-row list on. + const dropped: Array = []; const results = await ql.transaction(async (trxCtx: any) => { const out: any[] = []; - for (const op of ops) { + for (const [index, op] of ops.entries()) { const data = resolveRefs(op.data, out); if (op.action === 'create') { - out.push(await ql.insert(op.object, data, { context: trxCtx })); + // [#3835] Go through the protocol's create ingress — + // the SAME one `POST /data/:object` uses — rather than + // calling `ql.insert` directly. The engine's INSERT path + // is static-`readonly`-exempt by design (#3413), so the + // #3043 strip that stops a non-system caller from seeding + // a read-only column lives at that ingress. Bypassing it + // here made `readonly` mean two different things on two + // create paths: rejected on the single route, written + // through the batch. `createData` also owns the platform- + // object carve-out (a `sys_`/`managedBy` object's own + // guard must REJECT a forged value, not silently swallow + // it), which is why this routes to the ingress instead of + // re-implementing the strip here — one create ingress, + // and a future change to its policy covers the batch for + // free. `trxCtx` carries the caller's context (including + // `isSystem`) plus the open transaction, so the strip + // decides exactly as it does on the single route and the + // insert still joins this transaction. + const created: any = await p.createData({ object: op.object, data, context: trxCtx } as any); + for (const e of (created?.droppedFields ?? []) as DroppedFieldsEvent[]) { + dropped.push({ ...e, index }); + } + out.push(created?.record); } else if (op.action === 'update') { + // Update needs no ingress detour: the engine enforces + // both static `readonly` (#2948) and `readonlyWhen` + // (#3042) on its own update path, and reports them + // through this listener. + const onFieldsDropped = (e: DroppedFieldsEvent) => { dropped.push({ ...e, index }); }; const id = op.id ?? data?.id; - out.push(await ql.update(op.object, { ...data, id }, { context: trxCtx })); + out.push(await ql.update(op.object, { ...data, id }, { context: trxCtx, onFieldsDropped })); } else { // 'delete' out.push(await ql.delete(op.object, { where: { id: op.id }, context: trxCtx })); } @@ -6686,7 +6724,7 @@ export class RestServer { return out; }, context); - res.json({ results }); + res.json({ results, ...(dropped.length > 0 ? { droppedFields: dropped } : {}) }); } catch (error: any) { // Log only genuine server faults; client 4xx (validation, // unresolved ref, atomic rollback of a bad op) are expected. diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index faa324aeb4..432507f7a7 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -2378,6 +2378,8 @@ "CreateViewRequestSchema (const)", "CreateViewResponse (type)", "CreateViewResponseSchema (const)", + "CrossObjectBatchDroppedFields (type)", + "CrossObjectBatchDroppedFieldsSchema (const)", "CrossObjectBatchOperation (type)", "CrossObjectBatchOperationSchema (const)", "CrossObjectBatchRequest (type)", diff --git a/packages/spec/json-schema.manifest.json b/packages/spec/json-schema.manifest.json index a62bf88de5..b8e1ef1137 100644 --- a/packages/spec/json-schema.manifest.json +++ b/packages/spec/json-schema.manifest.json @@ -154,6 +154,7 @@ "api/CreateRequest", "api/CreateViewRequest", "api/CreateViewResponse", + "api/CrossObjectBatchDroppedFields", "api/CrossObjectBatchOperation", "api/CrossObjectBatchRequest", "api/CrossObjectBatchResponse", diff --git a/packages/spec/src/api/batch.zod.ts b/packages/spec/src/api/batch.zod.ts index 110b22196c..353459bfe7 100644 --- a/packages/spec/src/api/batch.zod.ts +++ b/packages/spec/src/api/batch.zod.ts @@ -298,6 +298,20 @@ export const CrossObjectBatchRequestSchema = lazySchema(() => z.object({ export type CrossObjectBatchRequest = z.input; +/** + * One strip event on a cross-object batch, tagged with the operation it + * belongs to (#3794). The per-object bulk paths hang `droppedFields` on their + * per-row result object; this batch's `results` entries are the raw record + * echoes (`z.unknown()`), with no envelope to hang anything on — so the events + * ride a parallel top-level list and carry `index` to point back at the + * operation that produced them. + */ +export const CrossObjectBatchDroppedFieldsSchema = lazySchema(() => DroppedFieldsEventSchema.extend({ + index: z.number().int().min(0).describe('Index of the operation in the request `operations` array'), +}).describe('A cross-object batch strip event: dropped fields plus the operation index')); + +export type CrossObjectBatchDroppedFields = z.infer; + /** * Response for the cross-object transactional batch — one result per operation, * index-aligned with the request `operations` (create/update echo the record, @@ -305,6 +319,16 @@ export type CrossObjectBatchRequest = z.input z.object({ results: z.array(z.unknown()).describe('Per-operation result, index-aligned with the request operations'), + droppedFields: z.array(CrossObjectBatchDroppedFieldsSchema).optional().describe( + 'Write-observability (#3407/#3431/#3455/#3794): caller-supplied fields the engine LEGALLY ' + + 'stripped from an operation before it was written — static `readonly` (#2948) or a TRUE ' + + '`readonlyWhen` predicate (#3042). This endpoint is the console record form\'s save path ' + + '(master-detail writes parent + children in one transaction), so without it the ONE surface ' + + 'where a user edits a `readonlyWhen` field reported plain success while the value never ' + + 'landed. Each event carries the `index` of its operation. Present ONLY when ≥1 field was ' + + 'dropped; the batch still committed without them (results/success semantics unchanged). ' + + 'Optional — omit-when-empty keeps the shape backward-compatible.' + ), })); export type CrossObjectBatchResponse = z.infer;