From 06eff384ff801a014590781c37f5de1a18ea2315 Mon Sep 17 00:00:00 2001 From: Matt Date: Fri, 5 Jun 2026 10:28:39 +0200 Subject: [PATCH] fix(workflow-executor): read composite-key records via the agent by-id route [PRD-450] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getRecord built the agent filter by zipping primaryKeyFields (alphabetical, from the orchestrator collection-schema) with the recordId (model primary_key order). For composite PKs whose model order differs from alphabetical this mis-paired the columns, so the record was never found ("Record not found"). Fetch by id through the agent's by-id route instead (like update/delete): the recordId is an opaque ordered token and the agent — the only party that knows the primary key column order — does the matching. The executor no longer needs to know the key composition. Same reasoning applied to getRelatedData composite recordIds (use the agent's opaque row id instead of rebuilding from primaryKeyFields). - add Collection#getOne(id, options) to @forestadmin/agent-client (by-id route) - rewrite getRecord to use it; remove buildPkFilter / resolveSchema For v1 agents the by-id read needs the agent to accept the pipe-joined id form (forest_liana: PR #774; forest-express: pending). v2 already supports it. Relates to PRD-450. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../agent-client/src/domains/collection.ts | 19 +++ .../src/adapters/agent-client-agent-port.ts | 71 ++++------- .../adapters/agent-client-agent-port.test.ts | 110 +++++++----------- 3 files changed, 80 insertions(+), 120 deletions(-) diff --git a/packages/agent-client/src/domains/collection.ts b/packages/agent-client/src/domains/collection.ts index 1996674006..8e95c0a1d1 100644 --- a/packages/agent-client/src/domains/collection.ts +++ b/packages/agent-client/src/domains/collection.ts @@ -171,6 +171,25 @@ export default class Collection extends CollectionChart { }); } + // Fetch a single record by its (possibly composite) id via the agent's by-id route. The id is + // serialized opaquely (serializeRecordId), so the agent — the only party that knows the primary + // key column order — does the matching. Returns null when the record does not exist (404, or a + // 200 with an empty payload, which some agents return for a missing composite-key record). + async getOne(id: RecordId, options?: SelectOptions): Promise { + try { + const record = await this.httpRequester.query({ + method: 'get', + path: `/forest/${this.name}/${serializeRecordId(id)}`, + query: QuerySerializer.serialize(options, this.name), + }); + + return record && Object.keys(record as object).length > 0 ? record : null; + } catch (error) { + if ((this.httpRequester.constructor as typeof HttpRequester).is404Error(error)) return null; + throw error; + } + } + private getActionInfo( actionEndpoints: ActionEndpointsByCollection, collectionName: string, diff --git a/packages/workflow-executor/src/adapters/agent-client-agent-port.ts b/packages/workflow-executor/src/adapters/agent-client-agent-port.ts index 49b19703ec..820d32e17e 100644 --- a/packages/workflow-executor/src/adapters/agent-client-agent-port.ts +++ b/packages/workflow-executor/src/adapters/agent-client-agent-port.ts @@ -9,8 +9,8 @@ import type { } from '../ports/agent-port'; import type SchemaCache from '../schema-cache'; import type { StepUser } from '../types/execution-context'; -import type { CollectionSchema, RecordData } from '../types/validated/collection'; -import type { ActionEndpointsByCollection, SelectOptions } from '@forestadmin/agent-client'; +import type { RecordData } from '../types/validated/collection'; +import type { ActionEndpointsByCollection } from '@forestadmin/agent-client'; import { createRemoteAgentClient } from '@forestadmin/agent-client'; import jsonwebtoken from 'jsonwebtoken'; @@ -45,24 +45,6 @@ function restoreFieldNames( return Object.fromEntries(Object.entries(values).map(([k, v]) => [camelToOriginal[k] ?? k, v])); } -function buildPkFilter( - primaryKeyFields: string[], - id: Array, -): SelectOptions['filters'] { - if (primaryKeyFields.length === 1) { - return { field: primaryKeyFields[0], operator: 'Equal', value: id[0] }; - } - - return { - aggregator: 'And', - conditions: primaryKeyFields.map((field, i) => ({ - field, - operator: 'Equal', - value: id[i], - })), - }; -} - export default class AgentClientAgentPort implements AgentPort { private readonly agentUrl: string; private readonly authSecret: string; @@ -77,21 +59,21 @@ export default class AgentClientAgentPort implements AgentPort { async getRecord({ collection, id, fields }: GetRecordQuery, user: StepUser): Promise { return this.callAgent('getRecord', async () => { const client = this.createClient(user); - const schema = this.resolveSchema(collection); - const records = await client.collection(collection).list>({ - filters: buildPkFilter(schema.primaryKeyFields, id), - pagination: { size: 1, number: 1 }, - ...(fields?.length && { fields }), - }); + // Fetch by id through the agent's by-id route (like update/delete): the recordId is an + // opaque ordered token and the agent — the only party that knows the primary key column + // order — does the matching. No buildPkFilter / primaryKeyFields ordering assumption here. + const record = await client + .collection(collection) + .getOne>(id, { ...(fields?.length && { fields }) }); - if (records.length === 0) { + if (!record) { throw new RecordNotFoundError(collection, id.join('|')); } return { collectionName: collection, recordId: id, - values: restoreFieldNames(records[0], fields), + values: restoreFieldNames(record, fields), }; }); } @@ -134,9 +116,18 @@ export default class AgentClientAgentPort implements AgentPort { relatedSchema.fields.map(f => f.fieldName), ); + // For composite PKs, rebuilding the id from primaryKeyFields assumes the schema's + // (alphabetical) order matches the agent's column order — it may not, which would + // mis-pair the key. Use the agent's opaque record id (pipe-joined when composite), + // like getSingleRelatedData, so it round-trips through the by-id route. + const recordId = + relatedSchema.primaryKeyFields.length > 1 + ? String(row.id).split('|') + : relatedSchema.primaryKeyFields.map(f => restored[f] as string | number); + return { collectionName: relatedSchema.collectionName, - recordId: relatedSchema.primaryKeyFields.map(f => restored[f] as string | number), + recordId, values: restored, }; }); @@ -289,26 +280,4 @@ export default class AgentClientAgentPort implements AgentPort { return endpoints; } - - private resolveSchema(collectionName: string): CollectionSchema { - const cached = this.schemaCache.get(collectionName); - - if (!cached) { - // eslint-disable-next-line no-console - console.warn( - `[workflow-executor] Schema not found in cache for collection "${collectionName}". ` + - 'Falling back to primaryKeyFields: ["id"]. Call getCollectionSchema first.', - ); - } - - return ( - cached ?? { - collectionName, - collectionDisplayName: collectionName, - primaryKeyFields: ['id'], - fields: [], - actions: [], - } - ); - } } diff --git a/packages/workflow-executor/test/adapters/agent-client-agent-port.test.ts b/packages/workflow-executor/test/adapters/agent-client-agent-port.test.ts index cf03360cc1..7066aefc0e 100644 --- a/packages/workflow-executor/test/adapters/agent-client-agent-port.test.ts +++ b/packages/workflow-executor/test/adapters/agent-client-agent-port.test.ts @@ -20,6 +20,7 @@ function createMockClient() { const mockRelation = { list: jest.fn() }; const mockCollection = { list: jest.fn(), + getOne: jest.fn(), update: jest.fn(), relation: jest.fn().mockReturnValue(mockRelation), action: jest.fn().mockResolvedValue(mockAction), @@ -102,14 +103,11 @@ describe('AgentClientAgentPort', () => { describe('getRecord', () => { it('should return a RecordData for a simple PK', async () => { - mockCollection.list.mockResolvedValue([{ id: 42, name: 'Alice' }]); + mockCollection.getOne.mockResolvedValue({ id: 42, name: 'Alice' }); const result = await port.getRecord({ collection: 'users', id: [42] }, user); - expect(mockCollection.list).toHaveBeenCalledWith({ - filters: { field: 'id', operator: 'Equal', value: 42 }, - pagination: { size: 1, number: 1 }, - }); + expect(mockCollection.getOne).toHaveBeenCalledWith([42], {}); expect(result).toEqual({ collectionName: 'users', recordId: [42], @@ -117,58 +115,43 @@ describe('AgentClientAgentPort', () => { }); }); - it('should build a composite And filter for composite PKs', async () => { - mockCollection.list.mockResolvedValue([{ tenantId: 1, orderId: 2 }]); + it('should pass the composite id opaquely (no primary-key ordering assumption)', async () => { + mockCollection.getOne.mockResolvedValue({ tenantId: 1, orderId: 2 }); await port.getRecord({ collection: 'orders', id: [1, 2] }, user); - expect(mockCollection.list).toHaveBeenCalledWith({ - filters: { - aggregator: 'And', - conditions: [ - { field: 'tenantId', operator: 'Equal', value: 1 }, - { field: 'orderId', operator: 'Equal', value: 2 }, - ], - }, - pagination: { size: 1, number: 1 }, - }); + // The id is forwarded as-is; the agent resolves the primary key column order. + expect(mockCollection.getOne).toHaveBeenCalledWith([1, 2], {}); }); it('should throw a RecordNotFoundError when no record is found', async () => { - mockCollection.list.mockResolvedValue([]); + mockCollection.getOne.mockResolvedValue(null); await expect(port.getRecord({ collection: 'users', id: [999] }, user)).rejects.toThrow( RecordNotFoundError, ); }); - it('should pass fields to list when fields is provided', async () => { - mockCollection.list.mockResolvedValue([{ id: 42, name: 'Alice' }]); + it('should pass fields to getOne when fields is provided', async () => { + mockCollection.getOne.mockResolvedValue({ id: 42, name: 'Alice' }); await port.getRecord({ collection: 'users', id: [42], fields: ['id', 'name'] }, user); - expect(mockCollection.list).toHaveBeenCalledWith({ - filters: { field: 'id', operator: 'Equal', value: 42 }, - pagination: { size: 1, number: 1 }, - fields: ['id', 'name'], - }); + expect(mockCollection.getOne).toHaveBeenCalledWith([42], { fields: ['id', 'name'] }); }); - it('should not pass fields to list when fields is an empty array', async () => { - mockCollection.list.mockResolvedValue([{ id: 42, name: 'Alice' }]); + it('should not pass fields to getOne when fields is an empty array', async () => { + mockCollection.getOne.mockResolvedValue({ id: 42, name: 'Alice' }); await port.getRecord({ collection: 'users', id: [42], fields: [] }, user); - expect(mockCollection.list).toHaveBeenCalledWith({ - filters: { field: 'id', operator: 'Equal', value: 42 }, - pagination: { size: 1, number: 1 }, - }); + expect(mockCollection.getOne).toHaveBeenCalledWith([42], {}); }); it('should restore snake_case field names when agent returns camelCase keys', async () => { // The agent-client HTTP layer deserializes JSON:API responses with camelCase keys. // restoreFieldNames must map them back to the original snake_case names. - mockCollection.list.mockResolvedValue([{ cardNumber: '4111', isActive: true }]); + mockCollection.getOne.mockResolvedValue({ cardNumber: '4111', isActive: true }); const result = await port.getRecord( { collection: 'users', id: [42], fields: ['card_number', 'is_active'] }, @@ -178,34 +161,27 @@ describe('AgentClientAgentPort', () => { expect(result.values).toEqual({ card_number: '4111', is_active: true }); }); - it('should not pass fields to list when fields is undefined', async () => { - mockCollection.list.mockResolvedValue([{ id: 42, name: 'Alice' }]); + it('should not pass fields to getOne when fields is undefined', async () => { + mockCollection.getOne.mockResolvedValue({ id: 42, name: 'Alice' }); await port.getRecord({ collection: 'users', id: [42] }, user); - expect(mockCollection.list).toHaveBeenCalledWith({ - filters: { field: 'id', operator: 'Equal', value: 42 }, - pagination: { size: 1, number: 1 }, - }); + expect(mockCollection.getOne).toHaveBeenCalledWith([42], {}); }); - it('should fallback to pk field "id" when collection is unknown', async () => { - mockCollection.list.mockResolvedValue([{ id: 1 }]); + it('works without a cached schema (no primary-key lookup needed)', async () => { + mockCollection.getOne.mockResolvedValue({ id: 1 }); const result = await port.getRecord({ collection: 'unknown', id: [1] }, user); - expect(mockCollection.list).toHaveBeenCalledWith( - expect.objectContaining({ - filters: { field: 'id', operator: 'Equal', value: 1 }, - }), - ); + expect(mockCollection.getOne).toHaveBeenCalledWith([1], {}); expect(result.collectionName).toBe('unknown'); }); }); describe('agent JWT', () => { it('signs both camelCase and snake_case identity claims for cross-runtime agents', async () => { - mockCollection.list.mockResolvedValue([{ id: 42 }]); + mockCollection.getOne.mockResolvedValue({ id: 42 }); await port.getRecord({ collection: 'users', id: [42] }, user); @@ -352,7 +328,7 @@ describe('AgentClientAgentPort', () => { ]); }); - it('extracts composite primary keys in the order declared by the schema', async () => { + it('uses the agent opaque record id for composite PKs (no schema-order assumption)', async () => { const compositeSchema = { ...postsSchema, primaryKeyFields: ['tenantId', 'postId'], @@ -371,7 +347,9 @@ describe('AgentClientAgentPort', () => { }, ], }; - mockRelation.list.mockResolvedValue([{ tenantId: 'acme', postId: 7 }]); + // The agent serializes the (composite) record id; we reuse it rather than rebuilding it + // from primaryKeyFields, so the column order is the agent's, not the schema's alphabetical one. + mockRelation.list.mockResolvedValue([{ id: 'acme|7', tenantId: 'acme', postId: 7 }]); const result = await port.getRelatedData( { @@ -384,7 +362,7 @@ describe('AgentClientAgentPort', () => { user, ); - expect(result[0].recordId).toEqual(['acme', 7]); + expect(result[0].recordId).toEqual(['acme', '7']); }); it('applies pagination when limit is a number', async () => { @@ -495,7 +473,7 @@ describe('AgentClientAgentPort', () => { }; it('projects the related PK on the parent and unpacks the linkage as RecordData', async () => { - mockCollection.list.mockResolvedValue([{ order: { id: '99' } }]); + mockCollection.getOne.mockResolvedValue({ order: { id: '99' } }); const result = await port.getSingleRelatedData( { @@ -507,9 +485,7 @@ describe('AgentClientAgentPort', () => { user, ); - expect(mockCollection.list).toHaveBeenCalledWith( - expect.objectContaining({ fields: ['order@@@id'] }), - ); + expect(mockCollection.getOne).toHaveBeenCalledWith([42], { fields: ['order@@@id'] }); expect(result).toEqual({ collectionName: 'orders', recordId: ['99'], @@ -518,7 +494,7 @@ describe('AgentClientAgentPort', () => { }); it('projects only the caller field (e.g. referenceField), not the PK — the linkage id comes free', async () => { - mockCollection.list.mockResolvedValue([{ order: { id: '99', reference: 'ORD-2026-001' } }]); + mockCollection.getOne.mockResolvedValue({ order: { id: '99', reference: 'ORD-2026-001' } }); const result = await port.getSingleRelatedData( { @@ -532,14 +508,12 @@ describe('AgentClientAgentPort', () => { ); // Single sub-field only: the agent can't parse `fields[order]=id,reference`. - expect(mockCollection.list).toHaveBeenCalledWith( - expect.objectContaining({ fields: ['order@@@reference'] }), - ); + expect(mockCollection.getOne).toHaveBeenCalledWith([42], { fields: ['order@@@reference'] }); expect(result?.values).toEqual({ id: '99', reference: 'ORD-2026-001' }); }); it('projects at most one sub-field even when the caller passes several', async () => { - mockCollection.list.mockResolvedValue([{ order: { id: '99', reference: 'ORD-2026-001' } }]); + mockCollection.getOne.mockResolvedValue({ order: { id: '99', reference: 'ORD-2026-001' } }); await port.getSingleRelatedData( { @@ -552,9 +526,7 @@ describe('AgentClientAgentPort', () => { user, ); - expect(mockCollection.list).toHaveBeenCalledWith( - expect.objectContaining({ fields: ['order@@@reference'] }), - ); + expect(mockCollection.getOne).toHaveBeenCalledWith([42], { fields: ['order@@@reference'] }); }); // Regression: jsonapi-serializer emits the nested linkage with camelCased attribute @@ -573,7 +545,7 @@ describe('AgentClientAgentPort', () => { }, ], }; - mockCollection.list.mockResolvedValue([{ order: { id: '99', fullName: 'John Doe' } }]); + mockCollection.getOne.mockResolvedValue({ order: { id: '99', fullName: 'John Doe' } }); const result = await port.getSingleRelatedData( { @@ -593,7 +565,7 @@ describe('AgentClientAgentPort', () => { // emits the linkage under the camelCased key (billingAddress), so looking it up by the raw // name returned null and the relation never loaded. it('finds the linkage when the relation name is snake_case', async () => { - mockCollection.list.mockResolvedValue([{ billingAddress: { id: '7|2' } }]); + mockCollection.getOne.mockResolvedValue({ billingAddress: { id: '7|2' } }); const result = await port.getSingleRelatedData( { @@ -605,9 +577,9 @@ describe('AgentClientAgentPort', () => { user, ); - expect(mockCollection.list).toHaveBeenCalledWith( - expect.objectContaining({ fields: ['billing_address@@@id'] }), - ); + expect(mockCollection.getOne).toHaveBeenCalledWith([42], { + fields: ['billing_address@@@id'], + }); expect(result).toEqual({ collectionName: 'addresses', recordId: ['7', '2'], @@ -634,7 +606,7 @@ describe('AgentClientAgentPort', () => { }, ], }; - mockCollection.list.mockResolvedValue([{ order: { id: 'acme|7' } }]); + mockCollection.getOne.mockResolvedValue({ order: { id: 'acme|7' } }); const result = await port.getSingleRelatedData( { @@ -650,7 +622,7 @@ describe('AgentClientAgentPort', () => { }); it('returns null when the parent has no linkage to the xToOne relation', async () => { - mockCollection.list.mockResolvedValue([{ order: null }]); + mockCollection.getOne.mockResolvedValue({ order: null }); const result = await port.getSingleRelatedData( { @@ -666,7 +638,7 @@ describe('AgentClientAgentPort', () => { }); it('returns null when the linkage object is present but has no id', async () => { - mockCollection.list.mockResolvedValue([{ order: {} }]); + mockCollection.getOne.mockResolvedValue({ order: {} }); const result = await port.getSingleRelatedData( {