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/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, 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 9de3d887fb..2e9663163a 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -2620,6 +2620,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 }); @@ -3131,7 +3150,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 @@ -3510,6 +3542,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: