fix(approvals,rest): tell the truth about approval writes — lock policy, dropped fields, and the batch create ingress (#3794, #3835)

.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: <opIndex> }` 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.

.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.

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<string, any>; … }[]` | ✅ | 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. |
 
 
 ---

packages/rest/src/rest-batch-endpoint.test.ts

@@ -42,13 +42,42 @@ function makeQl(overrides: Partial<Record<'insert' | 'update' | 'delete', any>>
   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 () => {

packages/rest/src/rest-server.ts

@@ -6670,23 +6670,61 @@ 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<DroppedFieldsEvent & { index: number }> = [];
                     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 }));
                             }
                         }
                         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.

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)",

packages/spec/json-schema.manifest.json

@@ -154,6 +154,7 @@
     "api/CreateRequest",
     "api/CreateViewRequest",
     "api/CreateViewResponse",
+    "api/CrossObjectBatchDroppedFields",
     "api/CrossObjectBatchOperation",
     "api/CrossObjectBatchRequest",
     "api/CrossObjectBatchResponse",