Fixes types for federation

[KoltesDigital]

Related #4722

[changeset-bot]

🦋 Changeset detected

Latest commit: a17a8d3

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

This PR includes changesets to release 6 packages

Name	Type
@graphql-codegen/flow-resolvers	Patch
@graphql-codegen/visitor-plugin-common	Patch
@graphql-codegen/typescript-resolvers	Patch
@graphql-codegen/plugin-helpers	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

[dotansimha]

Thank you @KoltesDigital . Can you please add a changeset file? (run yarn changeset, it will guide you). I think we need a patch bump in this PR.

@jakeblaxon it's related to changes you recently added. Do you mind give it a quick look?

[jakeblaxon]

I've looked over the changes, but it looks like this PR would cause the issues in #4493 to reappear. A major reason is that it changes the transformParentType to only address the resolveReference field, but not any of the other fields. It also eliminates checking if the type is an extended (external) federation type or a base type, which is necessary to resolve the above issue.

Looking at #4722, I see that you're using mapped types. I didn't address the mapped type case at all when working on adding this federation functionality, so it may be buggy and there is definitely room for improvement there. For your fullName resolver, however, the reason you're not able to access the firstName and lastName types from the external User parent type is because you haven't specified that they are required by adding them to a @requires directive. You can solve that issue by adding that:

extend type User @key(fields: "id") {
    id: ID! @external
    firstName: String! @external
    lastName: String! @external
    fullName: String! @requires(fields: "firstName lastName")
}

