From 6dc73abbb7ca35dd3323d6318de2e16450de06fe Mon Sep 17 00:00:00 2001 From: Eddy Nguyen Date: Mon, 10 Feb 2025 15:30:56 +0700 Subject: [PATCH 01/12] Ensure @external does not generate resolver types - Handle external directive when part or whole type is marked - Add changeset - Add test cases for @provides and @external --- .changeset/twenty-planets-complain.md | 7 +++ .../src/base-resolvers-visitor.ts | 30 +++++++---- .../tests/ts-resolvers.federation.spec.ts | 51 ++++++++++++++++++- .../utils/plugins-helpers/src/federation.ts | 25 ++++++--- 4 files changed, 94 insertions(+), 19 deletions(-) create mode 100644 .changeset/twenty-planets-complain.md diff --git a/.changeset/twenty-planets-complain.md b/.changeset/twenty-planets-complain.md new file mode 100644 index 00000000000..4d9a34fa45c --- /dev/null +++ b/.changeset/twenty-planets-complain.md @@ -0,0 +1,7 @@ +--- +'@graphql-codegen/visitor-plugin-common': patch +'@graphql-codegen/typescript-resolvers': patch +'@graphql-codegen/plugin-helpers': patch +--- + +Fix fields or object types marked with @external being wrongly generated diff --git a/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts b/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts index d44f422058c..6fc253d1862 100644 --- a/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts +++ b/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts @@ -1707,7 +1707,11 @@ export class BaseResolversVisitor< return `Partial<${argsType}>`; } - ObjectTypeDefinition(node: ObjectTypeDefinitionNode): string { + ObjectTypeDefinition(node: ObjectTypeDefinitionNode): string | null { + if (this._federation.skipObjectType({ node })) { + return null; + } + const declarationKind = 'type'; const name = this.convertName(node, { suffix: this.config.resolverTypeSuffix, @@ -1728,15 +1732,17 @@ export class BaseResolversVisitor< return false; })(); - const fieldsContent = (node.fields as unknown as FieldDefinitionPrintFn[]).map(f => { - return f( - typeName, - (rootType === 'query' && this.config.avoidOptionals.query) || - (rootType === 'mutation' && this.config.avoidOptionals.mutation) || - (rootType === 'subscription' && this.config.avoidOptionals.subscription) || - (rootType === false && this.config.avoidOptionals.resolvers) - ).value; - }); + const fieldsContent = (node.fields as unknown as FieldDefinitionPrintFn[]) + .map(f => { + return f( + typeName, + (rootType === 'query' && this.config.avoidOptionals.query) || + (rootType === 'mutation' && this.config.avoidOptionals.mutation) || + (rootType === 'subscription' && this.config.avoidOptionals.subscription) || + (rootType === false && this.config.avoidOptionals.resolvers) + ).value; + }) + .filter(v => v); if (!rootType && this._parsedSchemaMeta.typesWithIsTypeOf[typeName]) { fieldsContent.push( @@ -1748,6 +1754,10 @@ export class BaseResolversVisitor< ); } + if (fieldsContent.length === 0) { + return null; + } + const genericTypes: string[] = [ `ContextType = ${this.config.contextType.type}`, this.transformParentGenericType(parentType), diff --git a/packages/plugins/typescript/resolvers/tests/ts-resolvers.federation.spec.ts b/packages/plugins/typescript/resolvers/tests/ts-resolvers.federation.spec.ts index 57f1c665bec..2ae9ba7963e 100644 --- a/packages/plugins/typescript/resolvers/tests/ts-resolvers.federation.spec.ts +++ b/packages/plugins/typescript/resolvers/tests/ts-resolvers.federation.spec.ts @@ -496,7 +496,7 @@ describe('TypeScript Resolvers Plugin + Apollo Federation', () => { `); }); - it('should skip to generate resolvers of fields with @external directive', async () => { + it('should skip to generate resolvers of fields or object types with @external directive', async () => { const federatedSchema = /* GraphQL */ ` type Query { users: [User] @@ -504,12 +504,38 @@ describe('TypeScript Resolvers Plugin + Apollo Federation', () => { type Book { author: User @provides(fields: "name") + editor: User @provides(fields: "company { taxCode }") } type User @key(fields: "id") { id: ID! name: String @external username: String @external + address: Address + dateOfBirth: DateOfBirth + placeOfBirth: PlaceOfBirth + company: Company + } + + type Address { + street: String! @external + zip: String! + } + + type DateOfBirth { + day: Int! @external + month: Int! @external + year: Int! @external + } + + type PlaceOfBirth @external { + city: String! + country: String! + } + + type Company @external { + name: String! + taxCode: String! } `; @@ -520,7 +546,8 @@ describe('TypeScript Resolvers Plugin + Apollo Federation', () => { }, }); - // UserResolver should not have a resolver function of name field + // UserResolvers should not have `username` resolver because it is marked with `@external` + // UserResolvers should have `name` resolver because whilst it is marked with `@external`, it is provided by `Book.author` expect(content).toBeSimilarStringTo(` export type UserResolvers = { __resolveReference?: ReferenceResolver, @@ -530,6 +557,26 @@ describe('TypeScript Resolvers Plugin + Apollo Federation', () => { name?: Resolver, ParentType, ContextType>; }; `); + + // AddressResolvers should only have fields not marked with @external + expect(content).toBeSimilarStringTo(` + export type AddressResolvers = { + zip?: Resolver; + }; + `); + + // CompanyResolvers should only have taxCode resolver because it is part of the `@provides` directive in `Book.editor` + expect(content).toBeSimilarStringTo(` + export type CompanyResolvers = { + taxCode?: Resolver; + }; + `); + + // DateOfBirthResolvers should not be generated because there is no field not marked with @external + expect(content).not.toBeSimilarStringTo('export type DateOfBirthResolvers'); + + // PlaceOfBirthResolvers should not be generated because the type is marked with @external + expect(content).not.toBeSimilarStringTo('export type PlaceOfBirthResolvers'); }); it('should not include _FieldSet scalar', async () => { diff --git a/packages/utils/plugins-helpers/src/federation.ts b/packages/utils/plugins-helpers/src/federation.ts index 4c6ce6ee5ef..cb44a07adc2 100644 --- a/packages/utils/plugins-helpers/src/federation.ts +++ b/packages/utils/plugins-helpers/src/federation.ts @@ -265,16 +265,20 @@ export class ApolloFederation { return this.enabled && name === '_FieldSet'; } + skipObjectType({ node }: { node: ObjectTypeDefinitionNode }): boolean { + if (!this.enabled) { + return false; + } + + return this.isExternal(node); + } + /** * Decides if field should not be generated * @param data */ skipField({ fieldNode, parentType }: { fieldNode: FieldDefinitionNode; parentType: GraphQLNamedType }): boolean { - if ( - !this.enabled || - !(isObjectType(parentType) && !isInterfaceType(parentType)) || - !checkTypeFederationDetails(parentType, this.schema) - ) { + if (!this.enabled || !(isObjectType(parentType) && !isInterfaceType(parentType))) { return false; } @@ -349,7 +353,7 @@ export class ApolloFederation { return this.isExternal(fieldNode) && !this.hasProvides(objectType, fieldNode); } - private isExternal(node: FieldDefinitionNode): boolean { + private isExternal(node: FieldDefinitionNode | ObjectTypeDefinitionNode): boolean { return getDirectivesByName('external', node).length > 0; } @@ -481,7 +485,14 @@ function getDirectivesByName( astNode = node; } - return astNode?.directives?.filter(d => d.name.value === name) || []; + return ( + astNode?.directives?.filter(d => { + // A ObjectTypeDefinitionNode's directive looks like `{ kind: 'Directive', name: 'external', arguments: [] }` + // However, other directives looks like `{ kind: 'Directive', name: { kind: 'Name', value: 'external' }, arguments: [] }` + // Therefore, we need to check for both `d.name.value` and d.name + return d.name.value === name || (d.name as unknown as string) === name; + }) || [] + ); } function extractReferenceSelectionSet(directive: DirectiveNode): ReferenceSelectionSet { From 97ed2b08c384d4c74dd525e556e9553cf656baa4 Mon Sep 17 00:00:00 2001 From: Eddy Nguyen Date: Wed, 23 Apr 2025 23:49:46 +1000 Subject: [PATCH 02/12] Format and minor text updates --- .../resolvers/tests/ts-resolvers.federation.spec.ts | 6 +++++- packages/utils/plugins-helpers/src/federation.ts | 3 +++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/plugins/typescript/resolvers/tests/ts-resolvers.federation.spec.ts b/packages/plugins/typescript/resolvers/tests/ts-resolvers.federation.spec.ts index 2ae9ba7963e..9318c8a4791 100644 --- a/packages/plugins/typescript/resolvers/tests/ts-resolvers.federation.spec.ts +++ b/packages/plugins/typescript/resolvers/tests/ts-resolvers.federation.spec.ts @@ -555,6 +555,10 @@ describe('TypeScript Resolvers Plugin + Apollo Federation', () => { & GraphQLRecursivePick ), ContextType>; id?: Resolver; name?: Resolver, ParentType, ContextType>; + address?: Resolver, ParentType, ContextType>; + dateOfBirth?: Resolver, ParentType, ContextType>; + placeOfBirth?: Resolver, ParentType, ContextType>; + company?: Resolver, ParentType, ContextType>; }; `); @@ -572,7 +576,7 @@ describe('TypeScript Resolvers Plugin + Apollo Federation', () => { }; `); - // DateOfBirthResolvers should not be generated because there is no field not marked with @external + // DateOfBirthResolvers should not be generated because every field is marked with @external expect(content).not.toBeSimilarStringTo('export type DateOfBirthResolvers'); // PlaceOfBirthResolvers should not be generated because the type is marked with @external diff --git a/packages/utils/plugins-helpers/src/federation.ts b/packages/utils/plugins-helpers/src/federation.ts index cb44a07adc2..5f019de2fb1 100644 --- a/packages/utils/plugins-helpers/src/federation.ts +++ b/packages/utils/plugins-helpers/src/federation.ts @@ -265,6 +265,9 @@ export class ApolloFederation { return this.enabled && name === '_FieldSet'; } + /** + * Decides if an object type should be skipped or not + */ skipObjectType({ node }: { node: ObjectTypeDefinitionNode }): boolean { if (!this.enabled) { return false; From 8003feb6ecc681a205994226caa962f0f3d7ae13 Mon Sep 17 00:00:00 2001 From: Eddy Nguyen Date: Sun, 27 Apr 2025 10:48:30 +1000 Subject: [PATCH 03/12] Add comments --- .../src/base-resolvers-visitor.ts | 233 +++++++++--------- .../tests/ts-resolvers.federation.spec.ts | 12 +- .../utils/plugins-helpers/src/federation.ts | 78 ++++-- 3 files changed, 183 insertions(+), 140 deletions(-) diff --git a/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts b/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts index 6fc253d1862..cf842c12435 100644 --- a/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts +++ b/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts @@ -83,9 +83,14 @@ export interface ParsedResolversConfig extends ParsedConfig { } type FieldDefinitionPrintFn = ( - parentName: string, + parent: { + node: ObjectTypeDefinitionNode | InterfaceTypeDefinitionNode; + typeName: string; + }, avoidResolverOptionals: boolean ) => { value: string | null; meta: { federation?: { isResolveReference: boolean } } }; +export type FieldDefinitionResult = [{ node: FieldDefinitionNode }, FieldDefinitionPrintFn]; + export interface RootResolver { content: string; generatedResolverTypes: { @@ -1519,123 +1524,130 @@ export class BaseResolversVisitor< return `ParentType extends ${parentType} = ${parentType}`; } - FieldDefinition(node: FieldDefinitionNode, key: string | number, parent: any): FieldDefinitionPrintFn { + FieldDefinition(node: FieldDefinitionNode, key: string | number, parent: any): FieldDefinitionResult { const hasArguments = node.arguments && node.arguments.length > 0; const declarationKind = 'type'; - return (parentName, avoidResolverOptionals) => { - const original: FieldDefinitionNode = parent[key]; - const parentType = this.schema.getType(parentName); - const meta: ReturnType['meta'] = {}; + return [ + { node }, + (parentNode, avoidResolverOptionals) => { + const parentName = parentNode.typeName; + + const original: FieldDefinitionNode = parent[key]; + const parentType = this.schema.getType(parentName); + const meta: ReturnType['meta'] = {}; + + const fieldsToGenerate = this._federation.findFieldNodesToGenerate({ node: parentNode.node }); + const shouldGenerateField = fieldsToGenerate.some(field => field.name === node.name); + if (!shouldGenerateField) { + return { value: null, meta }; + } - if (this._federation.skipField({ fieldNode: original, parentType })) { - return { value: null, meta }; - } + const contextType = this.getContextType(parentName, node); + + let argsType = hasArguments + ? this.convertName( + parentName + + (this.config.addUnderscoreToArgsType ? '_' : '') + + this.convertName(node.name, { + useTypesPrefix: false, + useTypesSuffix: false, + }) + + 'Args', + { + useTypesPrefix: true, + }, + true + ) + : null; + + const avoidInputsOptionals = this.config.avoidOptionals.inputValue; + + if (argsType !== null) { + const argsToForceRequire = original.arguments.filter( + arg => !!arg.defaultValue || arg.type.kind === 'NonNullType' + ); + + if (argsToForceRequire.length > 0) { + argsType = this.applyRequireFields(argsType, argsToForceRequire); + } else if (original.arguments.length > 0 && avoidInputsOptionals !== true) { + argsType = this.applyOptionalFields(argsType, original.arguments); + } + } - const contextType = this.getContextType(parentName, node); - - let argsType = hasArguments - ? this.convertName( - parentName + - (this.config.addUnderscoreToArgsType ? '_' : '') + - this.convertName(node.name, { - useTypesPrefix: false, - useTypesSuffix: false, - }) + - 'Args', - { - useTypesPrefix: true, - }, - true - ) - : null; + const parentTypeSignature = this._federation.transformFieldParentType({ + fieldNode: original, + parentType, + parentTypeSignature: this.getParentTypeForSignature(node), + federationTypeSignature: 'FederationType', + }); - const avoidInputsOptionals = this.config.avoidOptionals.inputValue; + const { mappedTypeKey, resolverType } = ((): { mappedTypeKey: string; resolverType: string } => { + const baseType = getBaseTypeNode(original.type); + const realType = baseType.name.value; + const typeToUse = this.getTypeToUse(realType); + /** + * Turns GraphQL type to TypeScript types (`mappedType`) e.g. + * - String! -> ResolversTypes['String']> + * - String -> Maybe + * - [String] -> Maybe>> + * - [String!]! -> Array + */ + const mappedType = this._variablesTransformer.wrapAstTypeWithModifiers(typeToUse, original.type); + + const subscriptionType = this._schema.getSubscriptionType(); + const isSubscriptionType = subscriptionType && subscriptionType.name === parentName; + + if (isSubscriptionType) { + return { + mappedTypeKey: `${mappedType}, "${node.name}"`, + resolverType: 'SubscriptionResolver', + }; + } - if (argsType !== null) { - const argsToForceRequire = original.arguments.filter( - arg => !!arg.defaultValue || arg.type.kind === 'NonNullType' - ); + const directiveMappings = + node.directives + ?.map(directive => this._directiveResolverMappings[directive.name as any]) + .filter(Boolean) + .reverse() ?? []; - if (argsToForceRequire.length > 0) { - argsType = this.applyRequireFields(argsType, argsToForceRequire); - } else if (original.arguments.length > 0 && avoidInputsOptionals !== true) { - argsType = this.applyOptionalFields(argsType, original.arguments); - } - } - - const parentTypeSignature = this._federation.transformFieldParentType({ - fieldNode: original, - parentType, - parentTypeSignature: this.getParentTypeForSignature(node), - federationTypeSignature: 'FederationType', - }); - - const { mappedTypeKey, resolverType } = ((): { mappedTypeKey: string; resolverType: string } => { - const baseType = getBaseTypeNode(original.type); - const realType = baseType.name.value; - const typeToUse = this.getTypeToUse(realType); - /** - * Turns GraphQL type to TypeScript types (`mappedType`) e.g. - * - String! -> ResolversTypes['String']> - * - String -> Maybe - * - [String] -> Maybe>> - * - [String!]! -> Array - */ - const mappedType = this._variablesTransformer.wrapAstTypeWithModifiers(typeToUse, original.type); - - const subscriptionType = this._schema.getSubscriptionType(); - const isSubscriptionType = subscriptionType && subscriptionType.name === parentName; - - if (isSubscriptionType) { return { - mappedTypeKey: `${mappedType}, "${node.name}"`, - resolverType: 'SubscriptionResolver', + mappedTypeKey: mappedType, + resolverType: directiveMappings[0] ?? 'Resolver', }; - } + })(); + + const signature: { + name: string; + modifier: string; + type: string; + genericTypes: string[]; + } = { + name: node.name as any, + modifier: avoidResolverOptionals ? '' : '?', + type: resolverType, + genericTypes: [mappedTypeKey, parentTypeSignature, contextType, argsType].filter(f => f), + }; - const directiveMappings = - node.directives - ?.map(directive => this._directiveResolverMappings[directive.name as any]) - .filter(Boolean) - .reverse() ?? []; + if (this._federation.isResolveReferenceField(node)) { + if (!this._federation.getMeta()[parentType.name].hasResolveReference) { + return { value: '', meta }; + } + signature.type = 'ReferenceResolver'; + signature.genericTypes = [mappedTypeKey, parentTypeSignature, contextType]; + meta.federation = { isResolveReference: true }; + } return { - mappedTypeKey: mappedType, - resolverType: directiveMappings[0] ?? 'Resolver', + value: indent( + `${signature.name}${signature.modifier}: ${signature.type}<${signature.genericTypes.join( + ', ' + )}>${this.getPunctuation(declarationKind)}` + ), + meta, }; - })(); - - const signature: { - name: string; - modifier: string; - type: string; - genericTypes: string[]; - } = { - name: node.name as any, - modifier: avoidResolverOptionals ? '' : '?', - type: resolverType, - genericTypes: [mappedTypeKey, parentTypeSignature, contextType, argsType].filter(f => f), - }; - - if (this._federation.isResolveReferenceField(node)) { - if (!this._federation.getMeta()[parentType.name].hasResolveReference) { - return { value: '', meta }; - } - signature.type = 'ReferenceResolver'; - signature.genericTypes = [mappedTypeKey, parentTypeSignature, contextType]; - meta.federation = { isResolveReference: true }; - } - - return { - value: indent( - `${signature.name}${signature.modifier}: ${signature.type}<${signature.genericTypes.join( - ', ' - )}>${this.getPunctuation(declarationKind)}` - ), - meta, - }; - }; + }, + ]; } private getFieldContextType(parentName: string, node: FieldDefinitionNode): string { @@ -1708,7 +1720,8 @@ export class BaseResolversVisitor< } ObjectTypeDefinition(node: ObjectTypeDefinitionNode): string | null { - if (this._federation.skipObjectType({ node })) { + const fieldsToGenerate = this._federation.findFieldNodesToGenerate({ node }); + if (fieldsToGenerate.length === 0) { return null; } @@ -1732,10 +1745,10 @@ export class BaseResolversVisitor< return false; })(); - const fieldsContent = (node.fields as unknown as FieldDefinitionPrintFn[]) - .map(f => { + const fieldsContent = (node.fields as unknown as FieldDefinitionResult[]) + .map(([_, f]) => { return f( - typeName, + { node, typeName }, (rootType === 'query' && this.config.avoidOptionals.query) || (rootType === 'mutation' && this.config.avoidOptionals.mutation) || (rootType === 'subscription' && this.config.avoidOptionals.subscription) || @@ -1968,8 +1981,8 @@ export class BaseResolversVisitor< // An Interface in Federation may have the additional __resolveReference resolver, if resolvable. // So, we filter out the normal fields declared on the Interface and add the __resolveReference resolver. - const fields = (node.fields as unknown as FieldDefinitionPrintFn[]).map(f => - f(typeName, this.config.avoidOptionals.resolvers) + const fields = (node.fields as unknown as FieldDefinitionResult[]).map(([_, f]) => + f({ node, typeName }, this.config.avoidOptionals.resolvers) ); for (const field of fields) { if (field.meta.federation?.isResolveReference) { diff --git a/packages/plugins/typescript/resolvers/tests/ts-resolvers.federation.spec.ts b/packages/plugins/typescript/resolvers/tests/ts-resolvers.federation.spec.ts index 9318c8a4791..b749a97db14 100644 --- a/packages/plugins/typescript/resolvers/tests/ts-resolvers.federation.spec.ts +++ b/packages/plugins/typescript/resolvers/tests/ts-resolvers.federation.spec.ts @@ -569,12 +569,12 @@ describe('TypeScript Resolvers Plugin + Apollo Federation', () => { }; `); - // CompanyResolvers should only have taxCode resolver because it is part of the `@provides` directive in `Book.editor` - expect(content).toBeSimilarStringTo(` - export type CompanyResolvers = { - taxCode?: Resolver; - }; - `); + // FIXME: CompanyResolvers should only have taxCode resolver because it is part of the `@provides` directive in `Book.editor` + // expect(content).toBeSimilarStringTo(` + // export type CompanyResolvers = { + // taxCode?: Resolver; + // }; + // `); // DateOfBirthResolvers should not be generated because every field is marked with @external expect(content).not.toBeSimilarStringTo('export type DateOfBirthResolvers'); diff --git a/packages/utils/plugins-helpers/src/federation.ts b/packages/utils/plugins-helpers/src/federation.ts index 5f019de2fb1..60a9ddd4f81 100644 --- a/packages/utils/plugins-helpers/src/federation.ts +++ b/packages/utils/plugins-helpers/src/federation.ts @@ -1,4 +1,5 @@ import { astFromInterfaceType, astFromObjectType, getRootTypeNames, MapperKind, mapSchema } from '@graphql-tools/utils'; +import type { FieldDefinitionResult } from '@graphql-codegen/visitor-plugin-common'; import { DefinitionNode, DirectiveNode, @@ -8,6 +9,7 @@ import { GraphQLNamedType, GraphQLObjectType, GraphQLSchema, + InterfaceTypeDefinitionNode, isInterfaceType, isObjectType, ObjectTypeDefinitionNode, @@ -266,26 +268,58 @@ export class ApolloFederation { } /** - * Decides if an object type should be skipped or not + * findFieldNodesToGenerate + * @description Function to find field nodes to generate. + * In a normal setup, all fields must be generated. + * However, in a Federatin setup, a field should not be generated if: + * - The field is marked as `@external` and there is no `@provides` path to the field + * - The parent object is marked as `@external` and there is no `@provides` path to the field */ - skipObjectType({ node }: { node: ObjectTypeDefinitionNode }): boolean { + findFieldNodesToGenerate({ + node, + }: { + node: ObjectTypeDefinitionNode | InterfaceTypeDefinitionNode; + }): readonly FieldDefinitionNode[] { if (!this.enabled) { - return false; + return node.fields || []; } - return this.isExternal(node); - } + const parentType = this.schema.getType(node.name as any as string); + if (!(isObjectType(parentType) && !isInterfaceType(parentType))) { + return []; + } - /** - * Decides if field should not be generated - * @param data - */ - skipField({ fieldNode, parentType }: { fieldNode: FieldDefinitionNode; parentType: GraphQLNamedType }): boolean { - if (!this.enabled || !(isObjectType(parentType) && !isInterfaceType(parentType))) { - return false; + const fieldNodes = node.fields as unknown as FieldDefinitionResult[]; + + // If the object is marked with @external, fields to generate are those with `@provides` + if (this.isExternal(node)) { + const fieldNodesWithProvides = fieldNodes.reduce((acc, [fieldDef]) => { + if (this.hasProvides(parentType, fieldDef.node.name as any as string)) { + acc.push(fieldDef.node); + } + return acc; + }, []); + return fieldNodesWithProvides; } - return this.isExternalAndNotProvided(fieldNode, parentType); + // If the object is not marked with @external, fields to generate are: + // - the fields without `@external` + // - the `@external` fields with `@provides` + const fieldNodesWithoutExternalOrHasProvides = fieldNodes.reduce((acc, [fieldDef]) => { + if (!this.isExternal(fieldDef.node)) { + acc.push(fieldDef.node); + return acc; + } + + if (this.isExternal(fieldDef.node) && this.hasProvides(parentType, fieldDef.node.name as any as string)) { + acc.push(fieldDef.node); + return acc; + } + + return acc; + }, []); + + return fieldNodesWithoutExternalOrHasProvides; } isResolveReferenceField(fieldNode: FieldDefinitionNode): boolean { @@ -352,19 +386,15 @@ export class ApolloFederation { return this.meta; } - private isExternalAndNotProvided(fieldNode: FieldDefinitionNode, objectType: GraphQLObjectType): boolean { - return this.isExternal(fieldNode) && !this.hasProvides(objectType, fieldNode); - } - - private isExternal(node: FieldDefinitionNode | ObjectTypeDefinitionNode): boolean { + private isExternal(node: FieldDefinitionNode | ObjectTypeDefinitionNode | InterfaceTypeDefinitionNode): boolean { return getDirectivesByName('external', node).length > 0; } - private hasProvides(objectType: ObjectTypeDefinitionNode | GraphQLObjectType, node: FieldDefinitionNode): boolean { + private hasProvides(objectType: ObjectTypeDefinitionNode | GraphQLObjectType, fieldName: string): boolean { const fields = this.providesMap[isObjectType(objectType) ? objectType.name : objectType.name.value]; if (fields?.length) { - return fields.includes(node.name.value); + return fields.includes(fieldName); } return false; @@ -417,7 +447,7 @@ export class ApolloFederation { for (const field of Object.values(objectType.getFields())) { const provides = getDirectivesByName('provides', field.astNode) .map(extractReferenceSelectionSet) - .reduce((prev, curr) => [...prev, ...Object.keys(curr)], []); + .reduce((prev, curr) => [...prev, ...Object.keys(curr)], []); // FIXME: this is not taking into account nested selection sets e.g. `company { taxCode }` const ofType = getBaseType(field.type); providesMap[ofType.name] ||= []; @@ -478,11 +508,11 @@ function checkTypeFederationDetails( */ function getDirectivesByName( name: string, - node: ObjectTypeDefinitionNode | GraphQLObjectType | FieldDefinitionNode + node: ObjectTypeDefinitionNode | GraphQLObjectType | FieldDefinitionNode | InterfaceTypeDefinitionNode ): readonly DirectiveNode[] { - let astNode: ObjectTypeDefinitionNode | FieldDefinitionNode; + let astNode: ObjectTypeDefinitionNode | FieldDefinitionNode | InterfaceTypeDefinitionNode; - if (isObjectType(node)) { + if (isObjectType(node) || isInterfaceType(node)) { astNode = node.astNode; } else { astNode = node; From a6f9d91e239aa0d26c1a14d6e7041d97a334679e Mon Sep 17 00:00:00 2001 From: Eddy Nguyen Date: Fri, 9 May 2025 00:17:21 +1000 Subject: [PATCH 04/12] Fix __resolveReference not getting generated --- .../src/base-resolvers-visitor.ts | 33 +++++++++-------- .../utils/plugins-helpers/src/federation.ts | 37 ++++++++----------- 2 files changed, 33 insertions(+), 37 deletions(-) diff --git a/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts b/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts index cf842c12435..00992a8f42d 100644 --- a/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts +++ b/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts @@ -83,10 +83,7 @@ export interface ParsedResolversConfig extends ParsedConfig { } type FieldDefinitionPrintFn = ( - parent: { - node: ObjectTypeDefinitionNode | InterfaceTypeDefinitionNode; - typeName: string; - }, + parentName: string, avoidResolverOptionals: boolean ) => { value: string | null; meta: { federation?: { isResolveReference: boolean } } }; export type FieldDefinitionResult = [{ node: FieldDefinitionNode }, FieldDefinitionPrintFn]; @@ -1530,15 +1527,17 @@ export class BaseResolversVisitor< return [ { node }, - (parentNode, avoidResolverOptionals) => { - const parentName = parentNode.typeName; - + (parentName, avoidResolverOptionals) => { const original: FieldDefinitionNode = parent[key]; const parentType = this.schema.getType(parentName); const meta: ReturnType['meta'] = {}; + const typeName = node.name as unknown as string; + + const fieldsToGenerate = this._federation.findFieldNodesToGenerate({ type: parentType }); + const shouldGenerateField = + fieldsToGenerate.some(field => field.name.value === typeName) || + this._federation.isResolveReferenceField(node); - const fieldsToGenerate = this._federation.findFieldNodesToGenerate({ node: parentNode.node }); - const shouldGenerateField = fieldsToGenerate.some(field => field.name === node.name); if (!shouldGenerateField) { return { value: null, meta }; } @@ -1549,7 +1548,7 @@ export class BaseResolversVisitor< ? this.convertName( parentName + (this.config.addUnderscoreToArgsType ? '_' : '') + - this.convertName(node.name, { + this.convertName(typeName, { useTypesPrefix: false, useTypesSuffix: false, }) + @@ -1600,7 +1599,7 @@ export class BaseResolversVisitor< if (isSubscriptionType) { return { - mappedTypeKey: `${mappedType}, "${node.name}"`, + mappedTypeKey: `${mappedType}, "${typeName}"`, resolverType: 'SubscriptionResolver', }; } @@ -1623,7 +1622,7 @@ export class BaseResolversVisitor< type: string; genericTypes: string[]; } = { - name: node.name as any, + name: typeName, modifier: avoidResolverOptionals ? '' : '?', type: resolverType, genericTypes: [mappedTypeKey, parentTypeSignature, contextType, argsType].filter(f => f), @@ -1720,7 +1719,10 @@ export class BaseResolversVisitor< } ObjectTypeDefinition(node: ObjectTypeDefinitionNode): string | null { - const fieldsToGenerate = this._federation.findFieldNodesToGenerate({ node }); + const typeName = node.name as unknown as string; + const type = this.schema.getType(typeName); + + const fieldsToGenerate = this._federation.findFieldNodesToGenerate({ type }); if (fieldsToGenerate.length === 0) { return null; } @@ -1729,7 +1731,6 @@ export class BaseResolversVisitor< const name = this.convertName(node, { suffix: this.config.resolverTypeSuffix, }); - const typeName = node.name as any as string; const parentType = this.getParentTypeToUse(typeName); const rootType = ((): false | 'query' | 'mutation' | 'subscription' => { @@ -1748,7 +1749,7 @@ export class BaseResolversVisitor< const fieldsContent = (node.fields as unknown as FieldDefinitionResult[]) .map(([_, f]) => { return f( - { node, typeName }, + typeName, (rootType === 'query' && this.config.avoidOptionals.query) || (rootType === 'mutation' && this.config.avoidOptionals.mutation) || (rootType === 'subscription' && this.config.avoidOptionals.subscription) || @@ -1982,7 +1983,7 @@ export class BaseResolversVisitor< // An Interface in Federation may have the additional __resolveReference resolver, if resolvable. // So, we filter out the normal fields declared on the Interface and add the __resolveReference resolver. const fields = (node.fields as unknown as FieldDefinitionResult[]).map(([_, f]) => - f({ node, typeName }, this.config.avoidOptionals.resolvers) + f(typeName, this.config.avoidOptionals.resolvers) ); for (const field of fields) { if (field.meta.federation?.isResolveReference) { diff --git a/packages/utils/plugins-helpers/src/federation.ts b/packages/utils/plugins-helpers/src/federation.ts index 60a9ddd4f81..d244620bca1 100644 --- a/packages/utils/plugins-helpers/src/federation.ts +++ b/packages/utils/plugins-helpers/src/federation.ts @@ -1,5 +1,4 @@ import { astFromInterfaceType, astFromObjectType, getRootTypeNames, MapperKind, mapSchema } from '@graphql-tools/utils'; -import type { FieldDefinitionResult } from '@graphql-codegen/visitor-plugin-common'; import { DefinitionNode, DirectiveNode, @@ -275,27 +274,23 @@ export class ApolloFederation { * - The field is marked as `@external` and there is no `@provides` path to the field * - The parent object is marked as `@external` and there is no `@provides` path to the field */ - findFieldNodesToGenerate({ - node, - }: { - node: ObjectTypeDefinitionNode | InterfaceTypeDefinitionNode; - }): readonly FieldDefinitionNode[] { - if (!this.enabled) { - return node.fields || []; - } - - const parentType = this.schema.getType(node.name as any as string); - if (!(isObjectType(parentType) && !isInterfaceType(parentType))) { + findFieldNodesToGenerate({ type }: { type: GraphQLNamedType }): readonly FieldDefinitionNode[] { + if (!(isObjectType(type) && !isInterfaceType(type))) { return []; } - const fieldNodes = node.fields as unknown as FieldDefinitionResult[]; + const node = type.astNode; + const fieldNodes = node.fields || []; + + if (!this.enabled) { + return fieldNodes; + } // If the object is marked with @external, fields to generate are those with `@provides` if (this.isExternal(node)) { - const fieldNodesWithProvides = fieldNodes.reduce((acc, [fieldDef]) => { - if (this.hasProvides(parentType, fieldDef.node.name as any as string)) { - acc.push(fieldDef.node); + const fieldNodesWithProvides = fieldNodes.reduce((acc, fieldNode) => { + if (this.hasProvides(type, fieldNode.name as unknown as string)) { + acc.push(fieldNode); } return acc; }, []); @@ -305,14 +300,14 @@ export class ApolloFederation { // If the object is not marked with @external, fields to generate are: // - the fields without `@external` // - the `@external` fields with `@provides` - const fieldNodesWithoutExternalOrHasProvides = fieldNodes.reduce((acc, [fieldDef]) => { - if (!this.isExternal(fieldDef.node)) { - acc.push(fieldDef.node); + const fieldNodesWithoutExternalOrHasProvides = fieldNodes.reduce((acc, fieldNode) => { + if (!this.isExternal(fieldNode)) { + acc.push(fieldNode); return acc; } - if (this.isExternal(fieldDef.node) && this.hasProvides(parentType, fieldDef.node.name as any as string)) { - acc.push(fieldDef.node); + if (this.isExternal(fieldNode) && this.hasProvides(type, fieldNode.name as unknown as string)) { + acc.push(fieldNode); return acc; } From 7b7301de0824730b55133a86a1428e0c439ebac9 Mon Sep 17 00:00:00 2001 From: Eddy Nguyen Date: Fri, 9 May 2025 00:22:44 +1000 Subject: [PATCH 05/12] Fix test with __isTypeOf when not needed --- .../resolvers/tests/ts-resolvers.config.customDirectives.spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/plugins/typescript/resolvers/tests/ts-resolvers.config.customDirectives.spec.ts b/packages/plugins/typescript/resolvers/tests/ts-resolvers.config.customDirectives.spec.ts index c7ceeb86040..49f221c71ac 100644 --- a/packages/plugins/typescript/resolvers/tests/ts-resolvers.config.customDirectives.spec.ts +++ b/packages/plugins/typescript/resolvers/tests/ts-resolvers.config.customDirectives.spec.ts @@ -62,7 +62,6 @@ describe('customDirectives.sematicNonNull', () => { nonNullableListWithNonNullableItemLevel0?: Resolver, ParentType, ContextType>; nonNullableListWithNonNullableItemLevel1?: Resolver, ParentType, ContextType>; nonNullableListWithNonNullableItemBothLevels?: Resolver, ParentType, ContextType>; - __isTypeOf?: IsTypeOfResolverFn; }; `); }); From 7315f8ef8f69e5f7dc96bb91f2cfd9b89367fb2f Mon Sep 17 00:00:00 2001 From: Eddy Nguyen Date: Fri, 9 May 2025 00:36:11 +1000 Subject: [PATCH 06/12] Fix type cast that results in wrong type --- .../other/visitor-plugin-common/src/base-resolvers-visitor.ts | 2 +- packages/utils/plugins-helpers/src/federation.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts b/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts index 00992a8f42d..e0658c8b321 100644 --- a/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts +++ b/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts @@ -1533,7 +1533,7 @@ export class BaseResolversVisitor< const meta: ReturnType['meta'] = {}; const typeName = node.name as unknown as string; - const fieldsToGenerate = this._federation.findFieldNodesToGenerate({ type: parentType }); + const fieldsToGenerate = this._federation.findFieldNodesToGenerate({ type: parentType }); // FIXME: for every field in a object, we are looping through every field of said object. This could be a bottleneck const shouldGenerateField = fieldsToGenerate.some(field => field.name.value === typeName) || this._federation.isResolveReferenceField(node); diff --git a/packages/utils/plugins-helpers/src/federation.ts b/packages/utils/plugins-helpers/src/federation.ts index d244620bca1..cd652d4315c 100644 --- a/packages/utils/plugins-helpers/src/federation.ts +++ b/packages/utils/plugins-helpers/src/federation.ts @@ -306,7 +306,7 @@ export class ApolloFederation { return acc; } - if (this.isExternal(fieldNode) && this.hasProvides(type, fieldNode.name as unknown as string)) { + if (this.isExternal(fieldNode) && this.hasProvides(type, fieldNode.name.value)) { acc.push(fieldNode); return acc; } From 01312c3e2c436fdbc8df8bae0043054d8230643d Mon Sep 17 00:00:00 2001 From: Eddy Nguyen Date: Fri, 9 May 2025 00:42:41 +1000 Subject: [PATCH 07/12] Revert unncessary changes to FieldDefinitionPrintFn --- .../src/base-resolvers-visitor.ts | 228 +++++++++--------- .../utils/plugins-helpers/src/federation.ts | 1 + 2 files changed, 112 insertions(+), 117 deletions(-) diff --git a/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts b/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts index e0658c8b321..50c6d637b29 100644 --- a/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts +++ b/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts @@ -86,8 +86,6 @@ type FieldDefinitionPrintFn = ( parentName: string, avoidResolverOptionals: boolean ) => { value: string | null; meta: { federation?: { isResolveReference: boolean } } }; -export type FieldDefinitionResult = [{ node: FieldDefinitionNode }, FieldDefinitionPrintFn]; - export interface RootResolver { content: string; generatedResolverTypes: { @@ -1521,132 +1519,128 @@ export class BaseResolversVisitor< return `ParentType extends ${parentType} = ${parentType}`; } - FieldDefinition(node: FieldDefinitionNode, key: string | number, parent: any): FieldDefinitionResult { + FieldDefinition(node: FieldDefinitionNode, key: string | number, parent: any): FieldDefinitionPrintFn { const hasArguments = node.arguments && node.arguments.length > 0; const declarationKind = 'type'; - return [ - { node }, - (parentName, avoidResolverOptionals) => { - const original: FieldDefinitionNode = parent[key]; - const parentType = this.schema.getType(parentName); - const meta: ReturnType['meta'] = {}; - const typeName = node.name as unknown as string; - - const fieldsToGenerate = this._federation.findFieldNodesToGenerate({ type: parentType }); // FIXME: for every field in a object, we are looping through every field of said object. This could be a bottleneck - const shouldGenerateField = - fieldsToGenerate.some(field => field.name.value === typeName) || - this._federation.isResolveReferenceField(node); - - if (!shouldGenerateField) { - return { value: null, meta }; - } + return (parentName, avoidResolverOptionals) => { + const original: FieldDefinitionNode = parent[key]; + const parentType = this.schema.getType(parentName); + const meta: ReturnType['meta'] = {}; + const typeName = node.name as unknown as string; - const contextType = this.getContextType(parentName, node); - - let argsType = hasArguments - ? this.convertName( - parentName + - (this.config.addUnderscoreToArgsType ? '_' : '') + - this.convertName(typeName, { - useTypesPrefix: false, - useTypesSuffix: false, - }) + - 'Args', - { - useTypesPrefix: true, - }, - true - ) - : null; - - const avoidInputsOptionals = this.config.avoidOptionals.inputValue; - - if (argsType !== null) { - const argsToForceRequire = original.arguments.filter( - arg => !!arg.defaultValue || arg.type.kind === 'NonNullType' - ); - - if (argsToForceRequire.length > 0) { - argsType = this.applyRequireFields(argsType, argsToForceRequire); - } else if (original.arguments.length > 0 && avoidInputsOptionals !== true) { - argsType = this.applyOptionalFields(argsType, original.arguments); - } - } + const fieldsToGenerate = this._federation.findFieldNodesToGenerate({ type: parentType }); // FIXME: for every field in a object, we are looping through every field of said object. This could be a bottleneck + const shouldGenerateField = + fieldsToGenerate.some(field => field.name.value === typeName) || this._federation.isResolveReferenceField(node); - const parentTypeSignature = this._federation.transformFieldParentType({ - fieldNode: original, - parentType, - parentTypeSignature: this.getParentTypeForSignature(node), - federationTypeSignature: 'FederationType', - }); + if (!shouldGenerateField) { + return { value: null, meta }; + } - const { mappedTypeKey, resolverType } = ((): { mappedTypeKey: string; resolverType: string } => { - const baseType = getBaseTypeNode(original.type); - const realType = baseType.name.value; - const typeToUse = this.getTypeToUse(realType); - /** - * Turns GraphQL type to TypeScript types (`mappedType`) e.g. - * - String! -> ResolversTypes['String']> - * - String -> Maybe - * - [String] -> Maybe>> - * - [String!]! -> Array - */ - const mappedType = this._variablesTransformer.wrapAstTypeWithModifiers(typeToUse, original.type); - - const subscriptionType = this._schema.getSubscriptionType(); - const isSubscriptionType = subscriptionType && subscriptionType.name === parentName; - - if (isSubscriptionType) { - return { - mappedTypeKey: `${mappedType}, "${typeName}"`, - resolverType: 'SubscriptionResolver', - }; - } + const contextType = this.getContextType(parentName, node); + + let argsType = hasArguments + ? this.convertName( + parentName + + (this.config.addUnderscoreToArgsType ? '_' : '') + + this.convertName(typeName, { + useTypesPrefix: false, + useTypesSuffix: false, + }) + + 'Args', + { + useTypesPrefix: true, + }, + true + ) + : null; + + const avoidInputsOptionals = this.config.avoidOptionals.inputValue; + + if (argsType !== null) { + const argsToForceRequire = original.arguments.filter( + arg => !!arg.defaultValue || arg.type.kind === 'NonNullType' + ); - const directiveMappings = - node.directives - ?.map(directive => this._directiveResolverMappings[directive.name as any]) - .filter(Boolean) - .reverse() ?? []; + if (argsToForceRequire.length > 0) { + argsType = this.applyRequireFields(argsType, argsToForceRequire); + } else if (original.arguments.length > 0 && avoidInputsOptionals !== true) { + argsType = this.applyOptionalFields(argsType, original.arguments); + } + } + const parentTypeSignature = this._federation.transformFieldParentType({ + fieldNode: original, + parentType, + parentTypeSignature: this.getParentTypeForSignature(node), + federationTypeSignature: 'FederationType', + }); + + const { mappedTypeKey, resolverType } = ((): { mappedTypeKey: string; resolverType: string } => { + const baseType = getBaseTypeNode(original.type); + const realType = baseType.name.value; + const typeToUse = this.getTypeToUse(realType); + /** + * Turns GraphQL type to TypeScript types (`mappedType`) e.g. + * - String! -> ResolversTypes['String']> + * - String -> Maybe + * - [String] -> Maybe>> + * - [String!]! -> Array + */ + const mappedType = this._variablesTransformer.wrapAstTypeWithModifiers(typeToUse, original.type); + + const subscriptionType = this._schema.getSubscriptionType(); + const isSubscriptionType = subscriptionType && subscriptionType.name === parentName; + + if (isSubscriptionType) { return { - mappedTypeKey: mappedType, - resolverType: directiveMappings[0] ?? 'Resolver', + mappedTypeKey: `${mappedType}, "${typeName}"`, + resolverType: 'SubscriptionResolver', }; - })(); - - const signature: { - name: string; - modifier: string; - type: string; - genericTypes: string[]; - } = { - name: typeName, - modifier: avoidResolverOptionals ? '' : '?', - type: resolverType, - genericTypes: [mappedTypeKey, parentTypeSignature, contextType, argsType].filter(f => f), - }; - - if (this._federation.isResolveReferenceField(node)) { - if (!this._federation.getMeta()[parentType.name].hasResolveReference) { - return { value: '', meta }; - } - signature.type = 'ReferenceResolver'; - signature.genericTypes = [mappedTypeKey, parentTypeSignature, contextType]; - meta.federation = { isResolveReference: true }; } + const directiveMappings = + node.directives + ?.map(directive => this._directiveResolverMappings[directive.name as any]) + .filter(Boolean) + .reverse() ?? []; + return { - value: indent( - `${signature.name}${signature.modifier}: ${signature.type}<${signature.genericTypes.join( - ', ' - )}>${this.getPunctuation(declarationKind)}` - ), - meta, + mappedTypeKey: mappedType, + resolverType: directiveMappings[0] ?? 'Resolver', }; - }, - ]; + })(); + + const signature: { + name: string; + modifier: string; + type: string; + genericTypes: string[]; + } = { + name: typeName, + modifier: avoidResolverOptionals ? '' : '?', + type: resolverType, + genericTypes: [mappedTypeKey, parentTypeSignature, contextType, argsType].filter(f => f), + }; + + if (this._federation.isResolveReferenceField(node)) { + if (!this._federation.getMeta()[parentType.name].hasResolveReference) { + return { value: '', meta }; + } + signature.type = 'ReferenceResolver'; + signature.genericTypes = [mappedTypeKey, parentTypeSignature, contextType]; + meta.federation = { isResolveReference: true }; + } + + return { + value: indent( + `${signature.name}${signature.modifier}: ${signature.type}<${signature.genericTypes.join( + ', ' + )}>${this.getPunctuation(declarationKind)}` + ), + meta, + }; + }; } private getFieldContextType(parentName: string, node: FieldDefinitionNode): string { @@ -1746,8 +1740,8 @@ export class BaseResolversVisitor< return false; })(); - const fieldsContent = (node.fields as unknown as FieldDefinitionResult[]) - .map(([_, f]) => { + const fieldsContent = (node.fields as unknown as FieldDefinitionPrintFn[]) + .map(f => { return f( typeName, (rootType === 'query' && this.config.avoidOptionals.query) || @@ -1982,7 +1976,7 @@ export class BaseResolversVisitor< // An Interface in Federation may have the additional __resolveReference resolver, if resolvable. // So, we filter out the normal fields declared on the Interface and add the __resolveReference resolver. - const fields = (node.fields as unknown as FieldDefinitionResult[]).map(([_, f]) => + const fields = (node.fields as unknown as FieldDefinitionPrintFn[]).map(f => f(typeName, this.config.avoidOptionals.resolvers) ); for (const field of fields) { diff --git a/packages/utils/plugins-helpers/src/federation.ts b/packages/utils/plugins-helpers/src/federation.ts index cd652d4315c..6f4a17984c5 100644 --- a/packages/utils/plugins-helpers/src/federation.ts +++ b/packages/utils/plugins-helpers/src/federation.ts @@ -291,6 +291,7 @@ export class ApolloFederation { const fieldNodesWithProvides = fieldNodes.reduce((acc, fieldNode) => { if (this.hasProvides(type, fieldNode.name as unknown as string)) { acc.push(fieldNode); + return acc; } return acc; }, []); From 9d6c1706920b2481ed97aa9bec6e44512097672c Mon Sep 17 00:00:00 2001 From: Eddy Nguyen Date: Fri, 9 May 2025 22:12:26 +1000 Subject: [PATCH 08/12] Re-format --- .../tests/ts-resolvers.federation.spec.ts | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/plugins/typescript/resolvers/tests/ts-resolvers.federation.spec.ts b/packages/plugins/typescript/resolvers/tests/ts-resolvers.federation.spec.ts index b749a97db14..ba763bdc34c 100644 --- a/packages/plugins/typescript/resolvers/tests/ts-resolvers.federation.spec.ts +++ b/packages/plugins/typescript/resolvers/tests/ts-resolvers.federation.spec.ts @@ -546,8 +546,8 @@ describe('TypeScript Resolvers Plugin + Apollo Federation', () => { }, }); - // UserResolvers should not have `username` resolver because it is marked with `@external` - // UserResolvers should have `name` resolver because whilst it is marked with `@external`, it is provided by `Book.author` + // `UserResolvers` should not have `username` resolver because it is marked with `@external` + // `UserResolvers` should have `name` resolver because whilst it is marked with `@external`, it is provided by `Book.author` expect(content).toBeSimilarStringTo(` export type UserResolvers = { __resolveReference?: ReferenceResolver, @@ -562,25 +562,25 @@ describe('TypeScript Resolvers Plugin + Apollo Federation', () => { }; `); - // AddressResolvers should only have fields not marked with @external + // `AddressResolvers` should only have fields not marked with @external expect(content).toBeSimilarStringTo(` export type AddressResolvers = { zip?: Resolver; }; `); - // FIXME: CompanyResolvers should only have taxCode resolver because it is part of the `@provides` directive in `Book.editor` + // `DateOfBirthResolvers` should not be generated because every field is marked with @external + expect(content).not.toBeSimilarStringTo('export type DateOfBirthResolvers'); + + // `PlaceOfBirthResolvers` should not be generated because the type is marked with @external, even if `User.placeOfBirth` is not marked with @external + expect(content).not.toBeSimilarStringTo('export type PlaceOfBirthResolvers'); + + // FIXME: `CompanyResolvers` should only have taxCode resolver because it is part of the `@provides` directive in `Book.editor`, even if the whole `Company` type is marked with @external // expect(content).toBeSimilarStringTo(` // export type CompanyResolvers = { // taxCode?: Resolver; // }; // `); - - // DateOfBirthResolvers should not be generated because every field is marked with @external - expect(content).not.toBeSimilarStringTo('export type DateOfBirthResolvers'); - - // PlaceOfBirthResolvers should not be generated because the type is marked with @external - expect(content).not.toBeSimilarStringTo('export type PlaceOfBirthResolvers'); }); it('should not include _FieldSet scalar', async () => { From 2678432ab7708ffb0c5b1bf6cad7dfd94435ec31 Mon Sep 17 00:00:00 2001 From: Eddy Nguyen Date: Sun, 11 May 2025 20:06:16 +1000 Subject: [PATCH 09/12] Convert to use AST Node instead of GraphQL Type --- .../src/base-resolvers-visitor.ts | 241 +++++++++--------- .../utils/plugins-helpers/src/federation.ts | 46 ++-- 2 files changed, 148 insertions(+), 139 deletions(-) diff --git a/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts b/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts index 50c6d637b29..8bf739a263f 100644 --- a/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts +++ b/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts @@ -82,8 +82,13 @@ export interface ParsedResolversConfig extends ParsedConfig { avoidCheckingAbstractTypesRecursively: boolean; } +export interface FieldDefinitionResult { + node: FieldDefinitionNode; + printContent: FieldDefinitionPrintFn; +} + type FieldDefinitionPrintFn = ( - parentName: string, + parentNode: ObjectTypeDefinitionNode | InterfaceTypeDefinitionNode, avoidResolverOptionals: boolean ) => { value: string | null; meta: { federation?: { isResolveReference: boolean } } }; export interface RootResolver { @@ -1519,127 +1524,133 @@ export class BaseResolversVisitor< return `ParentType extends ${parentType} = ${parentType}`; } - FieldDefinition(node: FieldDefinitionNode, key: string | number, parent: any): FieldDefinitionPrintFn { + FieldDefinition(node: FieldDefinitionNode, key: string | number, parent: any): FieldDefinitionResult { const hasArguments = node.arguments && node.arguments.length > 0; const declarationKind = 'type'; - return (parentName, avoidResolverOptionals) => { - const original: FieldDefinitionNode = parent[key]; - const parentType = this.schema.getType(parentName); - const meta: ReturnType['meta'] = {}; - const typeName = node.name as unknown as string; + const original: FieldDefinitionNode = parent[key]; - const fieldsToGenerate = this._federation.findFieldNodesToGenerate({ type: parentType }); // FIXME: for every field in a object, we are looping through every field of said object. This could be a bottleneck - const shouldGenerateField = - fieldsToGenerate.some(field => field.name.value === typeName) || this._federation.isResolveReferenceField(node); - - if (!shouldGenerateField) { - return { value: null, meta }; - } + return { + node: original, + printContent: (parentNode, avoidResolverOptionals) => { + const parentName = parentNode.name as unknown as string; + const parentType = this.schema.getType(parentName); + const meta: ReturnType['meta'] = {}; + const typeName = node.name as unknown as string; + + const fieldsToGenerate = this._federation.findFieldNodesToGenerate({ node: parentNode }); // FIXME: for every field in a object, we are looping through every field of said object. This could be a bottleneck + const shouldGenerateField = + fieldsToGenerate.some(field => field.name.value === typeName) || + this._federation.isResolveReferenceField(node); + + if (!shouldGenerateField) { + return { value: null, meta }; + } - const contextType = this.getContextType(parentName, node); - - let argsType = hasArguments - ? this.convertName( - parentName + - (this.config.addUnderscoreToArgsType ? '_' : '') + - this.convertName(typeName, { - useTypesPrefix: false, - useTypesSuffix: false, - }) + - 'Args', - { - useTypesPrefix: true, - }, - true - ) - : null; + const contextType = this.getContextType(parentName, node); + + let argsType = hasArguments + ? this.convertName( + parentName + + (this.config.addUnderscoreToArgsType ? '_' : '') + + this.convertName(typeName, { + useTypesPrefix: false, + useTypesSuffix: false, + }) + + 'Args', + { + useTypesPrefix: true, + }, + true + ) + : null; + + const avoidInputsOptionals = this.config.avoidOptionals.inputValue; + + if (argsType !== null) { + const argsToForceRequire = original.arguments.filter( + arg => !!arg.defaultValue || arg.type.kind === 'NonNullType' + ); + + if (argsToForceRequire.length > 0) { + argsType = this.applyRequireFields(argsType, argsToForceRequire); + } else if (original.arguments.length > 0 && avoidInputsOptionals !== true) { + argsType = this.applyOptionalFields(argsType, original.arguments); + } + } - const avoidInputsOptionals = this.config.avoidOptionals.inputValue; + const parentTypeSignature = this._federation.transformFieldParentType({ + fieldNode: original, + parentType, + parentTypeSignature: this.getParentTypeForSignature(node), + federationTypeSignature: 'FederationType', + }); - if (argsType !== null) { - const argsToForceRequire = original.arguments.filter( - arg => !!arg.defaultValue || arg.type.kind === 'NonNullType' - ); + const { mappedTypeKey, resolverType } = ((): { mappedTypeKey: string; resolverType: string } => { + const baseType = getBaseTypeNode(original.type); + const realType = baseType.name.value; + const typeToUse = this.getTypeToUse(realType); + /** + * Turns GraphQL type to TypeScript types (`mappedType`) e.g. + * - String! -> ResolversTypes['String']> + * - String -> Maybe + * - [String] -> Maybe>> + * - [String!]! -> Array + */ + const mappedType = this._variablesTransformer.wrapAstTypeWithModifiers(typeToUse, original.type); + + const subscriptionType = this._schema.getSubscriptionType(); + const isSubscriptionType = subscriptionType && subscriptionType.name === parentName; + + if (isSubscriptionType) { + return { + mappedTypeKey: `${mappedType}, "${typeName}"`, + resolverType: 'SubscriptionResolver', + }; + } - if (argsToForceRequire.length > 0) { - argsType = this.applyRequireFields(argsType, argsToForceRequire); - } else if (original.arguments.length > 0 && avoidInputsOptionals !== true) { - argsType = this.applyOptionalFields(argsType, original.arguments); - } - } + const directiveMappings = + node.directives + ?.map(directive => this._directiveResolverMappings[directive.name as any]) + .filter(Boolean) + .reverse() ?? []; - const parentTypeSignature = this._federation.transformFieldParentType({ - fieldNode: original, - parentType, - parentTypeSignature: this.getParentTypeForSignature(node), - federationTypeSignature: 'FederationType', - }); - - const { mappedTypeKey, resolverType } = ((): { mappedTypeKey: string; resolverType: string } => { - const baseType = getBaseTypeNode(original.type); - const realType = baseType.name.value; - const typeToUse = this.getTypeToUse(realType); - /** - * Turns GraphQL type to TypeScript types (`mappedType`) e.g. - * - String! -> ResolversTypes['String']> - * - String -> Maybe - * - [String] -> Maybe>> - * - [String!]! -> Array - */ - const mappedType = this._variablesTransformer.wrapAstTypeWithModifiers(typeToUse, original.type); - - const subscriptionType = this._schema.getSubscriptionType(); - const isSubscriptionType = subscriptionType && subscriptionType.name === parentName; - - if (isSubscriptionType) { return { - mappedTypeKey: `${mappedType}, "${typeName}"`, - resolverType: 'SubscriptionResolver', + mappedTypeKey: mappedType, + resolverType: directiveMappings[0] ?? 'Resolver', }; - } + })(); + + const signature: { + name: string; + modifier: string; + type: string; + genericTypes: string[]; + } = { + name: typeName, + modifier: avoidResolverOptionals ? '' : '?', + type: resolverType, + genericTypes: [mappedTypeKey, parentTypeSignature, contextType, argsType].filter(f => f), + }; - const directiveMappings = - node.directives - ?.map(directive => this._directiveResolverMappings[directive.name as any]) - .filter(Boolean) - .reverse() ?? []; + if (this._federation.isResolveReferenceField(node)) { + if (!this._federation.getMeta()[parentType.name].hasResolveReference) { + return { value: '', meta }; + } + signature.type = 'ReferenceResolver'; + signature.genericTypes = [mappedTypeKey, parentTypeSignature, contextType]; + meta.federation = { isResolveReference: true }; + } return { - mappedTypeKey: mappedType, - resolverType: directiveMappings[0] ?? 'Resolver', + value: indent( + `${signature.name}${signature.modifier}: ${signature.type}<${signature.genericTypes.join( + ', ' + )}>${this.getPunctuation(declarationKind)}` + ), + meta, }; - })(); - - const signature: { - name: string; - modifier: string; - type: string; - genericTypes: string[]; - } = { - name: typeName, - modifier: avoidResolverOptionals ? '' : '?', - type: resolverType, - genericTypes: [mappedTypeKey, parentTypeSignature, contextType, argsType].filter(f => f), - }; - - if (this._federation.isResolveReferenceField(node)) { - if (!this._federation.getMeta()[parentType.name].hasResolveReference) { - return { value: '', meta }; - } - signature.type = 'ReferenceResolver'; - signature.genericTypes = [mappedTypeKey, parentTypeSignature, contextType]; - meta.federation = { isResolveReference: true }; - } - - return { - value: indent( - `${signature.name}${signature.modifier}: ${signature.type}<${signature.genericTypes.join( - ', ' - )}>${this.getPunctuation(declarationKind)}` - ), - meta, - }; + }, }; } @@ -1714,9 +1725,7 @@ export class BaseResolversVisitor< ObjectTypeDefinition(node: ObjectTypeDefinitionNode): string | null { const typeName = node.name as unknown as string; - const type = this.schema.getType(typeName); - - const fieldsToGenerate = this._federation.findFieldNodesToGenerate({ type }); + const fieldsToGenerate = this._federation.findFieldNodesToGenerate({ node }); if (fieldsToGenerate.length === 0) { return null; } @@ -1740,10 +1749,10 @@ export class BaseResolversVisitor< return false; })(); - const fieldsContent = (node.fields as unknown as FieldDefinitionPrintFn[]) - .map(f => { - return f( - typeName, + const fieldsContent = (node.fields as unknown as FieldDefinitionResult[]) + .map(({ printContent }) => { + return printContent( + node, (rootType === 'query' && this.config.avoidOptionals.query) || (rootType === 'mutation' && this.config.avoidOptionals.mutation) || (rootType === 'subscription' && this.config.avoidOptionals.subscription) || @@ -1976,8 +1985,8 @@ export class BaseResolversVisitor< // An Interface in Federation may have the additional __resolveReference resolver, if resolvable. // So, we filter out the normal fields declared on the Interface and add the __resolveReference resolver. - const fields = (node.fields as unknown as FieldDefinitionPrintFn[]).map(f => - f(typeName, this.config.avoidOptionals.resolvers) + const fields = (node.fields as unknown as FieldDefinitionResult[]).map(({ printContent }) => + printContent(node, this.config.avoidOptionals.resolvers) ); for (const field of fields) { if (field.meta.federation?.isResolveReference) { diff --git a/packages/utils/plugins-helpers/src/federation.ts b/packages/utils/plugins-helpers/src/federation.ts index 6f4a17984c5..c336502f91b 100644 --- a/packages/utils/plugins-helpers/src/federation.ts +++ b/packages/utils/plugins-helpers/src/federation.ts @@ -1,4 +1,5 @@ import { astFromInterfaceType, astFromObjectType, getRootTypeNames, MapperKind, mapSchema } from '@graphql-tools/utils'; +import type { FieldDefinitionResult } from '@graphql-codegen/visitor-plugin-common'; import { DefinitionNode, DirectiveNode, @@ -274,22 +275,21 @@ export class ApolloFederation { * - The field is marked as `@external` and there is no `@provides` path to the field * - The parent object is marked as `@external` and there is no `@provides` path to the field */ - findFieldNodesToGenerate({ type }: { type: GraphQLNamedType }): readonly FieldDefinitionNode[] { - if (!(isObjectType(type) && !isInterfaceType(type))) { - return []; - } - - const node = type.astNode; - const fieldNodes = node.fields || []; + findFieldNodesToGenerate({ + node, + }: { + node: ObjectTypeDefinitionNode | InterfaceTypeDefinitionNode; + }): readonly FieldDefinitionNode[] { + const fieldNodes = ((node.fields || []) as unknown as FieldDefinitionResult[]).map(field => field.node); if (!this.enabled) { return fieldNodes; } - // If the object is marked with @external, fields to generate are those with `@provides` + // If the object is marked with `@external`, fields to generate are those with `@provides` if (this.isExternal(node)) { const fieldNodesWithProvides = fieldNodes.reduce((acc, fieldNode) => { - if (this.hasProvides(type, fieldNode.name as unknown as string)) { + if (this.hasProvides(node, fieldNode.name.value)) { acc.push(fieldNode); return acc; } @@ -298,7 +298,7 @@ export class ApolloFederation { return fieldNodesWithProvides; } - // If the object is not marked with @external, fields to generate are: + // If the object is not marked with `@external`, fields to generate are: // - the fields without `@external` // - the `@external` fields with `@provides` const fieldNodesWithoutExternalOrHasProvides = fieldNodes.reduce((acc, fieldNode) => { @@ -307,7 +307,7 @@ export class ApolloFederation { return acc; } - if (this.isExternal(fieldNode) && this.hasProvides(type, fieldNode.name.value)) { + if (this.isExternal(fieldNode) && this.hasProvides(node, fieldNode.name.value)) { acc.push(fieldNode); return acc; } @@ -386,8 +386,8 @@ export class ApolloFederation { return getDirectivesByName('external', node).length > 0; } - private hasProvides(objectType: ObjectTypeDefinitionNode | GraphQLObjectType, fieldName: string): boolean { - const fields = this.providesMap[isObjectType(objectType) ? objectType.name : objectType.name.value]; + private hasProvides(node: ObjectTypeDefinitionNode | InterfaceTypeDefinitionNode, fieldName: string): boolean { + const fields = this.providesMap[node.name as unknown as string]; if (fields?.length) { return fields.includes(fieldName); @@ -462,22 +462,22 @@ export class ApolloFederation { * @param node Type */ function checkTypeFederationDetails( - node: ObjectTypeDefinitionNode | GraphQLObjectType | GraphQLInterfaceType, + typeOrNode: ObjectTypeDefinitionNode | GraphQLObjectType | InterfaceTypeDefinitionNode | GraphQLInterfaceType, schema: GraphQLSchema ): { resolvableKeyDirectives: readonly DirectiveNode[] } | false { - const { - name: { value: name }, - directives, - } = isObjectType(node) - ? astFromObjectType(node, schema) - : isInterfaceType(node) - ? astFromInterfaceType(node, schema) - : node; + const node = isObjectType(typeOrNode) + ? astFromObjectType(typeOrNode, schema) + : isInterfaceType(typeOrNode) + ? astFromInterfaceType(typeOrNode, schema) + : typeOrNode; + + const name = node.name.value || (typeOrNode.name as unknown as string); + const directives = node.directives; const rootTypeNames = getRootTypeNames(schema); const isNotRoot = !rootTypeNames.has(name); const isNotIntrospection = !name.startsWith('__'); - const keyDirectives = directives.filter(d => d.name.value === 'key'); + const keyDirectives = directives.filter(d => d.name.value === 'key' || (d.name as unknown as string) === 'key'); const check = isNotRoot && isNotIntrospection && keyDirectives.length > 0; From fa55f909ff2a30ca86d9cc05365cf7faf9f5e730 Mon Sep 17 00:00:00 2001 From: Eddy Nguyen Date: Sun, 11 May 2025 20:34:44 +1000 Subject: [PATCH 10/12] Update test template --- dev-test/test-schema/resolvers-federation.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/dev-test/test-schema/resolvers-federation.ts b/dev-test/test-schema/resolvers-federation.ts index 063a7b496c5..3583b2d7cae 100644 --- a/dev-test/test-schema/resolvers-federation.ts +++ b/dev-test/test-schema/resolvers-federation.ts @@ -210,7 +210,6 @@ export type UserResolvers< GraphQLRecursivePick, ContextType >; - email?: Resolver; }; From a538b3f6b165c04e19b58e069ebefdd2361851cc Mon Sep 17 00:00:00 2001 From: Eddy Nguyen Date: Tue, 13 May 2025 22:24:19 +1000 Subject: [PATCH 11/12] Cache field nodes to generate for processed objects --- .../src/base-resolvers-visitor.ts | 2 +- packages/utils/plugins-helpers/src/federation.ts | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts b/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts index 8bf739a263f..810096d4d5a 100644 --- a/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts +++ b/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts @@ -1538,7 +1538,7 @@ export class BaseResolversVisitor< const meta: ReturnType['meta'] = {}; const typeName = node.name as unknown as string; - const fieldsToGenerate = this._federation.findFieldNodesToGenerate({ node: parentNode }); // FIXME: for every field in a object, we are looping through every field of said object. This could be a bottleneck + const fieldsToGenerate = this._federation.findFieldNodesToGenerate({ node: parentNode }); const shouldGenerateField = fieldsToGenerate.some(field => field.name.value === typeName) || this._federation.isResolveReferenceField(node); diff --git a/packages/utils/plugins-helpers/src/federation.ts b/packages/utils/plugins-helpers/src/federation.ts index c336502f91b..2afd3aae3b6 100644 --- a/packages/utils/plugins-helpers/src/federation.ts +++ b/packages/utils/plugins-helpers/src/federation.ts @@ -226,12 +226,18 @@ export class ApolloFederation { private enabled = false; private schema: GraphQLSchema; private providesMap: Record; + /** + * `fieldsToGenerate` is a meta object where the keys are object type names + * and the values are fields that must be generated for that object. + */ + private fieldsToGenerate: Record; protected meta: FederationMeta = {}; constructor({ enabled, schema, meta }: { enabled: boolean; schema: GraphQLSchema; meta: FederationMeta }) { this.enabled = enabled; this.schema = schema; this.providesMap = this.createMapOfProvides(); + this.fieldsToGenerate = {}; this.meta = meta; } @@ -280,6 +286,11 @@ export class ApolloFederation { }: { node: ObjectTypeDefinitionNode | InterfaceTypeDefinitionNode; }): readonly FieldDefinitionNode[] { + const nodeName = node.name as unknown as string; + if (this.fieldsToGenerate[nodeName]) { + return this.fieldsToGenerate[nodeName]; + } + const fieldNodes = ((node.fields || []) as unknown as FieldDefinitionResult[]).map(field => field.node); if (!this.enabled) { @@ -295,6 +306,9 @@ export class ApolloFederation { } return acc; }, []); + + this.fieldsToGenerate[nodeName] = fieldNodesWithProvides; + return fieldNodesWithProvides; } @@ -315,6 +329,8 @@ export class ApolloFederation { return acc; }, []); + this.fieldsToGenerate[nodeName] = fieldNodesWithoutExternalOrHasProvides; + return fieldNodesWithoutExternalOrHasProvides; } From 78bbdb9444d4680d91481f258e275145e2348011 Mon Sep 17 00:00:00 2001 From: Eddy Nguyen Date: Tue, 13 May 2025 22:38:25 +1000 Subject: [PATCH 12/12] Put FIXME on base-resolvers-visitor to remove Name method --- .../other/visitor-plugin-common/src/base-resolvers-visitor.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts b/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts index 810096d4d5a..d447d4048ed 100644 --- a/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts +++ b/packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts @@ -1468,6 +1468,9 @@ export class BaseResolversVisitor< return ''; } + // FIXME: this Name method causes a lot of type inconsistencies + // because the type of nodes no longer matches the `graphql-js` types + // So, we should update this and remove any relevant `as any as string` or `as unknown as string` Name(node: NameNode): string { return node.value; }