Improve error handling on the supergraph

Some errors might be thrown while extracting subgraphs from the
supergraph but without being handled gracefully, resulting in messages
to the user that were less useful than they should be.

Author: Sylvain Lebresne

internals-js/src/__tests__/extractSubgraphsFromSupergraph.test.ts

@@ -533,3 +533,67 @@ test('handles unions types having no members in a subgraph correctly', () => {
   expect(b.type('C')).toBeUndefined();
   expect(a.type('D')).toBeDefined();
 })
+
+test('throw meaningful error for invalid federation directive fieldSet', () => {
+  const supergraph = `
+    schema
+      @core(feature: "https://specs.apollo.dev/core/v0.2"),
+      @core(feature: "https://specs.apollo.dev/join/v0.1", for: EXECUTION)
+    {
+      query: Query
+    }
+
+    directive @core(as: String, feature: String!, for: core__Purpose) repeatable on SCHEMA
+
+    directive @join__field(graph: join__Graph, provides: join__FieldSet, requires: join__FieldSet) on FIELD_DEFINITION
+
+    directive @join__graph(name: String!, url: String!) on ENUM_VALUE
+
+    directive @join__owner(graph: join__Graph!) on INTERFACE | OBJECT
+
+    directive @join__type(graph: join__Graph!, key: join__FieldSet) repeatable on INTERFACE | OBJECT
+
+    type A @join__owner(graph: SERVICEA) @join__type(graph: SERVICEA, key: "id") @join__type(graph: SERVICEB, key: "id") {
+      id: ID
+      a: Int @join__field(graph: SERVICEB, requires: "b { x }")
+      b: B
+    }
+
+    type B @join__owner(graph: SERVICEA) @join__type(graph: SERVICEA, key: "id") {
+      id: ID
+      x: Int
+    }
+
+    type Query {
+      q: A
+    }
+
+    enum core__Purpose {
+      """
+      \`EXECUTION\` features provide metadata necessary to for operation execution.
+      """
+      EXECUTION
+
+      """
+      \`SECURITY\` features provide metadata necessary to securely resolve fields.
+      """
+      SECURITY
+    }
+
+    scalar join__FieldSet
+
+    enum join__Graph {
+      SERVICEA @join__graph(name: "serviceA" url: "")
+      SERVICEB @join__graph(name: "serviceB" url: "")
+    }
+  `;
+
+  const schema = buildSupergraphSchema(supergraph)[0];
+  expect(() => extractSubgraphsFromSupergraph(schema)).toThrow(
+    'Error extracting subgraph "serviceB" from the supergraph: this might due to errors in subgraphs that were mistakenly ignored by federation 0.x versions but are rejected by federation 2.\n'
+    + 'Please try composing your subgraphs with federation 2: this should help precisely pinpoint the problems and, once fixed, generate a correct federation 2 supergraph.\n'
+    + '\n'
+    + 'Details:\n'
+    + '[serviceB] On field "A.a", for @requires(fields: "b { x }"): Cannot query field "b" on type "A" (if the field is defined in another subgraph, you need to add it to this subgraph with @external).'
+  );
+})

internals-js/src/extractSubgraphsFromSupergraph.ts

@@ -292,11 +292,11 @@ export function extractSubgraphsFromSupergraph(supergraph: Schema): Subgraphs {
       if (isFed1) {
         // Note that this could be a bug with the code handling fed1 as well, but it's more helpful to ask users to recompose their subgraphs with fed2 as either
         // it'll solve the issue and that's good, or we'll hit the other message anyway.
-        const msg = `Error extracting subgraph ${subgraph.name} from the supergraph: this might due to errors in subgraphs that were mistakenly ignored by federation 0.x versions but are rejected by federation 2.\n`
-          + 'Please try composing your subgraphs with federation 2: this should help precisely pinpoint the errors and generate a correct federation 2 supergraph.';
+        const msg = `Error extracting subgraph "${subgraph.name}" from the supergraph: this might due to errors in subgraphs that were mistakenly ignored by federation 0.x versions but are rejected by federation 2.\n`
+          + 'Please try composing your subgraphs with federation 2: this should help precisely pinpoint the problems and, once fixed, generate a correct federation 2 supergraph';
         throw new Error(`${msg}.\n\nDetails:\n${errorToString(e)}`);
       } else {
-        const msg = `Unexpected error extracting subgraph ${subgraph.name} from the supergraph: this is either a bug, or the supergraph has been corrupted.`;
+        const msg = `Unexpected error extracting subgraph ${subgraph.name} from the supergraph: this is either a bug, or the supergraph has been corrupted`;
         const dumpMsg = maybeDumpSubgraphSchema(subgraph);
         throw new Error(`${msg}.\n\nDetails:\n${errorToString(e)}\n\n${dumpMsg}`);
       }
@@ -479,7 +479,19 @@ function addExternalFieldsFromDirectiveFieldSet(
     }
     return created;
   };
-  parseFieldSetArgument({parentType, directive, fieldAccessor});
+  try {
+    parseFieldSetArgument({parentType, directive, fieldAccessor, validate: false});
+  } catch (e) {
+    // Ignored on purpose: for fed1 supergraphs, it's possible that some of the fields defined in a federation directive
+    // was _not_ defined in the subgraph because fed1 was not validating this properly (the validation wasn't handling
+    // nested fields as it should), which may result in an error when trying to add those as an external field.
+    // However, this is not the right place to throw. Instead, we ignore the problem and thus exit without having added
+    // all the necessary fields, and so this very same directive will fail validation at the end of the extraction when
+    // we do the final validation of the extracted subgraph (see end of `extractSubgraphsFromSupergraph`). And we prefer
+    // failing then because 1) that later validation will collect all errors instead of failing on the first one and
+    // 2) we already have special error messages and the ability to dump the extracted subgraphs for debug at that point,
+    // so it's a much better place.
+  }
 }
 
 function addExternalFieldsFromInterface(metadata: FederationMetadata, type: ObjectType | InterfaceType) {