From 1cea85b26b3a97ecdd0f0aa2af278ae5e658d4ce Mon Sep 17 00:00:00 2001 From: thribhuvan003 Date: Thu, 3 Sep 2026 22:08:12 +0530 Subject: [PATCH] fix(sql-orm-client): correlate includes on every key column resolveIncludeRelation kept only localFields[0] and targetFields[0], so an include across a composite foreign key emitted a predicate correlating on the first column alone. The child subquery then matched every row sharing that first column, and for an N:1 relation the result was unwrapped to the first of them, so every parent silently received the same related row. Nothing threw and the row shape stayed valid, which makes it hard to notice. Resolve every column pair and AND the equalities, mirroring buildJoinWhere in model-accessor.ts that the relation-filter path already uses for the same relations. Reject incomplete composite-key metadata when column array lengths mismatch or elements are missing. Single-column foreign keys are unaffected. Signed-off-by: thribhuvan003 --- .../sql-orm-client/src/collection-contract.ts | 46 ++++++++---- .../sql-orm-client/src/collection.ts | 4 +- .../sql-orm-client/src/query-plan-select.ts | 54 +++++++++----- .../3-extensions/sql-orm-client/src/types.ts | 6 +- .../test/collection-contract.test.ts | 74 ++++++++++++++++++- .../test/collection-dispatch.test.ts | 4 +- .../test/collection.state.test.ts | 6 +- .../test/query-plan-select.test.ts | 64 +++++++++++++++- ...ariant-include.collection-contract.test.ts | 12 +-- .../variant-include.query-plan-fixtures.ts | 4 +- 10 files changed, 220 insertions(+), 54 deletions(-) diff --git a/packages/3-extensions/sql-orm-client/src/collection-contract.ts b/packages/3-extensions/sql-orm-client/src/collection-contract.ts index c09d7626999e..9a3dee2b6dc6 100644 --- a/packages/3-extensions/sql-orm-client/src/collection-contract.ts +++ b/packages/3-extensions/sql-orm-client/src/collection-contract.ts @@ -300,8 +300,10 @@ export interface ResolvedIncludeRelation { readonly relatedNamespaceId: string; readonly relatedTableName: string; readonly localTableName: string; - readonly targetColumn: string; - readonly localColumn: string; + /** Target-side join columns, positionally paired with `localColumns`. */ + readonly targetColumns: readonly string[]; + /** Local-side join columns, positionally paired with `targetColumns`. */ + readonly localColumns: readonly string[]; readonly cardinality: RelationCardinalityTag | undefined; readonly through?: IncludeThroughDescriptor; } @@ -336,22 +338,38 @@ export function resolveIncludeRelation( { meta: { model: baseModelName, relation: relationName } }, ); } - const localField = relation.on.localFields[0]; - const targetField = relation.on.targetFields[0]; - if (!localField || !targetField) { + const localFields = relation.on.localFields; + const targetFields = relation.on.targetFields; + const localColumns: string[] = []; + const targetColumns: string[] = []; + + if (localFields.length !== targetFields.length) { + throw new InternalError( + `Relation '${relationName}' on model '${declaringModelName}' has incomplete join metadata (missing localFields or targetFields)`, + ); + } + + for (let i = 0; i < localFields.length; i++) { + const localField = localFields[i]; + const targetField = targetFields[i]; + if (!localField || !targetField) { + throw new InternalError( + `Relation '${relationName}' on model '${declaringModelName}' has incomplete join metadata (missing localFields or targetFields)`, + ); + } + localColumns.push(resolveFieldToColumn(contract, namespaceId, declaringModelName, localField)); + targetColumns.push( + resolveFieldToColumn(contract, relation.toNamespace, relation.to, targetField), + ); + } + + if (localColumns.length === 0) { throw new InternalError( `Relation '${relationName}' on model '${declaringModelName}' has incomplete join metadata (missing localFields or targetFields)`, ); } const relatedTableName = resolveModelTableName(contract, relation.toNamespace, relation.to); - const localColumn = resolveFieldToColumn(contract, namespaceId, declaringModelName, localField); - const targetColumn = resolveFieldToColumn( - contract, - relation.toNamespace, - relation.to, - targetField, - ); let through: IncludeThroughDescriptor | undefined; if (relation.through !== undefined) { @@ -373,8 +391,8 @@ export function resolveIncludeRelation( relatedNamespaceId: relation.toNamespace, relatedTableName, localTableName, - targetColumn, - localColumn, + targetColumns, + localColumns, cardinality: relation.cardinality, ...ifDefined('through', through), }; diff --git a/packages/3-extensions/sql-orm-client/src/collection.ts b/packages/3-extensions/sql-orm-client/src/collection.ts index 645db90f134e..6475b837aa78 100644 --- a/packages/3-extensions/sql-orm-client/src/collection.ts +++ b/packages/3-extensions/sql-orm-client/src/collection.ts @@ -621,8 +621,8 @@ class CollectionImpl< relatedNamespaceId: relation.relatedNamespaceId, relatedTableName: relation.relatedTableName, localTableName: relation.localTableName, - targetColumn: relation.targetColumn, - localColumn: relation.localColumn, + targetColumns: relation.targetColumns, + localColumns: relation.localColumns, cardinality: relation.cardinality, ...ifDefined('through', relation.through), nested: nestedState, diff --git a/packages/3-extensions/sql-orm-client/src/query-plan-select.ts b/packages/3-extensions/sql-orm-client/src/query-plan-select.ts index 8338d15ea81c..5d0b84b5244c 100644 --- a/packages/3-extensions/sql-orm-client/src/query-plan-select.ts +++ b/packages/3-extensions/sql-orm-client/src/query-plan-select.ts @@ -261,7 +261,39 @@ interface IncludeParentSource { } function localColumnsForRowInclude(include: IncludeExpr): readonly string[] { - return include.through?.parentLocalColumns ?? [include.localColumn]; + return include.through?.parentLocalColumns ?? include.localColumns; +} + +/** + * Correlate a child row back to its parent across every column of the + * relation's key. Composite foreign keys contribute one equality per + * column, ANDed together — mirroring the relation-filter join in + * `model-accessor.ts`. Correlating on a prefix of the key would match + * every child sharing that prefix. + */ +function buildIncludeJoinExpr( + include: IncludeExpr, + childTableRef: string, + parentLocalRefs: readonly ColumnRef[], +): AnyExpression { + const joinExprs: AnyExpression[] = []; + + if (parentLocalRefs.length !== include.targetColumns.length) { + throw new InternalError(`Include '${include.relationName}' has incomplete join metadata`); + } + + for (let i = 0; i < parentLocalRefs.length; i++) { + const parentLocalRef = parentLocalRefs[i]; + const targetColumn = include.targetColumns[i]; + if (parentLocalRef === undefined || !targetColumn) { + throw new InternalError(`Include '${include.relationName}' has incomplete join metadata`); + } + joinExprs.push(BinaryExpr.eq(ColumnRef.of(childTableRef, targetColumn), parentLocalRef)); + } + + const firstExpr = joinExprs[0]; + assertDefined(firstExpr, `Include '${include.relationName}' has no parent-local column ref`); + return joinExprs.length === 1 ? firstExpr : AndExpr.of(joinExprs); } function resolveParentLocalRefs( @@ -578,15 +610,7 @@ function buildIncludeChildRowsSelect( whereExpr = childWhere ? AndExpr.of([artifacts.whereExpr, childWhere]) : artifacts.whereExpr; junctionJoins = [artifacts.junctionJoin]; } else { - const parentLocalRef = parentLocalRefs[0]; - assertDefined( - parentLocalRef, - `Include '${include.relationName}' has no parent-local column ref`, - ); - const joinExpr = BinaryExpr.eq( - ColumnRef.of(childTableRef, include.targetColumn), - parentLocalRef, - ); + const joinExpr = buildIncludeJoinExpr(include, childTableRef, parentLocalRefs); whereExpr = childWhere ? AndExpr.of([joinExpr, childWhere]) : joinExpr; } @@ -1019,15 +1043,7 @@ function buildIncludeChildScalarSelect( whereExpr = childWhere ? AndExpr.of([artifacts.whereExpr, childWhere]) : artifacts.whereExpr; junctionJoins = [artifacts.junctionJoin]; } else { - const parentLocalRef = parentLocalRefs[0]; - assertDefined( - parentLocalRef, - `Include '${include.relationName}' has no parent-local column ref`, - ); - const joinExpr = BinaryExpr.eq( - ColumnRef.of(childTableRef, include.targetColumn), - parentLocalRef, - ); + const joinExpr = buildIncludeJoinExpr(include, childTableRef, parentLocalRefs); whereExpr = childWhere ? AndExpr.of([joinExpr, childWhere]) : joinExpr; } diff --git a/packages/3-extensions/sql-orm-client/src/types.ts b/packages/3-extensions/sql-orm-client/src/types.ts index 9c2016462278..b99dcf3fa414 100644 --- a/packages/3-extensions/sql-orm-client/src/types.ts +++ b/packages/3-extensions/sql-orm-client/src/types.ts @@ -72,8 +72,10 @@ export interface IncludeExpr { readonly relatedNamespaceId: string; readonly relatedTableName: string; readonly localTableName: string; - readonly targetColumn: string; - readonly localColumn: string; + /** Target-side join columns, positionally paired with `localColumns`. */ + readonly targetColumns: readonly string[]; + /** Local-side join columns, positionally paired with `targetColumns`. */ + readonly localColumns: readonly string[]; readonly cardinality: RelationCardinalityTag | undefined; readonly through?: IncludeThroughDescriptor; readonly nested: CollectionState; diff --git a/packages/3-extensions/sql-orm-client/test/collection-contract.test.ts b/packages/3-extensions/sql-orm-client/test/collection-contract.test.ts index b78e06c5841b..1d1d21c7c22b 100644 --- a/packages/3-extensions/sql-orm-client/test/collection-contract.test.ts +++ b/packages/3-extensions/sql-orm-client/test/collection-contract.test.ts @@ -102,8 +102,38 @@ describe('collection-contract capability detection', () => { relatedNamespaceId: 'public', relatedTableName: 'posts', localTableName: 'users', - targetColumn: 'user_id', - localColumn: 'id', + targetColumns: ['user_id'], + localColumns: ['id'], + cardinality: '1:N', + }); + }); + + it('resolveIncludeRelation() resolves every column of a composite foreign key', () => { + const composite = withPatchedDomainModels(getTestContract(), (models) => { + const user = models['User'] as Record; + return { + ...models, + User: { + ...user, + relations: { + ...(user['relations'] as Record), + posts: { + to: { model: 'Post', namespace: 'public' }, + cardinality: '1:N', + on: { localFields: ['id', 'email'], targetFields: ['userId', 'title'] }, + }, + }, + }, + }; + }); + + expect(resolveIncludeRelation(composite, 'public', 'User', 'posts')).toEqual({ + relatedModelName: 'Post', + relatedNamespaceId: 'public', + relatedTableName: 'posts', + localTableName: 'users', + targetColumns: ['user_id', 'title'], + localColumns: ['id', 'email'], cardinality: '1:N', }); }); @@ -164,6 +194,46 @@ describe('collection-contract capability detection', () => { ); }); + it('resolveIncludeRelation() throws when composite key arrays have unequal length', () => { + const unequal = withPatchedDomainModels(getTestContract(), (models) => ({ + ...models, + User: { + ...(models['User'] as Record), + relations: { + posts: { + to: { model: 'Post', namespace: 'public' }, + cardinality: '1:N', + on: { localFields: ['id', 'email'], targetFields: ['userId'] }, + }, + }, + }, + })); + + expect(() => resolveIncludeRelation(unequal, 'public', 'User', 'posts')).toThrow( + /incomplete join metadata/, + ); + }); + + it('resolveIncludeRelation() throws when a later composite key pair is empty', () => { + const emptyLater = withPatchedDomainModels(getTestContract(), (models) => ({ + ...models, + User: { + ...(models['User'] as Record), + relations: { + posts: { + to: { model: 'Post', namespace: 'public' }, + cardinality: '1:N', + on: { localFields: ['id', 'email'], targetFields: ['userId', ''] }, + }, + }, + }, + })); + + expect(() => resolveIncludeRelation(emptyLater, 'public', 'User', 'posts')).toThrow( + /incomplete join metadata/, + ); + }); + it('resolveUpsertConflictColumns() maps explicit criteria and falls back to primary key', () => { const contract = getTestContract(); diff --git a/packages/3-extensions/sql-orm-client/test/collection-dispatch.test.ts b/packages/3-extensions/sql-orm-client/test/collection-dispatch.test.ts index 551f60101276..0cc5a7dfbd23 100644 --- a/packages/3-extensions/sql-orm-client/test/collection-dispatch.test.ts +++ b/packages/3-extensions/sql-orm-client/test/collection-dispatch.test.ts @@ -31,8 +31,8 @@ function includeFor( relatedTableName: relation.relatedTableName, relatedNamespaceId: relation.relatedNamespaceId, localTableName: relation.localTableName, - targetColumn: relation.targetColumn, - localColumn: relation.localColumn, + targetColumns: relation.targetColumns, + localColumns: relation.localColumns, cardinality: relation.cardinality, nested, scalar: undefined, diff --git a/packages/3-extensions/sql-orm-client/test/collection.state.test.ts b/packages/3-extensions/sql-orm-client/test/collection.state.test.ts index fd56de8770ba..e5b5ec650240 100644 --- a/packages/3-extensions/sql-orm-client/test/collection.state.test.ts +++ b/packages/3-extensions/sql-orm-client/test/collection.state.test.ts @@ -188,7 +188,7 @@ describe('Collection', () => { relationName: 'posts', relatedModelName: 'Post', relatedTableName: 'posts', - targetColumn: 'user_id', + targetColumns: ['user_id'], cardinality: '1:N', }); expect(withPosts.state.includes[0]?.nested.filters).toEqual([ @@ -240,8 +240,8 @@ describe('Collection', () => { relationName: 'author', relatedModelName: 'User', relatedTableName: 'users', - targetColumn: 'id', - localColumn: 'user_id', + targetColumns: ['id'], + localColumns: ['user_id'], cardinality: 'N:1', }); diff --git a/packages/3-extensions/sql-orm-client/test/query-plan-select.test.ts b/packages/3-extensions/sql-orm-client/test/query-plan-select.test.ts index d547e0bc6397..45e177040fc5 100644 --- a/packages/3-extensions/sql-orm-client/test/query-plan-select.test.ts +++ b/packages/3-extensions/sql-orm-client/test/query-plan-select.test.ts @@ -144,6 +144,66 @@ describe('compileSelectWithIncludes', () => { ); }); + it('correlates a composite foreign key on every column pair', () => { + const include: IncludeExpr = { + relationName: 'posts', + relatedModelName: 'Post', + relatedNamespaceId: 'public', + relatedTableName: 'posts', + localTableName: 'users', + targetColumns: ['user_id', 'title'], + localColumns: ['id', 'email'], + cardinality: '1:N', + nested: emptyState(), + scalar: undefined, + combine: undefined, + }; + + const plan = compileSelectWithIncludes(baseContract, getTestAggregates(), 'public', 'users', { + ...emptyState(), + includes: [include], + }); + + expectSelectAst(plan.ast); + const postsProjection = plan.ast.projection.find((item) => item.alias === 'posts'); + expectSubqueryExpr(postsProjection?.expr); + + const childRowsSource = postsProjection.expr.query.from; + expectDerivedTableSource(childRowsSource); + + // Correlating on `user_id` alone would match every post sharing it, + // so both pairs of the key have to appear. + expect(childRowsSource.query.where).toEqual( + AndExpr.of([ + BinaryExpr.eq(ColumnRef.of('posts', 'user_id'), ColumnRef.of('users', 'id')), + BinaryExpr.eq(ColumnRef.of('posts', 'title'), ColumnRef.of('users', 'email')), + ]), + ); + }); + + it('throws when an include correlating a composite key has unequal column lists', () => { + const include: IncludeExpr = { + relationName: 'posts', + relatedModelName: 'Post', + relatedNamespaceId: 'public', + relatedTableName: 'posts', + localTableName: 'users', + targetColumns: ['user_id'], + localColumns: ['id', 'email'], + cardinality: '1:N', + nested: emptyState(), + scalar: undefined, + combine: undefined, + }; + + expect(() => + compileSelectWithIncludes(baseContract, getTestAggregates(), 'public', 'users', { + ...emptyState(), + includes: [include], + }), + ).toThrow(/incomplete join metadata/); + }); + it('builds lexicographic cursor filters with distinctOn, limit, and offset', () => { const { collection } = createCollection(); const state = collection @@ -1400,8 +1460,8 @@ describe('compileSelectWithIncludes polymorphic targets', () => { relatedTableName: relation.relatedTableName, relatedNamespaceId: relation.relatedNamespaceId, localTableName: relation.localTableName, - targetColumn: relation.targetColumn, - localColumn: relation.localColumn, + targetColumns: relation.targetColumns, + localColumns: relation.localColumns, cardinality: relation.cardinality, nested, scalar: undefined, diff --git a/packages/3-extensions/sql-orm-client/test/variant-include.collection-contract.test.ts b/packages/3-extensions/sql-orm-client/test/variant-include.collection-contract.test.ts index 6651e02409ac..18b17c165ad4 100644 --- a/packages/3-extensions/sql-orm-client/test/variant-include.collection-contract.test.ts +++ b/packages/3-extensions/sql-orm-client/test/variant-include.collection-contract.test.ts @@ -17,8 +17,8 @@ describe('resolveIncludeRelation() with a selected parent variant', () => { relatedNamespaceId: 'public', relatedTableName: 'assignees', localTableName: 'features', - localColumn: 'assignee_id', - targetColumn: 'id', + localColumns: ['assignee_id'], + targetColumns: ['id'], cardinality: 'N:1', }); }); @@ -37,8 +37,8 @@ describe('resolveIncludeRelation() with a selected parent variant', () => { relatedNamespaceId: 'public', relatedTableName: 'assignees', localTableName: 'tasks', - localColumn: 'assignee_id', - targetColumn: 'id', + localColumns: ['assignee_id'], + targetColumns: ['id'], cardinality: 'N:1', }); }); @@ -57,8 +57,8 @@ describe('resolveIncludeRelation() with a selected parent variant', () => { relatedNamespaceId: 'public', relatedTableName: 'tasks', localTableName: 'tasks', - localColumn: 'id', - targetColumn: 'parent_id', + localColumns: ['id'], + targetColumns: ['parent_id'], cardinality: '1:N', }); }); diff --git a/packages/3-extensions/sql-orm-client/test/variant-include.query-plan-fixtures.ts b/packages/3-extensions/sql-orm-client/test/variant-include.query-plan-fixtures.ts index 44df4657ba4b..82494b992bde 100644 --- a/packages/3-extensions/sql-orm-client/test/variant-include.query-plan-fixtures.ts +++ b/packages/3-extensions/sql-orm-client/test/variant-include.query-plan-fixtures.ts @@ -45,8 +45,8 @@ export function includeExpr(options: { relatedNamespaceId: 'public', relatedTableName: options.relatedTableName, localTableName: options.localTableName, - targetColumn: options.targetColumn, - localColumn: options.localColumn, + targetColumns: [options.targetColumn], + localColumns: [options.localColumn], cardinality: options.cardinality, ...ifDefined('through', options.through), nested: options.nested ?? emptyState(),