From e89c7e811f16aa5db24fe6f1f14ddca8abc457c6 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 08:53:05 +0000 Subject: [PATCH 1/2] fix(automation,objectql): a filter that loses a condition must not run (#3810) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three related holes, all ending in "the query matched rows the author excluded". 1. A FLOW FILTER COULD SILENTLY WIDEN TO MATCH EVERYTHING. The flow template interpolator expresses "this token did not resolve" as `undefined`. In a message that renders as empty text — harmless. In a FILTER it removes the condition, and a removed condition matches MORE rows. When it was the only condition, `{ owner: '{record.ownr}' }` became `{}`, and `{}` handed to `deleteMany` is every row in the table. So one mistyped field name in a `delete_record` node silently emptied the object. Reproduced with all four causes: a typo (`{record.ownr}`), an input the run never received, a lookup hop (`{record.account.name}` — the trigger record carries a scalar id), and a filter placeholder. `get_record` / `update_record` / `delete_record` now refuse to execute when interpolation erased any authored condition, naming the offending template. The guard keys on LOSS, not emptiness: an author who deliberately wrote no filter is unaffected, and losing one of two conditions still fails, because widening from "my open records" to "all open records" is the same class of bug. 2. FILTER PLACEHOLDERS NEVER REACHED THE ENGINE THAT RESOLVES THEM. `config.filter` is where two `{…}` dialects meet — the flow template dialect (`{record.owner}`) and the filter placeholder dialect (`{current_year_start}`, `{current_user_id}`, resolved by `resolveFilterTokens()`). Evaluation order picked the winner by accident: the flow interpolator ran first, found no flow variable by that name, and erased it. `interpolateFilter()` hands that position back to the dialect that owns it — a whole-string token that no flow variable resolves and that IS a recognised placeholder passes through verbatim for the engine to expand. Flow variables keep precedence, so a template that works today cannot change meaning. 3. THE ENGINE RESOLVED PLACEHOLDERS ON READS BUT NOT ON WRITES. `resolveFilterTokens()` reached find/findOne/count/aggregate only. So the SAME filter selected different rows depending on the verb: `find({owner: '{current_user_id}'})` matched the signed-in user's rows while `update`/`delete` compared the literal token text and matched none — a flow that previewed with one and acted with the other operated on two different row sets. The #3106 shape one layer down: the evaluator existed, only some call sites reached it. `update` and `delete` now resolve too, BEFORE the by-id fast path claims a scalar `where.id` (otherwise an unresolved `{current_user_id}` would be bound as the primary key itself). Caller options are never mutated. Docs corrected: the caveat added in #3582 saying flow filters cannot use tokens is now wrong, and the build lint's "resolves to an empty string silently" no longer describes filter positions. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EdCvGtER9SzJopS4Yh3Pa1 --- ...w-filter-collapse-and-write-path-tokens.md | 57 +++++ .../lint/src/validate-flow-template-paths.ts | 8 + .../objectql/src/engine-filter-tokens.test.ts | 86 +++++++- packages/objectql/src/engine.ts | 38 +++- .../src/builtin/crud-filter-guard.test.ts | 206 ++++++++++++++++++ .../src/builtin/crud-nodes.ts | 107 ++++++++- .../src/builtin/template.ts | 57 +++++ packages/spec/src/data/context-tokens.zod.ts | 11 +- packages/spec/src/data/date-macros.zod.ts | 14 +- skills/objectstack-query/rules/filters.md | 27 ++- 10 files changed, 586 insertions(+), 25 deletions(-) create mode 100644 .changeset/flow-filter-collapse-and-write-path-tokens.md create mode 100644 packages/services/service-automation/src/builtin/crud-filter-guard.test.ts diff --git a/.changeset/flow-filter-collapse-and-write-path-tokens.md b/.changeset/flow-filter-collapse-and-write-path-tokens.md new file mode 100644 index 0000000000..706695e0fd --- /dev/null +++ b/.changeset/flow-filter-collapse-and-write-path-tokens.md @@ -0,0 +1,57 @@ +--- +"@objectstack/objectql": minor +"@objectstack/service-automation": minor +"@objectstack/spec": patch +"@objectstack/lint": patch +--- + +fix(automation,objectql): a filter that loses a condition must not run (#3810) + +Three related holes, all of which end in "the query matched rows the author +excluded". + +**1. A flow filter could silently widen to match everything.** + +The flow template interpolator expresses "this token did not resolve" as +`undefined`. In a message that renders as empty text — harmless. In a FILTER it +removes the condition, and a removed condition matches MORE rows. When it was +the only condition, `{ owner: '{record.ownr}' }` became `{}`, and `{}` handed to +`deleteMany` is every row in the table. + +So one mistyped field name in a `delete_record` node silently emptied the +object. Reproduced with all four causes: a typo (`{record.ownr}`), an input the +run never received, a lookup hop (`{record.account.name}` — the trigger record +carries a scalar id), and a filter placeholder. + +`get_record` / `update_record` / `delete_record` now refuse to execute when +interpolation erased any authored condition, naming the offending template. The +guard keys on LOSS, not emptiness: an author who deliberately wrote no filter is +unaffected, and losing one of two conditions still fails, because widening from +"my open records" to "all open records" is the same class of bug. + +**2. Filter placeholders never reached the engine that resolves them.** + +`config.filter` is where two `{…}` dialects meet — the flow template dialect +(`{record.owner}`) and the filter placeholder dialect (`{current_year_start}`, +`{current_user_id}`, resolved by `resolveFilterTokens()`). Evaluation order +picked the winner by accident: the flow interpolator ran first, found no flow +variable by that name, and erased it. + +`interpolateFilter()` hands that position back to the dialect that owns it — a +whole-string token that no flow variable resolves and that IS a recognised +placeholder passes through verbatim for the engine to expand. Flow variables +keep precedence, so a template that works today cannot change meaning. + +**3. The engine resolved placeholders on reads but not on writes.** + +`resolveFilterTokens()` reached `find`/`findOne`/`count`/`aggregate` only. So +the SAME filter selected different rows depending on the verb: `find({ owner: +'{current_user_id}' })` matched the signed-in user's rows, while +`update`/`delete` compared the literal token text and matched none — a flow that +previewed with one and acted with the other operated on two different row sets. +This is the #3106 shape one layer down: the evaluator existed, only some call +sites reached it. + +`update` and `delete` now resolve too, BEFORE the by-id fast path claims a +scalar `where.id` (otherwise an unresolved `{current_user_id}` would be bound as +the primary key itself). Caller options are never mutated. diff --git a/packages/lint/src/validate-flow-template-paths.ts b/packages/lint/src/validate-flow-template-paths.ts index 32c89a7418..52e841c42c 100644 --- a/packages/lint/src/validate-flow-template-paths.ts +++ b/packages/lint/src/validate-flow-template-paths.ts @@ -24,6 +24,14 @@ // produces output (a blank), nothing is fully broken, and the head object may // legitimately come from another installed package (skipped — see below). // +// One position is no longer merely blank at run time: inside a CRUD node's +// `config.filter`, an unresolved token used to DELETE the condition from the +// query, which widens it — `delete_record` with its only condition gone matched +// every row. Since framework#3810 those nodes refuse to execute instead. This +// rule still earns its place there: catching the typo at build time beats a +// failed run, and it is the only signal for the other config blocks, where the +// blank-output behaviour is unchanged. +// // Deliberately conservative to keep false positives near zero: // - Only `record.`-prefixed tokens are checked. Other `{var}` tokens address // flow variables / node outputs the rule cannot resolve statically. diff --git a/packages/objectql/src/engine-filter-tokens.test.ts b/packages/objectql/src/engine-filter-tokens.test.ts index 7816997458..1cd2c08185 100644 --- a/packages/objectql/src/engine-filter-tokens.test.ts +++ b/packages/objectql/src/engine-filter-tokens.test.ts @@ -53,7 +53,10 @@ const DEAL_SCHEMA = { }; function makeDriver() { - const seen: { findAst?: any; findOneAst?: any; countAst?: any; aggregateAst?: any } = {}; + const seen: { + findAst?: any; findOneAst?: any; countAst?: any; aggregateAst?: any; + updateManyAst?: any; deleteManyAst?: any; updateId?: any; deleteId?: any; + } = {}; const driver: any = { name: 'memory', supports: {}, @@ -63,9 +66,11 @@ function makeDriver() { findOne: vi.fn(async (_o: string, ast: any) => { seen.findOneAst = ast; return null; }), count: vi.fn(async (_o: string, ast: any) => { seen.countAst = ast; return 0; }), aggregate: vi.fn(async (_o: string, ast: any) => { seen.aggregateAst = ast; return []; }), - create: vi.fn(), - update: vi.fn(), - delete: vi.fn(), + create: vi.fn(async (_o: string, d: any) => d), + update: vi.fn(async (_o: string, id: any, d: any) => { seen.updateId = id; return { id, ...d }; }), + updateMany: vi.fn(async (_o: string, ast: any) => { seen.updateManyAst = ast; return { modified: 0 }; }), + delete: vi.fn(async (_o: string, id: any) => { seen.deleteId = id; return true; }), + deleteMany: vi.fn(async (_o: string, ast: any) => { seen.deleteManyAst = ast; return { deleted: 0 }; }), }; return { driver, seen }; } @@ -175,6 +180,79 @@ describe('engine filter placeholders (framework#3582)', () => { expect(seen.findAst?.where).toEqual({ title: 'acme {x} deal', owner: 'usr_2' }); }); + // ── Write path (framework#3810) ──────────────────────────────────────── + // The evaluator originally reached only find/findOne/count/aggregate, so the + // SAME filter selected different rows depending on the verb: `find` matched + // the signed-in user's rows while `update`/`delete` compared the literal + // token text and matched none. #3106 one layer down — the switch was right, + // the call sites were incomplete. + describe('write path', () => { + it('updateMany: the driver receives the resolved filter, not the token', async () => { + const { driver, seen } = makeDriver(); + const ql = await makeEngine(driver); + + await ql.update('deal', { title: 'x' }, { + where: { owner: '{current_user_id}' }, multi: true, context: CTX, + } as any); + + expect(seen.updateManyAst?.where).toEqual({ owner: 'usr_1' }); + }); + + it('deleteMany: the driver receives the resolved filter, not the token', async () => { + const { driver, seen } = makeDriver(); + const ql = await makeEngine(driver); + + await ql.delete('deal', { + where: { close_date: { $lt: '{current_year_start}' } }, multi: true, context: CTX, + } as any); + + expect(seen.deleteManyAst?.where).toEqual({ close_date: { $lt: THIS_YEAR_START } }); + }); + + it('resolves BEFORE the by-id fast path claims a scalar where.id', async () => { + // Ordering regression: the token would otherwise be bound as the primary + // key itself (`WHERE id = '{current_user_id}'`). + const { driver, seen } = makeDriver(); + const ql = await makeEngine(driver); + + await ql.update('deal', { title: 'x' }, { where: { id: '{current_user_id}' }, context: CTX } as any); + + expect(seen.updateId).toBe('usr_1'); + }); + + it('read and write agree on the same filter', async () => { + const { driver, seen } = makeDriver(); + const ql = await makeEngine(driver); + const filter = { owner: '{current_user_id}' }; + + await ql.find('deal', { where: filter, context: CTX }); + await ql.update('deal', { title: 'x' }, { where: filter, multi: true, context: CTX } as any); + + expect(seen.updateManyAst?.where).toEqual(seen.findAst?.where); + }); + + it('an unknown placeholder throws before anything is written', async () => { + const { driver } = makeDriver(); + const ql = await makeEngine(driver); + + await expect( + ql.delete('deal', { where: { owner: '{current_user}' }, multi: true, context: CTX } as any), + ).rejects.toThrow(/current_user_id/); + expect(driver.deleteMany).not.toHaveBeenCalled(); + expect(driver.delete).not.toHaveBeenCalled(); + }); + + it('does not mutate the caller options — flow node config is reused', async () => { + const { driver } = makeDriver(); + const ql = await makeEngine(driver); + const options: any = { where: { owner: '{current_user_id}' }, multi: true, context: CTX }; + + await ql.update('deal', { title: 'x' }, options); + + expect(options.where).toEqual({ owner: '{current_user_id}' }); + }); + }); + it('does not mutate the caller filter — view metadata is shared across requests', async () => { const { driver } = makeDriver(); const ql = await makeEngine(driver); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 403da15d28..33270f94fc 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -2556,6 +2556,25 @@ export class ObjectQL implements IDataEngine { ast.where = resolveFilterTokens(ast.where, filterTokenContextFrom(execCtx)); } + /** + * The write-path counterpart of {@link resolveWhereTokens}: return `options` + * with `where` placeholders expanded (#3810). + * + * Returns the SAME object when nothing resolved — `resolveFilterTokens` + * returns its input by reference on a placeholder-free tree, so the common + * path allocates nothing. When something does resolve, a shallow copy is + * made rather than assigning through: `options` belongs to the caller, and + * writing back would bake one request's user id into a filter object the + * caller may reuse (view metadata and flow node config both get reused). + */ + private withResolvedWhere( + options: T, + ): T { + if (!options || options.where == null) return options; + const resolved = resolveFilterTokens(options.where, filterTokenContextFrom(options.context)); + return resolved === options.where ? options : ({ ...options, where: resolved } as T); + } + async find(object: string, query?: EngineQueryOptions, options?: EngineReadOptions): Promise { object = this.resolveObjectName(object); this.logger.debug('Find operation starting', { object, query }); @@ -3067,7 +3086,20 @@ export class ObjectQL implements IDataEngine { this.logger.debug('Update operation starting', { object }); this.assertWriteAllowed(object, 'update'); const driver = this.getDriver(object); - + + // Expand `{filter-placeholder}` values BEFORE the id is extracted (#3810). + // The read path resolves them; without the same call here the SAME filter + // selected different rows depending on the verb — `find({owner: + // '{current_user_id}'})` matched the signed-in user's rows while + // `update`/`delete` compared the literal token text and matched none. That + // is the #3106 shape one layer down: the evaluator existed, but only some + // call sites reached it. + // + // Ordering matters: a scalar `where.id` becomes the by-id fast path below, + // so an unresolved `{current_user_id}` would be bound as the primary key + // itself. Resolve first, then extract. + options = this.withResolvedWhere(options); + // 1. Extract ID from data or where if it's a single update by ID. // Only a SCALAR `where.id` means "update one row by primary key". An // operator object ({ $in: [...] }, { $ne: ... }, …) is a multi-row @@ -3446,6 +3478,10 @@ export class ObjectQL implements IDataEngine { this.assertWriteAllowed(object, 'delete'); const driver = this.getDriver(object); + // Expand `{filter-placeholder}` values before the id is extracted — same + // reasoning as update() above (#3810). + options = this.withResolvedWhere(options); + // Extract ID logic mirroring update(): only a SCALAR `where.id` means // "delete one row by primary key". An operator object ({ $in: [...] }, …) // is a multi-row predicate — treating it as an id would bind the object diff --git a/packages/services/service-automation/src/builtin/crud-filter-guard.test.ts b/packages/services/service-automation/src/builtin/crud-filter-guard.test.ts new file mode 100644 index 0000000000..cfe292cb2b --- /dev/null +++ b/packages/services/service-automation/src/builtin/crud-filter-guard.test.ts @@ -0,0 +1,206 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect, beforeEach } from 'vitest'; +import { AutomationEngine } from '../engine.js'; +import { registerCrudNodes } from './crud-nodes.js'; +import { interpolateFilter } from './template.js'; + +/** + * framework#3810 — a flow filter must never silently widen. + * + * The flow interpolator expresses "this token did not resolve" as `undefined`. + * Everywhere else that is harmless; in a FILTER it removes a condition, and a + * removed condition matches MORE rows. When it was the only condition, + * `{ owner: '{record.ownr}' }` became `{}` — and `{}` handed to `deleteMany` is + * every row in the table. One mistyped field name emptied the object, silently. + * + * Two independent fixes are covered here: + * 1. Filter placeholders (`{current_user_id}`, `{current_year_start}`) are + * passed through to the query engine, which owns that dialect. + * 2. Anything else that fails to resolve — a typo, a missing input, a lookup + * hop — refuses the node instead of executing a widened query. + */ + +function createTestLogger(): any { + return { info: () => {}, warn: () => {}, error: () => {}, debug: () => {}, child: () => createTestLogger() }; +} + +/** A data engine stub that records the `where` each verb actually received. */ +function fakeData() { + const seen: { find?: any; update?: any; delete?: any } = {}; + const service = { + find: async (_o: string, q: any) => { seen.find = q.where; return []; }, + findOne: async (_o: string, q: any) => { seen.find = q.where; return null; }, + update: async (_o: string, _d: any, o: any) => { seen.update = o?.where; return { modified: 1 }; }, + delete: async (_o: string, o: any) => { seen.delete = o?.where; return { deleted: 1 }; }, + getObject: () => ({ name: 'deal', fields: {} }), + }; + return { service, seen }; +} + +function ctxWith(data: any): any { + return { + logger: createTestLogger(), + getService(name: string) { + if (name === 'data') return data; + return undefined; + }, + }; +} + +function crudFlow(nodeType: string, config: Record) { + return { + name: 'crud_flow', + label: 'CRUD Flow', + type: 'autolaunched' as const, + // Explicit so the ADR-0049 runAs gate (#3760) does not reject the data + // op first — these tests are about the filter guard, and a refusal for + // the wrong reason would make the negative cases pass vacuously. + runAs: 'system' as const, + nodes: [ + { id: 'start', type: 'start' as const, label: 'Start' }, + { id: 'op', type: nodeType as any, label: 'Op', config }, + { id: 'end', type: 'end' as const, label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'op' }, + { id: 'e2', source: 'op', target: 'end' }, + ], + }; +} + +describe('interpolateFilter — filter placeholders are handed to the engine (#3810)', () => { + const vars = new Map([['record', { id: 'r1', owner: 'usr_7' }]]); + const ctx = { userId: 'usr_1' } as any; + + it('passes a date macro through verbatim', () => { + expect(interpolateFilter({ close_date: { $gte: '{current_year_start}' } }, vars, ctx)) + .toEqual({ close_date: { $gte: '{current_year_start}' } }); + }); + + it('passes a parameterised macro and a context token through verbatim', () => { + expect(interpolateFilter({ a: '{30_days_ago}', b: '{current_user_id}' }, vars, ctx)) + .toEqual({ a: '{30_days_ago}', b: '{current_user_id}' }); + }); + + it('still resolves flow variables', () => { + expect(interpolateFilter({ owner: '{record.owner}' }, vars, ctx)).toEqual({ owner: 'usr_7' }); + }); + + it('mixes both dialects in one filter', () => { + expect( + interpolateFilter({ owner: '{record.owner}', close_date: { $gte: '{current_year_start}' } }, vars, ctx), + ).toEqual({ owner: 'usr_7', close_date: { $gte: '{current_year_start}' } }); + }); + + it('a flow variable still shadows a same-named placeholder', () => { + // Precedence guard: a template that resolves today must not start + // meaning something else. + const shadowed = new Map([['current_user_id', 'from_flow_var']]); + expect(interpolateFilter({ owner: '{current_user_id}' }, shadowed, ctx)) + .toEqual({ owner: 'from_flow_var' }); + }); + + it('leaves a non-placeholder unresolved token as undefined for the guard to catch', () => { + expect(interpolateFilter({ owner: '{record.ownr}' }, vars, ctx)).toEqual({ owner: undefined }); + }); +}); + +describe('crud nodes — refuse a filter that lost a condition (#3810)', () => { + let engine: AutomationEngine; + let data: ReturnType; + + beforeEach(() => { + data = fakeData(); + engine = new AutomationEngine(createTestLogger()); + registerCrudNodes(engine, ctxWith(data.service)); + }); + + it('delete_record: a typo\'d field name refuses instead of emptying the object', async () => { + engine.registerFlow('crud_flow', crudFlow('delete_record', { + objectName: 'deal', + filter: { owner: '{record.ownr}' }, + })); + + const result = await engine.execute('crud_flow', { record: { id: 'r1', owner: 'usr_7' } } as any); + + expect(result.success).toBe(false); + expect(result.error).toContain('{record.ownr}'); + expect(result.error).toMatch(/deleted it|every remaining row/); + expect(data.seen.delete).toBeUndefined(); + }); + + it('update_record: a missing input variable refuses instead of overwriting everything', async () => { + engine.registerFlow('crud_flow', crudFlow('update_record', { + objectName: 'deal', + filter: { owner: '{someInputNeverSet}' }, + fields: { title: 'x' }, + })); + + const result = await engine.execute('crud_flow'); + + expect(result.success).toBe(false); + expect(result.error).toContain('{someInputNeverSet}'); + expect(data.seen.update).toBeUndefined(); + }); + + it('refuses when only SOME conditions are lost — a partial filter still widens', async () => { + engine.registerFlow('crud_flow', crudFlow('delete_record', { + objectName: 'deal', + filter: { status: 'open', owner: '{record.ownr}' }, + })); + + const result = await engine.execute('crud_flow', { record: { id: 'r1', owner: 'usr_7' } } as any); + + expect(result.success).toBe(false); + expect(data.seen.delete).toBeUndefined(); + }); + + it('names the lookup hop and points at config.expand', async () => { + engine.registerFlow('crud_flow', crudFlow('delete_record', { + objectName: 'deal', + filter: { owner: '{record.account.name}' }, + })); + + const result = await engine.execute('crud_flow', { record: { id: 'r1', account: 'acc_1' } } as any); + + expect(result.success).toBe(false); + expect(result.error).toContain('expand'); + }); + + it('a filter placeholder is NOT treated as erased — it reaches the engine', async () => { + engine.registerFlow('crud_flow', crudFlow('delete_record', { + objectName: 'deal', + filter: { close_date: { $lt: '{current_year_start}' } }, + })); + + const result = await engine.execute('crud_flow', { record: { id: 'r1' } } as any); + + expect(result.success).toBe(true); + // Handed on verbatim; the query engine owns the expansion. + expect(data.seen.delete).toEqual({ close_date: { $lt: '{current_year_start}' } }); + }); + + it('a fully-resolvable filter still runs untouched', async () => { + engine.registerFlow('crud_flow', crudFlow('delete_record', { + objectName: 'deal', + filter: { owner: '{record.owner}' }, + })); + + const result = await engine.execute('crud_flow', { record: { id: 'r1', owner: 'usr_7' } } as any); + + expect(result.success).toBe(true); + expect(data.seen.delete).toEqual({ owner: 'usr_7' }); + }); + + it('an intentionally empty filter is still allowed (nothing was erased)', async () => { + // "Delete everything" must stay expressible — the guard fires on LOSS, + // not on emptiness, so an author who wrote no filter is unaffected. + engine.registerFlow('crud_flow', crudFlow('delete_record', { objectName: 'deal', filter: {} })); + + const result = await engine.execute('crud_flow'); + + expect(result.success).toBe(true); + expect(data.seen.delete).toEqual({}); + }); +}); diff --git a/packages/services/service-automation/src/builtin/crud-nodes.ts b/packages/services/service-automation/src/builtin/crud-nodes.ts index 19a588e771..dad7e02628 100644 --- a/packages/services/service-automation/src/builtin/crud-nodes.ts +++ b/packages/services/service-automation/src/builtin/crud-nodes.ts @@ -5,10 +5,92 @@ import { defineActionDescriptor } from '@objectstack/spec/automation'; import type { IDataEngine } from '@objectstack/spec/contracts'; import type { DroppedFieldsEvent } from '@objectstack/spec/data'; import type { AutomationEngine } from '../engine.js'; -import { interpolate } from './template.js'; +import { interpolate, interpolateFilter, type VariableMap } from './template.js'; import { readAliasedConfig } from './config-aliases.js'; import { resolveRunDataContext } from '../runtime-identity.js'; +/** + * A filter condition that an author WROTE but that interpolation erased + * (framework#3810). + * + * The flow interpolator expresses "this token did not resolve" as `undefined`. + * In every other config block that is harmless — an unresolved `{x}` in a + * message renders as empty text. In a FILTER it is the opposite of harmless: + * a condition whose value is `undefined` is not a narrower query, it is an + * ABSENT one, and an absent condition matches MORE rows. When the erased + * condition was the only one, `{ owner: '{record.ownr}' }` becomes `{}` — and + * `{}` handed to `deleteMany` is every row in the table. + * + * So a single mistyped field name in a `delete_record` node silently emptied + * the object. Not a hypothetical: `{record.ownr}` (typo), `{someInput}` (an + * input the run did not receive) and `{record.account.name}` (a lookup hop — + * the trigger record carries a scalar id) all reach this state, and none of + * them produced a diagnostic anywhere. + * + * The guard below refuses to execute such a node. It is deliberately keyed on + * "a condition the author wrote is gone", not on "the filter is empty": losing + * ONE of two conditions still silently widens the blast radius from "my open + * records" to "all open records". + */ +function erasedFilterConditions( + before: unknown, + after: unknown, + path: string[] = [], +): Array<{ path: string; template: string }> { + const out: Array<{ path: string; template: string }> = []; + const at = (p: string[]) => p.join('.') || '(root)'; + + if (typeof before === 'string') { + if (after === undefined && before.includes('{')) { + out.push({ path: at(path), template: before }); + } + return out; + } + if (Array.isArray(before)) { + before.forEach((v, i) => + out.push(...erasedFilterConditions(v, (after as unknown[] | undefined)?.[i], [...path, String(i)])), + ); + return out; + } + if (before && typeof before === 'object') { + const afterRec = (after ?? {}) as Record; + for (const [k, v] of Object.entries(before as Record)) { + out.push(...erasedFilterConditions(v, afterRec[k], [...path, k])); + } + } + return out; +} + +/** + * Interpolate a node's filter and refuse the node if any authored condition was + * erased. Returns either the usable filter or a ready-to-return failure. + * + * `verb` names the operation in the error so the message says what WOULD have + * happened ("would have matched every row and deleted it"). + */ +function resolveNodeFilter( + rawFilter: unknown, + variables: VariableMap, + context: Parameters[2], + nodeType: string, + consequence: string, +): { filter: Record } | { error: string } { + const filter = interpolateFilter(rawFilter ?? {}, variables, context) as Record; + const erased = erasedFilterConditions(rawFilter ?? {}, filter); + if (erased.length > 0) { + const detail = erased.map((e) => `\`${e.template}\` (at ${e.path})`).join(', '); + return { + error: + `${nodeType}: refusing to run — ${erased.length} filter condition(s) resolved to nothing ` + + `and were dropped from the query: ${detail}. An absent condition does not narrow a query, ` + + `it widens it, so this ${consequence}. Check the field name, confirm the flow variable is ` + + `set on this run, and note that a relation field holds a scalar id — a \`{record..}\` ` + + `hop needs the relation in the start node's \`config.expand\`.`, + }; + } + return { filter }; +} + /** * #3407 — render a data-layer strip event as a step warning. The write itself * SUCCEEDED; the warning tells the flow author which requested fields never @@ -84,7 +166,12 @@ export function registerCrudNodes(engine: AutomationEngine, ctx: PluginContext): // `filters` → `filter` is now handled at load by the ADR-0087 D2 // conversion layer ('flow-node-crud-filter-alias'), so the executor // reads the canonical key directly (PD #12 fallback retired). - const filter = interpolate(cfg.filter ?? {}, variables, context) as Record; + const filterResult = resolveNodeFilter( + cfg.filter, variables, context, 'get_record', + 'would have read rows the filter was written to exclude', + ); + if ('error' in filterResult) return { success: false, error: filterResult.error }; + const filter = filterResult.filter; const fields = cfg.fields as string[] | undefined; const limit = typeof cfg.limit === 'number' ? cfg.limit : undefined; const outputVariable = cfg.outputVariable as string | undefined; @@ -219,7 +306,12 @@ export function registerCrudNodes(engine: AutomationEngine, ctx: PluginContext): if (!objectName) return { success: false, error: 'update_record: objectName required' }; // `filters` → `filter` converted at load (ADR-0087 D2); read canonical. - const filter = interpolate(cfg.filter ?? {}, variables, context) as Record; + const filterResult = resolveNodeFilter( + cfg.filter, variables, context, 'update_record', + 'would have matched — and overwritten — rows the filter was written to exclude', + ); + if ('error' in filterResult) return { success: false, error: filterResult.error }; + const filter = filterResult.filter; // `fields` is the single canonical write-map key — no alias (the wrong key // `fieldValues` is corrected at the authoring source + rejected by graph-lint). const fields = interpolate(cfg.fields ?? {}, variables, context) as Record; @@ -287,7 +379,14 @@ export function registerCrudNodes(engine: AutomationEngine, ctx: PluginContext): if (!objectName) return { success: false, error: 'delete_record: objectName required' }; // `filters` → `filter` converted at load (ADR-0087 D2); read canonical. - const filter = interpolate(cfg.filter ?? {}, variables, context) as Record; + // The highest-stakes of the three: an erased condition here is the + // difference between deleting one row and emptying the object. + const filterResult = resolveNodeFilter( + cfg.filter, variables, context, 'delete_record', + 'would have matched every remaining row and deleted it', + ); + if ('error' in filterResult) return { success: false, error: filterResult.error }; + const filter = filterResult.filter; const data = getData(); if (!data) return { success: true }; diff --git a/packages/services/service-automation/src/builtin/template.ts b/packages/services/service-automation/src/builtin/template.ts index 142b8d5b72..d23cae846b 100644 --- a/packages/services/service-automation/src/builtin/template.ts +++ b/packages/services/service-automation/src/builtin/template.ts @@ -23,6 +23,7 @@ */ import type { AutomationContext } from '@objectstack/spec/contracts'; +import { isKnownFilterToken } from '@objectstack/spec/data'; export type VariableMap = Map; @@ -188,3 +189,59 @@ export function interpolate( } return value; } + +/** + * Interpolate a node's **filter** block (framework#3810). + * + * A filter value position is the one place where two `{…}` dialects meet: the + * flow template dialect (`{record.owner}`, `{$User.Id}`) and the filter + * placeholder dialect (`{current_user_id}`, `{current_year_start}` — declared + * in `@objectstack/spec` and resolved by `resolveFilterTokens()` in the query + * engine). Evaluation order decided the winner by accident: the flow + * interpolator ran first, found no flow variable named `current_year_start`, + * and returned `undefined` — so the placeholder never reached the engine that + * knows how to resolve it, and the condition silently vanished from the query. + * + * This hands that position back to the dialect that owns it. A whole-string + * token that (a) no flow variable resolves and (b) IS a recognised filter + * placeholder is passed through **verbatim** for the engine to expand. That is + * a transfer of ownership, not a lenient fallback: flow variables still win + * when both could match, and a token belonging to neither dialect is left to + * the caller's collapse guard to report. + * + * Only filter blocks use this. Everywhere else (`title`, `message`, `fields`, + * `url`) keeps plain {@link interpolate}, where a bare `{current_year_start}` + * is a nonsense reference rather than a query bound. + */ +export function interpolateFilter( + value: T, + variables: VariableMap, + context: AutomationContext, +): T { + if (typeof value === 'string') { + const single = /^\{([^{}]+)\}$/.exec(value); + if (single) { + const resolved = resolveToken(single[1], variables, context); + // Flow variables keep precedence — only an unresolved token is + // considered for hand-off, so a flow variable that happens to share + // a placeholder's name still shadows it (no silent reinterpretation + // of a template that works today). + if (resolved === undefined && isKnownFilterToken(single[1].trim())) { + return value; + } + return resolved as unknown as T; + } + return interpolateString(value, variables, context) as unknown as T; + } + if (Array.isArray(value)) { + return value.map(v => interpolateFilter(v, variables, context)) as unknown as T; + } + if (value && typeof value === 'object') { + const out: Record = {}; + for (const [k, v] of Object.entries(value as Record)) { + out[k] = interpolateFilter(v, variables, context); + } + return out as unknown as T; + } + return value; +} diff --git a/packages/spec/src/data/context-tokens.zod.ts b/packages/spec/src/data/context-tokens.zod.ts index e7bf24db64..4208a3c237 100644 --- a/packages/spec/src/data/context-tokens.zod.ts +++ b/packages/spec/src/data/context-tokens.zod.ts @@ -20,9 +20,14 @@ import { DATE_MACRO_WRAPPED_RE, isDateMacroToken } from './date-macros.zod.js'; * the wire (framework#3582): `resolveContextTokens()` in * `@object-ui/core` before the filter leaves the browser, and * `resolveFilterTokens()` in `@objectstack/core` on the ObjectQL read - * path and the analytics dataset executor for filters that reach the - * database without passing through a renderer. The DRIVER only ever - * sees concrete ids, never `{tokens}`. + * AND write paths and the analytics dataset executor, for filters that + * reach the database without passing through a renderer. The DRIVER + * only ever sees concrete ids, never `{tokens}`. + * + * The write verbs matter as much as the read ones (#3810): a filter has + * to select the same rows whether `find`, `update` or `delete` consumes + * it, or a flow that previews with one and acts with the other operates + * on two different row sets. * * The server resolver reads `ExecutionContext` — `{current_user_id}` is * `userId`, `{current_org_id}` is `tenantId`. A request that carries diff --git a/packages/spec/src/data/date-macros.zod.ts b/packages/spec/src/data/date-macros.zod.ts index 5c071cff24..5f8e9c3f08 100644 --- a/packages/spec/src/data/date-macros.zod.ts +++ b/packages/spec/src/data/date-macros.zod.ts @@ -21,11 +21,15 @@ import { z } from 'zod'; * - **Client** — `resolveDateMacros()` in `@object-ui/core`, just * before the filter is handed to the data source. * - **Server** — `resolveFilterTokens()` in `@objectstack/core`, wired - * into the ObjectQL read path (`find`/`findOne`/`count`/`aggregate`) - * and the analytics dataset executor. Filters that reach the database - * WITHOUT passing through a renderer — dashboard widgets, dataset - * definitions, REST query params — need this: before it, the token - * compared as a literal string and matched nothing. + * into the ObjectQL read AND write paths (`find`/`findOne`/`count`/ + * `aggregate`/`update`/`delete`) and the analytics dataset executor. + * Filters that reach the database WITHOUT passing through a renderer — + * dashboard widgets, dataset definitions, REST query params, flow node + * filters — need this: before it, the token compared as a literal + * string and matched nothing. The write verbs are covered for the same + * reason (#3810): one filter must select one row set regardless of + * which verb consumes it, or a flow's `find` preview and its + * `update` act on different rows. * * Either way the DRIVER only ever sees ISO date / timestamp strings, * never `{tokens}`. Translating an ISO comparand into a column's on-disk diff --git a/skills/objectstack-query/rules/filters.md b/skills/objectstack-query/rules/filters.md index 212f1bde40..1c59687a78 100644 --- a/skills/objectstack-query/rules/filters.md +++ b/skills/objectstack-query/rules/filters.md @@ -253,14 +253,25 @@ where: { owner: '{current_user_id}', close_date: { $gte: '{current_year_start}' filter behaves the same wherever it runs — client-side by `resolveDateMacros()` / `resolveContextTokens()` in `@object-ui/core`, and server-side by `resolveFilterTokens()` in `@objectstack/core` (wired into the ObjectQL read -path and the analytics dataset executor). The driver only ever sees ISO -date/timestamp strings and concrete ids, never `{tokens}`. You may therefore -use tokens in a query issued directly against the engine — and you should: -computing "today" at module load freezes the date into the built artifact. - -One exception: a **flow node's** `config.filter` is interpolated by the flow -template engine first, which owns `{…}` in that position and blanks anything -that is not a flow variable. Compute the bound in an earlier node instead. +AND write paths — `find`/`findOne`/`count`/`aggregate`/`update`/`delete` — plus +the analytics dataset executor). The driver only ever sees ISO date/timestamp +strings and concrete ids, never `{tokens}`. You may therefore use tokens in a +query issued directly against the engine — and you should: computing "today" at +module load freezes the date into the built artifact. + +This includes a **flow node's** `config.filter`. The flow template engine runs +first there, but it hands a recognised filter placeholder through untouched for +the engine to expand. Flow variables still win — `{record.owner}` resolves as +always, and a flow variable named after a placeholder shadows it. + +**A flow filter that loses a condition refuses to run.** In a filter, a token +that resolves to nothing does not narrow the query — it removes the condition, +which matches *more* rows, and a `delete_record` with every condition gone +means the whole object. So `get_record` / `update_record` / `delete_record` +fail the step, naming the offending template, rather than executing a widened +query. That covers a mistyped field (`{record.ownr}`), an input the run never +received, and a lookup hop (`{record.account.name}` — the trigger record +carries a scalar id; add the relation to the start node's `config.expand`). **Unknown tokens are rejected, not ignored.** `{current_user}` (the RLS expression root) and `{this_quarter_start}` are near-misses, not tokens: From 89a5d71e9e0f119077b547c9d8c5eba3ffc1f62c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 09:04:31 +0000 Subject: [PATCH 2/2] chore(spec): regenerate the references for the #3810 doc edits `content/docs/references/` is generated from the spec TSDoc; the resolver description in `date-macros.zod.ts` / `context-tokens.zod.ts` changed in the previous commit without the regenerate step, so `check:docs` failed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EdCvGtER9SzJopS4Yh3Pa1 --- .../docs/references/data/context-tokens.mdx | 14 +++++++++++--- content/docs/references/data/date-macros.mdx | 18 +++++++++++++----- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/content/docs/references/data/context-tokens.mdx b/content/docs/references/data/context-tokens.mdx index 957a1e3057..3f2d292007 100644 --- a/content/docs/references/data/context-tokens.mdx +++ b/content/docs/references/data/context-tokens.mdx @@ -29,11 +29,19 @@ the wire (framework#3582): `resolveContextTokens()` in `resolveFilterTokens()` in `@objectstack/core` on the ObjectQL read -path and the analytics dataset executor for filters that reach the +AND write paths and the analytics dataset executor, for filters that -database without passing through a renderer. The DRIVER only ever +reach the database without passing through a renderer. The DRIVER -sees concrete ids, never `\{tokens\}`. +only ever sees concrete ids, never `\{tokens\}`. + +The write verbs matter as much as the read ones (#3810): a filter has + +to select the same rows whether `find`, `update` or `delete` consumes + +it, or a flow that previews with one and acts with the other operates + +on two different row sets. The server resolver reads `ExecutionContext` — `\{current_user_id\}` is diff --git a/content/docs/references/data/date-macros.mdx b/content/docs/references/data/date-macros.mdx index b1f68800a1..938aeb2506 100644 --- a/content/docs/references/data/date-macros.mdx +++ b/content/docs/references/data/date-macros.mdx @@ -31,15 +31,23 @@ before the filter is handed to the data source. - **Server** — `resolveFilterTokens()` in `@objectstack/core`, wired -into the ObjectQL read path (`find`/`findOne`/`count`/`aggregate`) +into the ObjectQL read AND write paths (`find`/`findOne`/`count`/ -and the analytics dataset executor. Filters that reach the database +`aggregate`/`update`/`delete`) and the analytics dataset executor. -WITHOUT passing through a renderer — dashboard widgets, dataset +Filters that reach the database WITHOUT passing through a renderer — -definitions, REST query params — need this: before it, the token +dashboard widgets, dataset definitions, REST query params, flow node -compared as a literal string and matched nothing. +filters — need this: before it, the token compared as a literal + +string and matched nothing. The write verbs are covered for the same + +reason (#3810): one filter must select one row set regardless of + +which verb consumes it, or a flow's `find` preview and its + +`update` act on different rows. Either way the DRIVER only ever sees ISO date / timestamp strings,