diff --git a/packages/agent-bff/src/data/pack-id.ts b/packages/agent-bff/src/data/pack-id.ts index 92255533bc..6f71a8f0df 100644 --- a/packages/agent-bff/src/data/pack-id.ts +++ b/packages/agent-bff/src/data/pack-id.ts @@ -4,7 +4,7 @@ import { recordKey } from '@forestadmin/agent-client'; import { mappingError } from '../http/bff-local-errors'; -export const PACKED_ID_SEPARATOR = '|'; +const PACKED_ID_SEPARATOR = '|'; // The only column type unpacked to a number, mirroring the agent's `IdUtils.unpackId`. const NUMBER_COLUMN_TYPE = 'Number'; @@ -73,6 +73,29 @@ function comparableValue(record: Record, key: PrimaryKeyField): * * With no record, the values are returned whole and the pairing is the positional one. */ +function matchedByRecord( + values: string[], + primaryKeys: PrimaryKeyField[], + record: Record, +): { segments: string[]; unread: number } { + const claimed = values.map(() => false); + const matched = primaryKeys.map(key => { + const wanted = comparableValue(record, key); + const index = wanted === null ? -1 : values.findIndex((v, i) => !claimed[i] && v === wanted); + + if (index !== -1) claimed[index] = true; + + return index === -1 ? null : values[index]; + }); + + const leftovers = values.filter((_, index) => !claimed[index]); + + return { + segments: matched.map(value => value ?? (leftovers.shift() as string)), + unread: matched.filter(value => value === null).length, + }; +} + function segmentsByKey( values: string[], primaryKeys: PrimaryKeyField[], @@ -80,27 +103,83 @@ function segmentsByKey( ): string[] { if (!record) return values; - const claimed = values.map(() => false); - const matched = primaryKeys.map(key => { - const wanted = comparableValue(record, key); - const index = wanted === null ? -1 : values.findIndex((v, i) => !claimed[i] && v === wanted); + const { segments, unread } = matchedByRecord(values, primaryKeys, record); + + return unread > 1 ? values : segments; +} - if (index === -1) return null; - claimed[index] = true; +function parseJsonArrayId(packedId: string, keyCount: number): unknown[] | null { + if (keyCount < 2 || !packedId.startsWith('[') || !packedId.endsWith(']')) return null; + + try { + const parsed: unknown = JSON.parse(packedId); + + return Array.isArray(parsed) && parsed.length === keyCount ? parsed : null; + } catch { + return null; + } +} - return values[index]; +function jsonArrayValues(elements: unknown[]): string[] | null { + const values = elements.map(element => { + if (typeof element === 'string') return element; + + return typeof element === 'number' && Number.isSafeInteger(element) ? String(element) : null; }); - if (matched.filter(value => value === null).length > 1) return values; + return values.every((value): value is string => value !== null) ? values : null; +} - const leftovers = values.filter((_, index) => !claimed[index]); +function jsonArraySegments( + packedId: string, + primaryKeys: PrimaryKeyField[], + record?: Record, +): string[] { + const values = jsonArrayValues(parseJsonArrayId(packedId, primaryKeys.length) ?? []); + const pipeValues = packedId.split(PACKED_ID_SEPARATOR); - return matched.map(value => value ?? (leftovers.shift() as string)); + const read = (candidate: string[] | null, unreadAllowed: number): string[] | null => { + if (!candidate || !record) return null; + + const { segments, unread } = matchedByRecord(candidate, primaryKeys, record); + + return unread <= unreadAllowed ? segments : null; + }; + + const segments = + pipeValues.length === primaryKeys.length + ? read(pipeValues, 0) ?? read(values, 0) + : read(values, 1); + + if (!segments) { + throw mappingError( + 'Cannot build primary key: the record does not say which composite id value is whose', + ); + } + + return segments; +} + +function pipeSegments( + packedId: string, + primaryKeys: PrimaryKeyField[], + record?: Record, +): string[] { + const values = packedId.split(PACKED_ID_SEPARATOR); + + if (values.length !== primaryKeys.length) { + throw mappingError( + `Cannot build primary key: expected ${primaryKeys.length} values, found ${values.length}`, + ); + } + + return segmentsByKey(values, primaryKeys, record); } /** * Rebuild the structured primary key of a record from its opaque packed id, mirroring the agent's - * `IdUtils.packId`/`unpackId` (`|`-joined values, `Number` columns cast back to numbers). Returns a + * `IdUtils.packId`/`unpackId` (`|`-joined values, `Number` columns cast back to numbers), or the + * JSON array `forest_liana` serializes a composite key as, paired by the record only. Returns a * `{ pkField: value }` map for `__forest.primaryKey`. Throws a mapping error rather than emitting a * malformed key when the schema lacks key metadata or the packed id shape does not match it. * @@ -126,15 +205,9 @@ export default function unpackPrimaryKey( }; } - const values = packedId.split(PACKED_ID_SEPARATOR); - - if (values.length !== primaryKeys.length) { - throw mappingError( - `Cannot build primary key: expected ${primaryKeys.length} values, found ${values.length}`, - ); - } - - const segments = segmentsByKey(values, primaryKeys, record); + const segments = parseJsonArrayId(packedId, primaryKeys.length) + ? jsonArraySegments(packedId, primaryKeys, record) + : pipeSegments(packedId, primaryKeys, record); const result: Record = {}; primaryKeys.forEach(({ name, type }, index) => { diff --git a/packages/agent-bff/src/openapi/record-schemas.ts b/packages/agent-bff/src/openapi/record-schemas.ts index ff0042f132..527533e22f 100644 --- a/packages/agent-bff/src/openapi/record-schemas.ts +++ b/packages/agent-bff/src/openapi/record-schemas.ts @@ -5,7 +5,6 @@ import { groupByRecordKey } from '@forestadmin/agent-client'; import toFieldSchema from './field-schemas'; import { quoted } from './names'; -import { PACKED_ID_SEPARATOR } from '../data/pack-id'; // The flat id is the JSON:API resource id, which is a string by specification whatever the key // column holds. `__forest.primaryKey` is the same id unpacked and typed, so the two forms of one @@ -15,8 +14,8 @@ const ID_SCHEMA: SchemaObject = { description: 'The record id, always a string — the agent serializes it as the JSON:API resource id, even ' + 'when the key column is a Number. `__forest.primaryKey` carries the same id TYPED, so ' + - `comparing the two without coercion fails. A composite key is its values joined by ` + - `${quoted(PACKED_ID_SEPARATOR)}.`, + "comparing the two without coercion fails. A composite key is packed in the agent's own " + + 'format, so pass it back as is rather than splitting it.', }; function isReference(schema: SchemaObject | ReferenceObject): schema is ReferenceObject { diff --git a/packages/agent-bff/src/openapi/schemas.ts b/packages/agent-bff/src/openapi/schemas.ts index d922431180..eb774df420 100644 --- a/packages/agent-bff/src/openapi/schemas.ts +++ b/packages/agent-bff/src/openapi/schemas.ts @@ -1,7 +1,6 @@ import { allOperators } from '@forestadmin/datasource-toolkit'; import { z } from './zod-openapi'; -import { PACKED_ID_SEPARATOR } from '../data/pack-id'; import { CountFlatInputs, ListFlatInputs, @@ -303,7 +302,7 @@ export const ListResponseSchema = z .openapi('ListResponse', { description: 'Records are flat, each carrying a `__forest` envelope. A record always holds `id`, the ' + - `agent id as a string — a composite key is its values joined by \`${PACKED_ID_SEPARATOR}\` — ` + + "agent id as a string — a composite key packed in the agent's own format — " + 'while `__forest.primaryKey` holds that same id typed and split per column — with the one ' + 'exception `ForestRecordMeta` describes, where the name it carries is not a column. ' + 'The list never ' + diff --git a/packages/agent-bff/src/openapi/unfolded-paths.ts b/packages/agent-bff/src/openapi/unfolded-paths.ts index b46371cc6d..3ff41ea134 100644 --- a/packages/agent-bff/src/openapi/unfolded-paths.ts +++ b/packages/agent-bff/src/openapi/unfolded-paths.ts @@ -31,7 +31,6 @@ import { SortClauseSchema, TimezoneSchema, } from './schemas'; -import { PACKED_ID_SEPARATOR } from '../data/pack-id'; import { NON_BLANK_PATTERN } from '../data/request-schemas'; import { enumOptionsOf } from '../read-model/field-type'; @@ -443,7 +442,7 @@ const PARENT_ID_SHAPE = { * whatever the key's type is, and forwards it to the agent as a string. So the SHAPE stays that * union — narrowing a numeric key to `number` would make a generated client reject `"123"`, which * the endpoint accepts. What the read-model knows goes in the description: which column the id - * belongs to, and, for a composite key, the `|`-joined order `unpackPrimaryKey` expects. + * belongs to, and, for a composite key, that it is packed in the agent's own format. */ function parentIdSchema( pool: ComponentPool, @@ -461,11 +460,9 @@ function parentIdSchema( // would send a client to build an id the agent unpacks onto the wrong columns. description: `The composite id of the parent ${quoted(parent)} record, taken verbatim from that ` + - `record's own id: the values of ${primaryKeys.map(key => key.name).join(', ')} joined by ` + - `${quoted( - PACKED_ID_SEPARATOR, - )}, in the order the agent packs them. Copy it from a listed ` + - `record rather than assembling it.`, + `record's own id: the values of ${primaryKeys.map(key => key.name).join(', ')} packed ` + + `in the agent's own format and order. Copy it from a listed record rather than ` + + `assembling it.`, }; } diff --git a/packages/agent-bff/test/data/composite-primary-key.test.ts b/packages/agent-bff/test/data/composite-primary-key.test.ts new file mode 100644 index 0000000000..cc70cfd040 --- /dev/null +++ b/packages/agent-bff/test/data/composite-primary-key.test.ts @@ -0,0 +1,69 @@ +import type { ForestSchemaCollection } from '@forestadmin/forestadmin-client'; + +import { mapListResponse } from '../../src/data/response-mappers'; +import ReadModel from '../../src/read-model/read-model'; + +const COLLECTION = 'EdgeCompositePk'; + +function schemaWithKeys(keys: string[]): ForestSchemaCollection[] { + return [ + { + name: COLLECTION, + fields: [ + { field: 'created_at', type: 'Date', isPrimaryKey: false }, + { field: 'payload', type: 'String', isPrimaryKey: false }, + { field: 'seq', type: 'Number', isPrimaryKey: keys.includes('seq') }, + { field: 'tenant_id', type: 'String', isPrimaryKey: keys.includes('tenant_id') }, + ], + }, + ] as unknown as ForestSchemaCollection[]; +} + +const ROWS = [ + { tenantId: 'acme', seq: 1 }, + { tenantId: 'acme', seq: 2 }, + { tenantId: 'globex', seq: 1 }, +]; + +function listKeys(keys: string[], idOf: (row: (typeof ROWS)[number]) => string) { + const primaryKeys = new ReadModel(schemaWithKeys(keys)).getPrimaryKeys(COLLECTION); + const records = ROWS.map(row => ({ id: idOf(row), ...row, payload: 'p' })); + + return mapListResponse(COLLECTION, records, primaryKeys).data.map( + ({ __forest: { primaryKey } }) => primaryKey, + ); +} + +describe('a (tenant_id, seq) composite primary key, per agent stack', () => { + it('should unfold both columns on forest-express-sequelize, which packs in declaration order', () => { + expect(listKeys(['seq', 'tenant_id'], row => `${row.tenantId}|${row.seq}`)).toEqual([ + { seq: 1, tenant_id: 'acme' }, + { seq: 2, tenant_id: 'acme' }, + { seq: 1, tenant_id: 'globex' }, + ]); + }); + + it('should unfold both columns on a v2 agent, which packs in schema order', () => { + expect(listKeys(['seq', 'tenant_id'], row => `${row.seq}|${row.tenantId}`)).toEqual([ + { seq: 1, tenant_id: 'acme' }, + { seq: 2, tenant_id: 'acme' }, + { seq: 1, tenant_id: 'globex' }, + ]); + }); + + it('should report the single key forest_liana declares when its model narrows the key to one column', () => { + expect(listKeys(['tenant_id'], row => row.tenantId)).toEqual([ + { tenant_id: 'acme' }, + { tenant_id: 'acme' }, + { tenant_id: 'globex' }, + ]); + }); + + it('should unfold both columns on forest_liana, which serializes a composite key as a JSON array', () => { + expect(listKeys(['seq', 'tenant_id'], row => JSON.stringify([row.tenantId, row.seq]))).toEqual([ + { seq: 1, tenant_id: 'acme' }, + { seq: 2, tenant_id: 'acme' }, + { seq: 1, tenant_id: 'globex' }, + ]); + }); +}); diff --git a/packages/agent-bff/test/data/pack-id.test.ts b/packages/agent-bff/test/data/pack-id.test.ts index beb18753f3..2953d0b10e 100644 --- a/packages/agent-bff/test/data/pack-id.test.ts +++ b/packages/agent-bff/test/data/pack-id.test.ts @@ -256,4 +256,131 @@ describe('unpackPrimaryKey', () => { }); }); }); + + describe('when a forest_liana agent serializes its composite id as a JSON array', () => { + const LIANA_KEYS = [ + { name: 'seq', type: 'Number' }, + { name: 'tenant_id', type: 'String' }, + ]; + const MAPPING_ERROR = expect.objectContaining({ type: 'mapping_error', status: 500 }); + + it('should place each value on its key by the record, the array following the model order', () => { + expect( + unpackPrimaryKey('["acme",1]', LIANA_KEYS, { tenantId: 'acme', seq: 1, id: '["acme",1]' }), + ).toEqual({ seq: 1, tenant_id: 'acme' }); + }); + + it('should give the one key the record cannot read the value left over', () => { + expect(unpackPrimaryKey('["acme",1]', LIANA_KEYS, { tenantId: 'acme' })).toEqual({ + seq: 1, + tenant_id: 'acme', + }); + }); + + it('should throw rather than pair by position when two keys go unread', () => { + expect(() => unpackPrimaryKey('["acme",1]', LIANA_KEYS, {})).toThrow(MAPPING_ERROR); + expect(() => unpackPrimaryKey('["acme",1]', LIANA_KEYS)).toThrow(MAPPING_ERROR); + }); + + it('should throw on an integer past the safe range instead of rounding it', () => { + expect(() => + unpackPrimaryKey('["acme",9007199254740993]', LIANA_KEYS, { + tenantId: 'acme', + seq: 9007199254740992, + }), + ).toThrow(MAPPING_ERROR); + }); + + it.each([ + ['a boolean', '["acme",true]'], + ['a null', '["acme",null]'], + ['an object', '["acme",{"seq":1}]'], + ['a nested array', '["acme",[1]]'], + ['a fractional number', '["acme",1.5]'], + ])('should throw on %s element rather than build a key from it', (_, packedId) => { + expect(() => unpackPrimaryKey(packedId, LIANA_KEYS, { tenantId: 'acme', seq: 1 })).toThrow( + MAPPING_ERROR, + ); + }); + + it('should keep the segment count error when the array length does not match the keys', () => { + expect(() => + unpackPrimaryKey('["acme",1,2]', LIANA_KEYS, { tenantId: 'acme', seq: 1 }), + ).toThrow( + expect.objectContaining({ + message: 'Cannot build primary key: expected 2 values, found 1', + }), + ); + }); + + it('should read a JSON array whose string value holds the pipe separator', () => { + expect(unpackPrimaryKey('["a|b",1]', LIANA_KEYS, { tenantId: 'a|b', seq: 1 })).toEqual({ + seq: 1, + tenant_id: 'a|b', + }); + }); + + describe('when the id reads both as a JSON array and as pipe segments', () => { + const STRING_KEYS = [ + { name: 'k1', type: 'String' }, + { name: 'k2', type: 'String' }, + ]; + + it('should keep the pipe reading when the record backs every segment', () => { + expect(unpackPrimaryKey('[")a|b",1]', STRING_KEYS, { k1: '[")a', k2: 'b",1]' })).toEqual({ + k1: '[")a', + k2: 'b",1]', + }); + }); + + it('should keep the pipe reading over a JSON reading the record only partly backs', () => { + const packedId = '["a|b","[\\"a"]'; + + expect(unpackPrimaryKey(packedId, STRING_KEYS, { k1: '["a', k2: 'b","[\\"a"]' })).toEqual({ + k1: '["a', + k2: 'b","[\\"a"]', + }); + }); + + it('should throw rather than give a key the leftover value when neither reading is fully backed', () => { + expect(() => unpackPrimaryKey('["a|b",1]', STRING_KEYS, { k1: '["a' })).toThrow( + MAPPING_ERROR, + ); + }); + + it('should throw when only a JSON reading with an unread key is left', () => { + expect(() => unpackPrimaryKey('["a|b",1]', LIANA_KEYS, { tenantId: 'a|b' })).toThrow( + MAPPING_ERROR, + ); + }); + }); + + it('should leave a pipe id on the pipe path', () => { + expect(unpackPrimaryKey('acme|1', LIANA_KEYS, { tenantId: 'acme', seq: 1 })).toEqual({ + seq: 1, + tenant_id: 'acme', + }); + }); + + it('should leave a bracketed pipe id that is not JSON on the pipe path', () => { + expect( + unpackPrimaryKey('[x|y]', [ + { name: 'a', type: 'String' }, + { name: 'b', type: 'String' }, + ]), + ).toEqual({ a: '[x', b: 'y]' }); + }); + + it('should keep a single key whole, even when its id looks like a JSON array', () => { + expect(unpackPrimaryKey('["a"]', [{ name: 'code', type: 'String' }])).toEqual({ + code: '["a"]', + }); + }); + + it('should keep a derived key whole, even when its id is a JSON array', () => { + expect( + unpackPrimaryKey('["acme",1]', [{ name: 'id', type: 'String', derived: true }]), + ).toEqual({ id: '["acme",1]' }); + }); + }); }); diff --git a/packages/agent-bff/test/openapi/openapi-unfolded.test.ts b/packages/agent-bff/test/openapi/openapi-unfolded.test.ts index 198ad038c8..ab7466df0d 100644 --- a/packages/agent-bff/test/openapi/openapi-unfolded.test.ts +++ b/packages/agent-bff/test/openapi/openapi-unfolded.test.ts @@ -481,8 +481,9 @@ describe('the unfolded document', () => { }; expect(request.properties.parentId.type).toBe('string'); - expect(request.properties.parentId.description).toContain('shop, number joined by "|"'); - expect(request.properties.parentId.description).toContain('in the order the agent packs them'); + expect(request.properties.parentId.description).toContain( + "shop, number packed in the agent's own format and order", + ); expect(request.properties.parentId.description).toContain( 'Copy it from a listed record rather than assembling it', );