[resolvers][federation] Fix fields or types being wrong generated when marked with @external #10287

eddeee888 · 2025-02-10T09:15:23Z

Description

When a field or type is marked with @external, the resolver should not be generated (unless part of @provides). This PR takes care of the following scenarios:

If a field is marked as @external, do not generate it
If any field is marked with @external, the object type's resolvers signature must still use ParentType for its first param
If all fields in an object type is marked as @external, do not generate the type
If the object type is marked as @external, do not generate the type
Question: if a field returns object type and not marked with @external, but the object type itself is marked with @external, what happens? (PlaceOfBirth in the test setup)
- The return type of User.placeOfBirth points to the base PlaceOfBirth, i.e. we don't need to generate PlaceOfBirthResolvers because it is unused.
Question: same as above but with @provides in play? (Company in the test setup)
- It should work the same way as normal @provides

Related #10206

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Unit test

changeset-bot · 2025-02-10T09:15:27Z

🦋 Changeset detected

Latest commit: 78bbdb9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 10 packages

Name	Type
@graphql-codegen/visitor-plugin-common	Patch
@graphql-codegen/typescript-resolvers	Patch
@graphql-codegen/plugin-helpers	Patch
@graphql-codegen/typescript-document-nodes	Patch
@graphql-codegen/gql-tag-operations	Patch
@graphql-codegen/typescript-operations	Patch
@graphql-codegen/typed-document-node	Patch
@graphql-codegen/typescript	Patch
@graphql-codegen/graphql-modules-preset	Patch
@graphql-codegen/client-preset	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

eddeee888 · 2025-02-10T09:52:09Z

packages/utils/plugins-helpers/src/federation.ts

+      // 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;


This is an observation from running unit tests. I'm not sure right now if this is a bug in graphql lib or we did something in our plugins that caused this. Either way, I'm putting this in first to unblock this PR

Related comment: https://github.com/dotansimha/graphql-code-generator/pull/10287/files#r2086739962

@provides

- Handle external directive when part or whole type is marked - Add changeset - Add test cases for @provides and @external

packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts

eddeee888 · 2025-05-12T11:34:25Z

packages/plugins/typescript/resolvers/tests/ts-resolvers.config.customDirectives.spec.ts

