Skip to content

[resolvers][federation] Bring Federation reference selection set to ResolversParentTypes instead of each resolver #10297

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 5, 2025

Conversation

eddeee888
Copy link
Collaborator

@eddeee888 eddeee888 commented Feb 19, 2025

Ref CODEGEN-512

Description

Context

A Federation entity's resolvers may be triggered by one of these two paths:

  1. From the same subgraph - if there's a path from the operation root to the relevant field in the query plan
  2. From a different subgraph - after gateway calls __resolveReference to resolve the relevant field

Let's look at this example:

# User subgraph
type Query {
  me: User
}

type User @key(fields: 'id') {
  id: ID!
  name: String!
}

# Book subgraph
type Query {
  book: Book
}

type Book {
  id: ID!
  author: User!
}

type User @key(fields: 'id', resolvable: false) {
  id: ID!
}

The following query triggers the 1st scenario for User.name:

query {
  me {
    id
    name # triggers `User.name` from within the same subgraph
  }
}

When the 1st scenario triggers, it behaves like a non-Federated Graph i.e. User.name's Parent is User


The following query triggers the 2nd scenario for User.name:

query {
  book {
    author {
      name # triggers `User.name` through `__resolveReference`
    }
  }
}

In this scenario:

  • __resolveReference receives {__typename: 'User', id: string} as ref
  • if __resolveReference is not declared, {__typename: 'User', id: string} is automatically passed on and becomes the User.name's Parent

Breaking Change Proposal

Currently, User.name's Parent type:

  • is being typed inline as { __typename: 'User' } & GraphQLRecursivePick<ParentType, {"id":true}> if a field is marked as @external (implemented in here but I don't agree with this implementation, or my understanding of @external is wrong 🤔 ) ...
  • OR is User...
    ... but never both at the same time.

This is not correct because there are two ways to get to User.name so the Parent should be either User or { __typename: 'User' } & GraphQLRecursivePick<ParentType, {"id":true}>.

So, here's how I think this should be changed for a federated entity (User in this case):

  • ResolversParentTypes['User'] should be User | { __typename: 'User' } & GraphQLRecursivePick<ParentType, {"id":true}> (plus any extra possibilities from added by @provides - More on this in another PR)
    • (*) This means User.name need to know/check the parent, because we don't know which flow it comes from.
  • If a UserMapper is used, then both paths require the previous resolver to return the UserMapper. i.e. ResolversParentTypes['User'] simply becomes UserMapper (existing behaviour)
    • (**) This means __resolveReference MUST be explicitly implemented to turn ref into UserMapper

This solution makes it simpler to write plugin code, because we "normalise" both paths' Parent types. However, (*) and (**) could cause friction for users to understand and use - unless we make Server Preset to auto generate resolvers, to make sure users implement resolvers to return mappers, to avoid runtime errors for Federation use cases (which can be done)

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

  • Unit test

Copy link

changeset-bot bot commented Feb 19, 2025

⚠️ No Changeset found

Latest commit: 9a95221

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@eddeee888 eddeee888 marked this pull request as draft February 19, 2025 11:16
@eddeee888 eddeee888 force-pushed the fix-federation-entities-parent-type branch from 6d084b0 to 066f1bb Compare February 27, 2025 11:26
@eddeee888 eddeee888 marked this pull request as ready for review February 27, 2025 13:34
@eddeee888 eddeee888 merged commit d8644ae into federation-fixes Mar 5, 2025
16 checks passed
@eddeee888 eddeee888 deleted the fix-federation-entities-parent-type branch March 5, 2025 09:52
@eddeee888 eddeee888 mentioned this pull request Mar 5, 2025
35 tasks
eddeee888 added a commit that referenced this pull request Mar 21, 2025
…esolversParentTypes 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
eddeee888 added a commit that referenced this pull request Apr 23, 2025
…esolversParentTypes 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
eddeee888 added a commit that referenced this pull request May 9, 2025
…esolversParentTypes 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant