Skip to content
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

Add example for Apollo Federation v2 #1448

Merged
merged 11 commits into from
Oct 4, 2023

Conversation

jhanggi
Copy link
Contributor

@jhanggi jhanggi commented Apr 26, 2023

Originally found a bug that ended up being a bug in Apollo. After they fixed that, this PR changed to be a new example demonstrating how to set up Federation 2.


Describe the Bug
When using Apollo Federation, when using a reference resolver to resolve an interface, it it unable to resolve the type properly when the resolver is async.

To Reproduce
Start up the apollo-federation example and run the sample graphql in the apollo explorer.

Cannot resolve type for interface Product! You need to return instance of object type class, not a plain object!

Expected Behavior
It should resolve the products to their specific types.

Logs

"Error: Cannot resolve type for interface Product! You need to return instance of object type class, not a plain object!",
            "    at type.graphql_1.GraphQLInterfaceType.resolveType (/Users/jhanggi/code/type-graphql/src/schema/schema-generator.ts:467:27)",
            "    at withResolvedType (/Users/jhanggi/code/type-graphql/node_modules/@apollo/subgraph/src/types.ts:151:23)",
            "    at /Users/jhanggi/code/type-graphql/node_modules/@apollo/subgraph/src/types.ts:206:14",
            "    at Array.map (<anonymous>)",
            "    at entitiesResolver (/Users/jhanggi/code/type-graphql/node_modules/@apollo/subgraph/src/types.ts:175:26)",
            "    at _entities (/Users/jhanggi/code/type-graphql/node_modules/@apollo/subgraph/src/buildSubgraphSchema.ts:78:85)",
            "    at field.resolve (/Users/jhanggi/code/type-graphql/node_modules/apollo-server-core/src/utils/schemaInstrumentation.ts:106:18)",
            "    at executeField (/Users/jhanggi/code/type-graphql/node_modules/graphql/execution/execute.js:481:20)",
            "    at executeFields (/Users/jhanggi/code/type-graphql/node_modules/graphql/execution/execute.js:413:20)",
            "    at executeOperation (/Users/jhanggi/code/type-graphql/node_modules/graphql/execution/execute.js:344:14)"

Environment (please complete the following information):

  • OS: Mac
  • Node 16.13.3
  • Package version main and 2.0.0-beta.1
  • TypeScript version ~4.9.5 (running within this repo off of main)

Additional Context

In order to reproduce the error in a slimmed down application, I updated the apollo-federation example to v2. Then I turned Product into an interface and created two subtypes for it.

If I changed the resolveProductReference to be synchronous the issue went away, but in my case I need it to be async.

Thanks to the stacktrace I was able to track it down to the instance in the resolveType function in schema-generator being a promise in this scenario. If we await it, everything looks good.

Not being super familiar with this repository, there might be a better place to await this instance. I'm also happy to write a test if you could point me to the best spot.

Additionally, it might be nice to have some documentation around how to spin up one of the examples. I added "start:federation": "ts-node ./examples/apollo-federation/index.ts" to package.json so I could npm run start:federation, but I wonder if there's a better way

@MichalLytek MichalLytek added Community 👨‍👧 Something initiated by a community Bugfix 🐛 🔨 PR that fixes a known bug labels Apr 28, 2023
@MichalLytek MichalLytek self-requested a review April 28, 2023 07:37
@MichalLytek
Copy link
Owner

MichalLytek commented Apr 28, 2023

Something is wrong with your apollo federation setup, that it injects Promise as a value to resolveType.

graphql-js types clearly define that the argument is the source object, the awaited data, not MaybePromise:

export declare type GraphQLTypeResolver<TSource, TContext> = (
  value: TSource,
  context: TContext,
  info: GraphQLResolveInfo,
  abstractType: GraphQLAbstractType,
) => PromiseOrValue<string | undefined>;

@jhanggi
Copy link
Contributor Author

jhanggi commented May 1, 2023