@@ -62,7 +62,6 @@ describe('customDirectives.sematicNonNull', () => {
        nonNullableListWithNonNullableItemLevel0?: Resolver<Array<ResolversTypes['String']>, ParentType, ContextType>;
        nonNullableListWithNonNullableItemLevel1?: Resolver<Array<ResolversTypes['String']>, ParentType, ContextType>;
        nonNullableListWithNonNullableItemBothLevels?: Resolver<Array<ResolversTypes['String']>, ParentType, ContextType>;
-        __isTypeOf?: IsTypeOfResolverFn<ParentType, ContextType>;


This is a new test from master.
__isTypeOf is not required here after this PR in the feature branch: #10283

eddeee888 · 2025-05-12T11:35:32Z

packages/plugins/typescript/resolvers/tests/ts-resolvers.federation.spec.ts

+    // 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<ContextType = any, ParentType extends ResolversParentTypes['Company'] = ResolversParentTypes['Company']> = {
+    //     taxCode?: Resolver<ResolversTypes['String'], ParentType, ContextType>;
+    //   };
+    // `);


I feel this edge case is quite hard to implement, will come back in another PR, or maybe after the major version bump.

eddeee888 · 2025-05-12T11:37:42Z

packages/utils/plugins-helpers/src/federation.ts

    return getDirectivesByName('external', node).length > 0;
  }

-  private hasProvides(objectType: ObjectTypeDefinitionNode | GraphQLObjectType, node: FieldDefinitionNode): boolean {
-    const fields = this.providesMap[isObjectType(objectType) ? objectType.name : objectType.name.value];
+  private hasProvides(node: ObjectTypeDefinitionNode | InterfaceTypeDefinitionNode, fieldName: string): boolean {


Note for the future: I find allowing both types (e.g. GraphQLObjectType) and nodes (e.g. ObjectTypeDefinitionNode) can get confusing very quickly.

Maybe we should just only accept nodes in the future?

eddeee888 · 2025-05-12T11:38:10Z

packages/utils/plugins-helpers/src/federation.ts

@@ -410,7 +443,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 }`


This comment is for this test here: https://github.com/dotansimha/graphql-code-generator/pull/10287/files#r2084481189

eddeee888 · 2025-05-13T12:48:07Z

packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts

+  // 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`


Drive-by put a FIXME here, so we remember why we need to do as any as string in a lot of places, so we can fix it later.

eddeee888 · 2025-05-13T12:50:43Z

packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts

    const hasArguments = node.arguments && node.arguments.length > 0;
    const declarationKind = 'type';

-    return (parentName, avoidResolverOptionals) => {


This function is used to print field definition. We need more details when we handle fields.

So, this now returns the original node, etc. as part of the result object. We might return more things in the future.

eddeee888 commented Feb 10, 2025

View reviewed changes

eddeee888 marked this pull request as draft February 13, 2025 15:38

eddeee888 force-pushed the fix-external-signature branch from ddfa8e3 to fbab7d6 Compare February 13, 2025 15:53

Base automatically changed from fix-is-typeof-when-needed to federation-fixes February 19, 2025 09:42

eddeee888 mentioned this pull request Feb 19, 2025

Next major version feature branch #10218

Draft

eddeee888 force-pushed the federation-fixes branch from b0b7e86 to 0b0c9bf Compare March 21, 2025 10:23

eddeee888 force-pushed the federation-fixes branch from 1a64f6d to 6a2bca7 Compare April 23, 2025 13:00

eddeee888 force-pushed the fix-external-signature branch from fbab7d6 to 7949e63 Compare April 23, 2025 13:17

eddeee888 force-pushed the fix-external-signature branch 2 times, most recently from f1a4aab to b03b692 Compare May 8, 2025 14:58

eddeee888 force-pushed the federation-fixes branch from 6a2bca7 to 30ac893 Compare May 9, 2025 13:40

eddeee888 added 8 commits May 9, 2025 23:41

Ensure @external does not generate resolver types

6dc73ab

- Handle external directive when part or whole type is marked - Add changeset - Add test cases for @provides and @external

Format and minor text updates

97ed2b0

Add comments

8003feb

Fix __resolveReference not getting generated

a6f9d91

Fix test with __isTypeOf when not needed

7b7301d

Fix type cast that results in wrong type

7315f8e

Revert unncessary changes to FieldDefinitionPrintFn

01312c3

Re-format

9d6c170

eddeee888 force-pushed the fix-external-signature branch from b03b692 to 9d6c170 Compare May 9, 2025 13:41

eddeee888 added 2 commits May 11, 2025 20:06

Convert to use AST Node instead of GraphQL Type

2678432

Update test template

fa55f90

eddeee888 commented May 12, 2025

View reviewed changes

packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts Show resolved Hide resolved

eddeee888 commented May 12, 2025

View reviewed changes

eddeee888 added 2 commits May 13, 2025 22:24

Cache field nodes to generate for processed objects

a538b3f

Put FIXME on base-resolvers-visitor to remove Name method

78bbdb9

eddeee888 marked this pull request as ready for review May 13, 2025 12:47

eddeee888 commented May 13, 2025

View reviewed changes

eddeee888 requested a review from dotansimha May 13, 2025 12:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[resolvers][federation] Fix fields or types being wrong generated when marked with @external #10287

[resolvers][federation] Fix fields or types being wrong generated when marked with @external #10287

eddeee888 commented Feb 10, 2025 •

edited

Loading

changeset-bot bot commented Feb 10, 2025 •

edited

Loading

eddeee888 Feb 10, 2025

eddeee888 May 13, 2025

eddeee888 May 12, 2025

eddeee888 May 12, 2025

eddeee888 May 12, 2025

eddeee888 May 12, 2025

eddeee888 May 13, 2025 •

edited

Loading

eddeee888 May 13, 2025

[resolvers][federation] Fix fields or types being wrong generated when marked with @external #10287

Are you sure you want to change the base?

[resolvers][federation] Fix fields or types being wrong generated when marked with @external #10287

Conversation

eddeee888 commented Feb 10, 2025 • edited Loading

Description

Type of change

How Has This Been Tested?

changeset-bot bot commented Feb 10, 2025 • edited Loading

🦋 Changeset detected

eddeee888 Feb 10, 2025

Choose a reason for hiding this comment

eddeee888 May 13, 2025

Choose a reason for hiding this comment

eddeee888 May 12, 2025

Choose a reason for hiding this comment

eddeee888 May 12, 2025

Choose a reason for hiding this comment

eddeee888 May 12, 2025

Choose a reason for hiding this comment

eddeee888 May 12, 2025

Choose a reason for hiding this comment

eddeee888 May 13, 2025 • edited Loading

Choose a reason for hiding this comment

eddeee888 May 13, 2025

Choose a reason for hiding this comment

eddeee888 commented Feb 10, 2025 •

edited

Loading

changeset-bot bot commented Feb 10, 2025 •

edited

Loading

eddeee888 May 13, 2025 •

edited

Loading