Move __resolveReference resolvers on to extensions (version-0.x)

[trevor-scheer]

Backport of #1746

[netlify]

✅ Deploy Preview for apollo-federation-docs ready!

Name	Link
🔨 Latest commit	a05dff4
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-federation-docs/deploys/6260866acb55ef0008515567
😎 Deploy Preview	https://deploy-preview-1747--apollo-federation-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

[benweatherman]

🐐

[queerviolet]

this is a breaking change, right? how does that play with our versioning? i guess we just cut another experimental minor?

[trevor-scheer]

I don't think this would be a breaking change unless users are for some reason reaching into what is already a hack and definitely not in a recommended public API way. This is strictly an implementation detail with no effect on the public API, users should never know any different.

I know @martijnwalraven may have thought otherwise (ardatan/graphql-tools#2857 (comment)) but I'm not sure this affects anything public.

[dobrynin]

I'm not positive, but it seems that this commit broke our usage of federation v1. Should an equivalent change have been made in @apollo/subgraph? We have the following code that no longer seems to run

  const federationResolvers = baseTypes.reduce(
    (acc, { object: { name }}) => {
      acc[name] = {
        async __resolveReference({ id }) {
          console.log("Resolving reference", name, id);
          ...
          return Item;
        },
      };
      return acc;
    },
    {}
  );

I believe this is (or was) standard usage, see here for example: https://www.apollographql.com/docs/federation/api/apollo-subgraph/

[dobrynin]

We are still using @apollo/[email protected] and @apollo/[email protected] because we are blocked from upgrading to federation v2 by type-graphql's incompatibility w/ GraphQL v16.

[trevor-scheer]

@dobrynin I'm not sure I follow.

Can you please share a reproduction of the issue you're seeing now? Either:

• A cloneable repository with reproduction steps
• A failing test case (which used to pass in a previous version of gateway / subgraph)

I don't know what it means that "the code no longer runs". Is there a useful error message? Does it blow up at runtime?

I also don't understand why you're constructing an object of resolvers whose types consist only of a __resolveReference function. This looks like an abstraction but I'm not sure what the goal is. Our examples you linked to don't display any code that looks like this.

[dobrynin]

@trevor-scheer, I will look more into this tomorrow. On our latest deploy to a development environment some of our queries broke w/ errors like Cannot return null for non-nullable field Account.name. Previously, cross-service type resolution worked as expected. I've looked for a relevant change in our code base that could have caused this but haven't found anything, so I started looking at recent changes to __resolveReference and addResolversToSchema.

By "the code no longer runs", I mean that we are not seeing the console logs I posted above in our Accounts service. Seemingly __resolveReference is no longer being invoked.

We dynamically add federation resolvers to our service schemas, so that is why we have those types w/ just __resolveReference. The relevant code looks like this

  if (federationResolvers) {
    addResolversToSchema(federatedSchema, federationResolvers);
  }

Hope this helps in understanding our usage.

[trevor-scheer]

@dobrynin I think I understand the issue. Is the addResolversToSchema function coming from graphql-tools or from us?

I don't think this is a subgraph issue. By adding __resolveReference to the types yourself, you were mimicking an implementation detail of our own addResolversToSchema. Ultimately, if the reference resolvers are still being added to the type itself rather than in the (new) proper place on Type.extensions, the gateway will not be able to find the reference resolver.

We could update the gateway to look in both places (requiring a // @ts-expect-error) but given that this is the only issue reported so far, I'm inclined to think the impact of this change is small (and as mentioned above, is not part of the public API).

[dobrynin]

@trevor-scheer appreciate your thoughts, I'll take another look at our code w/ this in mind and report back if the issue persists.

[dobrynin]

Just reporting back that the issue disappeared when reverting to "@apollo/subgraph": "0.3.3" and "@apollo/gateway": "0.49.0". We'll pin these package versions for now until we have time to figure out how to make our code play nicely with the changes in Apollo's code.

At the moment, I'm not sure how to adapt our code. We used the addResolversToSchema pattern based on an example in the type-graphql library (https://github.com/MichalLytek/type-graphql/blob/master/examples/apollo-federation/helpers/buildFederatedSchema.ts).

[dobrynin]

@MichalLytek FYI^

[trevor-scheer]

@dobrynin addResolversToSchema from apollo-graphql is the predecessor to the addResolversToSchema which was changed in this PR. The one being used in the example is dated and specifically doesn't include the necessary changes to be compatible with the change that was made to gateway in this PR.

addResolversToSchema is not really intended to be used externally. But given that you already are, it's not unreasonable to offer a workaround. This is still not public API, so I recommend not using it as soon as you're able.

Replace your current addResolversToSchema import with this:

import { addResolversToSchema } from '@apollo/subgraph/dist/schema-helper';

The example you provided from type-graphql appears to be working around an issue which specifically this PR unblocked: adding custom directives to a federated schema. Others in the community seem to have gravitated toward graphql-tools's mapSchema function for this. Prior to this PR, this didn't work (it wouldn't preserve the __resolveReference functions - which is not a shortcoming on their end, but an abuse of the types on our end). It does preserve extensions as expected, and this should now be considered a viable option. https://www.graphql-tools.com/docs/schema-directives#implementing-schema-directives

I would advise moving to the mapSchema approach instead, and away from the workaround I've provided as quickly as possible. addResolversToSchema is not considered public API and thus changes to implementation details within that function should be expected.

Note: the workaround I've recommended should only be migrated to in tandem with the upgrade of both packages.

[dobrynin]

Thank you @trevor-scheer! Will go with the workaround for the moment; your recommended solution goes a bit beyond my comprehension 😅 Shouldn't type-graphql be the one to call mapSchema internally for the directives that it makes use of? And I'm unclear on how we would be able to add the __resolveReference. I don't think it's possible right now w/ type-graphql except by using addResolversToSchema (at least that's what I gather from reading MichalLytek/type-graphql#369).

[trevor-scheer]

Yeah, I'm not that familiar with type-graphql to be honest, so I'm not sure where the responsibility lies. What I understood was that you were taking after an example they provided. I think that example can be simplified in the way that I described (which in turn would mean that your code should be able to be simplified in the same way).

In any case, let us know here if that workaround works for you or not.

[dobrynin]

@trevor-scheer your workaround appears to working w/ the latest of the federation v1 packages. I'll make an issue in the type-graphql repo that their example is out of date and link to our discussion here. Thank you!

[trevor-scheer]

@dobrynin thanks for following up!