This may possibly be an issue with federation then, and I'm not quite sure how to debug that. I've taken the type-graphql out of so it's just plain subgraph logic and we still end up with a promise being passed into __resolveType.

I'm going to try to dig in a little more.

import { ApolloServer } from "apollo-server";

import { buildSubgraphSchema } from "@apollo/subgraph";
import { GraphQLResolverMap } from "@apollo/subgraph/dist/schema-helper";
import gql from "graphql-tag";
import { products } from "./data";
import Product from "./product";
import ProductsResolver from "./resolver";

/**
 * This is what federation runs. You can run this directly against this subgraph's port (3003)
 * query ($representations: [_Any!]!) {
    _entities(representations: $representations) {
      ... on Product {
        name
      }
    }
  }

  Variables:
   {"representations":[{"__typename":"Product","upc":"1"},{"__typename":"Product","upc":"2"},{"__typename":"Product","upc":"3"},{"__typename":"Product","upc":"1"},{"__typename":"Product","upc":"1"},{"__typename":"Product","upc":"2"},{"__typename":"Product","upc":"3"},{"__typename":"Product","upc":"1"}]}
 */

const typeDefs = gql`
  extend schema @link(url: "https://specs.apollo.dev/federation/v2.3", import: ["@key"])

  type Dining implements Product @key(fields: "upc") {
    height: String!
    name: String!
    price: Float!
    upc: String!
    weight: Float!
  }

  interface Product @key(fields: "upc") {
    name: String!
    price: Float!
    upc: String!
    weight: Float!
  }

  type Query {
    topProducts(first: Float! = 5): [Product!]!
  }

  type Seating implements Product @key(fields: "upc") {
    name: String!
    price: Float!
    seats: Float!
    upc: String!
    weight: Float!
  }
`;

const resolvers: GraphQLResolverMap<any> = {
  Query: {
    topProducts: async (_root, args, _ctx) => {
      return new ProductsResolver().topProducts(args.first);
    },
  },
  Product: {
    __resolveReference: async function resolveProductReference(
      reference: Pick<Product, "upc">,
    ): Promise<Product | undefined> {
      const prod = products.find(p => p.upc === reference.upc);
      return prod;
    },
    // If you remove __resolvetype, you get
    // ""Abstract type \"Product\" `__resolveReference` method must resolve to an Object type
    // at runtime.Either the object returned by \"Product.__resolveReference\" must include a valid `__typename`
    // field, or the \"Product\" type should provide a \"resolveType\" function or each possible type should provide an \"isTypeOf\" function."
    __resolveType(obj: Product): string {
      // If __resolveReference is async, then obj is a Promise, if __resolveReference
      // is syncronous, then this works just fine.
      console.info("resolvetype", obj);
      return obj.constructor.name;
    },
  },
};

export async function listen(port: number): Promise<string> {
  const schema = buildSubgraphSchema({ typeDefs, resolvers });

  const server = new ApolloServer({ schema });

  const { url } = await server.listen({ port });

  console.log(`Products service ready at ${url}`);

  return url;
}

@MichalLytek
Copy link
Owner

Looks like the Apollo team is taking care of this issue 😉
Marking this PR as blocked, so we can come back to it later and verify if it's fixed or not.

@MichalLytek MichalLytek added the Blocked 🚫 Resolving this issue is blocked by other issue or 3rd party stuffs label May 3, 2023
@jhanggi
Copy link
Contributor Author

jhanggi commented May 19, 2023

@MichalLytek The issue is fixed in @apollo/subgraph 2.5 2.4.5. So the change to schema-generator can go away. What do you think of the rest of this updating of the example for federation 2.X?

@jhanggi jhanggi force-pushed the handle_ref_resolver_promise branch 2 times, most recently from ca32676 to aea0268 Compare May 31, 2023 17:23
jhanggi added 2 commits May 31, 2023 12:23
I encountered an error on my application while trying to resolve an interface from a reference resolver in a federated graph. I was able to replicate the issue in the apollo-federation example.

