From b88d592a269bebce4239c147cb3f7ce0f55ac75a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8C=85=E5=91=A8=E6=B6=9B?= Date: Tue, 14 Jul 2026 11:53:02 -0700 Subject: [PATCH] =?UTF-8?q?fix(objectql,spec,rest):=20#2922=20import=20aut?= =?UTF-8?q?omation=20chain=20=E2=80=94=20per-row=20batch=20hooks,=20effect?= =?UTF-8?q?ive=20skipAutomations,=20run-automations=20default=20ON?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - engine.insert now triggers beforeInsert/afterInsert once per row with single-record hook contexts, so flat-input proxies, declarative hook conditions, audit writers and record-change triggers see real records instead of arrays. - New ExecutionContext.skipAutomations (mirrored into HookContext.session) suppresses metadata-bound automation hooks and implies skipTriggers for flow dispatch; code-registered system hooks (audit, security, sharing) still run. Makes the import wizard checkbox and import undo effective. - REST import defaults runAutomations to true unless the request explicitly opts out, matching historical behavior. Closes #2922 --- .changeset/import-automation-hooks-2922.md | 17 +++ packages/objectql/src/engine.test.ts | 83 +++++++++++++ packages/objectql/src/engine.ts | 114 +++++++++++------- packages/rest/src/import-integration.test.ts | 10 +- packages/rest/src/import-prepare.test.ts | 24 ++++ packages/rest/src/import-prepare.ts | 5 +- packages/spec/src/data/hook.zod.ts | 1 + .../spec/src/kernel/execution-context.zod.ts | 13 ++ 8 files changed, 222 insertions(+), 45 deletions(-) create mode 100644 .changeset/import-automation-hooks-2922.md diff --git a/.changeset/import-automation-hooks-2922.md b/.changeset/import-automation-hooks-2922.md new file mode 100644 index 0000000000..08a52ff7ba --- /dev/null +++ b/.changeset/import-automation-hooks-2922.md @@ -0,0 +1,17 @@ +--- +'@objectstack/spec': patch +'@objectstack/objectql': patch +'@objectstack/rest': patch +--- + +Fix the data-import automation chain (#2922). Batch `engine.insert` now fires +`beforeInsert`/`afterInsert` once **per row** with single-record hook contexts, +so flat-input proxies, declarative hook conditions, audit writers, and +record-change triggers see real records instead of arrays. A new +`ExecutionContext.skipAutomations` flag (mirrored into `HookContext.session`) +lets callers suppress metadata-bound automation hooks and flow dispatch while +code-registered system hooks (audit, security, sharing) still run — making the +import wizard's "run automations & triggers" checkbox and import undo actually +effective. The REST import default flips to running automations unless the +request explicitly opts out (`runAutomations: false`), matching historical +behavior. diff --git a/packages/objectql/src/engine.test.ts b/packages/objectql/src/engine.test.ts index 01b85f9664..708c0b4154 100644 --- a/packages/objectql/src/engine.test.ts +++ b/packages/objectql/src/engine.test.ts @@ -400,6 +400,89 @@ describe('ObjectQL Engine', () => { }); }); + describe('batch insert triggers hooks per row (#2922)', () => { + beforeEach(async () => { + engine.registerDriver(mockDriver, true); + await engine.init(); + vi.mocked(SchemaRegistry.getObject).mockReturnValue({ name: 'task', fields: { title: { type: 'text' } } } as any); + }); + + it('fires beforeInsert/afterInsert once per row with the single-record context shape', async () => { + const beforeRows: any[] = []; + const afterResults: any[] = []; + engine.registerHook('beforeInsert', async (ctx: any) => { + beforeRows.push(ctx.input.data); + // Same mutation contract as single insert: write one row's field. + ctx.input.data.stamped = ctx.input.data.title.toUpperCase(); + }, { object: 'task' }); + engine.registerHook('afterInsert', async (ctx: any) => { + afterResults.push(ctx.result); + }, { object: 'task' }); + (mockDriver.create as any).mockImplementation(async (_o: string, row: any) => ({ id: `id-${row.title}`, ...row })); + + const result = await engine.insert('task', [{ title: 'a' }, { title: 'b' }]); + + expect(beforeRows).toHaveLength(2); + expect(Array.isArray(beforeRows[0])).toBe(false); + expect(beforeRows[0]).toMatchObject({ title: 'a' }); + expect(beforeRows[1]).toMatchObject({ title: 'b' }); + // The per-row mutation reached the driver for each row. + expect((mockDriver.create as any).mock.calls[0][1]).toMatchObject({ title: 'a', stamped: 'A' }); + expect((mockDriver.create as any).mock.calls[1][1]).toMatchObject({ title: 'b', stamped: 'B' }); + // afterInsert sees one record per row, never the whole array. + expect(afterResults).toHaveLength(2); + expect(Array.isArray(afterResults[0])).toBe(false); + expect(afterResults[0]).toMatchObject({ id: 'id-a' }); + expect(afterResults[1]).toMatchObject({ id: 'id-b' }); + expect(result).toHaveLength(2); + }); + + it('pairs each afterInsert result with its own row when the driver bulk-creates', async () => { + (mockDriver as any).bulkCreate = vi.fn(async (_o: string, rows: any[]) => + rows.map((r: any) => ({ id: `id-${r.title}`, ...r }))); + const afterResults: any[] = []; + engine.registerHook('afterInsert', async (ctx: any) => { afterResults.push(ctx.result); }, { object: 'task' }); + + await engine.insert('task', [{ title: 'a' }, { title: 'b' }]); + + expect((mockDriver as any).bulkCreate).toHaveBeenCalledTimes(1); + expect(afterResults.map((r) => r.id)).toEqual(['id-a', 'id-b']); + }); + }); + + describe('skipAutomations suppresses metadata-bound hooks (#2922)', () => { + beforeEach(async () => { + engine.registerDriver(mockDriver, true); + await engine.init(); + vi.mocked(SchemaRegistry.getObject).mockReturnValue({ name: 'task', fields: {} } as any); + }); + + it('skips hooks bound from metadata but still runs code-registered system hooks', async () => { + const calls: string[] = []; + // Metadata-bound automation hook: `meta` present (bindHooksToEngine shape). + engine.registerHook('beforeInsert', async () => { calls.push('automation'); }, + { object: 'task', meta: { name: 'auto_hook', events: ['beforeInsert'] } }); + // Code-registered system hook (audit/security shape): no `meta`. + engine.registerHook('beforeInsert', async () => { calls.push('system'); }, { object: 'task' }); + + await engine.insert('task', { title: 'x' }, { context: { skipAutomations: true } as any }); + expect(calls).toEqual(['system']); + + calls.length = 0; + await engine.insert('task', { title: 'y' }); + expect(calls.sort()).toEqual(['automation', 'system']); + }); + + it('implies skipTriggers on the hook session so flow dispatch is suppressed too', async () => { + let session: any; + engine.registerHook('afterInsert', async (ctx: any) => { session = ctx.session; }, { object: 'task' }); + + await engine.insert('task', { title: 'x' }, { context: { userId: 'u1', skipAutomations: true } as any }); + + expect(session).toMatchObject({ skipAutomations: true, skipTriggers: true }); + }); + }); + describe('execution context via the trailing options arg (read methods)', () => { // Regression: reads took context inside the query while writes took it in // a trailing options arg — so `find(obj, q, { context })` silently dropped diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 43e998646f..8e542e22a5 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -590,7 +590,16 @@ export class ObjectQL implements IDataEngine { } this.logger.debug('Triggering hooks', { event, count: entries.length }); - + + // `session.skipAutomations` (set from ExecutionContext.skipAutomations — + // import with "run automations & triggers" unchecked, import undo) + // suppresses hooks bound FROM METADATA (`bindHooksToEngine` stamps + // `entry.meta`). Hooks registered in code by plugins — audit, capability + // gates, sharing projection — have no `meta` and always run: the opt-out + // must never bypass security or audit (#2922). + const skipAutomations = + (context.session as { skipAutomations?: boolean } | undefined)?.skipAutomations === true; + for (const entry of entries) { // Per-object matching if (entry.object) { @@ -599,6 +608,10 @@ export class ObjectQL implements IDataEngine { continue; // Skip non-matching hooks } } + if (skipAutomations && entry.meta) { + this.logger.debug('Skipping metadata-bound hook (skipAutomations)', { event, hook: entry.hookName }); + continue; + } await entry.handler(context); } } @@ -692,8 +705,13 @@ export class ObjectQL implements IDataEngine { ...((execCtx as any).isSystem ? { isSystem: true } : {}), // Propagate the automation-suppression flag so the record-change trigger // can skip flow dispatch for seed/bulk writes (ADR: seed loads end-state - // data, not user events). - ...((execCtx as any).skipTriggers ? { skipTriggers: true } : {}), + // data, not user events). `skipAutomations` implies `skipTriggers` — + // suppressing metadata hooks while still dispatching flows would leave + // the "run automations & triggers" opt-out half-working (#2922). + ...((execCtx as any).skipTriggers || (execCtx as any).skipAutomations ? { skipTriggers: true } : {}), + // Propagate the full automation opt-out so `triggerHooks` can skip + // metadata-bound hooks (import with "run automations" unchecked, undo). + ...((execCtx as any).skipAutomations ? { skipAutomations: true } : {}), } as HookContext['session']; } @@ -2175,28 +2193,45 @@ export class ObjectQL implements IDataEngine { // any defaulted field. `applyFieldDefaults` returns a fresh copy and only // fills fields left `undefined`, so client-supplied values are untouched. const nowSnap = new Date(); - const defaultedData = Array.isArray(opCtx.data) + const isBatch = Array.isArray(opCtx.data); + const defaultedData = isBatch ? (opCtx.data as any[]).map((row) => this.applyFieldDefaults(object, row as Record, opCtx.context, nowSnap), ) : this.applyFieldDefaults(object, opCtx.data as Record, opCtx.context, nowSnap); - const hookContext: HookContext = { + // Batch inserts trigger beforeInsert/afterInsert PER ROW, each with the + // exact single-record context shape (`input.data` = one row, `result` = + // its returned record). A single array-shaped context broke every + // consumer built for the single shape — the flat-input proxy read + // `undefined`s, declarative `condition`s evaluated against an array, + // audit rows and flow-trigger contexts came out mangled (#2922). + const rowHookContexts: HookContext[] = (isBatch ? (defaultedData as any[]) : [defaultedData]).map( + (row) => ({ object, event: 'beforeInsert', - input: { data: defaultedData, options: opCtx.options }, + input: { data: row, options: opCtx.options }, session: this.buildSession(opCtx.context), api: this.buildHookApi(opCtx.context), transaction: opCtx.context?.transaction, - ql: this - }; - await this.triggerHooks('beforeInsert', hookContext); + ql: this, + }), + ); + for (const rowCtx of rowHookContexts) { + await this.triggerHooks('beforeInsert', rowCtx); + } // Thread the open transaction (if any) into the driver-facing // options so that knex's `.transacting(trx)` is honoured. Without // this, calls inside a `engine.transaction(...)` block would deadlock // on SQLite's single-connection pool. Also propagates tenantId so // the driver can enforce per-tenant isolation. - hookContext.input.options = this.buildDriverOptions(opCtx.context, hookContext.input.options as any); + // Base the merge on the first row context's options: hooks share the + // same underlying options object (in-place mutations are visible), and + // for single inserts this is exactly the pre-#2922 behaviour. + const driverOptions = this.buildDriverOptions(opCtx.context, rowHookContexts[0]?.input.options as any); + for (const rowCtx of rowHookContexts) { + rowCtx.input.options = driverOptions; + } try { let result; @@ -2204,47 +2239,42 @@ export class ObjectQL implements IDataEngine { // When the driver generates autonumbers natively (persistent SQL // sequence), the engine defers to it — see #1603. const driverOwnsAutonumber = (driver as any)?.supports?.autonumber === true; - if (Array.isArray(hookContext.input.data)) { - // Defaults are already resolved above (pre-hook, #2703); the hook may - // have overridden or added fields — take its data as-is. - const rows = hookContext.input.data as Array>; - for (const r of rows) { - await this.applyAutonumbers(object, r as Record, opCtx.context, driverOwnsAutonumber); - } - for (const r of rows) { - await this.encryptSecretFields(object, r, opCtx.context, hookContext.input.options); - } - for (const r of rows) { - normalizeMultiValueFields(schemaForValidation, r); - validateRecord(schemaForValidation, r, 'insert'); - evaluateValidationRules(schemaForValidation as any, r, 'insert', { logger: this.logger, currentUser: this.buildEvalUser(opCtx.context) }); - } + // Defaults are already resolved above (pre-hook, #2703); a hook may + // have overridden fields or replaced `input.data` — take its data as-is. + const rows = rowHookContexts.map((rowCtx) => rowCtx.input.data as Record); + for (const r of rows) { + await this.applyAutonumbers(object, r, opCtx.context, driverOwnsAutonumber); + } + for (const r of rows) { + await this.encryptSecretFields(object, r, opCtx.context, driverOptions); + } + for (const r of rows) { + normalizeMultiValueFields(schemaForValidation, r); + validateRecord(schemaForValidation, r, 'insert'); + evaluateValidationRules(schemaForValidation as any, r, 'insert', { logger: this.logger, currentUser: this.buildEvalUser(opCtx.context) }); + } + if (isBatch) { if (driver.bulkCreate) { - result = await driver.bulkCreate(object, rows, hookContext.input.options as any); + result = await driver.bulkCreate(object, rows, driverOptions); } else { // Fallback loop - result = await Promise.all(rows.map((item) => driver.create(object, item, hookContext.input.options as any))); + result = await Promise.all(rows.map((item) => driver.create(object, item, driverOptions))); } } else { - // Defaults already resolved pre-hook (#2703); use the hook's data. - const row = hookContext.input.data as Record; - await this.applyAutonumbers(object, row, opCtx.context, driverOwnsAutonumber); - await this.encryptSecretFields(object, row, opCtx.context, hookContext.input.options); - normalizeMultiValueFields(schemaForValidation, row); - validateRecord(schemaForValidation, row, 'insert'); - evaluateValidationRules(schemaForValidation as any, row, 'insert', { logger: this.logger, currentUser: this.buildEvalUser(opCtx.context) }); - result = await driver.create(object, row, hookContext.input.options as any); + result = await driver.create(object, rows[0], driverOptions); } - hookContext.event = 'afterInsert'; // Coerce `boolean` fields (SQLite/libsql return 0/1) to real booleans on // the after-hook view so flow trigger conditions (`record.is_escalated // != true`) and `{record.}` interpolation see JS booleans, not // ints. A shallow copy — the value returned to the caller is untouched. - hookContext.result = Array.isArray(result) - ? result.map((r) => coerceBooleanFields(schemaForValidation as any, r as any)) - : coerceBooleanFields(schemaForValidation as any, result as any); - await this.triggerHooks('afterInsert', hookContext); + const resultRows: any[] = isBatch ? (Array.isArray(result) ? result : [result]) : [result]; + for (let i = 0; i < rowHookContexts.length; i++) { + const rowCtx = rowHookContexts[i]; + rowCtx.event = 'afterInsert'; + rowCtx.result = coerceBooleanFields(schemaForValidation as any, resultRows[i] as any); + await this.triggerHooks('afterInsert', rowCtx); + } // Roll-up: recompute parent summary fields that aggregate this object. await this.recomputeSummaries(object, result, null, opCtx.context); @@ -2285,7 +2315,9 @@ export class ObjectQL implements IDataEngine { } } - return hookContext.result; + // Return the (possibly hook-mutated) after-view: the array of per-row + // results for batch, the single record otherwise. + return isBatch ? rowHookContexts.map((rowCtx) => rowCtx.result) : rowHookContexts[0].result; } catch (e) { this.logger.error('Insert operation failed', e as Error, { object }); throw e; diff --git a/packages/rest/src/import-integration.test.ts b/packages/rest/src/import-integration.test.ts index 51b30ba060..b16f4d4315 100644 --- a/packages/rest/src/import-integration.test.ts +++ b/packages/rest/src/import-integration.test.ts @@ -357,15 +357,19 @@ describe('import route — required-field dry-run fidelity', () => { { id: 'm1', member_name: 'Alice' }, // status missing → must fail { id: 'm2', member_name: 'Bob', status: 'active' }, // complete → ok ]; + // The pre-check only runs with automations OFF (a beforeInsert hook may + // populate a required field, so with automations on we defer to the + // engine's own validation). runAutomations defaults to true since #2922, + // so the opt-out is explicit here. // Dry run: no longer reports success for the row the insert will reject. - const dry = await imp({ format: 'json', dryRun: true, rows }); + const dry = await imp({ format: 'json', dryRun: true, runAutomations: false, rows }); expect(dry._json).toMatchObject({ dryRun: true, total: 2, ok: 1, errors: 1 }); expect(dry._json.results.find((r: any) => !r.ok)).toMatchObject({ row: 1, field: 'status', code: 'required' }); expect(await engine.findOne('member', { where: { id: 'm2' } })).toBeNull(); // dry run never writes // Real insert: SAME verdict (parity), and a readable `status is required` // instead of a raw `NOT NULL constraint failed: member.status`. - const real = await imp({ format: 'json', rows }); + const real = await imp({ format: 'json', runAutomations: false, rows }); expect(real._json).toMatchObject({ total: 2, ok: 1, errors: 1, created: 1 }); expect(real._json.results.find((r: any) => !r.ok)).toMatchObject({ field: 'status', code: 'required', error: 'status is required' }); expect((await engine.findOne('member', { where: { id: 'm2' } }))?.status).toBe('active'); @@ -382,7 +386,7 @@ describe('import route — required-field dry-run fidelity', () => { }); it('flags a required text field too (not just selects); a blank cell counts as missing', async () => { - const res = await imp({ format: 'json', dryRun: true, rows: [ + const res = await imp({ format: 'json', dryRun: true, runAutomations: false, rows: [ { id: 'm4', status: 'active' }, // member_name missing { id: 'm5', member_name: ' ', status: 'active' }, // member_name blank ] }); diff --git a/packages/rest/src/import-prepare.test.ts b/packages/rest/src/import-prepare.test.ts index 2068327720..3b93f01656 100644 --- a/packages/rest/src/import-prepare.test.ts +++ b/packages/rest/src/import-prepare.test.ts @@ -99,3 +99,27 @@ describe('prepareImportRequest — locale-translated option synonyms', () => { expect(matchOption('Backlog', prep.prepared.metaMap.get('status')!.options!)).toBe('backlog'); }); }); + +describe('prepareImportRequest — runAutomations default (#2922)', () => { + const prepWith = async (body: Record) => { + const prep = await prepareImportRequest( + { format: 'json', rows: [{ title: 'a' }], ...body }, + { p, objectName: 'task', maxRows: 10 }, + ); + expect(prep.ok).toBe(true); + return prep.ok ? prep.prepared.runAutomations : undefined; + }; + + it('defaults to true when the flag is omitted (automations always ran historically)', async () => { + expect(await prepWith({})).toBe(true); + }); + + it('honours an explicit opt-out', async () => { + expect(await prepWith({ runAutomations: false })).toBe(false); + }); + + it('treats any non-false value as true', async () => { + expect(await prepWith({ runAutomations: true })).toBe(true); + expect(await prepWith({ runAutomations: 'no' })).toBe(true); + }); +}); diff --git a/packages/rest/src/import-prepare.ts b/packages/rest/src/import-prepare.ts index fa7ed65e3c..843848e4b8 100644 --- a/packages/rest/src/import-prepare.ts +++ b/packages/rest/src/import-prepare.ts @@ -252,7 +252,10 @@ export async function prepareImportRequest( let matchFields: string[] = Array.isArray(body?.matchFields) ? body.matchFields.filter((f: any) => typeof f === 'string' && f.length > 0) : []; - const runAutomations = body?.runAutomations === true; + // Default ON: automations always ran historically (the engine ignored the + // flag until #2922), so opt-out must be explicit — matches platform + // convention (Salesforce runs triggers on import by default). + const runAutomations = body?.runAutomations !== false; const trimWhitespace = body?.trimWhitespace !== false; const nullValues: string[] | undefined = Array.isArray(body?.nullValues) ? body.nullValues.filter((v: any) => typeof v === 'string') diff --git a/packages/spec/src/data/hook.zod.ts b/packages/spec/src/data/hook.zod.ts index 0f848ef36c..2572470c2d 100644 --- a/packages/spec/src/data/hook.zod.ts +++ b/packages/spec/src/data/hook.zod.ts @@ -207,6 +207,7 @@ export const HookContextSchema = lazySchema(() => z.object({ accessToken: z.string().optional(), isSystem: z.boolean().optional().describe('True when the call was made with an elevated system context (engine self-writes)'), skipTriggers: z.boolean().optional().describe('True when record-change automation (flow triggers) must be suppressed for this write — e.g. package seed replay. Lifecycle hooks still run.'), + skipAutomations: z.boolean().optional().describe('True when metadata-bound automation hooks must be suppressed for this write — e.g. data import with "run automations" unchecked, or import undo. Implies skipTriggers; code-registered system hooks (audit, security) still run.'), }).optional().describe('Current session context'), /** diff --git a/packages/spec/src/kernel/execution-context.zod.ts b/packages/spec/src/kernel/execution-context.zod.ts index 2edbeca323..b5853e30ac 100644 --- a/packages/spec/src/kernel/execution-context.zod.ts +++ b/packages/spec/src/kernel/execution-context.zod.ts @@ -167,6 +167,19 @@ export const ExecutionContextSchema = lazySchema(() => z.object({ */ skipTriggers: z.boolean().optional(), + /** + * Suppress metadata-bound AUTOMATION entirely for writes made under this + * context: lifecycle hooks bound from metadata (`bindHooksToEngine`) are + * skipped by the engine's `triggerHooks`, and record-change flow dispatch + * is suppressed too (implies {@link skipTriggers}). Hooks registered in + * code by plugins — audit, capability gates, sharing projection — carry no + * metadata binding and STILL run: this flag must never bypass security or + * audit. Set by the data-import runner when the user opts out of + * "run automations & triggers", and by import undo (reversing writes must + * not re-fire automation). + */ + skipAutomations: z.boolean().optional(), + /** * OAuth 2.1 scopes granted to the access token that authenticated this * request, when the principal was resolved from an OAuth bearer token