diff --git a/.changeset/22344-get-record-id-projection.md b/.changeset/22344-get-record-id-projection.md new file mode 100644 index 00000000000..799dc3f49b4 --- /dev/null +++ b/.changeset/22344-get-record-id-projection.md @@ -0,0 +1,11 @@ +--- +'@objectstack/service-automation': patch +--- + +fix(service-automation): a `get_record` whose `fields` leave out `id` answers the row's id as its `id` output on the SQL drivers + +Clause-②: no + +- **What was wrong.** On its single-row branch (no `limit` above 1) a `get_record` node answers `output: { record, id, object }`, and a later node reads that `id` as `{read.id}` (for a node whose id is `read`). The node handed the author's `fields` to the engine unchanged, and the SQL drivers return exactly the columns a projection names. So a `get_record` with `fields: ['title']` read a row with no `id` column, and `{read.id}` was empty with no error. A later `update_record` filtering on it was refused for a filter condition that resolved to nothing, and any other reference read nothing. +- **What it does now.** That branch names `id` in the projection it hands the engine, so `{read.id}` is the row's id whatever `fields` names, on every driver. The record the node serves, and binds to its `outputVariable`, carries that `id` beside the columns the author named, so the variable's `id` reads the same value. The stored-metadata tables (`sys_metadata`, `sys_metadata_history`) are read the same way: the served row is the named columns plus `id`. +- **Unchanged.** A `get_record` with no `fields`, or an empty list, still reads the whole row. A projection that already names `id` is handed over as written. The multi-row branch (`limit` above 1) answers `records` only and is untouched. diff --git a/packages/services/service-automation/src/builtin/crud-nodes.ts b/packages/services/service-automation/src/builtin/crud-nodes.ts index 3f13728119c..94641f32745 100644 --- a/packages/services/service-automation/src/builtin/crud-nodes.ts +++ b/packages/services/service-automation/src/builtin/crud-nodes.ts @@ -278,6 +278,32 @@ async function serveFamilyRead( return answer; } +/** + * [#22344] The projection `get_record`'s single-row (`findOne`) branch hands + * the engine: the author's `fields`, with `id` named. + * + * That branch answers `output.id` from the row it read, and a later node reads + * it as `{.id}`. The SQL drivers return exactly the columns a projection + * names, so the author's `fields` handed over unchanged read a row with no + * `id` whenever they left it out: `output.id` was `undefined`, with no error. + * Naming `id` here makes the declared output independent of the author's + * projection, on every driver and on the stored-metadata family path alike + * ({@link serveFamilyRead} takes this projection as the node's own). + * + * `record` then carries the `id` the engine returned, so `{var.id}` through the + * node's `outputVariable` agrees with `{.id}`; it is not taken back off, + * which would be a second projection layer over the engine's answer. + * + * Only a non-empty projection is touched: no `fields`, or an empty list, is a + * whole-row read already, and a projection that names `id` is handed over as + * written. The `find` branch (`limit > 1`) answers rows only, with no + * top-level `id`, and does not come through here. + */ +function projectionNamingId(fields: string[] | undefined): string[] | undefined { + if (!fields || fields.length === 0 || fields.includes('id')) return fields; + return [...fields, 'id']; +} + /** * [#21623] Refuse a node whose filter EVALUATES the stored-metadata family's * body or content hash, the way the generic data door refuses the same filter. @@ -498,7 +524,9 @@ export function registerCrudNodes(engine: AutomationEngine, ctx: PluginContext): metrics: { selected: Array.isArray(records) ? records.length : 0 }, }; } - const record = await serveFamilyRead(data, objectName, fields, (projection) => + // [#22344] `output.id` is read off the row, so the + // projection the engine is handed names `id`. + const record = await serveFamilyRead(data, objectName, projectionNamingId(fields), (projection) => data.findOne(objectName, { where: filter, fields: projection, context: dataCtx })); if (outputVariable) variables.set(outputVariable, record); return { diff --git a/packages/services/service-automation/src/builtin/get-record-formula-projection.integration.test.ts b/packages/services/service-automation/src/builtin/get-record-formula-projection.integration.test.ts index bb02079c059..f6dca4fb7f3 100644 --- a/packages/services/service-automation/src/builtin/get-record-formula-projection.integration.test.ts +++ b/packages/services/service-automation/src/builtin/get-record-formula-projection.integration.test.ts @@ -16,6 +16,10 @@ * both run identities. The reference is the same node with the formula left * out of `fields`: the formula read must serve that row plus the formula. * + * [#22344] The one-row branch also names `id` in the projection it hands the + * engine (its `output.id` is read off the row), so the row it serves carries + * `id` beside the named fields; the list branch serves the named fields only. + * * Composition: the real stack the other `*.integration.test.ts` files in this * package boot — `ObjectKernel`, `ObjectQLPlugin`, `driver-sql` on * better-sqlite3 `:memory:`, and the real `AutomationServicePlugin`. @@ -129,16 +133,19 @@ describe('flow get_record serves exactly its `fields` when one is a formula (#22 const CASES: Array<[RunAs, Branch]> = [['system', 'one'], ['system', 'list'], ['user', 'one'], ['user', 'list']]; + /** The one-row branch's `id` (#22344); the list branch adds nothing. */ + const idOf = (branch: Branch): Record => (branch === 'one' ? { id: 'line_1' } : {}); + it.each(CASES)('runAs %s, %s: the formula read serves the named fields plus the formula, nothing else', async (runAs, branch) => { const row = await served(runAs, branch, ['name', 'total_price']); - expect(Object.keys(row).sort()).toEqual(['name', 'total_price']); + expect(Object.keys(row).sort()).toEqual([...Object.keys(idOf(branch)), 'name', 'total_price']); // 3 × 7 — computed from two columns the node never named. expect(row.total_price).toBe(21); }); it.each(CASES)('runAs %s, %s: it equals the same read without the formula, plus the formula value', async (runAs, branch) => { const without = await served(runAs, branch, ['name', 'quantity']); - expect(without, 'control: a projection with no formula is the named columns').toStrictEqual({ name: 'Widget', quantity: 3 }); + expect(without, 'control: a projection with no formula is the named columns').toStrictEqual({ ...idOf(branch), name: 'Widget', quantity: 3 }); const withFormula = await served(runAs, branch, ['name', 'quantity', 'total_price']); expect(withFormula).toStrictEqual({ ...without, total_price: 21 }); }); diff --git a/packages/services/service-automation/src/builtin/get-record-id-projection.integration.test.ts b/packages/services/service-automation/src/builtin/get-record-id-projection.integration.test.ts new file mode 100644 index 00000000000..f36815d228a --- /dev/null +++ b/packages/services/service-automation/src/builtin/get-record-id-projection.integration.test.ts @@ -0,0 +1,187 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * A `get_record` node answers `output: { record, id, object }` on its + * single-row (`findOne`) branch, and a later node reads that `id` as + * `{.id}`. The `id` must be the row's id whatever the author's `fields` + * projection names. + * + * The SQL drivers return exactly the columns a projection names. The node used + * to hand the author's `fields` to the engine unchanged, so a projection + * without `id` read the row with no `id` column: `output.id` was `undefined`, + * and a downstream `update_record` filtering on `{read.id}` was refused for an + * empty template. The node now names `id` in the projection it hands the + * engine on that branch, and only there: + * - a projection that already names `id` is handed over as written (no + * second `id`); + * - no projection, or an empty one, stays a whole-row read (no projection is + * invented); + * - the `find` branch (`limit > 1`) declares no top-level `id` and is not + * touched. + * + * `record` carries the `id` the engine returned, so `{rec.id}` through the + * node's `outputVariable` agrees with `{read.id}`; the row's other unnamed + * columns stay out. + * + * Composition: the real stack the other `*.integration.test.ts` files in this + * package boot: `ObjectKernel`, `ObjectQLPlugin`, `driver-sql` on + * better-sqlite3 `:memory:`, and the real `AutomationServicePlugin` for the + * flow-level case. The node-level cases run the registered `get_record` + * executor itself over the same engine, behind a recorder that keeps the + * projection each read handed it. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { ObjectKernel } from '@objectstack/core'; +import { ObjectQLPlugin, type ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { AutomationServicePlugin } from '../plugin.js'; +import { AutomationEngine } from '../engine.js'; +import { registerCrudNodes } from './crud-nodes.js'; + +const SYS = { isSystem: true } as const; + +const OBJECT = { + name: 'pin_get_record_id', + label: 'Pin get_record id', + fields: { + title: { name: 'title', label: 'Title', type: 'text' }, + status: { name: 'status', label: 'Status', type: 'text' }, + }, +}; + +function makeLogger(): any { + const l: any = { info() {}, warn() {}, error() {}, debug() {}, trace() {}, fatal() {} }; + l.child = () => l; + return l; +} + +type Executor = { execute: (node: any, variables: Map, context: any) => Promise }; + +describe('get_record answers the row id as output.id whatever its fields projection names', () => { + let kernel: ObjectKernel; + let ql: ObjectQL; + let automation: AutomationEngine; + let rowId: string; + /** The projection each engine read was handed, in order. */ + const handed: Array<{ method: string; fields: unknown }> = []; + let getRecord: Executor; + + beforeAll(async () => { + kernel = new ObjectKernel({ logger: { level: 'fatal' } }); + await kernel.use(new ObjectQLPlugin()); + await kernel.use(new AutomationServicePlugin({ suspendedRunStore: 'memory' })); + await kernel.bootstrap(); + ql = kernel.getService('objectql'); + automation = kernel.getService('automation'); + + const driver = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }); + await driver.connect(); + ql.registerDriver(driver, true); + ql.registry.registerObject(OBJECT as any, 'pin-get-record-id', 'pin-get-record-id'); + await ql.syncSchemas(); + + const row: any = await ql.insert(OBJECT.name, { title: 'alpha', status: 'open' }, { context: SYS }); + rowId = row.id; + expect(typeof rowId, 'control: the engine assigned the row an id').toBe('string'); + + // The executor itself, over the same engine: a recorder in front of the + // engine keeps the projection each read was handed. + const recorder = new Proxy(ql as any, { + get(target, key) { + const value = Reflect.get(target, key, target); + if (key === 'findOne' || key === 'find') { + return (object: string, query: any) => { + handed.push({ method: key, fields: Array.isArray(query?.fields) ? [...query.fields] : query?.fields }); + return value.call(target, object, query); + }; + } + return typeof value === 'function' ? value.bind(target) : value; + }, + }); + const executors = new Map(); + const capture = new Proxy(new AutomationEngine(makeLogger()), { + get(target, key, receiver) { + if (key === 'registerNodeExecutor') return (e: any) => { executors.set(e.type, e); }; + return Reflect.get(target, key, receiver); + }, + }); + registerCrudNodes(capture as AutomationEngine, { logger: makeLogger(), getService: (n: string) => (n === 'data' ? recorder : undefined) } as any); + getRecord = executors.get('get_record')!; + expect(getRecord, 'control: the get_record executor was registered').toBeDefined(); + }, 60_000); + + afterAll(async () => { + try { await kernel?.shutdown(); } catch { /* best-effort teardown */ } + }); + + /** Run the node once and return its result and the projection the engine was handed. */ + async function readOne(config: Record): Promise<{ result: any; fields: unknown }> { + handed.length = 0; + const result = await getRecord.execute( + { id: 'read', type: 'get_record', config: { objectName: OBJECT.name, filter: { title: 'alpha' }, ...config } }, + new Map(), + { runAs: 'system', flowName: 'pin_get_record_id' }, + ); + expect(result.success, `the read failed: ${JSON.stringify(result)}`).toBe(true); + expect(handed, 'the node reads through findOne exactly once').toHaveLength(1); + expect(handed[0].method).toBe('findOne'); + return { result, fields: handed[0].fields }; + } + + it('control: the SQL driver returns exactly the columns a projection names, so a projection without id reads no id', async () => { + const direct: any = await ql.findOne(OBJECT.name, { where: { title: 'alpha' }, fields: ['title'], context: SYS }); + expect(direct).toEqual({ title: 'alpha' }); + }); + + it('fields that omit id: output.id is the row id, and record carries it beside the named columns', async () => { + const { result, fields } = await readOne({ fields: ['title'] }); + expect(result.output.id).toBe(rowId); + expect(result.output.record).toEqual({ id: rowId, title: 'alpha' }); + expect(fields).toEqual(['title', 'id']); + }); + + it('fields that name id: handed to the engine as written, with id once', async () => { + const { result, fields } = await readOne({ fields: ['id', 'title'] }); + expect(fields).toEqual(['id', 'title']); + expect(result.output.id).toBe(rowId); + expect(result.output.record).toEqual({ id: rowId, title: 'alpha' }); + }); + + for (const [label, config] of [['no fields', {}], ['empty fields', { fields: [] }]] as const) { + it(`control, ${label}: the whole row is read and the output is the engine's own answer`, async () => { + const whole: any = await ql.findOne(OBJECT.name, { where: { title: 'alpha' }, context: SYS }); + expect(whole.status, 'control: a whole-row read carries the unnamed columns').toBe('open'); + const { result, fields } = await readOne(config); + expect(fields, 'no projection is invented').toEqual((config as { fields?: unknown }).fields); + expect(result.output).toEqual({ record: whole, id: rowId, object: OBJECT.name }); + }); + } + + it('a downstream update_record filtering on {read.id} updates the row the narrowed get_record read', async () => { + await ql.insert(OBJECT.name, { title: 'beta', status: 'open' }, { context: SYS }); + const def = { + name: 'pin_get_record_id_flow', label: 'pin_get_record_id_flow', type: 'autolaunched', runAs: 'system', + variables: [{ name: 'rec', type: 'object', isOutput: true }], + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'read', type: 'get_record', label: 'Read', config: { objectName: OBJECT.name, filter: { title: 'beta' }, fields: ['title'], outputVariable: 'rec' } }, + { id: 'close', type: 'update_record', label: 'Close', config: { objectName: OBJECT.name, filter: { id: '{read.id}' }, fields: { status: 'closed' } } }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'read' }, + { id: 'e2', source: 'read', target: 'close' }, + { id: 'e3', source: 'close', target: 'end' }, + ], + }; + automation.registerFlow(def.name, def as any); + const res: any = await automation.execute(def.name, {}); + expect(res.success, `run failed: ${JSON.stringify(res.error ?? res)}`).toBe(true); + const beta: any = await ql.findOne(OBJECT.name, { where: { title: 'beta' }, context: SYS }); + expect(beta.status).toBe('closed'); + expect(res.output.rec).toEqual({ id: beta.id, title: 'beta' }); + const alpha: any = await ql.findOne(OBJECT.name, { where: { title: 'alpha' }, context: SYS }); + expect(alpha.status, 'the update reached only the row the node read').toBe('open'); + }); +}); diff --git a/packages/services/service-automation/src/builtin/get-record-stored-metadata-family.integration.test.ts b/packages/services/service-automation/src/builtin/get-record-stored-metadata-family.integration.test.ts index 10019baddc2..9bb8396ff33 100644 --- a/packages/services/service-automation/src/builtin/get-record-stored-metadata-family.integration.test.ts +++ b/packages/services/service-automation/src/builtin/get-record-stored-metadata-family.integration.test.ts @@ -235,11 +235,16 @@ describe('flow get_record serves the stored-metadata family the way the data doo expect(JSON.parse(output.metadata)).toEqual(JSON.parse(control.metadata)); }); - it('a projection naming the body without its type is served projected, with exactly the columns named', async () => { + // [#22344] The single-row branch names `id` in the projection it hands the + // engine (its `output.id` is read off the row), so the served row carries the + // columns named plus `id`; the `type` column the family's projection adds is + // still taken back off. + it('a projection naming the body without its type is served projected, with exactly the columns named and the row id', async () => { const control = await doorRow('sys_metadata'); const { output } = await runFlow(familyReadFlow('fam_projection', 'sys_metadata', 'system', 'one', ['name', 'metadata', 'checksum'])); expectServedLikeTheDoor(output, control, 'the run output'); - expect(Object.keys(output).sort()).toEqual(['checksum', 'metadata', 'name']); + expect(Object.keys(output).sort()).toEqual(['checksum', 'id', 'metadata', 'name']); + expect(output.id).toBe('meta_pin_flow_ds'); expect(JSON.parse(output.metadata)).toEqual(JSON.parse(control.metadata)); expect(output.checksum).toBe(control.checksum); });