The first thing I did was update the example graphs to federation v2. Then I turned Product into an interface and created two subtypes for it.

I then used the example graphql and got my error.

If I changed the `resolveProductReference` to be synchronous the issue went away, but in my case I need it to be async.

Thanks to the stacktrace I was able to track it down to the instance in the resolveType function being a promise in this scenario. If we await it, everything looks good.
@jhanggi jhanggi force-pushed the handle_ref_resolver_promise branch from aea0268 to 3274b3d Compare May 31, 2023 17:23
@jhanggi
Copy link
Contributor Author

jhanggi commented May 31, 2023

@MichalLytek I've reverted the schema-generator change and upgraded @apollo/subgraph to a version containing the fix. Everything seems to be working now. I suppose the title of this PR would now be "Update apollo-federation example for Federation 2"

@jhanggi
Copy link
Contributor Author

jhanggi commented May 31, 2023

On a related note, I found that Nest has a @ResolveReference decorator.. I don't know if nestjs uses typegraphql under the hood (I think they at least did at one point), but would it be worth adding that decorator in?

@MichalLytek
Copy link
Owner

MichalLytek commented Jun 2, 2023

@ResolveReference is tricky because graphql-js does not allow __ prefix on field names, so I can't build an executable GraphQL schema. The trick I use in commercial projects is doing single _ and then using graphql-tools to map schema and the resolve reference names to the Apollo format (and remove from typedefs).

@MichalLytek
Copy link
Owner

I suppose the title of this PR would now be "Update apollo-federation example for Federation 2"

I think we should keep the old example and create a new, separated one with Federation 2 stuff, like I did in NestJS integration:
https://github.com/MichalLytek/typegraphql-nestjs/tree/master/examples

We can make the second example a bit more complex if needed, to show @interfaceObject and other things 😉

@jhanggi
Copy link
Contributor Author

jhanggi commented Jun 7, 2023

I think we should keep the old example and create a new, separated one with Federation 2 stuff

Makes sense to me. I'm heading off on vacation for a little over a week, but I can do that when I'm back. Thanks!

@jhanggi jhanggi changed the title Fix error when resolving interface in federation Add example for Apollo Federation v2 Jun 21, 2023
@MichalLytek
Copy link
Owner

@jhanggi Can you rebase your changes? 🙏

@MichalLytek MichalLytek added Documentation 📖 Issues about docs and removed Blocked 🚫 Resolving this issue is blocked by other issue or 3rd party stuffs Bugfix 🐛 🔨 PR that fixes a known bug labels Aug 9, 2023
@jhanggi
Copy link
Contributor Author

jhanggi commented Aug 9, 2023

@MichalLytek Glad to see #1400 merged in! I've merged the latest from master into my branch. I can do a rebase, but the changes upstream have some conflicts with my interim commits (when I was working against the original example before making a separate one). It looks like you do squash merges so it should go in cleanly, but I can do a full rebase if that's better for you.

@MichalLytek MichalLytek force-pushed the master branch 4 times, most recently from a190360 to f50d1d0 Compare August 17, 2023 16:54
@MichalLytek
Copy link
Owner

I've done small tweaks but it's now ready to merge 💪

@MichalLytek MichalLytek force-pushed the handle_ref_resolver_promise branch from 7fdc242 to bb7d352 Compare October 4, 2023 12:47
@codecov
Copy link

codecov bot commented Oct 4, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (615680e) 95.52% compared to head (bb7d352) 95.52%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1448   +/-   ##
=======================================
  Coverage   95.52%   95.52%           
=======================================
  Files         113      113           
  Lines        1854     1854           
  Branches      367      367           
=======================================
  Hits         1771     1771           
  Misses         83       83           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MichalLytek MichalLytek merged commit 9841e0f into MichalLytek:master Oct 4, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community 👨‍👧 Something initiated by a community Documentation 📖 Issues about docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants