[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

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

@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

        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


-  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?

Yeah, dealing with both can be annoying. I think nodes are more relevant here. can be done later

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

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

          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.

@external

…n marked with @external (#10287) * 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 * Format and minor text updates * Add comments * Fix __resolveReference not getting generated * Fix test with __isTypeOf when not needed * Fix type cast that results in wrong type * Revert unncessary changes to FieldDefinitionPrintFn * Re-format * Convert to use AST Node instead of GraphQL Type * Update test template * Cache field nodes to generate for processed objects * Put FIXME on base-resolvers-visitor to remove Name method

@external

…n marked with @external (#10287) * 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 * Format and minor text updates * Add comments * Fix __resolveReference not getting generated * Fix test with __isTypeOf when not needed * Fix type cast that results in wrong type * Revert unncessary changes to FieldDefinitionPrintFn * Re-format * Convert to use AST Node instead of GraphQL Type * Update test template * Cache field nodes to generate for processed objects * Put FIXME on base-resolvers-visitor to remove Name method

@external

…n marked with @external (#10287) * 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 * Format and minor text updates * Add comments * Fix __resolveReference not getting generated * Fix test with __isTypeOf when not needed * Fix type cast that results in wrong type * Revert unncessary changes to FieldDefinitionPrintFn * Re-format * Convert to use AST Node instead of GraphQL Type * Update test template * Cache field nodes to generate for processed objects * Put FIXME on base-resolvers-visitor to remove Name method

@external

* [resolvers][federation] Fix mapper being incorrectly used as the base type for reference (#10216) * [resolvers][federation] Add `__resolveReference` to applicable `Interface` entities, fix Interface types having non-meta resolver fields (#10221) * Add __resolveReference for applicable Interfaces - Deprecate generateInternalResolversIfNeeded.__resolveReference - Fix tests - Deprecate onlyResolveTypeForInterfaces - Add changeset - Cleanup - Handle __resolveReference generation in Interface - Let FieldDefinition decide whether to generate __resolveReference by checking whether parent has resolvable key * Fix test * chore(dependencies): updated changesets for modified dependencies * chore(dependencies): updated changesets for modified dependencies * [resolvers] Ensure `__isTypeof` is only generated for implementing types (of Interfaces) or Union members (#10283) * Implement logic to only generate __isTypeOf for implementing types OR union members * Remove unused types * Add changeset * Remove generateInternalResolversIfNeeded * Fix dev tests * Refactor to use parsedSchemaMeta * [resolvers][federation] Bring Federation reference selection set to ResolversParentTypes instead of each resolver (#10297) * Bring reference selection set to ResolversParentTypes * Put back old types to extractReferenceSelectionSet * Update tests for TDD * Handle parent type consistently for __resolveReference and subsequent resolvers * Update tests * chore(dependencies): updated changesets for modified dependencies * Add missing changeset * chore(dependencies): updated changesets for modified dependencies * [resolvers][federation] Fix fields or types being wrong generated when marked with @external (#10287) * 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 * Format and minor text updates * Add comments * Fix __resolveReference not getting generated * Fix test with __isTypeOf when not needed * Fix type cast that results in wrong type * Revert unncessary changes to FieldDefinitionPrintFn * Re-format * Convert to use AST Node instead of GraphQL Type * Update test template * Cache field nodes to generate for processed objects * Put FIXME on base-resolvers-visitor to remove Name method * Fix unit test * [resolvers][federation] Fix federation @requires type (#10366) * Update test setup * Implement @requires combination * Add changeset * Force release alpha * Fix issue with empty array, set up tests * Generate FederationReferenceTypes once * Update tests related to FederationReferenceTypes * Update dev-tests * Revert force release * Update test related to mapper * [resolvers] Refactor to remove NameNode override and simplify federation functions (#10377) * Remove NameNode override and refactor relevant references * Simplify federation utils by making functions handle nodes * Fix lint issue * CODEGEN-834 - [cli] Handle partial generation success (#10376) * Add writeOnPartialSuccess flag to partially write successful generateiong * Update config name from writeOnPartialSuccess to allowPartialOutputs * Ensure consistent experience on complete failure, update tests * Restructure code and comment * Update website schema * Update doc * Drop Node 18 support (#10392) * Drop Node 18 support * Add changeset * Drop graphql tools prisma loader (#10400) * Drop @graphql-tools/prisma-loader * Add changeset * Update yarn.lock * Update tsconfig for Node 20 (#10403) * Update tsconfig to Node 20 recommended version * Make babel allow declare TS field * Bump packages (#10404) * Bump dependency-graph to ^1.0.0 * Bump nock to 14.0.0 * Bump debounce to v2 and remove types package * Bump ESM packages - chalk - detect-indent - log-symbols - auto-bind * Revert "Bump ESM packages" This reverts commit 7b79aaa. * Migrate jitit to v2 * Bump cosmiconfig to v9 * Add changesets * Use min. jiti v2.3.0 to handle default import like before * [CLI] Bump deps for next major version (#10405) * Bump listr2 * Migrate implementation and update tests for listr2 * Migrate inquirer to inquirer/prompts * Remove old resolutions & update lock file (#10407) * Remove unnecessary resolutions * Remove deprecated config options for next major version (#10408) * Remove deprecated config - watchConfig - dedupeFragments - noGraphQLTag * Add changesets * Regen website schema * CODEGEN-840 - Handle empty object type better (#10409) * Update ts-resolvers to handle {} correctly * Refactor types * Update ts-documents to handle {} better * Add changeset * Update tests * Migrate to Vitest (#10410) * Install vitest * Set up vitest config * Install tsconfigPaths * Set up root vite config * Set up vitest project for typescript-resolvers * Set up vitest for plugins-helpers and fix ESM test * Set up client preset vitest project * Prepare for vitest.setup.ts * Set up vitest for graphql-modules-preset * Set up vitest for typescript plugin * Set up vitest for typed-document-node * Set up vitest for typescript-operations * Set up vitest for gql-tag-operations * Set up vitest in typescript-document-nodes * Set up vitest for visitor-plugin-common * Set up vitest for time plugin * Set up vitest for schema-ast plugin * Set up vitest for introspection plugin * Set up vitest for fragment-matcher plugin * Set up vitest for add * Set up vitest for graphql-codegen-core * Install memfs to support fs testing * Set up test config for cli * Fix init tests * Set up vitest for graphql-codegen-testing * Update linting rules for test files * Fix cli-* specs * Handle cwd mocking for CLI command * Fix cjs vs esm issues in codegen.spec * Fix config.spec * Fix watcher.spec and trim down the mocks * Migrate rest of examples to vitest * By jest things. Not even jesting 🤡 * Fix cache jest -> vitest * Fix lint issues and unncessary disables * Fix vitest spec * Fix test and generated things in examples * Fix vitest things * Bump ESM packages (#10415) * Bump ESM packages * Add changeset * [typescript] Remove NameNode overrides (#10416) * Avoid NameNode and StringValue string conversion * Add changeset * Remove CI config used for dev * [resolvers] Report if an object can have `__isTypeOf` resolver to output meta (#10417) * Report hasIsTypeOf in meta * Add changeset * Revert "Remove CI config used for dev" This reverts commit 6d2c6e4. * Revert "Bump ESM packages (#10415)" (#10423) This reverts commit 939752c. * Remove branch debug * Update test setup: test once by default, and add watch flag to watch --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

eddeee888 commented Feb 10, 2025

View reviewed changes

packages/utils/plugins-helpers/src/federation.ts Show resolved Hide resolved

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

Merged

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

dotansimha approved these changes May 20, 2025

View reviewed changes

eddeee888 merged commit b446e84 into federation-fixes May 22, 2025
15 checks passed

eddeee888 deleted the fix-external-signature branch May 22, 2025 11:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[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

Uh oh!

eddeee888 commented Feb 10, 2025 •

edited

Loading

Uh oh!

changeset-bot bot commented Feb 10, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

eddeee888 May 12, 2025

Uh oh!

eddeee888 May 12, 2025

Uh oh!

eddeee888 May 12, 2025

Uh oh!

dotansimha May 20, 2025

Uh oh!

eddeee888 May 12, 2025

Uh oh!

eddeee888 May 13, 2025 •

edited

Loading

Uh oh!

eddeee888 May 13, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

[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

Uh oh!

Conversation

eddeee888 commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Uh oh!

changeset-bot bot commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

Uh oh!

Uh oh!

eddeee888 May 12, 2025

Choose a reason for hiding this comment

Uh oh!

eddeee888 May 12, 2025

Choose a reason for hiding this comment

Uh oh!

eddeee888 May 12, 2025

Choose a reason for hiding this comment

Uh oh!

dotansimha May 20, 2025

Choose a reason for hiding this comment

Uh oh!

eddeee888 May 12, 2025

Choose a reason for hiding this comment

Uh oh!

eddeee888 May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddeee888 May 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

eddeee888 commented Feb 10, 2025 •

edited

Loading

changeset-bot bot commented Feb 10, 2025 •

edited

Loading

eddeee888 May 13, 2025 •

edited

Loading