From 4660c25d782109e7576c548e560150388fe886ee Mon Sep 17 00:00:00 2001 From: Arda TANRIKULU Date: Fri, 7 Feb 2025 17:50:40 +0300 Subject: [PATCH 1/3] fix(stitch): handle isolated abstract types correctly --- .changeset/honest-terms-jump.md | 8 ++++ .changeset/tender-geese-perform.md | 5 ++ .../stitch/src/createDelegationPlanBuilder.ts | 4 +- .../isolateComputedFieldsTransformer.ts | 47 +++++++------------ .../isolateComputedFieldsTransformer.test.ts | 24 +++++----- 5 files changed, 45 insertions(+), 43 deletions(-) create mode 100644 .changeset/honest-terms-jump.md create mode 100644 .changeset/tender-geese-perform.md diff --git a/.changeset/honest-terms-jump.md b/.changeset/honest-terms-jump.md new file mode 100644 index 000000000..3c6308cdf --- /dev/null +++ b/.changeset/honest-terms-jump.md @@ -0,0 +1,8 @@ +--- +'@graphql-tools/stitch': patch +--- + +Fix a bug while isolating computed abstract type fields + +When a field in an interface needs to be isolated, +it should not remove the field from the base subschema if it is used by other members of the base subschema. diff --git a/.changeset/tender-geese-perform.md b/.changeset/tender-geese-perform.md new file mode 100644 index 000000000..4f8e09a77 --- /dev/null +++ b/.changeset/tender-geese-perform.md @@ -0,0 +1,5 @@ +--- +'@graphql-tools/stitch': patch +--- + +While calculating the best subschema for a selection set, respect fragments diff --git a/packages/stitch/src/createDelegationPlanBuilder.ts b/packages/stitch/src/createDelegationPlanBuilder.ts index 92fb2bbd2..f315eb77d 100644 --- a/packages/stitch/src/createDelegationPlanBuilder.ts +++ b/packages/stitch/src/createDelegationPlanBuilder.ts @@ -211,7 +211,7 @@ function calculateDelegationStage( } if (delegationMap.size > 1) { - optimizeDelegationMap(delegationMap, mergedTypeInfo.typeName); + optimizeDelegationMap(delegationMap, mergedTypeInfo.typeName, fragments); } return { @@ -452,6 +452,7 @@ export function createDelegationPlanBuilder( function optimizeDelegationMap( delegationMap: Map, typeName: string, + fragments: Record, ): Map { for (const [subschema, selectionSet] of delegationMap) { for (const [subschema2, selectionSet2] of delegationMap) { @@ -464,6 +465,7 @@ function optimizeDelegationMap( subschema2.transformedSchema.getType(typeName) as GraphQLObjectType, selectionSet, () => true, + fragments, ); if (!unavailableFields.length) { delegationMap.set(subschema2, { diff --git a/packages/stitch/src/subschemaConfigTransforms/isolateComputedFieldsTransformer.ts b/packages/stitch/src/subschemaConfigTransforms/isolateComputedFieldsTransformer.ts index afc5bbb12..d16eb8bf0 100644 --- a/packages/stitch/src/subschemaConfigTransforms/isolateComputedFieldsTransformer.ts +++ b/packages/stitch/src/subschemaConfigTransforms/isolateComputedFieldsTransformer.ts @@ -82,6 +82,7 @@ export function isolateComputedFieldsTransformer( if (mergedTypeConfig.selectionSet) { const parsedSelectionSet = parseSelectionSet( mergedTypeConfig.selectionSet, + { noLocation: true }, ); const keyFields = collectFields( subschemaConfig.schema, @@ -96,6 +97,7 @@ export function isolateComputedFieldsTransformer( if (entryPoint.selectionSet) { const parsedSelectionSet = parseSelectionSet( entryPoint.selectionSet, + { noLocation: true }, ); const keyFields = collectFields( subschemaConfig.schema, @@ -168,6 +170,7 @@ export function isolateComputedFieldsTransformer( const keyFieldNames: string[] = []; const parsedSelectionSet = parseSelectionSet( returnTypeSelectionSet, + { noLocation: true }, ); const keyFields = collectFields( subschemaConfig.schema, @@ -182,6 +185,7 @@ export function isolateComputedFieldsTransformer( if (entryPoint.selectionSet) { const parsedSelectionSet = parseSelectionSet( entryPoint.selectionSet, + { noLocation: true }, ); const keyFields = collectFields( subschemaConfig.schema, @@ -244,10 +248,7 @@ export function isolateComputedFieldsTransformer( if (Object.keys(isolatedSchemaTypes).length) { return [ - filterIsolatedSubschema({ - ...subschemaConfig, - merge: isolatedSchemaTypes, - }), + filterIsolatedSubschema(subschemaConfig, isolatedSchemaTypes), filterBaseSubschema( { ...subschemaConfig, merge: baseSchemaTypes }, isolatedSchemaTypes, @@ -322,7 +323,7 @@ function filterBaseSubschema( } } const allTypes = [typeName, ...iFacesForType]; - const isIsolatedFieldName = allTypes.some((implementingTypeName) => + const isIsolatedFieldName = allTypes.every((implementingTypeName) => isIsolatedField(implementingTypeName, fieldName, isolatedSchemaTypes), ); const isKeyFieldName = allTypes.some((implementingTypeName) => @@ -356,7 +357,7 @@ function filterBaseSubschema( ...iFacesForType, ...typesForInterface[typeName], ]; - const isIsolatedFieldName = allTypes.some((implementingTypeName) => + const isIsolatedFieldName = allTypes.every((implementingTypeName) => isIsolatedField(implementingTypeName, fieldName, isolatedSchemaTypes), ); const isKeyFieldName = allTypes.some((implementingTypeName) => @@ -409,14 +410,11 @@ function filterBaseSubschema( return filteredSubschema; } -type IsolatedSubschemaInput = Exclude & { - merge: Record; -}; - function filterIsolatedSubschema( - subschemaConfig: IsolatedSubschemaInput, - computedFieldTypes: Record = {}, // contains types of computed fields that have no root field + subschemaConfig: SubschemaConfig, + isolatedSchemaTypes: Record, ): SubschemaConfig { + const computedFieldTypes: Record = {}; const queryRootFields: Record = {}; function listReachableTypesToIsolate( subschemaConfig: SubschemaConfig, @@ -427,7 +425,7 @@ function filterIsolatedSubschema( return typeNames; } else if ( (isObjectType(type) || isInterfaceType(type)) && - subschemaConfig.merge?.[type.name]?.selectionSet + subschemaConfig.merge?.[type.name] ) { // this is a merged type, no need to descend further typeNames.add(type.name); @@ -562,11 +560,9 @@ function filterIsolatedSubschema( return true; } return ( - subschemaConfig.merge[typeName] == null || + subschemaConfig.merge?.[typeName] == null || subschemaConfig.merge[typeName]?.fields?.[fieldName] != null || - (subschemaConfig.merge[typeName]?.keyFieldNames ?? []).includes( - fieldName, - ) + (isolatedSchemaTypes[typeName]?.keyFieldNames ?? []).includes(fieldName) ); }, interfaceFieldFilter: (typeName, fieldName, config) => { @@ -588,12 +584,8 @@ function filterIsolatedSubschema( } const isIsolatedFieldName = typesForInterface[typeName].some((implementingTypeName) => - isIsolatedField( - implementingTypeName, - fieldName, - subschemaConfig.merge, - ), - ) || subschemaConfig.merge[typeName]?.fields?.[fieldName] != null; + isIsolatedField(implementingTypeName, fieldName, isolatedSchemaTypes), + ) || subschemaConfig.merge?.[typeName]?.fields?.[fieldName] != null; const isComputedFieldType = typesForInterface[typeName].some( (implementingTypeName) => { if (computedFieldTypes[implementingTypeName]) { @@ -615,19 +607,16 @@ function filterIsolatedSubschema( isComputedFieldType || typesForInterface[typeName].some((implementingTypeName) => ( - subschemaConfig.merge[implementingTypeName]?.keyFieldNames ?? [] + isolatedSchemaTypes?.[implementingTypeName]?.keyFieldNames ?? [] ).includes(fieldName), ) || - (subschemaConfig.merge[typeName]?.keyFieldNames ?? []).includes( - fieldName, - ) + (isolatedSchemaTypes[typeName]?.keyFieldNames ?? []).includes(fieldName) ); }, }); - const merge = Object.fromEntries( // get rid of keyFieldNames again - Object.entries(subschemaConfig.merge).map( + Object.entries(isolatedSchemaTypes).map( ([typeName, { keyFieldNames, ...config }]) => [typeName, config], ), ); diff --git a/packages/stitch/tests/isolateComputedFieldsTransformer.test.ts b/packages/stitch/tests/isolateComputedFieldsTransformer.test.ts index 29d58e6b9..64fb66902 100644 --- a/packages/stitch/tests/isolateComputedFieldsTransformer.test.ts +++ b/packages/stitch/tests/isolateComputedFieldsTransformer.test.ts @@ -465,15 +465,16 @@ describe('isolateComputedFieldsTransformer', () => { const baseSubschema = new Subschema(baseConfig); const computedSubschema = new Subschema(computedConfig); - expect( - Object.keys( - ( - baseSubschema.transformedSchema.getType( - 'IProduct', - ) as GraphQLInterfaceType - ).getFields(), - ), - ).toEqual(['base']); + // /* It doesn't matter what interface has */ + // expect( + // Object.keys( + // ( + // baseSubschema.transformedSchema.getType( + // 'IProduct', + // ) as GraphQLInterfaceType + // ).getFields(), + // ), + // ).toEqual(['base']); expect( Object.keys( ( @@ -543,10 +544,7 @@ describe('isolateComputedFieldsTransformer', () => { throw new Error('Expected both configs to be defined'); } expect(printSchema(new Subschema(baseConfig).transformedSchema).trim()) - .toBe(`interface IProduct { - base: String! -} - + .toContain(` type Product implements IProduct { base: String! } From 5edffeca8780b926aed3d2b70fa56959f522b0f8 Mon Sep 17 00:00:00 2001 From: Arda TANRIKULU Date: Fri, 7 Feb 2025 19:21:36 +0300 Subject: [PATCH 2/3] Add tests for ffragment handling in delegation optimization' --- .../stitch/src/createDelegationPlanBuilder.ts | 2 +- .../tests/createDelegationPlanBuilder.test.ts | 144 ++++++++++++++++++ 2 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 packages/stitch/tests/createDelegationPlanBuilder.test.ts diff --git a/packages/stitch/src/createDelegationPlanBuilder.ts b/packages/stitch/src/createDelegationPlanBuilder.ts index f315eb77d..753df35a7 100644 --- a/packages/stitch/src/createDelegationPlanBuilder.ts +++ b/packages/stitch/src/createDelegationPlanBuilder.ts @@ -449,7 +449,7 @@ export function createDelegationPlanBuilder( }); } -function optimizeDelegationMap( +export function optimizeDelegationMap( delegationMap: Map, typeName: string, fragments: Record, diff --git a/packages/stitch/tests/createDelegationPlanBuilder.test.ts b/packages/stitch/tests/createDelegationPlanBuilder.test.ts new file mode 100644 index 000000000..bec49b7db --- /dev/null +++ b/packages/stitch/tests/createDelegationPlanBuilder.test.ts @@ -0,0 +1,144 @@ +import { Subschema } from '@graphql-tools/delegate'; +import { makeExecutableSchema } from '@graphql-tools/schema'; +import { FragmentDefinitionNode, Kind } from 'graphql'; +import { describe, expect, it } from 'vitest'; +import { optimizeDelegationMap } from '../src/createDelegationPlanBuilder'; + +/** + * Tests for fragment handling in delegation optimization. + * These tests verify that the delegation plan builder correctly handles GraphQL fragments + * when optimizing the delegation map across multiple subschemas. + */ +describe('fragment handling in delegation optimization', () => { + /** + * First test schema representing a basic User type with profile information. + * This schema lacks the email field in Profile to test partial field availability. + */ + const schema1 = makeExecutableSchema({ + typeDefs: /* GraphQL */ ` + type User { + id: ID! + name: String! + profile: Profile! + } + type Profile { + bio: String! + location: String! + } + type Query { + user(id: ID!): User + } + `, + }); + + /** + * Second test schema with an extended Profile type that includes an email field. + * Used to test fragment spreading across schemas with different capabilities. + */ + const schema2 = makeExecutableSchema({ + typeDefs: /* GraphQL */ ` + type User { + id: ID! + name: String! + profile: Profile! + } + type Profile { + bio: String! + location: String! + email: String! + } + type Query { + user(id: ID!): User + } + `, + }); + + const subschema1 = new Subschema({ schema: schema1 }); + const subschema2 = new Subschema({ schema: schema2 }); + + /** + * Tests that the delegation map optimization correctly handles fragments + * by merging fragment fields into the appropriate subschema selections. + */ + it('should optimize delegation map considering fragments', async () => { + // Define a fragment that selects basic User fields + const fragments: Record = { + UserFields: { + kind: Kind.FRAGMENT_DEFINITION, + name: { kind: Kind.NAME, value: 'UserFields' }, + typeCondition: { + kind: Kind.NAMED_TYPE, + name: { kind: Kind.NAME, value: 'User' }, + }, + selectionSet: { + kind: Kind.SELECTION_SET, + selections: [ + { + kind: Kind.FIELD, + name: { kind: Kind.NAME, value: 'id' }, + }, + { + kind: Kind.FIELD, + name: { kind: Kind.NAME, value: 'name' }, + }, + ], + }, + }, + }; + + // Set up initial delegation map with fragment spread and profile selection + const delegationMap = new Map(); + delegationMap.set(subschema1, { + kind: Kind.SELECTION_SET, + selections: [ + { + kind: Kind.FRAGMENT_SPREAD, + name: { kind: Kind.NAME, value: 'UserFields' }, + }, + ], + }); + delegationMap.set(subschema2, { + kind: Kind.SELECTION_SET, + selections: [ + { + kind: Kind.FIELD, + name: { kind: Kind.NAME, value: 'profile' }, + selectionSet: { + kind: Kind.SELECTION_SET, + selections: [ + { + kind: Kind.FIELD, + name: { kind: Kind.NAME, value: 'email' }, + }, + ], + }, + }, + ], + }); + + optimizeDelegationMap(delegationMap, 'User', fragments); + // Verify optimization results + expect(delegationMap.size).toBe(1); + expect(delegationMap.has(subschema2)).toBe(true); + const schema2Selections = delegationMap.get(subschema2)!.selections; + expect(schema2Selections).toEqual([ + { + kind: Kind.FIELD, + name: { kind: Kind.NAME, value: 'profile' }, + selectionSet: { + kind: Kind.SELECTION_SET, + selections: [ + { + kind: Kind.FIELD, + name: { kind: Kind.NAME, value: 'email' }, + }, + ], + }, + }, + { + kind: Kind.FRAGMENT_SPREAD, + name: { kind: Kind.NAME, value: 'UserFields' }, + }, + ]); + }); +}); From 2be78396970d203b3f02c218e47936545ef66ba9 Mon Sep 17 00:00:00 2001 From: Arda TANRIKULU Date: Mon, 10 Feb 2025 17:21:58 +0300 Subject: [PATCH 3/3] .. --- packages/stitch/tests/createDelegationPlanBuilder.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/stitch/tests/createDelegationPlanBuilder.test.ts b/packages/stitch/tests/createDelegationPlanBuilder.test.ts index bec49b7db..5483115b5 100644 --- a/packages/stitch/tests/createDelegationPlanBuilder.test.ts +++ b/packages/stitch/tests/createDelegationPlanBuilder.test.ts @@ -60,7 +60,7 @@ describe('fragment handling in delegation optimization', () => { * Tests that the delegation map optimization correctly handles fragments * by merging fragment fields into the appropriate subschema selections. */ - it('should optimize delegation map considering fragments', async () => { + it('should optimize delegation map considering fragments', () => { // Define a fragment that selects basic User fields const fragments: Record = { UserFields: {