Using fastify-gql in place of Apollo Gateway does not work

[flux627]

Hi,

As a proof of concept, I tried taking this demo repository and replace Apollo Gateway with fastify-gql. The federated services start fine (as expected). Then, when I try to start the Gateway server, I get this error:

Error: The directive "@key" can only be used once at this location.
    at assertValidSDLExtension (/Users/julienheller/projects/federation-demo/node_modules/graphql/validation/validate.js:124:11)
    at extendSchema (/Users/julienheller/projects/federation-demo/node_modules/graphql/utilities/extendSchema.js:78:45)
    at buildFederationSchema (/Users/julienheller/projects/federation-demo/node_modules/fastify-gql/lib/federation.js:253:22)
    at buildGateway (/Users/julienheller/projects/federation-demo/node_modules/fastify-gql/lib/gateway.js:151:18)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at async fp.name (/Users/julienheller/projects/federation-demo/node_modules/fastify-gql/index.js:119:15)

My guess is that both the original definition for Product and its extended clause have a @key directive, which are trying to be merged together. I wonder how Apollo handles this situation?

Here's my fork to try it for yourself.

npm i
npm run start-services
npm run start-fastify-gateway

[mcollina]

Wow, good finding and thanks for checking this out.

I’m a bit unsure how that would work... would you like to send a Pull Request to address this issue? Remember to add unit tests.

[flux627]

I would help, but I don't know the proper expected implementation of the Gateway (in this case the expected way to handle conflicting directives when creating the combined schema). I tried looking for a specification, but could only find an implementation specification for the Federated services, and an API specification for the Gateway, but not a specification for implementing a Gateway. Under what basis was the Gateway implementation designed? By just reverse engineering the Apollo Gateway implementation?

I could do my best to try different things regarding directives using the Apollo implementation and copy the observed functionality over to fastify-gql, if there's no better expected methodology.

[flux627]

I just found this:

At composition time, ApolloGateway strips all definitions and uses of type system directives from your composed schema. This has no effect on your implementing service schemas, which retain this information.

So, I guess we can strip directives from the service schemas before merging them? I'll see if doing that seems to bring parity in behavior.

[mcollina]

Amazing, I think that would work!

[flux627]

Did some work on this today. The problem seems to be that, as stated above, the Gateway should not expose TypeSystemDirectives (however ExecutableDirectives will be exposed, and they need to match between services). We can exclude TypeSystemDirectives from within getStubTypes(), and then the server starts up. But since makeResolver() references the built schema for this directive information, queries get messed up.

From where I'm seeing it, we need to refactor makeResolver() to not reference the schema for directive information, although I don't fully understand all aspects of this system and this might not be the best solution. I see that @frikille did most of the work on this, maybe he could give some input?

[flux627]

The problem seems to be partially resolved by passing true to assumeValid to the last extendSchema() call in buildFederationSchema() when building for the Gateway:

  // Add all extensions
  federationSchema = extendSchema(
    federationSchema,
    {
      kind: Kind.DOCUMENT,
      definitions: extensions
    },
    { assumeValid: isGateway },
  )

This change allows the service to run and successfully execute queries. However, I'm not sure if this is opening up the system to problems due to skipping validation. Furthermore, the TypeSystemDirectives are still being passed to the final schema incorrectly, which seems to be a fundamental problem with the implementation, since makeResolver() depends on these directives.

[mcollina]