(You will need to add all required external fields to your extended type, otherwise the Apollo Gateway will throw an error, hence why I've added them here). If you don't specify that these fields are required, then the Apollo Gateway may not provide them to you for certain requests. For example, what if I requested the following:

{
  getUser(...) {
    id
    firstName
    fullName
  }
}

In this case, the Apollo Gateway will only request { id firstName } from the base schema and pass this on as the parent type to your fullName resolver. Since only the id is specified as a key, there's no reason the Apollo Gateway would need to request lastName from the base schema. It doesn't know that you need it for your resolver. That's why you need to tell this to the Apollo Gateway with the @requires directive.

Since User is an extended type, I believe the __resolveReference resolver will never get called (although I have to verify this). Thinking about it intuitively though, each custom field that you add to an extended type will have its own resolver, so the Apollo Gateway will just call these field-specific resolvers instead of _resolveReference. It really only makes sense to have a __resolveReference in the base type, and we already account for this (as of #4542). Again, the __resolveReference resolver gets called directly from Apollo Gateway, where the parent type is the User field not from the base schema but from a schema that extends it. The only guaranteed fields that are available in this case are the @key fields, because these are the only fields that are guaranteed to be in this type across every schema. That's why we only pass in the @key fields subset of the parentType for the __resolveReference resolver.

With this in mind, I would recommend closing this PR without merging and also closing #4722 as a misunderstanding (@dotansimha ). Please let me know if you have any questions regarding this though, and I'll be happy to help out more. This is complicated and counterintuitive stuff, so I definitely don't blame you for any misunderstandings. I myself have only gotten familiar with these details through many months of intimate trial and error. Thanks!

[KoltesDigital]

I disagree with your understanding of federation. My understanding is as follow.

With or without federation, the parent type is not required at all to mirror the GraphQL entity type. My example of User, without the federation, would be correct anyway: the JS object has firstName and lastName properties, there's a resolver fullName, a GraphQL query can only retrieve the fullName, that's totally correct. I will call them JS model and GraphQL model, they may overlap but they don't have to.

Now, Apollo Federation exposes entities whose fields are served by different services. These different services may have different JS models, so they need to bind one to one. That's the purpose of __resolveReference. This is the entry point in a service. This function receives a "plain" object with fields corresponding to @keys, and returns another object (JS model) which is latter used as the first argument of related resolvers in that service.

In this example, __resolveReference returns a JS model, which is used by a resolver to return a value to a GraphQL query. Perfectly valid, no @requires needed.

Speaking of users and names, I admit it makes sense to have all these fields, but it's just the easiest example I found to show the bug. I just didn't wanted to call these fields foo and bar. But the basic principle is: JS models and GraphQL models are distinct, though they may overlap, they don't have to.

Besides, the example is based on the project I'm working on for several months, with currently five federated services. For now the types are written by hand and it works as I claim. With the current codegen, it doesn't compile anymore. With my patch, it compiles (and the generated JS is obviously the same, as it's only an issue with TS type declarations).

You can easily code a new project which replicates this example and verify my claims.

---

That being said, I admit that this patch is based on my experience that every service have strong JS models. This might not always be the case: __resolveReference is optional, and if not present, the resolvers are given the "plain object" as first argument (i.e. __resolveReference is an intermediary transform function). My experience is that this plain object is only the primary key of the JS model, which is always bigger than the key (otherwise there's no need for this service), so all my extended entities always have a __resolveReference.

If we want to support both cases, I believe we need to distinct whether an entry is given in the mapper config. If there is a parent type (JS model), it means a __resolveReference needs to be defined to get the JS model. If there is not, it means the resolvers expect the "plain object" as first argument, in which case __resolveReference is not needed.

[KoltesDigital]

Regarding #4493 (which I haven't searched for earlier, sorry!) I believe there is a modeling issue which leads to (or comes from) a wrong understanding of federation or even GraphQL itself. Service 1 should not give propertyId, as it gives property which has an id key. So both getRoomType(...) { propertyId } and getRoomType(...) { property { id } } give the same id, and is only executed on service 1. One may argue there's a performance difference, but that would be the only difference, and I'm not sure it's noticeable. Service 2 is involved as soon as a field it provides is queried (e.g. getRoomType(...) { property { id name } }).

Searching online for "GraphQL best practices" give the same advice: https://atheros.ai/blog/graphql-best-practices-for-graphql-schema-design , https://towardsdatascience.com/graphql-best-practices-3fda586538c4 , etc.

Given that, the JS model has a propertyId field, the GraphQL model has a property field, there's a property resolver which returns an object which at least contains the primary key fields (id in this case) so that the other services can bind to their own local JS models of Property. Perfectly valid, no @requires needed.

[jakeblaxon]

Ahh, I see what you're saying now. I was assuming that lastName and firstName were fields from the base type, but they're actually gotten from the resolveReference from the extended type. I admit I've never used a __resolveReference for an extended federation type before, so I did not know this was possible. Knowing this now, I see two cases that we need to address for an extended federation type:

1. If there is no __resolveReference provided, then use the @key @requires subset as the parent type for each resolver.
2. If __resolveReference is provided, then just use the full parent type without taking a subset for each resolver, but still use the @key subset for the __resolveReference resolver's parent type (which was the default behavior before my PR). This will use the mapped type if it's provided, otherwise it'll just use the graphql type.

Right now, only case 1 is accounted for, so this PR should seek to add case 2 while preserving case 1. Does this sound correct to you? If so, let's start looking into ways that we can do this. In the meantime, though, I'd convert this PR to a draft since it would break case 1.

[jakeblaxon]

Actually, case 2 is a little more complicated since we can still use @requires for certain resolvers, and the required fields aren't necessarily in the mapped type. I think we'd have to combine the graphql model's @key @requires subset with the mapped type to make sure we cover all available fields. So the parent type for fullName in this case should be something like:

{ __typename: 'User' } & GraphQLRecursivePick<ParentType, {"id":true}> & MappedType

That way, if we provide firstName and lastName from __resolveReference but we also add a @requires(fields: "address") to the fullName field, we'll have access to address from the base type as well. In this case the parent type would look like:

{ __typename: 'User' } & GraphQLRecursivePick<ParentType, {"id":true, "address":true}> & MappedType

I need to test this to make sure this is what actually happens though. This gets pretty complicated intuitively when you can use __resolveReference and @requires together. Does the Apollo Gateway know to merge the required fields into the js object that __resolveReference returns?

[KoltesDigital]

The problem is, as a TS types provider, the tool can't generate types based on the way the types are used. I do not recommend a type union neither, because it would require adding a check in the TS file (otherwise TS would complain) but this check is useless because we know upfront that the parentType is either resolved or not.

Regarding @requires, this is tedious: __resolveReference gets the additional fields, and they are not later merged into the JS model! So if a GraphQL field requires other fields, the entity's JS model must have a way to forward these optional fields to the requested resolver. That's why in my patch __resolveReference optionally accepts any field required by any field, of the entity (see

graphql-code-generator/packages/plugins/typescript/resolvers/tests/federation.spec.ts

Line 119 in 39f71c9

__resolveReference?: ReferenceResolver<Maybe<ParentType>, { __typename: 'User' } & GraphQLRecursivePick<ParentType, {"id":true}> & DeepPartial<GraphQLRecursivePick<ParentType, {"name":true,"age":true}>>, ContextType>;

). The second case you listed does not actually happen.

That's why I mentioned relying on the mapper config key: if there's one, __resolveReference must be present and the resolvers' first argument is of ParentType, if not, __resolveReference should not and the resolvers' first argument is the same as for __resolveReference. What do you think about this solution?

---

To be even more spec-compliant: without __resolveReference, required fields are given to all resolvers anyway. For instance, this code would be valid, even though I doubt anyone would ever use this behavior.

extend type T {
  a: String!
  b: String! @requires(field: "a")
  c: String!
}

export const resolvers: RoomTypeSchemaResolvers<any> = {
  T: {
    c(parent) {
      return parent.a ? 'foo': 'bar';
    }
  }
};

Unless someone complains, I recommend to not care about this behavior :)

[KoltesDigital]

@jakeblaxon Hi! Any news?

[jakeblaxon]

I apologize for the delay on this. I just reviewed this thread and I agree with your solution. I think using the mapper config key to determine whether to require the __resolveReference resolver is a good idea. For those who use __resolveReference but don't use a mapped type, they can work around this by manually specifying the desired parent type of each resolver.

With this in mind, we should be able to use most of what's currently in your PR. We just need to make a few adjustments to preserve current functionality and add the conditional check. Do you want to make the changes? I'll be able to review pretty frequently over the next couple of weeks, so just let me know when changes are ready and I'll take a look as soon as I can. Thank you for your patience!

[KoltesDigital]

Hi! No worry, it's great to make progress on that. I'll try to make the change. I also remember that I was bothered by a test I changed but the "expected" result might not be what we actually expect, I'll have a look at that. And I'll make the changeset.

[KoltesDigital]

@jakeblaxon now as discussed.

One thing I'm bothered with is that all parent types are generated, even when they are overriden by custom mapped types. I use to name my models the same as their GraphQL counterparts, thus it would conflict. It looks like the generation of these types leads to the generation of the related <TypeName>Resolvers types. I don't know the project enough to be able to change that. How do you feel about that?

[mingmeih]

Hi! Is there any update on this?

[kamilkisiela]

I'm reviewing it currently

[kamilkisiela]

I added few comments

[kamilkisiela]

This is not true. The __resolveReference should be available for all types with @key that are local to the service.

[kamilkisiela]

why do you added mappers to all tests?

[kamilkisiela]

again, __resolveReference is used to resolve an a reference to an entity which means mappers can't change that behavior. Mappers are only for "us" to define an exact type instead of the default one from the schema.

[kamilkisiela]

We try to avoid any breaking changes and the thing I pointed out is one. We're going to rewrite federation support from scratch soon.

[kamilkisiela]

Ok, I reviewed your changes and created a document about Federation and typescript-resolvers. Feedback needed https://www.notion.so/Federation-typescript-resolvers-3b47e825ff9a43b7a970c28e8784a6f7. This will help to rewrite the federation part in codegen.

[kamilkisiela]

Created this PR #5645 to progress with Federation support step by step.

[bsmedberg-xometry]

Is there anything I can do to rebase or fix this up for merge? The bugs this fixes affect my usage frequently and I'm available to help with this if there's something I can do.

[Urigo]

@bsmedberg-xometry we've just rebased the PR here: #5645

Would be great if you could pick this work up and get it to the finish line

[charlypoly]

See #5645 for future plans regarding Federation types improvements.