Repository: mcollina/fastify-gql
Pull request #227 (2020-07-27 13:43:49)

Remove UniqueDirectivesPerLocationRule from validation for federated schemas
This fixes https://github.com/mcollina/fastify-gql/issues/184

As discussed with @mcollina with this PR I'm going with the simplest fix just for the linked issue. 
We replicate Apollo's behaviour by moving validations in our code and skipping the `UniqueDirectivesPerLocationRule` rule.

mcollina: @flux627 can you check?
mcollina: cc @asciidisco 
mcollina: cc @pscarey 
pscarey: @mcollina I'm not an expert in schema federation (to clarify we're not using the federation aspect of fastify-gql but between JIT and Fastify have hit a 7x performance improvement over Apollo server on some critical workloads) but this fix seems true to Apollo's solution/workaround. 

It may be worth filing a further issue for more test cases and tweaks to validate the behaviour around the other rules which Apollo skips, found [here](https://github.com/apollographql/apollo-server/blob/00887a1e57a26075483ba397098f6d088a706499/packages/apollo-federation/src/composition/rules.ts).

@paolochiodi Nice work getting to the root of the issue.


Patch:
diff --git a/lib/federation.js b/lib/federation.js
index f7531f5..4d55b8d 100644
--- a/lib/federation.js
+++ b/lib/federation.js
@@ -34,6 +34,8 @@ const {
   isTypeExtensionNode,
   isObjectType
 } = require('graphql')
+const { validateSDL } = require('graphql/validation/validate')
+const compositionRules = require('./federation/compositionRules')
 
 const BASE_FEDERATION_TYPES = `
   scalar _Any
@@ -250,10 +252,25 @@ function buildFederationSchema (schema, isGateway) {
   })
 
   // Add all extensions
-  federationSchema = extendSchema(federationSchema, {
+  const extensionsDocument = {
     kind: Kind.DOCUMENT,
     definitions: extensions
-  })
+  }
+
+  // instead of relying on extendSchema internal validations
+  // we run validations in our code so that we can use slightly different rules
+  // as extendSchema internal rules are meant for regular usage
+  // and federated schemas have different constraints
+  const errors = validateSDL(extensionsDocument, federationSchema, compositionRules)
+  if (errors.length === 1) {
+    throw errors[0]
+  } else if (errors.length > 1) {
+    const err = new Error('Schema issues, check out the .errors property on the Error.')
+    err.errors = errors
+    throw err
+  }
+
+  federationSchema = extendSchema(federationSchema, extensionsDocument, { assumeValidSDL: true })
 
   if (!federationSchema.getType('Query')) {
     federationSchema = new GraphQLSchema({
diff --git a/lib/federation/compositionRules.js b/lib/federation/compositionRules.js
new file mode 100644
index 0000000..18bd447
--- /dev/null
+++ b/lib/federation/compositionRules.js
@@ -0,0 +1,10 @@
+'use strict'
+
+const { specifiedSDLRules } = require('graphql/validation/specifiedRules')
+const { UniqueDirectivesPerLocationRule } = require('graphql')
+
+// this rules overwrite the built-in rules for federation schema
+// here we only remove the UniqueDirectivesPerLocationRule from the standard rules
+// to fix https://github.com/mcollina/fastify-gql/issues/184
+// this is also the point were we may need to add federation-specific rules
+module.exports = specifiedSDLRules.filter(rule => rule !== UniqueDirectivesPerLocationRule)


Test patch:
diff --git a/test/federation.js b/test/federation.js
index a4506c6..a736bd3 100644
--- a/test/federation.js
+++ b/test/federation.js
@@ -445,6 +445,103 @@ test('buildFederationSchema function adds stub types', async (t) => {
   t.matchSnapshot(printSchema(federationSchema))
 })
 
+test('buildFederationSchema works correctly with multiple type extensions', async (t) => {
+  const schema = `
+    extend type Query {
+      topPosts: [Post]
+    }
+
+    type Post @key(fields: "id") {
+      id: ID!
+      title: String
+      content: String
+      author: User
+    }
+
+    extend type User @key(fields: "id") {
+      id: ID! @external
+      posts: [Post]
+    }
+
+    extend type User @key(fields: "id") {
+      other: String
+    }
+  `
+  try {
+    buildFederationSchema(schema)
+    t.pass('schema built without errors')
+  } catch (err) {
+    t.fail('it should not throw errors', err)
+  }
+})
+
+test('buildFederationSchema still validate schema for errors (1 error)', async (t) => {
+  const schema = `
+    extend type Query {
+      topPosts: [Post]
+    }
+
+    type Post @key(fields: "id") {
+      id: ID!
+      title: String
+      content: String
+      author: User
+    }
+
+    extend type User @key(fields: "id") {
+      id: ID! @external
+      posts: [Post]
+    }
+
+    extend type User @key(fields: "id") {
+      id: ID! @external
+    }
+  `
+  try {
+    buildFederationSchema(schema)
+    t.fail('it should throw validation error')
+  } catch (err) {
+    // expected error: Field "User.id" can only be defined once.
+    t.pass('it should throw error')
+  }
+})
+
+test('buildFederationSchema still validate schema for errors (multiple error)', async (t) => {
+  const schema = `
+    extend type Query {
+      topPosts: [Post]
+    }
+
+    type Post @key(fields: "id") {
+      id: ID!
+      title: String
+      content: String
+      author: User
+    }
+
+    extend type User @key(fields: "id") {
+      id: ID! @external
+      posts: [Post]
+    }
+
+    extend type User @key(fields: "id") {
+      id: ID! @external
+      activity: [Actions]
+    }
+  `
+  try {
+    buildFederationSchema(schema)
+    t.fail('it should throw validation error')
+  } catch (err) {
+    // expected errors:
+    // Field "User.id" can only be defined once.
+    // Unknown type "Actions"
+    t.ok(err.errors)
+    t.equal(err.errors.length, 2)
+    t.pass('it should throw error')
+  }
+})
+
 test('mutation works with federation support', async (t) => {
   const app = Fastify()
   const schema = `