Would you like to attempt a PR? I would not be able to help much in the coming weeks :(. I think your hot fix is feasible and should unstuck you.

[flux627]

I'm not stuck per-se. I'm shopping around for a solution that can offer Federation as well as subscriptions, and am willing to put work into bridging gaps. I don't feel comfortable submitting a PR that might introduce bugs and/or repercussive implications due to not fully understanding the system.

I really do think that the current implementation needs to change from depending on schema directives at query time to depending on configuration that is derived from the directives before they are removed. This will also lay a foundation for providing validation of compatibility across federated services, which Apollo Server currently provides.

[flux627]

I am still curious as to what the answers are to questions I asked originally:

Under what basis was the Gateway implementation designed? By just reverse engineering the Apollo Gateway implementation?

[mcollina]

@frikille might be able to answer that.

[paolochiodi]

I've done some investigation on this whilst I was looking at #220

I think I have found the underlying issue, these are my findings:

1. The issue is not with service B extending a type from service A nor the fact the the extend also includes the same @key directive as the type it is extended
2. It is actually expected that service B extending a type from service A also includes the sane @key directive, according to the apollo website: https://www.apollographql.com/docs/apollo-server/federation/entities/#referencing

The @key directive indicates that Product uses the upc field as its primary key. This value must match the value of exactly one @key defined in the entity's originating service, even if the entity defines multiple primary keys.

3. The bug exposed here occurs when two services both extend the same type from another service. In the example from the original post it is the Product type that is extended by both the reviews and the inventory service
4. The errors occurs in https://github.com/mcollina/fastify-gql/blob/master/lib/federation.js#L253. Apparently graphql's extendSchema raises an error when it is passed two definitions that extend the same type at the same type.
5. If we make multiple calls to extendSchema for the extensions and keep the extensions to the same type in separate extendSchema it will work
6. I think we may raise this to graphql-js
7. In the meantime a quick albeit inefficient fix is to add extensions to the schema one by one

[paolochiodi]

I've created a quick PR to demonstrate the issue and a potential fix: #221

[flux627]

I don't have time to look into this, but just wanted to comment on

It is actually expected that service B extending a type from service A also includes the sane @key directive, according to the apollo website: https://www.apollographql.com/docs/apollo-server/federation/entities/#referencing

While this is true that this is expected, that doesn't mean that it's valid to attach the same directives to extended types. The way Federation defines how separate services need to attach directives to their own schemas does not dictate what is valid within the same schema.

The errors occurs in https://github.com/mcollina/fastify-gql/blob/master/lib/federation.js#L253. Apparently graphql's extendSchema raises an error when it is passed two definitions that extend the same type at the same type.

Based on what I remember when testing, this is not the case. If you have a type with a directive, and extend the type with the same directive, then it will give the error. It makes sense that you'd be getting the error with multiple extensions of the same type with the same directive, for this reason. It's possible I'm remembering wrong, however.

In any case, in order to abide by Apollo's documentation we still need to strip the directives from the final schema. Since this is necessary, then removing them before extending kills two birds with one stone. It just means we need to capture the configuration as defined by these directives beforehand.

[paolochiodi]

I don't have time to look into this, but just wanted to comment on

It is actually expected that service B extending a type from service A also includes the sane @key directive, according to the apollo website: https://www.apollographql.com/docs/apollo-server/federation/entities/#referencing

While this is true that this is expected, that doesn't mean that it's valid to attach the same directives to extended types. The way Federation defines how separate services need to attach directives to their own schemas does not dictate what is valid within the same schema.

Let me rephrase that. The Apollo docs clearly state that when extending entities, the extend must include the same @key directive as the original type definition. This is the case for stubs and actual extensions.

The errors occurs in https://github.com/mcollina/fastify-gql/blob/master/lib/federation.js#L253. Apparently graphql's extendSchema raises an error when it is passed two definitions that extend the same type at the same type.

Based on what I remember when testing, this is not the case. If you have a type with a directive, and extend the type with the same directive, then it will give the error. It makes sense that you'd be getting the error with multiple extensions of the same type with the same directive, for this reason. It's possible I'm remembering wrong, however.

As far as I can see, this is not the case - or at least it's not enforced at the graphql-js level.
Fastify-gql builds the schema in buildFederationSchema (https://github.com/mcollina/fastify-gql/blob/master/lib/federation.js#L230). In there the concatenation of the schemas from the federated services is parsed and then split in type stubs, extensions and definitions. It then proceed to make a call to extendSchema with each of those three groups.

The error we get in the demo repository (and also in fastify-gql own example in https://github.com/mcollina/fastify-gql/blob/master/examples/gateway-subscription.js) is thrown by the call to extendSchema that tries to add all extensions at the same time.
This happens regardless of how many type extensions with the same @key directive have been added beforehand: the problem is just with calling extendSchema with the extend for the same type with the same directive.

To prove that I made this small change https://github.com/mcollina/fastify-gql/pull/221/files#diff-d9185daf57698ed90d572d1966e61b16R253-R258 where extensions are added one at the time and that fixed the issue: a sign that extendSchema is accepting multiple extend with the same key directive if each one happens in their own call to extendSchema

In any case, in order to abide by Apollo's documentation we still need to strip the directives from the final schema. Since this is necessary, then removing them before extending kills two birds with one stone. It just means we need to capture the configuration as defined by these directives beforehand.

I agree with this. Regardless of what is actually causing the exception we are seeing, the correct implementation would be to replicate the behaviour of Apollo, which is to strip the directives in the federated schema.

I took a look at Apollo's code and that appears to be happening in https://github.com/apollographql/apollo-server/blob/5bab3aedeeb1e70c636f3aa3ac7a147679f450f7/packages/apollo-federation/src/composition/normalize.ts#L20 (more specifically in https://github.com/apollographql/apollo-server/blob/5bab3aedeeb1e70c636f3aa3ac7a147679f450f7/packages/apollo-federation/src/composition/normalize.ts#L270).

So, to sum up my point:

1. I find the behaviour of parse and extendSchema from graphql-js weird: a schema parsed without errors by parse throws and error in extendSchema, and that also depends on the fact that you make only one call or multiple calls to it. May be worth further investigating into this and report to graphql-js
2. Regardless of that, fastify-gql should replicate Apollo's behaviour and strip the directives

[paolochiodi]

I've spent more time debugging this, and I have come to the conclusion that my understanding of the issue as expressed in the previous message was incomplete.

Here's my new understanding:

1. Apollo-server is indeed stripping directives, but only the directive definitions from the implementing service schemas, not the directive usages in extensions. I.e.

    extend type User @key(fields: "id") {
      id: ID! @external
      comments: [Comment]
    }

gets added to the schema as is, still retaining the @key part
2. Apollo-server overcomes the error we are getting by calling extendSchema with the option assumeValidSDL as @flux627 suggested at one point. See it here: https://github.com/apollographql/apollo-server/blob/00887a1e57a26075483ba397098f6d088a706499/packages/apollo-federation/src/composition/compose.ts#L373
3. Apollo-server however add its own validations before extending the schema: https://github.com/apollographql/apollo-server/blob/00887a1e57a26075483ba397098f6d088a706499/packages/apollo-federation/src/composition/compose.ts#L371. It uses a private function from graphql-js: validateSDL. It's the same function function used internally by validateSchema, but apollo-server replaces the rules it uses

I'm working on a PR to replicate the behaviour by calling extendSchema with assumeValidSDL set to true but also adding the validations.