Improve error handling on the supergraph

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) {