From e715ab688c04dedfa9f1adbfc15eed2ebc3bc2ff Mon Sep 17 00:00:00 2001
From: Trevor Scheer <trevor.scheer@gmail.com>
Date: Thu, 8 Apr 2021 15:36:36 -0700
Subject: [PATCH 01/20] Refine / split metadata types (instead of ? -> !)

---
 query-planner-js/src/buildQueryPlan.ts        | 15 +++++++++---
 .../src/composedSchema/buildComposedSchema.ts | 14 +++++++++--
 .../src/composedSchema/metadata.ts            | 23 +++++++++++++++----
 3 files changed, 43 insertions(+), 9 deletions(-)

diff --git a/query-planner-js/src/buildQueryPlan.ts b/query-planner-js/src/buildQueryPlan.ts
index 805b3ec45..cd5117b92 100644
--- a/query-planner-js/src/buildQueryPlan.ts
+++ b/query-planner-js/src/buildQueryPlan.ts
@@ -52,7 +52,11 @@ import {
 } from './QueryPlan';
 import { getFieldDef, getResponseName } from './utilities/graphql';
 import { MultiMap } from './utilities/MultiMap';
-import { getFederationMetadataForType, getFederationMetadataForField } from './composedSchema';
+import {
+  getFederationMetadataForType,
+  getFederationMetadataForField,
+  isValueTypeMetadata,
+} from './composedSchema';
 import { DebugLogger } from './utilities/debug';
 
 
@@ -1136,7 +1140,8 @@ export class QueryPlanningContext {
   }
 
   getBaseService(parentType: GraphQLObjectType): string | undefined {
-    return getFederationMetadataForType(parentType)?.graphName;
+    const type = getFederationMetadataForType(parentType);
+    return (type && !isValueTypeMetadata(type)) ? type.graphName : undefined;
   }
 
   getOwningService(
@@ -1170,7 +1175,11 @@ export class QueryPlanningContext {
     });
 
     for (const possibleType of this.getPossibleTypes(parentType)) {
-      const keys = getFederationMetadataForType(possibleType)?.keys?.get(serviceName);
+      const type = getFederationMetadataForType(possibleType);
+      const keys =
+        type && !isValueTypeMetadata(type)
+          ? type.keys.get(serviceName)
+          : undefined;
 
       if (!(keys && keys.length > 0)) continue;
 
diff --git a/query-planner-js/src/composedSchema/buildComposedSchema.ts b/query-planner-js/src/composedSchema/buildComposedSchema.ts
index 9630c9b19..2b5e3b9af 100644
--- a/query-planner-js/src/composedSchema/buildComposedSchema.ts
+++ b/query-planner-js/src/composedSchema/buildComposedSchema.ts
@@ -21,8 +21,10 @@ import { MultiMap } from '../utilities/MultiMap';
 import {
   FederationFieldMetadata,
   FederationTypeMetadata,
+  FederationEntityTypeMetadata,
   FieldSet,
   GraphMap,
+  isValueTypeMetadata,
 } from './metadata';
 
 export function buildComposedSchema(document: DocumentNode): GraphQLSchema {
@@ -148,8 +150,11 @@ export function buildComposedSchema(document: DocumentNode): GraphQLSchema {
       type.astNode,
     );
 
+    // The assertion here guarantees the safety of the type cast below
+    // (typeMetadata as FederationEntityTypeMetadata). Adjustments to this assertion
+    // should account for this dependency.
     assert(
-      !(typeMetadata.isValueType && typeDirectivesArgs.length >= 1),
+      !(isValueTypeMetadata(typeMetadata) && typeDirectivesArgs.length >= 1),
       `GraphQL type "${type.name}" cannot have a @${typeDirective.name} \
 directive without an @${ownerDirective.name} directive`,
     );
@@ -159,7 +164,12 @@ directive without an @${ownerDirective.name} directive`,
 
       const keyFields = parseFieldSet(typeDirectiveArgs['key']);
 
-      typeMetadata.keys?.add(graphName, keyFields);
+      // We know we won't actually be looping here in the case of a value type
+      // based on the assertion above, but TS is not able to infer that.
+      (typeMetadata as FederationEntityTypeMetadata).keys.add(
+        graphName,
+        keyFields,
+      );
     }
 
     for (const fieldDef of Object.values(type.getFields())) {
diff --git a/query-planner-js/src/composedSchema/metadata.ts b/query-planner-js/src/composedSchema/metadata.ts
index 45f5174bf..a8c96edac 100644
--- a/query-planner-js/src/composedSchema/metadata.ts
+++ b/query-planner-js/src/composedSchema/metadata.ts
@@ -43,10 +43,25 @@ export type GraphMap = { [graphName: string]: Graph };
 export interface FederationSchemaMetadata {
   graphs: GraphMap;
 }
-export interface FederationTypeMetadata {
-  graphName?: GraphName;
-  keys?: MultiMap<GraphName, FieldSet>;
-  isValueType: boolean;
+
+export type FederationTypeMetadata =
+  | FederationEntityTypeMetadata
+  | FederationValueTypeMetadata;
+
+export interface FederationEntityTypeMetadata {
+  graphName: GraphName;
+  keys: MultiMap<GraphName, FieldSet>,
+  isValueType: false;
+}
+
+interface FederationValueTypeMetadata {
+  isValueType: true;
+}
+
+export function isValueTypeMetadata(
+  metadata: FederationTypeMetadata,
+): metadata is FederationValueTypeMetadata {
+  return metadata.isValueType;
 }
 
 export interface FederationFieldMetadata {

From bdad2094eb16fe850a62bd1e65ba02ca9a050855 Mon Sep 17 00:00:00 2001
From: Trevor Scheer <trevor.scheer@gmail.com>
Date: Thu, 8 Apr 2021 15:38:03 -0700
Subject: [PATCH 02/20] getJoins -> getJoinDefinitions

---
 federation-js/src/__tests__/joinSpec.test.ts    | 4 ++--
 federation-js/src/joinSpec.ts                   | 2 +-
 federation-js/src/service/printSupergraphSdl.ts | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/federation-js/src/__tests__/joinSpec.test.ts b/federation-js/src/__tests__/joinSpec.test.ts
index 381fffa4b..341224c78 100644
--- a/federation-js/src/__tests__/joinSpec.test.ts
+++ b/federation-js/src/__tests__/joinSpec.test.ts
@@ -1,5 +1,5 @@
 import { fixtures } from 'apollo-federation-integration-testsuite';
-import { getJoins } from "../joinSpec";
+import { getJoinDefinitions } from "../joinSpec";
 
 const questionableNamesRemap = {
   accounts: 'ServiceA',
@@ -17,7 +17,7 @@ const fixturesWithQuestionableServiceNames = fixtures.map((service) => ({
 
 describe('join__Graph enum', () => {
   it('correctly uniquifies and sanitizes service names', () => {
-    const { sanitizedServiceNames } = getJoins(
+    const { sanitizedServiceNames } = getJoinDefinitions(
       fixturesWithQuestionableServiceNames,
     );
 
diff --git a/federation-js/src/joinSpec.ts b/federation-js/src/joinSpec.ts
index 6d5422e4f..40f03b82e 100644
--- a/federation-js/src/joinSpec.ts
+++ b/federation-js/src/joinSpec.ts
@@ -115,7 +115,7 @@ function getJoinOwnerDirective(JoinGraphEnum: GraphQLEnumType) {
   });
 }
 
-export function getJoins(serviceList: ServiceDefinition[]) {
+export function getJoinDefinitions(serviceList: ServiceDefinition[]) {
   const { sanitizedServiceNames, JoinGraphEnum } = getJoinGraphEnum(serviceList);
   const JoinFieldDirective = getJoinFieldDirective(JoinGraphEnum);
   const JoinOwnerDirective = getJoinOwnerDirective(JoinGraphEnum);
diff --git a/federation-js/src/service/printSupergraphSdl.ts b/federation-js/src/service/printSupergraphSdl.ts
index 9957907b3..1abe07534 100644
--- a/federation-js/src/service/printSupergraphSdl.ts
+++ b/federation-js/src/service/printSupergraphSdl.ts
@@ -30,7 +30,7 @@ import {
 } from 'graphql';
 import { Maybe, FederationType, FederationField, ServiceDefinition } from '../composition';
 import { CoreDirective } from '../coreSpec';
-import { getJoins } from '../joinSpec';
+import { getJoinDefinitions } from '../joinSpec';
 
 type Options = {
   /**
@@ -73,7 +73,7 @@ export function printSupergraphSdl(
     JoinGraphEnum,
     JoinGraphDirective,
     sanitizedServiceNames,
-  } = getJoins(serviceList);
+  } = getJoinDefinitions(serviceList);
 
   schema = new GraphQLSchema({
     ...config,

From b3bb32e6931c52464c022be8bbcf372b9a4e3697 Mon Sep 17 00:00:00 2001
From: Trevor Scheer <trevor.scheer@gmail.com>
Date: Thu, 8 Apr 2021 15:39:33 -0700
Subject: [PATCH 03/20] sanitizedServiceNames -> graphNameToEnumValueName

---
 federation-js/src/__tests__/joinSpec.test.ts    |  4 ++--
 federation-js/src/joinSpec.ts                   | 12 ++++++------
 federation-js/src/service/printSupergraphSdl.ts | 14 +++++++-------
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/federation-js/src/__tests__/joinSpec.test.ts b/federation-js/src/__tests__/joinSpec.test.ts
index 341224c78..3eff4f565 100644
--- a/federation-js/src/__tests__/joinSpec.test.ts
+++ b/federation-js/src/__tests__/joinSpec.test.ts
@@ -17,7 +17,7 @@ const fixturesWithQuestionableServiceNames = fixtures.map((service) => ({
 
 describe('join__Graph enum', () => {
   it('correctly uniquifies and sanitizes service names', () => {
-    const { sanitizedServiceNames } = getJoinDefinitions(
+    const { graphNameToEnumValueName } = getJoinDefinitions(
       fixturesWithQuestionableServiceNames,
     );
 
@@ -33,7 +33,7 @@ describe('join__Graph enum', () => {
      * (serviceA) tests the edge case of colliding with a name we generated
      * (servicea_2_) tests a collision against (documents) post-transformation
      */
-    expect(sanitizedServiceNames).toMatchObject({
+    expect(graphNameToEnumValueName).toMatchObject({
       '9product*!': '_9PRODUCT__',
       ServiceA: 'SERVICEA',
       reviews_9: 'REVIEWS_9_',
diff --git a/federation-js/src/joinSpec.ts b/federation-js/src/joinSpec.ts
index 40f03b82e..c5b3e5b77 100644
--- a/federation-js/src/joinSpec.ts
+++ b/federation-js/src/joinSpec.ts
@@ -39,7 +39,7 @@ function getJoinGraphEnum(serviceList: ServiceDefinition[]) {
   // Track whether we've seen a name and how many times
   const nameMap: Map<string, number> = new Map();
   // Build a map of original service name to generated name
-  const sanitizedServiceNames: Record<string, string> = Object.create(null);
+  const graphNameToEnumValueName: Record<string, string> = Object.create(null);
 
   function uniquifyAndSanitizeGraphQLName(name: string) {
     // Transforms to ensure valid graphql `Name`
@@ -62,17 +62,17 @@ function getJoinGraphEnum(serviceList: ServiceDefinition[]) {
       const uniquified = `${toUpper}_${nameCount + 1}`;
       // We also now need another entry for the name we just generated
       nameMap.set(uniquified, 1);
-      sanitizedServiceNames[name] = uniquified;
+      graphNameToEnumValueName[name] = uniquified;
       return uniquified;
     } else {
       nameMap.set(toUpper, 1);
-      sanitizedServiceNames[name] = toUpper;
+      graphNameToEnumValueName[name] = toUpper;
       return toUpper;
     }
   }
 
   return {
-    sanitizedServiceNames,
+    graphNameToEnumValueName,
     JoinGraphEnum: new GraphQLEnumType({
       name: 'join__Graph',
       values: Object.fromEntries(
@@ -116,7 +116,7 @@ function getJoinOwnerDirective(JoinGraphEnum: GraphQLEnumType) {
 }
 
 export function getJoinDefinitions(serviceList: ServiceDefinition[]) {
-  const { sanitizedServiceNames, JoinGraphEnum } = getJoinGraphEnum(serviceList);
+  const { graphNameToEnumValueName, JoinGraphEnum } = getJoinGraphEnum(serviceList);
   const JoinFieldDirective = getJoinFieldDirective(JoinGraphEnum);
   const JoinOwnerDirective = getJoinOwnerDirective(JoinGraphEnum);
 
@@ -135,7 +135,7 @@ export function getJoinDefinitions(serviceList: ServiceDefinition[]) {
   });
 
   return {
-    sanitizedServiceNames,
+    graphNameToEnumValueName,
     FieldSetScalar,
     JoinTypeDirective,
     JoinFieldDirective,
diff --git a/federation-js/src/service/printSupergraphSdl.ts b/federation-js/src/service/printSupergraphSdl.ts
index 1abe07534..0d2e95607 100644
--- a/federation-js/src/service/printSupergraphSdl.ts
+++ b/federation-js/src/service/printSupergraphSdl.ts
@@ -47,7 +47,7 @@ type Options = {
 interface PrintingContext {
   // Core addition: we need access to a map from serviceName to its corresponding
   // sanitized / uniquified enum value `Name` from the `join__Graph` enum
-  sanitizedServiceNames?: Record<string, string>;
+  graphNameToEnumValueName?: Record<string, string>;
 }
 
 /**
@@ -72,7 +72,7 @@ export function printSupergraphSdl(
     JoinOwnerDirective,
     JoinGraphEnum,
     JoinGraphDirective,
-    sanitizedServiceNames,
+    graphNameToEnumValueName,
   } = getJoinDefinitions(serviceList);
 
   schema = new GraphQLSchema({
@@ -89,7 +89,7 @@ export function printSupergraphSdl(
   });
 
   const context: PrintingContext = {
-    sanitizedServiceNames,
+    graphNameToEnumValueName,
   }
 
   return printFilteredSchema(
@@ -248,7 +248,7 @@ function printTypeJoinDirectives(
   const shouldPrintOwner = isObjectType(type);
   const joinOwnerString = shouldPrintOwner
     ? `\n  @join__owner(graph: ${
-        context.sanitizedServiceNames?.[ownerService] ?? ownerService
+        context.graphNameToEnumValueName?.[ownerService] ?? ownerService
       })`
     : '';
 
@@ -260,7 +260,7 @@ function printTypeJoinDirectives(
           .map(
             (selections) =>
               `\n  @join__type(graph: ${
-                context.sanitizedServiceNames?.[service] ?? service
+                context.graphNameToEnumValueName?.[service] ?? service
               }, key: ${printStringLiteral(printFieldSet(selections))})`,
           )
           .join(''),
@@ -398,7 +398,7 @@ function printJoinFieldDirectives(
       return (
         printed +
         `graph: ${
-          context.sanitizedServiceNames?.[
+          context.graphNameToEnumValueName?.[
             parentType.extensions?.federation.serviceName
           ] ?? parentType.extensions?.federation.serviceName
         })`
@@ -417,7 +417,7 @@ function printJoinFieldDirectives(
 
   if (serviceName && serviceName.length > 0) {
     directiveArgs.push(
-      `graph: ${context.sanitizedServiceNames?.[serviceName] ?? serviceName}`,
+      `graph: ${context.graphNameToEnumValueName?.[serviceName] ?? serviceName}`,
     );
   }
 

From afd924c3d1df4a6ca9537be54ac03d0554219093 Mon Sep 17 00:00:00 2001
From: Trevor Scheer <trevor.scheer@gmail.com>
Date: Mon, 12 Apr 2021 15:35:59 -0700
Subject: [PATCH 04/20] Address enum sanitization/uniquification comments

---
 federation-js/src/__tests__/joinSpec.test.ts |  6 +-
 federation-js/src/joinSpec.ts                | 88 +++++++++++++-------
 2 files changed, 59 insertions(+), 35 deletions(-)

diff --git a/federation-js/src/__tests__/joinSpec.test.ts b/federation-js/src/__tests__/joinSpec.test.ts
index 3eff4f565..7ab6d7e3b 100644
--- a/federation-js/src/__tests__/joinSpec.test.ts
+++ b/federation-js/src/__tests__/joinSpec.test.ts
@@ -35,10 +35,10 @@ describe('join__Graph enum', () => {
      */
     expect(graphNameToEnumValueName).toMatchObject({
       '9product*!': '_9PRODUCT__',
-      ServiceA: 'SERVICEA',
+      ServiceA: 'SERVICEA_2',
       reviews_9: 'REVIEWS_9_',
-      serviceA: 'SERVICEA_2',
-      servicea_2: 'SERVICEA_2_',
+      serviceA: 'SERVICEA_1',
+      servicea_2: 'SERVICEA_2__1',
       servicea_2_: 'SERVICEA_2__2',
     });
   })
diff --git a/federation-js/src/joinSpec.ts b/federation-js/src/joinSpec.ts
index c5b3e5b77..a36866838 100644
--- a/federation-js/src/joinSpec.ts
+++ b/federation-js/src/joinSpec.ts
@@ -27,59 +27,83 @@ const JoinGraphDirective = new GraphQLDirective({
 
 /**
  * Expectations
- * 1. Non-Alphanumeric characters are replaced with _ (alphaNumericUnderscoreOnly)
- * 2. Numeric first characters are prefixed with _ (noNumericFirstChar)
- * 3. Names ending in an underscore followed by numbers `_\d+` are suffixed with _ (noUnderscoreNumericEnding)
- * 4. Names are uppercased (toUpper)
- * 5. After transformations 1-4, duplicates are suffixed with _{n} where {n} is number of times we've seen the dupe
+ * 1. The input is first sorted using `String.localeCompare`, so the output is deterministic
+ * 2. Non-Alphanumeric characters are replaced with _ (alphaNumericUnderscoreOnly)
+ * 3. Numeric first characters are prefixed with _ (noNumericFirstChar)
+ * 4. Names ending in an underscore followed by numbers `_\d+` are suffixed with _ (noUnderscoreNumericEnding)
+ * 5. Names are uppercased (toUpper)
+ * 6. After transformations 1-4, duplicates are suffixed with _{n} where {n} is number of times we've seen the dupe
  *
  * Note: Collisions with name's we've generated are also accounted for
  */
 function getJoinGraphEnum(serviceList: ServiceDefinition[]) {
-  // Track whether we've seen a name and how many times
-  const nameMap: Map<string, number> = new Map();
-  // Build a map of original service name to generated name
-  const graphNameToEnumValueName: Record<string, string> = Object.create(null);
+  const sortedServiceList = serviceList
+    .slice()
+    .sort((a, b) => a.name.localeCompare(b.name));
 
-  function uniquifyAndSanitizeGraphQLName(name: string) {
-    // Transforms to ensure valid graphql `Name`
-    const alphaNumericUnderscoreOnly = name.replace(/[^_a-zA-Z0-9]/g, '_');
-    const noNumericFirstChar = alphaNumericUnderscoreOnly.match(/^[0-9]/)
+  function sanitizeGraphQLName(name: string) {
+    // replace all non-word characters (\W). Word chars are _a-zA-Z0-9
+    const alphaNumericUnderscoreOnly = name.replace(/[\W]/g, '_');
+    // prefix a digit in the first position with an _
+    const noNumericFirstChar = alphaNumericUnderscoreOnly.match(/^\d/)
       ? '_' + alphaNumericUnderscoreOnly
       : alphaNumericUnderscoreOnly;
-    const noUnderscoreNumericEnding = noNumericFirstChar.match(/_[0-9]+$/)
+    // suffix an underscore + digit in the last position with an _
+    const noUnderscoreNumericEnding = noNumericFirstChar.match(/_\d+$/)
       ? noNumericFirstChar + '_'
       : noNumericFirstChar;
 
     // toUpper not really necessary but follows convention of enum values
     const toUpper = noUnderscoreNumericEnding.toLocaleUpperCase();
+    return toUpper;
+  }
+
+  // duplicate enum values can occur due to sanitization and must be accounted for
+  // collect the duplicates in an array so we can uniquify them in a second pass.
+  const sanitizedNameToServiceDefinitions: Map<
+    string,
+    ServiceDefinition[]
+  > = new Map();
+  for (const service of sortedServiceList) {
+    const { name } = service;
+    const sanitized = sanitizeGraphQLName(name);
 
-    // Uniquifying post-transform
-    const nameCount = nameMap.get(toUpper);
-    if (nameCount) {
-      // Collision - bump counter by one
-      nameMap.set(toUpper, nameCount + 1);
-      const uniquified = `${toUpper}_${nameCount + 1}`;
-      // We also now need another entry for the name we just generated
-      nameMap.set(uniquified, 1);
-      graphNameToEnumValueName[name] = uniquified;
-      return uniquified;
+    const existingEntry = sanitizedNameToServiceDefinitions.get(sanitized);
+    if (existingEntry) {
+      sanitizedNameToServiceDefinitions.set(sanitized, [
+        ...existingEntry,
+        service,
+      ]);
     } else {
-      nameMap.set(toUpper, 1);
-      graphNameToEnumValueName[name] = toUpper;
-      return toUpper;
+      sanitizedNameToServiceDefinitions.set(sanitized, [service]);
     }
   }
 
+  // if no duplicates for a given name, add it as is
+  // if duplicates exist, append _{n} (index-1) to each duplicate in the array
+  const generatedNameToServiceDefinition: Record<
+    string,
+    ServiceDefinition
+  > = Object.create(null);
+  for (const [name, services] of sanitizedNameToServiceDefinitions) {
+    if (services.length === 1) {
+      generatedNameToServiceDefinition[name] = services[0];
+    } else {
+      for (const [index, service] of services.entries()) {
+        generatedNameToServiceDefinition[`${name}_${index + 1}`] = service;
+      }
+    }
+  }
+
+  const entries = Object.entries(generatedNameToServiceDefinition);
   return {
-    graphNameToEnumValueName,
+    graphNameToEnumValueName: Object.fromEntries(
+      entries.map(([name, service]) => [service.name, name]),
+    ),
     JoinGraphEnum: new GraphQLEnumType({
       name: 'join__Graph',
       values: Object.fromEntries(
-        serviceList.map((service) => [
-          uniquifyAndSanitizeGraphQLName(service.name),
-          { value: service },
-        ]),
+        entries.map(([name, service]) => [name, { value: service }]),
       ),
     }),
   };

From 5bd3f101a929130421fb96f90d99ef8494b9a3dd Mon Sep 17 00:00:00 2001
From: Trevor Scheer <trevor.scheer@gmail.com>
Date: Tue, 13 Apr 2021 16:06:13 -0700
Subject: [PATCH 05/20] Use actual map for GraphMap instead to account for
 undefined-ness

---
 gateway-js/src/index.ts                       |  7 +--
 .../src/composedSchema/buildComposedSchema.ts | 44 ++++++++++++-------
 .../src/composedSchema/metadata.ts            |  2 +-
 3 files changed, 31 insertions(+), 22 deletions(-)

diff --git a/gateway-js/src/index.ts b/gateway-js/src/index.ts
index f00f2af78..e450b164c 100644
--- a/gateway-js/src/index.ts
+++ b/gateway-js/src/index.ts
@@ -695,12 +695,7 @@ export class ApolloGateway implements GraphQLService {
       throw Error(`Couldn't find graph map in composed schema`);
     }
 
-    const serviceList = Object.values(graphMap).map(graph => ({
-      name: graph.name,
-      url: graph.url
-    }))
-
-    return serviceList;
+    return Array.from(graphMap.values());
   }
 
   private createSchemaFromSupergraphSdl(supergraphSdl: string) {
diff --git a/query-planner-js/src/composedSchema/buildComposedSchema.ts b/query-planner-js/src/composedSchema/buildComposedSchema.ts
index 2b5e3b9af..2dfb4031f 100644
--- a/query-planner-js/src/composedSchema/buildComposedSchema.ts
+++ b/query-planner-js/src/composedSchema/buildComposedSchema.ts
@@ -84,7 +84,7 @@ export function buildComposedSchema(document: DocumentNode): GraphQLSchema {
   const graphEnumType = schema.getType(`${joinName}__Graph`);
   assert(isEnumType(graphEnumType), `${joinName}__Graph should be an enum`);
 
-  const graphMap: GraphMap = Object.create(null);
+  const graphMap: GraphMap = new Map();
 
   schema.extensions = {
     ...schema.extensions,
@@ -108,10 +108,10 @@ export function buildComposedSchema(document: DocumentNode): GraphQLSchema {
     const graphName: string = graphDirectiveArgs['name'];
     const url: string = graphDirectiveArgs['url'];
 
-    graphMap[name] = {
+    graphMap.set(name, {
       name: graphName,
       url,
-    };
+    });
   }
 
   for (const type of Object.values(schema.getTypeMap())) {
@@ -130,15 +130,24 @@ export function buildComposedSchema(document: DocumentNode): GraphQLSchema {
       type.astNode,
     );
 
-    const typeMetadata: FederationTypeMetadata = ownerDirectiveArgs
-      ? {
-          graphName: graphMap[ownerDirectiveArgs?.['graph']].name,
-          keys: new MultiMap(),
-          isValueType: false,
-        }
-      : {
-          isValueType: true,
-        };
+    let typeMetadata: FederationTypeMetadata;
+    if (ownerDirectiveArgs) {
+      const graph = graphMap.get(ownerDirectiveArgs.graph);
+      assert(
+        graph,
+        `@${ownerDirective.name} directive requires a \`graph\` argument`,
+      );
+
+      typeMetadata = {
+        graphName: graph.name,
+        keys: new MultiMap(),
+        isValueType: false,
+      };
+    } else {
+      typeMetadata = {
+        isValueType: true,
+      };
+    }
 
     type.extensions = {
       ...type.extensions,
@@ -160,14 +169,19 @@ directive without an @${ownerDirective.name} directive`,
     );
 
     for (const typeDirectiveArgs of typeDirectivesArgs) {
-      const graphName = graphMap[typeDirectiveArgs['graph']].name;
+      const graph = graphMap.get(typeDirectiveArgs.graph);
+
+      assert(
+        graph,
+        `GraphQL type "${type.name}" must provide a \`graph\` argument to the @${typeDirective.name} directive`,
+      );
 
       const keyFields = parseFieldSet(typeDirectiveArgs['key']);
 
       // We know we won't actually be looping here in the case of a value type
       // based on the assertion above, but TS is not able to infer that.
       (typeMetadata as FederationEntityTypeMetadata).keys.add(
-        graphName,
+        graph.name,
         keyFields,
       );
     }
@@ -186,7 +200,7 @@ directive without an @${ownerDirective.name} directive`,
       if (!fieldDirectiveArgs) continue;
 
       const fieldMetadata: FederationFieldMetadata = {
-        graphName: graphMap[fieldDirectiveArgs?.['graph']]?.name,
+        graphName: graphMap.get(fieldDirectiveArgs.graph)?.name,
       };
 
       fieldDef.extensions = {
diff --git a/query-planner-js/src/composedSchema/metadata.ts b/query-planner-js/src/composedSchema/metadata.ts
index a8c96edac..23006f90d 100644
--- a/query-planner-js/src/composedSchema/metadata.ts
+++ b/query-planner-js/src/composedSchema/metadata.ts
@@ -39,7 +39,7 @@ export interface Graph {
   url: string;
 }
 
-export type GraphMap = { [graphName: string]: Graph };
+export type GraphMap = Map<string, Graph>;
 export interface FederationSchemaMetadata {
   graphs: GraphMap;
 }

From 4378987271e5d408ee1d1af9dc5a85ed7c287d12 Mon Sep 17 00:00:00 2001
From: Trevor Scheer <trevor.scheer@gmail.com>
Date: Tue, 13 Apr 2021 16:33:54 -0700
Subject: [PATCH 06/20] Clean up usages of printWithReducedWhitespace in favor
 of stripIgnoredCharacters from graphql-js

---
 .../postComposition/keysMatchBaseService.ts         | 13 ++++++++++---
 .../service/__tests__/printSupergraphSdl.test.ts    |  8 ++++----
 federation-js/src/service/printFederatedSchema.ts   |  7 -------
 federation-js/src/service/printSupergraphSdl.ts     | 12 ++++--------
 .../__tests__/loadSupergraphSdlFromStorage.test.ts  |  8 ++++----
 gateway-js/src/utilities/graphql.ts                 |  7 -------
 query-planner-js/src/utilities/graphql.ts           |  7 -------
 7 files changed, 22 insertions(+), 40 deletions(-)

diff --git a/federation-js/src/composition/validate/postComposition/keysMatchBaseService.ts b/federation-js/src/composition/validate/postComposition/keysMatchBaseService.ts
index ae0cf8adc..9a67db782 100644
--- a/federation-js/src/composition/validate/postComposition/keysMatchBaseService.ts
+++ b/federation-js/src/composition/validate/postComposition/keysMatchBaseService.ts
@@ -1,11 +1,16 @@
-import { isObjectType, GraphQLError, SelectionNode } from 'graphql';
+import {
+  isObjectType,
+  GraphQLError,
+  SelectionNode,
+  stripIgnoredCharacters,
+  print,
+} from 'graphql';
 import {
   logServiceAndType,
   errorWithCode,
   getFederationMetadata,
 } from '../../utils';
 import { PostCompositionValidator } from '.';
-import { printWithReducedWhitespace } from '../../../service';
 
 /**
  *  1. KEY_MISSING_ON_BASE - Originating types must specify at least 1 @key directive
@@ -82,5 +87,7 @@ export const keysMatchBaseService: PostCompositionValidator = function ({
 };
 
 function printFieldSet(selections: readonly SelectionNode[]): string {
-  return selections.map(printWithReducedWhitespace).join(' ');
+  return selections
+    .map((selection) => stripIgnoredCharacters(print(selection)))
+    .join(' ');
 }
diff --git a/federation-js/src/service/__tests__/printSupergraphSdl.test.ts b/federation-js/src/service/__tests__/printSupergraphSdl.test.ts
index 009c95bc1..9c5b86806 100644
--- a/federation-js/src/service/__tests__/printSupergraphSdl.test.ts
+++ b/federation-js/src/service/__tests__/printSupergraphSdl.test.ts
@@ -75,7 +75,7 @@ describe('printSupergraphSdl', () => {
         price: String @join__field(graph: PRODUCT)
         details: ProductDetailsBook @join__field(graph: PRODUCT)
         reviews: [Review] @join__field(graph: REVIEWS)
-        relatedReviews: [Review!]! @join__field(graph: REVIEWS, requires: \\"similarBooks { isbn }\\")
+        relatedReviews: [Review!]! @join__field(graph: REVIEWS, requires: \\"similarBooks{isbn}\\")
       }
 
       union Brand = Ikea | Amazon
@@ -251,7 +251,7 @@ describe('printSupergraphSdl', () => {
       type User
         @join__owner(graph: ACCOUNTS)
         @join__type(graph: ACCOUNTS, key: \\"id\\")
-        @join__type(graph: ACCOUNTS, key: \\"username name { first last }\\")
+        @join__type(graph: ACCOUNTS, key: \\"username name{first last}\\")
         @join__type(graph: INVENTORY, key: \\"id\\")
         @join__type(graph: PRODUCT, key: \\"id\\")
         @join__type(graph: REVIEWS, key: \\"id\\")
@@ -262,12 +262,12 @@ describe('printSupergraphSdl', () => {
         birthDate(locale: String): String @join__field(graph: ACCOUNTS)
         account: AccountType @join__field(graph: ACCOUNTS)
         metadata: [UserMetadata] @join__field(graph: ACCOUNTS)
-        goodDescription: Boolean @join__field(graph: INVENTORY, requires: \\"metadata { description }\\")
+        goodDescription: Boolean @join__field(graph: INVENTORY, requires: \\"metadata{description}\\")
         vehicle: Vehicle @join__field(graph: PRODUCT)
         thing: Thing @join__field(graph: PRODUCT)
         reviews: [Review] @join__field(graph: REVIEWS)
         numberOfReviews: Int! @join__field(graph: REVIEWS)
-        goodAddress: Boolean @join__field(graph: REVIEWS, requires: \\"metadata { address }\\")
+        goodAddress: Boolean @join__field(graph: REVIEWS, requires: \\"metadata{address}\\")
       }
 
       type UserMetadata {
diff --git a/federation-js/src/service/printFederatedSchema.ts b/federation-js/src/service/printFederatedSchema.ts
index 0d963d6c0..b96c46957 100644
--- a/federation-js/src/service/printFederatedSchema.ts
+++ b/federation-js/src/service/printFederatedSchema.ts
@@ -31,7 +31,6 @@ import {
   GraphQLEnumValue,
   GraphQLString,
   DEFAULT_DEPRECATION_REASON,
-  ASTNode,
 } from 'graphql';
 import { Maybe } from '../composition';
 import { isFederationType } from '../types';
@@ -305,12 +304,6 @@ function printFederationDirectives(
   return dedupedDirectives.length > 0 ? ' ' + dedupedDirectives.join(' ') : '';
 }
 
-export function printWithReducedWhitespace(ast: ASTNode): string {
-  return print(ast)
-    .replace(/\s+/g, ' ')
-    .trim();
-}
-
 function printBlock(items: string[]) {
   return items.length !== 0 ? ' {\n' + items.join('\n') + '\n}' : '';
 }
diff --git a/federation-js/src/service/printSupergraphSdl.ts b/federation-js/src/service/printSupergraphSdl.ts
index 0d2e95607..a5af68b4a 100644
--- a/federation-js/src/service/printSupergraphSdl.ts
+++ b/federation-js/src/service/printSupergraphSdl.ts
@@ -25,8 +25,8 @@ import {
   GraphQLEnumValue,
   GraphQLString,
   DEFAULT_DEPRECATION_REASON,
-  ASTNode,
   SelectionNode,
+  stripIgnoredCharacters,
 } from 'graphql';
 import { Maybe, FederationType, FederationField, ServiceDefinition } from '../composition';
 import { CoreDirective } from '../coreSpec';
@@ -353,19 +353,15 @@ function printFields(
   return printBlock(fields, isEntity);
 }
 
-export function printWithReducedWhitespace(ast: ASTNode): string {
-  return print(ast)
-    .replace(/\s+/g, ' ')
-    .trim();
-}
-
 /**
  * Core change: print fieldsets for @join__field's @key, @requires, and @provides args
  *
  * @param selections
  */
 function printFieldSet(selections: readonly SelectionNode[]): string {
-  return `${selections.map(printWithReducedWhitespace).join(' ')}`;
+  return selections
+    .map((selection) => stripIgnoredCharacters(print(selection)))
+    .join(' ');
 }
 
 /**
diff --git a/gateway-js/src/__tests__/loadSupergraphSdlFromStorage.test.ts b/gateway-js/src/__tests__/loadSupergraphSdlFromStorage.test.ts
index a524a7a59..40caa74de 100644
--- a/gateway-js/src/__tests__/loadSupergraphSdlFromStorage.test.ts
+++ b/gateway-js/src/__tests__/loadSupergraphSdlFromStorage.test.ts
@@ -74,7 +74,7 @@ describe('loadSupergraphSdlFromStorage', () => {
         price: String @join__field(graph: PRODUCT)
         details: ProductDetailsBook @join__field(graph: PRODUCT)
         reviews: [Review] @join__field(graph: REVIEWS)
-        relatedReviews: [Review!]! @join__field(graph: REVIEWS, requires: \\"similarBooks { isbn }\\")
+        relatedReviews: [Review!]! @join__field(graph: REVIEWS, requires: \\"similarBooks{isbn}\\")
       }
 
       union Brand = Ikea | Amazon
@@ -250,7 +250,7 @@ describe('loadSupergraphSdlFromStorage', () => {
       type User
         @join__owner(graph: ACCOUNTS)
         @join__type(graph: ACCOUNTS, key: \\"id\\")
-        @join__type(graph: ACCOUNTS, key: \\"username name { first last }\\")
+        @join__type(graph: ACCOUNTS, key: \\"username name{first last}\\")
         @join__type(graph: INVENTORY, key: \\"id\\")
         @join__type(graph: PRODUCT, key: \\"id\\")
         @join__type(graph: REVIEWS, key: \\"id\\")
@@ -261,12 +261,12 @@ describe('loadSupergraphSdlFromStorage', () => {
         birthDate(locale: String): String @join__field(graph: ACCOUNTS)
         account: AccountType @join__field(graph: ACCOUNTS)
         metadata: [UserMetadata] @join__field(graph: ACCOUNTS)
-        goodDescription: Boolean @join__field(graph: INVENTORY, requires: \\"metadata { description }\\")
+        goodDescription: Boolean @join__field(graph: INVENTORY, requires: \\"metadata{description}\\")
         vehicle: Vehicle @join__field(graph: PRODUCT)
         thing: Thing @join__field(graph: PRODUCT)
         reviews: [Review] @join__field(graph: REVIEWS)
         numberOfReviews: Int! @join__field(graph: REVIEWS)
-        goodAddress: Boolean @join__field(graph: REVIEWS, requires: \\"metadata { address }\\")
+        goodAddress: Boolean @join__field(graph: REVIEWS, requires: \\"metadata{address}\\")
       }
 
       type UserMetadata {
diff --git a/gateway-js/src/utilities/graphql.ts b/gateway-js/src/utilities/graphql.ts
index 44feedf81..f1c746da2 100644
--- a/gateway-js/src/utilities/graphql.ts
+++ b/gateway-js/src/utilities/graphql.ts
@@ -10,7 +10,6 @@ import {
   NamedTypeNode,
   OperationDefinitionNode,
   parse,
-  print,
   SelectionNode,
   TypeNode,
 } from 'graphql';
@@ -42,12 +41,6 @@ export function astFromType(type: GraphQLType): TypeNode {
   }
 }
 
-export function printWithReducedWhitespace(ast: ASTNode): string {
-  return print(ast)
-    .replace(/\s+/g, ' ')
-    .trim();
-}
-
 export function parseSelections(source: string): ReadonlyArray<SelectionNode> {
   return (parse(`query { ${source} }`)
     .definitions[0] as OperationDefinitionNode).selectionSet.selections;
diff --git a/query-planner-js/src/utilities/graphql.ts b/query-planner-js/src/utilities/graphql.ts
index a843ceb31..e8845af35 100644
--- a/query-planner-js/src/utilities/graphql.ts
+++ b/query-planner-js/src/utilities/graphql.ts
@@ -19,7 +19,6 @@ import {
   NamedTypeNode,
   OperationDefinitionNode,
   parse,
-  print,
   SchemaMetaFieldDef,
   SelectionNode,
   SelectionSetNode,
@@ -97,12 +96,6 @@ export function astFromType(type: GraphQLType): TypeNode {
   }
 }
 
-export function printWithReducedWhitespace(ast: ASTNode): string {
-  return print(ast)
-    .replace(/\s+/g, ' ')
-    .trim();
-}
-
 export function parseSelectionSet(source: string): SelectionSetNode {
   return (parse(`query ${source}`)
     .definitions[0] as OperationDefinitionNode).selectionSet;

From 5e40df81dfb805cac4ce86e03c3ea0a15e9ac8e4 Mon Sep 17 00:00:00 2001
From: Trevor Scheer <trevor.scheer@gmail.com>
Date: Wed, 14 Apr 2021 17:22:38 -0700
Subject: [PATCH 07/20] Confirm parsed FieldSets do not have an injected
 operation

Validate their definition length === 1.

Also:
* Update sibling fns in all 3 packages and add a note stating
their existence in other places.
* Remove extraneous 'query' in parsing function
---
 federation-js/src/composition/utils.ts        | 34 +++++++++++++++++--
 gateway-js/src/utilities/assert.ts            | 14 ++++++++
 gateway-js/src/utilities/graphql.ts           | 18 ++++++++--
 .../src/utilities/__tests__/graphql.test.ts   | 28 +++++++++++++++
 query-planner-js/src/utilities/assert.ts      |  9 +++++
 query-planner-js/src/utilities/graphql.ts     | 23 ++++++++-----
 6 files changed, 113 insertions(+), 13 deletions(-)
 create mode 100644 gateway-js/src/utilities/assert.ts
 create mode 100644 query-planner-js/src/utilities/__tests__/graphql.test.ts

diff --git a/federation-js/src/composition/utils.ts b/federation-js/src/composition/utils.ts
index 9f7909d94..afbc734f3 100644
--- a/federation-js/src/composition/utils.ts
+++ b/federation-js/src/composition/utils.ts
@@ -147,9 +147,37 @@ function removeExternalFieldsFromExtensionVisitor<
   };
 }
 
-export function parseSelections(source: string) {
-  return (parse(`query { ${source} }`)
-    .definitions[0] as OperationDefinitionNode).selectionSet.selections;
+/**
+ * For lack of a "home of federation utilities", this function is copy/pasted
+ * verbatim across the federation, gateway, and query-planner packages. Any changes
+ * made here should be reflected in the other two locations as well.
+ *
+ * @param condition
+ * @param message
+ * @throws
+ */
+export function assert(condition: any, message: string): asserts condition {
+  if (!condition) {
+    throw new Error(message);
+  }
+}
+
+/**
+ * For lack of a "home of federation utilities", this function is copy/pasted
+ * verbatim across the federation, gateway, and query-planner packages. Any changes
+ * made here should be reflected in the other two locations as well.
+ *
+ * @param source A string representing a FieldSet
+ * @returns A parsed FieldSet
+ */
+export function parseSelections(source: string): ReadonlyArray<SelectionNode> {
+  const parsed = parse(`{${source}}`);
+  assert(
+    parsed.definitions.length === 1,
+    `Invalid FieldSet provided: '${source}'. FieldSets may not contain operations within them.`,
+  );
+  return (parsed.definitions[0] as OperationDefinitionNode).selectionSet
+    .selections;
 }
 
 export function hasMatchingFieldInDirectives({
diff --git a/gateway-js/src/utilities/assert.ts b/gateway-js/src/utilities/assert.ts
new file mode 100644
index 000000000..a92dfc302
--- /dev/null
+++ b/gateway-js/src/utilities/assert.ts
@@ -0,0 +1,14 @@
+/**
+ * For lack of a "home of federation utilities", this function is copy/pasted
+ * verbatim across the federation, gateway, and query-planner packages. Any changes
+ * made here should be reflected in the other two locations as well.
+ *
+ * @param condition
+ * @param message
+ * @throws {@link Error}
+ */
+export function assert(condition: any, message: string): asserts condition {
+  if (!condition) {
+    throw new Error(message);
+  }
+}
diff --git a/gateway-js/src/utilities/graphql.ts b/gateway-js/src/utilities/graphql.ts
index f1c746da2..592c02973 100644
--- a/gateway-js/src/utilities/graphql.ts
+++ b/gateway-js/src/utilities/graphql.ts
@@ -13,6 +13,7 @@ import {
   SelectionNode,
   TypeNode,
 } from 'graphql';
+import { assert } from './assert';
 
 export function getResponseName(node: FieldNode): string {
   return node.alias ? node.alias.value : node.name.value;
@@ -41,7 +42,20 @@ export function astFromType(type: GraphQLType): TypeNode {
   }
 }
 
+/**
+ * For lack of a "home of federation utilities", this function is copy/pasted
+ * verbatim across the federation, gateway, and query-planner packages. Any changes
+ * made here should be reflected in the other two locations as well.
+ *
+ * @param source A string representing a FieldSet
+ * @returns A parsed FieldSet
+ */
 export function parseSelections(source: string): ReadonlyArray<SelectionNode> {
-  return (parse(`query { ${source} }`)
-    .definitions[0] as OperationDefinitionNode).selectionSet.selections;
+  const parsed = parse(`{${source}}`);
+  assert(
+    parsed.definitions.length === 1,
+    `Invalid FieldSet provided: '${source}'. FieldSets may not contain operations within them.`,
+  );
+  return (parsed.definitions[0] as OperationDefinitionNode).selectionSet
+    .selections;
 }
diff --git a/query-planner-js/src/utilities/__tests__/graphql.test.ts b/query-planner-js/src/utilities/__tests__/graphql.test.ts
new file mode 100644
index 000000000..84302b4cf
--- /dev/null
+++ b/query-planner-js/src/utilities/__tests__/graphql.test.ts
@@ -0,0 +1,28 @@
+import { FieldNode } from 'graphql';
+import { parseSelections } from '../graphql';
+
+describe('graphql utility functions', () => {
+  describe('parseSelections', () => {
+    it('parses a valid FieldSet', () => {
+      const fieldSet = 'foo bar';
+      const parsed = parseSelections(fieldSet);
+      expect(parsed).toHaveLength(2);
+    });
+
+    it('parses a nested FieldSet', () => {
+      const fieldSet = 'foo { bar }';
+      const parsed = parseSelections(fieldSet);
+      expect(parsed).toHaveLength(1);
+      expect((parsed[0] as FieldNode).selectionSet?.selections).toHaveLength(1);
+    });
+
+    it('throws when injecting an extra operation', () => {
+      const invalidFieldSet = 'foo } query X { bar';
+      expect(() =>
+        parseSelections(invalidFieldSet),
+      ).toThrowErrorMatchingInlineSnapshot(
+        `"Invalid FieldSet provided: 'foo } query X { bar'. FieldSets may not contain operations within them."`,
+      );
+    });
+  });
+});
diff --git a/query-planner-js/src/utilities/assert.ts b/query-planner-js/src/utilities/assert.ts
index e4c53793f..be1bbf0ca 100644
--- a/query-planner-js/src/utilities/assert.ts
+++ b/query-planner-js/src/utilities/assert.ts
@@ -1,3 +1,12 @@
+/**
+ * For lack of a "home of federation utilities", this function is copy/pasted
+ * verbatim across the federation, gateway, and query-planner packages. Any changes
+ * made here should be reflected in the other two locations as well.
+ *
+ * @param condition
+ * @param message
+ * @throws
+ */
 export function assert(condition: any, message: string): asserts condition {
   if (!condition) {
     throw new Error(message);
diff --git a/query-planner-js/src/utilities/graphql.ts b/query-planner-js/src/utilities/graphql.ts
index e8845af35..3f791decf 100644
--- a/query-planner-js/src/utilities/graphql.ts
+++ b/query-planner-js/src/utilities/graphql.ts
@@ -21,7 +21,6 @@ import {
   parse,
   SchemaMetaFieldDef,
   SelectionNode,
-  SelectionSetNode,
   TypeMetaFieldDef,
   TypeNameMetaFieldDef,
   TypeNode,
@@ -96,14 +95,22 @@ export function astFromType(type: GraphQLType): TypeNode {
   }
 }
 
-export function parseSelectionSet(source: string): SelectionSetNode {
-  return (parse(`query ${source}`)
-    .definitions[0] as OperationDefinitionNode).selectionSet;
-}
-
+/**
+ * For lack of a "home of federation utilities", this function is copy/pasted
+ * verbatim across the federation, gateway, and query-planner packages. Any changes
+ * made here should be reflected in the other two locations as well.
+ *
+ * @param source A string representing a FieldSet
+ * @returns A parsed FieldSet
+ */
 export function parseSelections(source: string): ReadonlyArray<SelectionNode> {
-  return (parse(`query { ${source} }`)
-    .definitions[0] as OperationDefinitionNode).selectionSet.selections;
+  const parsed = parse(`{${source}}`);
+  assert(
+    parsed.definitions.length === 1,
+    `Invalid FieldSet provided: '${source}'. FieldSets may not contain operations within them.`,
+  );
+  return (parsed.definitions[0] as OperationDefinitionNode).selectionSet
+    .selections;
 }
 
 // Using `getArgumentValues` from `graphql-js` ensures that arguments are of the right type,

From e37e2a651b33a354bed015adac967296fed9f22e Mon Sep 17 00:00:00 2001
From: Trevor Scheer <trevor.scheer@gmail.com>
Date: Wed, 14 Apr 2021 18:00:15 -0700
Subject: [PATCH 08/20] Ensure no FragmentSpreads nested in a FieldSet

---
 .../src/composedSchema/buildComposedSchema.ts | 17 +---------
 .../src/composedSchema/metadata.ts            |  5 +++
 .../src/utilities/__tests__/graphql.test.ts   | 31 ++++++++++++++++++-
 query-planner-js/src/utilities/graphql.ts     | 30 ++++++++++++++++++
 4 files changed, 66 insertions(+), 17 deletions(-)

diff --git a/query-planner-js/src/composedSchema/buildComposedSchema.ts b/query-planner-js/src/composedSchema/buildComposedSchema.ts
index 2dfb4031f..375fc8dfe 100644
--- a/query-planner-js/src/composedSchema/buildComposedSchema.ts
+++ b/query-planner-js/src/composedSchema/buildComposedSchema.ts
@@ -14,15 +14,13 @@ import { assert } from '../utilities/assert';
 import {
   getArgumentValuesForDirective,
   getArgumentValuesForRepeatableDirective,
-  isASTKind,
-  parseSelections,
+  parseFieldSet,
 } from '../utilities/graphql';
 import { MultiMap } from '../utilities/MultiMap';
 import {
   FederationFieldMetadata,
   FederationTypeMetadata,
   FederationEntityTypeMetadata,
-  FieldSet,
   GraphMap,
   isValueTypeMetadata,
 } from './metadata';
@@ -249,16 +247,3 @@ directive without an @${ownerDirective.name} directive`,
 }
 
 type NamedSchemaElement = GraphQLDirective | GraphQLNamedType;
-
-function parseFieldSet(source: string): FieldSet {
-  const selections = parseSelections(source);
-
-  assert(
-    selections.every(isASTKind('Field', 'InlineFragment')),
-    `Field sets may not contain fragment spreads, but found: "${source}"`,
-  );
-
-  assert(selections.length > 0, `Field sets may not be empty`);
-
-  return selections;
-}
diff --git a/query-planner-js/src/composedSchema/metadata.ts b/query-planner-js/src/composedSchema/metadata.ts
index 23006f90d..f7b7307c8 100644
--- a/query-planner-js/src/composedSchema/metadata.ts
+++ b/query-planner-js/src/composedSchema/metadata.ts
@@ -32,6 +32,11 @@ export function getFederationMetadataForField(
 }
 
 export type GraphName = string;
+
+// Without rewriting a number of AST types from graphql-js, this typing is
+// technically too relaxed. Recursive selections are not excluded from containing
+// FragmentSpreads, which is what this type is aiming to achieve (and accomplishes
+// at the root level, but not recursively)
 export type FieldSet = readonly (FieldNode | InlineFragmentNode)[];
 
 export interface Graph {
diff --git a/query-planner-js/src/utilities/__tests__/graphql.test.ts b/query-planner-js/src/utilities/__tests__/graphql.test.ts
index 84302b4cf..633c6938e 100644
--- a/query-planner-js/src/utilities/__tests__/graphql.test.ts
+++ b/query-planner-js/src/utilities/__tests__/graphql.test.ts
@@ -1,5 +1,5 @@
 import { FieldNode } from 'graphql';
-import { parseSelections } from '../graphql';
+import { parseFieldSet, parseSelections } from '../graphql';
 
 describe('graphql utility functions', () => {
   describe('parseSelections', () => {
@@ -25,4 +25,33 @@ describe('graphql utility functions', () => {
       );
     });
   });
+
+  describe('parseFieldSet', () => {
+    it('parses valid `FieldSet`s', () => {
+      const fieldSet = 'foo bar';
+      const parsed = parseFieldSet(fieldSet);
+      expect(parsed).toHaveLength(2);
+    });
+
+    it('disallows empty `FieldSet`s', () => {
+      const invalid = '';
+      expect(() => parseFieldSet(invalid)).toThrowErrorMatchingInlineSnapshot(
+        `"Syntax Error: Expected Name, found \\"}\\"."`,
+      );
+    });
+
+    it('disallows `FragmentSpread`s', () => {
+      const invalid = 'foo ...Bar';
+      expect(() => parseFieldSet(invalid)).toThrowErrorMatchingInlineSnapshot(
+        `"Field sets may not contain fragment spreads, but found: \\"foo ...Bar\\""`,
+      );
+    });
+
+    it('disallows nested `FragmentSpread`s', () => {
+      const invalid = 'foo { ...Bar }';
+      expect(() => parseFieldSet(invalid)).toThrowErrorMatchingInlineSnapshot(
+        `"Field sets may not contain fragment spreads, but found: \\"foo { ...Bar }\\""`,
+      );
+    });
+  });
 });
diff --git a/query-planner-js/src/utilities/graphql.ts b/query-planner-js/src/utilities/graphql.ts
index 3f791decf..14cd44b2a 100644
--- a/query-planner-js/src/utilities/graphql.ts
+++ b/query-planner-js/src/utilities/graphql.ts
@@ -12,6 +12,7 @@ import {
   GraphQLSchema,
   GraphQLType,
   GraphQLUnionType,
+  InlineFragmentNode,
   isListType,
   isNonNullType,
   Kind,
@@ -24,8 +25,10 @@ import {
   TypeMetaFieldDef,
   TypeNameMetaFieldDef,
   TypeNode,
+  visit,
 } from 'graphql';
 import { getArgumentValues } from 'graphql/execution/values';
+import { FieldSet } from '../composedSchema';
 import { assert } from './assert';
 
 /**
@@ -113,6 +116,33 @@ export function parseSelections(source: string): ReadonlyArray<SelectionNode> {
     .selections;
 }
 
+// TODO: should we be using this everywhere we're using `parseSelections`?
+export function parseFieldSet(source: string): FieldSet {
+  const selections = parseSelections(source);
+
+  const selectionSetNode = {
+    kind: Kind.SELECTION_SET,
+    selections,
+  };
+
+  visit(selectionSetNode, {
+    FragmentSpread() {
+      throw Error(
+        `Field sets may not contain fragment spreads, but found: "${source}"`,
+      );
+    },
+  });
+
+  // I'm not sure this case is possible - an empty string will first throw a
+  // graphql syntax error. Can you get 0 selections any other way?
+  assert(selections.length > 0, `Field sets may not be empty`);
+
+  // This cast is asserted above by the visitor, ensuring that both `selections`
+  // and any recursive `selections` are not `FragmentSpreadNode`s
+  return selections as readonly (FieldNode | InlineFragmentNode)[];
+}
+
+
 // Using `getArgumentValues` from `graphql-js` ensures that arguments are of the right type,
 // and that required arguments are present.
 

From 2aa3cf5a067235158799bf9242c7734235ebe249 Mon Sep 17 00:00:00 2001
From: Trevor Scheer <trevor.scheer@gmail.com>
Date: Thu, 15 Apr 2021 11:20:44 -0700
Subject: [PATCH 09/20] Capture caveats in comments from commit messages

---
 query-planner-js/src/FieldSet.ts       |  4 ++++
 query-planner-js/src/buildQueryPlan.ts | 21 +++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/query-planner-js/src/FieldSet.ts b/query-planner-js/src/FieldSet.ts
index a508c6510..57b8645e1 100644
--- a/query-planner-js/src/FieldSet.ts
+++ b/query-planner-js/src/FieldSet.ts
@@ -200,6 +200,10 @@ function mergeFieldNodeSelectionSets(
     (node): node is FieldNode => node.kind === Kind.FIELD,
   );
 
+  // Committed by @trevor-scheer but authored by @martijnwalraven
+  // XXX: This code has more problems and should be replaced by proper recursive
+  // selection set merging, but removing the unnecessary distinction between
+  // aliased fields and non-aliased fields at least fixes the test.
   const mergedFieldNodes = Array.from(
     groupBy((node: FieldNode) => node.alias?.value ?? node.name.value)(
       fieldNodes,
diff --git a/query-planner-js/src/buildQueryPlan.ts b/query-planner-js/src/buildQueryPlan.ts
index cd5117b92..abed720db 100644
--- a/query-planner-js/src/buildQueryPlan.ts
+++ b/query-planner-js/src/buildQueryPlan.ts
@@ -436,6 +436,12 @@ function splitSubfields(
     const { scope, fieldNode, fieldDef } = field;
     const { parentType } = scope;
 
+    // Committed by @trevor-scheer but authored by @martijnwalraven
+    // Treat abstract types as value types to replicate type explosion fix
+    // XXX: this replicates the behavior of the Rust query planner implementation,
+    // in order to get the tests passing before making further changes. But the
+    // type explosion fix this depends on is fundamentally flawed and needs to
+    // be replaced.
     const parentIsValueType = !isObjectType(parentType) || getFederationMetadataForType(parentType)?.isValueType;
 
     let baseService, owningService;
@@ -846,6 +852,21 @@ function collectFields(
           break;
         }
 
+        // Committed by @trevor-scheer but authored by @martijnwalraven
+        // This replicates the behavior added to the Rust query planner in #178.
+        // Unfortunately, the logic in there is seriously flawed, and may lead to
+        // unexpected results. The assumption seems to be that fields with the
+        // same parent type are always nested in the same inline fragment. That
+        // is not necessarily true however, and because we take the directives
+        // from the scope of the first field with a particular parent type,
+        // those directives will be applied to all other fields that have the
+        // same parent type even if the directives aren't meant to apply to them
+        // because they were nested in a different inline fragment. (That also
+        // means that if the scope of the first field doesn't have directives,
+        // directives that would have applied to other fields will be lost.)
+        // Note that this also applies to `@skip` and `@include`, which could
+        // lead to invalid query plans that fail at runtime because expected
+        // fields are missing from a subgraph response.
         newScope.directives = selection.directives;
 
         collectFields(

From fb0f6e928d22199ef2fe57cbcbea7bcd61c18412 Mon Sep 17 00:00:00 2001
From: Trevor Scheer <trevor.scheer@gmail.com>
Date: Thu, 15 Apr 2021 11:43:42 -0700
Subject: [PATCH 10/20] Remove incorrect nullish coalesce to ownerService

This didn't make sense. Rather than falling back to something
that isn't correct, we should just assert the existence of the
thing that is expected to always be there.
---
 federation-js/src/service/printSupergraphSdl.ts | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/federation-js/src/service/printSupergraphSdl.ts b/federation-js/src/service/printSupergraphSdl.ts
index a5af68b4a..0efc88b72 100644
--- a/federation-js/src/service/printSupergraphSdl.ts
+++ b/federation-js/src/service/printSupergraphSdl.ts
@@ -29,6 +29,7 @@ import {
   stripIgnoredCharacters,
 } from 'graphql';
 import { Maybe, FederationType, FederationField, ServiceDefinition } from '../composition';
+import { assert } from '../composition/utils';
 import { CoreDirective } from '../coreSpec';
 import { getJoinDefinitions } from '../joinSpec';
 
@@ -246,10 +247,14 @@ function printTypeJoinDirectives(
 
   // We don't want to print an owner for interface types
   const shouldPrintOwner = isObjectType(type);
+
+  const enumValue = context.graphNameToEnumValueName?.[ownerService];
+  assert(
+    enumValue,
+    `Unexpected enum value missing for subgraph ${ownerService}`,
+  );
   const joinOwnerString = shouldPrintOwner
-    ? `\n  @join__owner(graph: ${
-        context.graphNameToEnumValueName?.[ownerService] ?? ownerService
-      })`
+    ? `\n  @join__owner(graph: ${enumValue})`
     : '';
 
   return (

From 5ac14f28cf917763f9ce59b59953113f36e21e27 Mon Sep 17 00:00:00 2001
From: Trevor Scheer <trevor.scheer@gmail.com>
Date: Fri, 16 Apr 2021 12:54:47 -0700
Subject: [PATCH 11/20] Update ordering of join__Graph enum in test mocks

---
 harmonizer/src/snapshots/harmonizer__tests__it_works.snap     | 2 +-
 .../__tests__/features/multiple-keys/supergraphSdl.graphql    | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/harmonizer/src/snapshots/harmonizer__tests__it_works.snap b/harmonizer/src/snapshots/harmonizer__tests__it_works.snap
index 294ddcb37..c910cef0a 100644
--- a/harmonizer/src/snapshots/harmonizer__tests__it_works.snap
+++ b/harmonizer/src/snapshots/harmonizer__tests__it_works.snap
@@ -23,8 +23,8 @@ directive @join__graph(name: String!, url: String!) on ENUM_VALUE
 scalar join__FieldSet
 
 enum join__Graph {
-  USERS @join__graph(name: "users" url: "undefined")
   MOVIES @join__graph(name: "movies" url: "undefined")
+  USERS @join__graph(name: "users" url: "undefined")
 }
 
 type Movie {
diff --git a/query-planner-js/src/__tests__/features/multiple-keys/supergraphSdl.graphql b/query-planner-js/src/__tests__/features/multiple-keys/supergraphSdl.graphql
index 785c190d9..51bb4ed06 100644
--- a/query-planner-js/src/__tests__/features/multiple-keys/supergraphSdl.graphql
+++ b/query-planner-js/src/__tests__/features/multiple-keys/supergraphSdl.graphql
@@ -23,9 +23,9 @@ type Group {
 scalar join__FieldSet
 
 enum join__Graph {
-  USERS @join__graph(name: "users" url: "undefined")
-  REVIEWS @join__graph(name: "reviews" url: "undefined")
   ACTUARY @join__graph(name: "actuary" url: "undefined")
+  REVIEWS @join__graph(name: "reviews" url: "undefined")
+  USERS @join__graph(name: "users" url: "undefined")
 }
 
 type Query {

From 778427618f9c5ae3c4f0f152bd4403dc22041e5e Mon Sep 17 00:00:00 2001
From: Trevor Scheer <trevor.scheer@gmail.com>
Date: Wed, 21 Apr 2021 10:59:00 -0700
Subject: [PATCH 12/20] Invert metadata predicate which was always negated to
 its opposite

---
 query-planner-js/src/buildQueryPlan.ts                     | 6 +++---
 query-planner-js/src/composedSchema/buildComposedSchema.ts | 4 ++--
 query-planner-js/src/composedSchema/metadata.ts            | 6 +++---
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/query-planner-js/src/buildQueryPlan.ts b/query-planner-js/src/buildQueryPlan.ts
index abed720db..e3a7af5f0 100644
--- a/query-planner-js/src/buildQueryPlan.ts
+++ b/query-planner-js/src/buildQueryPlan.ts
@@ -55,7 +55,7 @@ import { MultiMap } from './utilities/MultiMap';
 import {
   getFederationMetadataForType,
   getFederationMetadataForField,
-  isValueTypeMetadata,
+  isEntityTypeMetadata,
 } from './composedSchema';
 import { DebugLogger } from './utilities/debug';
 
@@ -1162,7 +1162,7 @@ export class QueryPlanningContext {
 
   getBaseService(parentType: GraphQLObjectType): string | undefined {
     const type = getFederationMetadataForType(parentType);
-    return (type && !isValueTypeMetadata(type)) ? type.graphName : undefined;
+    return (type && isEntityTypeMetadata(type)) ? type.graphName : undefined;
   }
 
   getOwningService(
@@ -1198,7 +1198,7 @@ export class QueryPlanningContext {
     for (const possibleType of this.getPossibleTypes(parentType)) {
       const type = getFederationMetadataForType(possibleType);
       const keys =
-        type && !isValueTypeMetadata(type)
+        type && isEntityTypeMetadata(type)
           ? type.keys.get(serviceName)
           : undefined;
 
diff --git a/query-planner-js/src/composedSchema/buildComposedSchema.ts b/query-planner-js/src/composedSchema/buildComposedSchema.ts
index 375fc8dfe..818719369 100644
--- a/query-planner-js/src/composedSchema/buildComposedSchema.ts
+++ b/query-planner-js/src/composedSchema/buildComposedSchema.ts
@@ -22,7 +22,7 @@ import {
   FederationTypeMetadata,
   FederationEntityTypeMetadata,
   GraphMap,
-  isValueTypeMetadata,
+  isEntityTypeMetadata,
 } from './metadata';
 
 export function buildComposedSchema(document: DocumentNode): GraphQLSchema {
@@ -161,7 +161,7 @@ export function buildComposedSchema(document: DocumentNode): GraphQLSchema {
     // (typeMetadata as FederationEntityTypeMetadata). Adjustments to this assertion
     // should account for this dependency.
     assert(
-      !(isValueTypeMetadata(typeMetadata) && typeDirectivesArgs.length >= 1),
+      isEntityTypeMetadata(typeMetadata) || typeDirectivesArgs.length === 0,
       `GraphQL type "${type.name}" cannot have a @${typeDirective.name} \
 directive without an @${ownerDirective.name} directive`,
     );
diff --git a/query-planner-js/src/composedSchema/metadata.ts b/query-planner-js/src/composedSchema/metadata.ts
index f7b7307c8..9549331f2 100644
--- a/query-planner-js/src/composedSchema/metadata.ts
+++ b/query-planner-js/src/composedSchema/metadata.ts
@@ -63,10 +63,10 @@ interface FederationValueTypeMetadata {
   isValueType: true;
 }
 
-export function isValueTypeMetadata(
+export function isEntityTypeMetadata(
   metadata: FederationTypeMetadata,
-): metadata is FederationValueTypeMetadata {
-  return metadata.isValueType;
+): metadata is FederationEntityTypeMetadata {
+  return !metadata.isValueType;
 }
 
 export interface FederationFieldMetadata {

From 4b7d50f0250439c3d1c61c9f42c80a70d4a9a0c4 Mon Sep 17 00:00:00 2001
From: Trevor Scheer <trevor.scheer@gmail.com>
Date: Wed, 21 Apr 2021 11:00:21 -0700
Subject: [PATCH 13/20] Update expectations comment

---
 federation-js/src/__tests__/joinSpec.test.ts | 2 +-
 federation-js/src/joinSpec.ts                | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/federation-js/src/__tests__/joinSpec.test.ts b/federation-js/src/__tests__/joinSpec.test.ts
index 7ab6d7e3b..1605e755d 100644
--- a/federation-js/src/__tests__/joinSpec.test.ts
+++ b/federation-js/src/__tests__/joinSpec.test.ts
@@ -27,7 +27,7 @@ describe('join__Graph enum', () => {
      * 2. Numeric first characters are prefixed with _ (9product*!)
      * 3. Names ending in an underscore followed by numbers `_\d+` are suffixed with _ (reviews_9, servicea_2)
      * 4. Names are uppercased (all)
-     * 5. After transformations 1-4, duplicates are suffixed with _{n} where {n} is number of times we've seen the dupe (ServiceA + serviceA, servicea_2 + servicea_2_)
+     * 5. After transformations 1-5, duplicates are suffixed with _{n} where {n} is number of times we've seen the dupe (ServiceA + serviceA, servicea_2 + servicea_2_)
      *
      * Miscellany
      * (serviceA) tests the edge case of colliding with a name we generated
diff --git a/federation-js/src/joinSpec.ts b/federation-js/src/joinSpec.ts
index a36866838..f6fb8d6b3 100644
--- a/federation-js/src/joinSpec.ts
+++ b/federation-js/src/joinSpec.ts
@@ -32,7 +32,7 @@ const JoinGraphDirective = new GraphQLDirective({
  * 3. Numeric first characters are prefixed with _ (noNumericFirstChar)
  * 4. Names ending in an underscore followed by numbers `_\d+` are suffixed with _ (noUnderscoreNumericEnding)
  * 5. Names are uppercased (toUpper)
- * 6. After transformations 1-4, duplicates are suffixed with _{n} where {n} is number of times we've seen the dupe
+ * 6. After transformations 1-5, duplicates are suffixed with _{n} where {n} is number of times we've seen the dupe
  *
  * Note: Collisions with name's we've generated are also accounted for
  */

From 6c4261b0c57dea42de27eb808b81f01582c861a2 Mon Sep 17 00:00:00 2001
From: Trevor Scheer <trevor.scheer@gmail.com>
Date: Wed, 21 Apr 2021 11:17:20 -0700
Subject: [PATCH 14/20] Create nice helper for working with Maps (mapGetOrSet)

Also move some generic utils out of the composition utils file
and into their own tiny files in a utilities folder. This more
closely matches the structure of the gateway's code as well.
---
 federation-js/src/composition/compose.ts      |  2 +-
 federation-js/src/composition/utils.ts        | 36 +------------------
 .../keyFieldsMissingExternal.ts               |  2 +-
 federation-js/src/joinSpec.ts                 | 11 ++----
 .../src/service/printSupergraphSdl.ts         |  2 +-
 federation-js/src/utilities/assert.ts         | 14 ++++++++
 federation-js/src/utilities/index.ts          |  4 +++
 .../src/utilities/isNotNullOrUndefined.ts     |  5 +++
 federation-js/src/utilities/mapGetOrSet.ts    |  6 ++++
 federation-js/src/utilities/mapValues.ts      | 13 +++++++
 10 files changed, 48 insertions(+), 47 deletions(-)
 create mode 100644 federation-js/src/utilities/assert.ts
 create mode 100644 federation-js/src/utilities/index.ts
 create mode 100644 federation-js/src/utilities/isNotNullOrUndefined.ts
 create mode 100644 federation-js/src/utilities/mapGetOrSet.ts
 create mode 100644 federation-js/src/utilities/mapValues.ts

diff --git a/federation-js/src/composition/compose.ts b/federation-js/src/composition/compose.ts
index 37f595e06..57d7e2c1e 100644
--- a/federation-js/src/composition/compose.ts
+++ b/federation-js/src/composition/compose.ts
@@ -27,7 +27,6 @@ import {
   mapFieldNamesToServiceName,
   stripExternalFieldsFromTypeDefs,
   typeNodesAreEquivalent,
-  mapValues,
   isFederationDirective,
   executableDirectiveLocations,
   stripTypeSystemDirectivesFromTypeDefs,
@@ -46,6 +45,7 @@ import {
 import { validateSDL } from 'graphql/validation/validate';
 import { compositionRules } from './rules';
 import { printSupergraphSdl } from '../service/printSupergraphSdl';
+import { mapValues } from '../utilities';
 
 const EmptyQueryDefinition = {
   kind: Kind.OBJECT_TYPE_DEFINITION,
diff --git a/federation-js/src/composition/utils.ts b/federation-js/src/composition/utils.ts
index afbc734f3..81086bff0 100644
--- a/federation-js/src/composition/utils.ts
+++ b/federation-js/src/composition/utils.ts
@@ -42,6 +42,7 @@ import {
   FederationField,
 } from './types';
 import federationDirectives from '../directives';
+import { assert, isNotNullOrUndefined } from '../utilities';
 
 export function isStringValueNode(node: any): node is StringValueNode {
   return node.kind === Kind.STRING;
@@ -147,21 +148,6 @@ function removeExternalFieldsFromExtensionVisitor<
   };
 }
 
-/**
- * For lack of a "home of federation utilities", this function is copy/pasted
- * verbatim across the federation, gateway, and query-planner packages. Any changes
- * made here should be reflected in the other two locations as well.
- *
- * @param condition
- * @param message
- * @throws
- */
-export function assert(condition: any, message: string): asserts condition {
-  if (!condition) {
-    throw new Error(message);
-  }
-}
-
 /**
  * For lack of a "home of federation utilities", this function is copy/pasted
  * verbatim across the federation, gateway, and query-planner packages. Any changes
@@ -584,26 +570,6 @@ export const defKindToExtKind: { [kind: string]: string } = {
   [Kind.INPUT_OBJECT_TYPE_DEFINITION]: Kind.INPUT_OBJECT_TYPE_EXTENSION,
 };
 
-// Transform an object's values via a callback function
-export function mapValues<T, U = T>(
-  object: Record<string, T>,
-  callback: (value: T) => U,
-): Record<string, U> {
-  const result: Record<string, U> = Object.create(null);
-
-  for (const [key, value] of Object.entries(object)) {
-    result[key] = callback(value);
-  }
-
-  return result;
-}
-
-export function isNotNullOrUndefined<T>(
-  value: T | null | undefined,
-): value is T {
-  return value !== null && typeof value !== 'undefined';
-}
-
 export const executableDirectiveLocations = [
   'QUERY',
   'MUTATION',
diff --git a/federation-js/src/composition/validate/preComposition/keyFieldsMissingExternal.ts b/federation-js/src/composition/validate/preComposition/keyFieldsMissingExternal.ts
index be0bffd76..febd1b9a6 100644
--- a/federation-js/src/composition/validate/preComposition/keyFieldsMissingExternal.ts
+++ b/federation-js/src/composition/validate/preComposition/keyFieldsMissingExternal.ts
@@ -15,8 +15,8 @@ import {
   isStringValueNode,
   logServiceAndType,
   errorWithCode,
-  isNotNullOrUndefined
 } from '../../utils';
+import { isNotNullOrUndefined } from '../../../utilities';
 
 /**
  *  For every @key directive, it must reference a field marked as @external
diff --git a/federation-js/src/joinSpec.ts b/federation-js/src/joinSpec.ts
index f6fb8d6b3..583509e5a 100644
--- a/federation-js/src/joinSpec.ts
+++ b/federation-js/src/joinSpec.ts
@@ -7,6 +7,7 @@ import {
   GraphQLNonNull,
 } from 'graphql';
 import { ServiceDefinition } from './composition';
+import { mapGetOrSet } from './utilities/mapGetOrSet';
 
 const FieldSetScalar = new GraphQLScalarType({
   name: 'join__FieldSet',
@@ -68,15 +69,7 @@ function getJoinGraphEnum(serviceList: ServiceDefinition[]) {
     const { name } = service;
     const sanitized = sanitizeGraphQLName(name);
 
-    const existingEntry = sanitizedNameToServiceDefinitions.get(sanitized);
-    if (existingEntry) {
-      sanitizedNameToServiceDefinitions.set(sanitized, [
-        ...existingEntry,
-        service,
-      ]);
-    } else {
-      sanitizedNameToServiceDefinitions.set(sanitized, [service]);
-    }
+    mapGetOrSet(sanitizedNameToServiceDefinitions, sanitized, [service]);
   }
 
   // if no duplicates for a given name, add it as is
diff --git a/federation-js/src/service/printSupergraphSdl.ts b/federation-js/src/service/printSupergraphSdl.ts
index 0efc88b72..b5a484038 100644
--- a/federation-js/src/service/printSupergraphSdl.ts
+++ b/federation-js/src/service/printSupergraphSdl.ts
@@ -29,7 +29,7 @@ import {
   stripIgnoredCharacters,
 } from 'graphql';
 import { Maybe, FederationType, FederationField, ServiceDefinition } from '../composition';
-import { assert } from '../composition/utils';
+import { assert } from '../utilities';
 import { CoreDirective } from '../coreSpec';
 import { getJoinDefinitions } from '../joinSpec';
 
diff --git a/federation-js/src/utilities/assert.ts b/federation-js/src/utilities/assert.ts
new file mode 100644
index 000000000..be1bbf0ca
--- /dev/null
+++ b/federation-js/src/utilities/assert.ts
@@ -0,0 +1,14 @@
+/**
+ * For lack of a "home of federation utilities", this function is copy/pasted
+ * verbatim across the federation, gateway, and query-planner packages. Any changes
+ * made here should be reflected in the other two locations as well.
+ *
+ * @param condition
+ * @param message
+ * @throws
+ */
+export function assert(condition: any, message: string): asserts condition {
+  if (!condition) {
+    throw new Error(message);
+  }
+}
diff --git a/federation-js/src/utilities/index.ts b/federation-js/src/utilities/index.ts
new file mode 100644
index 000000000..ce06261ed
--- /dev/null
+++ b/federation-js/src/utilities/index.ts
@@ -0,0 +1,4 @@
+export { assert } from './assert';
+export { isNotNullOrUndefined } from './isNotNullOrUndefined';
+export { mapGetOrSet } from './mapGetOrSet';
+export { mapValues } from './mapValues';
diff --git a/federation-js/src/utilities/isNotNullOrUndefined.ts b/federation-js/src/utilities/isNotNullOrUndefined.ts
new file mode 100644
index 000000000..6837314ae
--- /dev/null
+++ b/federation-js/src/utilities/isNotNullOrUndefined.ts
@@ -0,0 +1,5 @@
+export function isNotNullOrUndefined<T>(
+  value: T | null | undefined,
+): value is T {
+  return value !== null && typeof value !== 'undefined';
+}
diff --git a/federation-js/src/utilities/mapGetOrSet.ts b/federation-js/src/utilities/mapGetOrSet.ts
new file mode 100644
index 000000000..afc835d4e
--- /dev/null
+++ b/federation-js/src/utilities/mapGetOrSet.ts
@@ -0,0 +1,6 @@
+export function mapGetOrSet<K, V>(map: Map<K, V>, key: K, valueToSet: V): V {
+  if (!map.has(key)) {
+    map.set(key, valueToSet);
+  }
+  return map.get(key)!;
+}
diff --git a/federation-js/src/utilities/mapValues.ts b/federation-js/src/utilities/mapValues.ts
new file mode 100644
index 000000000..f493eb6db
--- /dev/null
+++ b/federation-js/src/utilities/mapValues.ts
@@ -0,0 +1,13 @@
+// Transform an object's values via a callback function
+export function mapValues<T, U = T>(
+  object: Record<string, T>,
+  callback: (value: T) => U,
+): Record<string, U> {
+  const result: Record<string, U> = Object.create(null);
+
+  for (const [key, value] of Object.entries(object)) {
+    result[key] = callback(value);
+  }
+
+  return result;
+}

From 524809d9051fa08b999292e15b1390804ce87056 Mon Sep 17 00:00:00 2001
From: Trevor Scheer <trevor.scheer@gmail.com>
Date: Wed, 21 Apr 2021 11:23:41 -0700
Subject: [PATCH 15/20] Fix usage of mapGetOrSet

---
 federation-js/src/joinSpec.ts | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/federation-js/src/joinSpec.ts b/federation-js/src/joinSpec.ts
index 583509e5a..9f863f5d8 100644
--- a/federation-js/src/joinSpec.ts
+++ b/federation-js/src/joinSpec.ts
@@ -68,8 +68,7 @@ function getJoinGraphEnum(serviceList: ServiceDefinition[]) {
   for (const service of sortedServiceList) {
     const { name } = service;
     const sanitized = sanitizeGraphQLName(name);
-
-    mapGetOrSet(sanitizedNameToServiceDefinitions, sanitized, [service]);
+    mapGetOrSet(sanitizedNameToServiceDefinitions, sanitized, []).push(service);
   }
 
   // if no duplicates for a given name, add it as is

From 208a0415f0e9fca314c4621b8d7a36dbf2f50fa5 Mon Sep 17 00:00:00 2001
From: Trevor Scheer <trevor.scheer@gmail.com>
Date: Wed, 21 Apr 2021 11:28:14 -0700
Subject: [PATCH 16/20] Add clarity to names

---
 federation-js/src/joinSpec.ts | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/federation-js/src/joinSpec.ts b/federation-js/src/joinSpec.ts
index 9f863f5d8..1412999a5 100644
--- a/federation-js/src/joinSpec.ts
+++ b/federation-js/src/joinSpec.ts
@@ -73,29 +73,34 @@ function getJoinGraphEnum(serviceList: ServiceDefinition[]) {
 
   // if no duplicates for a given name, add it as is
   // if duplicates exist, append _{n} (index-1) to each duplicate in the array
-  const generatedNameToServiceDefinition: Record<
+  const enumValueNameToServiceDefinition: Record<
     string,
     ServiceDefinition
   > = Object.create(null);
-  for (const [name, services] of sanitizedNameToServiceDefinitions) {
+  for (const [sanitizedName, services] of sanitizedNameToServiceDefinitions) {
     if (services.length === 1) {
-      generatedNameToServiceDefinition[name] = services[0];
+      enumValueNameToServiceDefinition[sanitizedName] = services[0];
     } else {
       for (const [index, service] of services.entries()) {
-        generatedNameToServiceDefinition[`${name}_${index + 1}`] = service;
+        enumValueNameToServiceDefinition[
+          `${sanitizedName}_${index + 1}`
+        ] = service;
       }
     }
   }
 
-  const entries = Object.entries(generatedNameToServiceDefinition);
+  const entries = Object.entries(enumValueNameToServiceDefinition);
   return {
     graphNameToEnumValueName: Object.fromEntries(
-      entries.map(([name, service]) => [service.name, name]),
+      entries.map(([enumValueName, service]) => [service.name, enumValueName]),
     ),
     JoinGraphEnum: new GraphQLEnumType({
       name: 'join__Graph',
       values: Object.fromEntries(
-        entries.map(([name, service]) => [name, { value: service }]),
+        entries.map(([enumValueName, service]) => [
+          enumValueName,
+          { value: service },
+        ]),
       ),
     }),
   };

From f85a597eb136e3250b3dd5a006cec95d719e1eae Mon Sep 17 00:00:00 2001
From: Trevor Scheer <trevor.scheer@gmail.com>
Date: Wed, 21 Apr 2021 11:51:31 -0700
Subject: [PATCH 17/20] Correct error message

---
 .../src/composedSchema/buildComposedSchema.ts | 33 ++++++++++++++-----
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/query-planner-js/src/composedSchema/buildComposedSchema.ts b/query-planner-js/src/composedSchema/buildComposedSchema.ts
index 818719369..49fbcace6 100644
--- a/query-planner-js/src/composedSchema/buildComposedSchema.ts
+++ b/query-planner-js/src/composedSchema/buildComposedSchema.ts
@@ -23,6 +23,7 @@ import {
   FederationEntityTypeMetadata,
   GraphMap,
   isEntityTypeMetadata,
+  Graph,
 } from './metadata';
 
 export function buildComposedSchema(document: DocumentNode): GraphQLSchema {
@@ -130,11 +131,12 @@ export function buildComposedSchema(document: DocumentNode): GraphQLSchema {
 
     let typeMetadata: FederationTypeMetadata;
     if (ownerDirectiveArgs) {
-      const graph = graphMap.get(ownerDirectiveArgs.graph);
       assert(
-        graph,
+        ownerDirectiveArgs.graph,
         `@${ownerDirective.name} directive requires a \`graph\` argument`,
       );
+      const graph = graphMap.get(ownerDirectiveArgs.graph);
+      assertGraphFound(graph, ownerDirectiveArgs.graph, ownerDirective.name);
 
       typeMetadata = {
         graphName: graph.name,
@@ -167,12 +169,12 @@ directive without an @${ownerDirective.name} directive`,
     );
 
     for (const typeDirectiveArgs of typeDirectivesArgs) {
-      const graph = graphMap.get(typeDirectiveArgs.graph);
-
       assert(
-        graph,
+        typeDirectiveArgs.graph,
         `GraphQL type "${type.name}" must provide a \`graph\` argument to the @${typeDirective.name} directive`,
       );
+      const graph = graphMap.get(typeDirectiveArgs.graph);
+      assertGraphFound(graph, typeDirectiveArgs.graph, typeDirective.name);
 
       const keyFields = parseFieldSet(typeDirectiveArgs['key']);
 
@@ -197,9 +199,15 @@ directive without an @${ownerDirective.name} directive`,
 
       if (!fieldDirectiveArgs) continue;
 
-      const fieldMetadata: FederationFieldMetadata = {
-        graphName: graphMap.get(fieldDirectiveArgs.graph)?.name,
-      };
+      let fieldMetadata: FederationFieldMetadata;
+      if (fieldDirectiveArgs.graph) {
+        const graph = graphMap.get(fieldDirectiveArgs.graph);
+        // This should never happen, but the assertion guarantees the existence of `graph`
+        assertGraphFound(graph, fieldDirectiveArgs.graph, fieldDirective.name);
+        fieldMetadata = { graphName: graph.name };
+      } else {
+        fieldMetadata = { graphName: undefined };
+      }
 
       fieldDef.extensions = {
         ...fieldDef.extensions,
@@ -247,3 +255,12 @@ directive without an @${ownerDirective.name} directive`,
 }
 
 type NamedSchemaElement = GraphQLDirective | GraphQLNamedType;
+
+// This should never happen, hence 'programming error', but this assertion
+// guarantees the existence of `graph`.
+function assertGraphFound(graph: Graph | undefined, graphName: string, directiveName: string): asserts graph {
+  assert(
+    graph,
+    `Programming error: found unexpected \`graph\` argument value "${graphName}" in @${directiveName} directive`,
+  );
+}

From 2a2c429abeba6e24598e52ee2be772e601d28176 Mon Sep 17 00:00:00 2001
From: Trevor Scheer <trevor.scheer@gmail.com>
Date: Wed, 21 Apr 2021 14:19:22 -0700
Subject: [PATCH 18/20] Simplify extra } error message

---
 federation-js/src/composition/utils.ts | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/federation-js/src/composition/utils.ts b/federation-js/src/composition/utils.ts
index 81086bff0..9e6fd2239 100644
--- a/federation-js/src/composition/utils.ts
+++ b/federation-js/src/composition/utils.ts
@@ -158,10 +158,7 @@ function removeExternalFieldsFromExtensionVisitor<
  */
 export function parseSelections(source: string): ReadonlyArray<SelectionNode> {
   const parsed = parse(`{${source}}`);
-  assert(
-    parsed.definitions.length === 1,
-    `Invalid FieldSet provided: '${source}'. FieldSets may not contain operations within them.`,
-  );
+  assert(parsed.definitions.length === 1, `Unexpected } found in FieldSet`);
   return (parsed.definitions[0] as OperationDefinitionNode).selectionSet
     .selections;
 }

From a1156a85161022e3f9358d630644e9db956ad7c5 Mon Sep 17 00:00:00 2001
From: Trevor Scheer <trevor.scheer@gmail.com>
Date: Wed, 21 Apr 2021 14:33:25 -0700
Subject: [PATCH 19/20] Fix remaining accesses to
 context.graphNameToEnumValueName

---
 .../src/service/printSupergraphSdl.ts         | 41 ++++++++++---------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/federation-js/src/service/printSupergraphSdl.ts b/federation-js/src/service/printSupergraphSdl.ts
index b5a484038..7381772f2 100644
--- a/federation-js/src/service/printSupergraphSdl.ts
+++ b/federation-js/src/service/printSupergraphSdl.ts
@@ -248,13 +248,13 @@ function printTypeJoinDirectives(
   // We don't want to print an owner for interface types
   const shouldPrintOwner = isObjectType(type);
 
-  const enumValue = context.graphNameToEnumValueName?.[ownerService];
+  const ownerGraphEnumValue = context.graphNameToEnumValueName?.[ownerService];
   assert(
-    enumValue,
+    ownerGraphEnumValue,
     `Unexpected enum value missing for subgraph ${ownerService}`,
   );
   const joinOwnerString = shouldPrintOwner
-    ? `\n  @join__owner(graph: ${enumValue})`
+    ? `\n  @join__owner(graph: ${ownerGraphEnumValue})`
     : '';
 
   return (
@@ -262,12 +262,17 @@ function printTypeJoinDirectives(
     [ownerEntry, ...restEntries]
       .map(([service, keys = []]) =>
         keys
-          .map(
-            (selections) =>
-              `\n  @join__type(graph: ${
-                context.graphNameToEnumValueName?.[service] ?? service
-              }, key: ${printStringLiteral(printFieldSet(selections))})`,
-          )
+          .map((selections) => {
+            const typeGraphEnumValue =
+              context.graphNameToEnumValueName?.[service];
+            assert(
+              typeGraphEnumValue,
+              `Unexpected enum value missing for subgraph ${service}`,
+            );
+            return `\n  @join__type(graph: ${typeGraphEnumValue}, key: ${printStringLiteral(
+              printFieldSet(selections),
+            )})`;
+          })
           .join(''),
       )
       .join('')
@@ -392,18 +397,14 @@ function printJoinFieldDirectives(
     // Because we print `@join__type` directives based on the keys, but only used to
     // look at the owning service here, that meant we would print `@join__field`
     // without a corresponding `@join__type`, which is invalid according to the spec.
-    if (
-      parentType.extensions?.federation?.serviceName &&
-      parentType.extensions?.federation?.keys
-    ) {
-      return (
-        printed +
-        `graph: ${
-          context.graphNameToEnumValueName?.[
-            parentType.extensions?.federation.serviceName
-          ] ?? parentType.extensions?.federation.serviceName
-        })`
+    const { serviceName, keys } = parentType.extensions?.federation;
+    if (serviceName && keys) {
+      const enumValue = context.graphNameToEnumValueName?.[serviceName];
+      assert(
+        enumValue,
+        `Unexpected enum value missing for subgraph ${serviceName}`,
       );
+      return printed + `graph: ${enumValue})`;
     }
     return '';
   }

From 53e0ad50c9d96d70c512242183d37cfcfd17f929 Mon Sep 17 00:00:00 2001
From: Trevor Scheer <trevor.scheer@gmail.com>
Date: Thu, 22 Apr 2021 13:43:24 -0700
Subject: [PATCH 20/20] Update changelogs

---
 federation-js/CHANGELOG.md    | 2 +-
 query-planner-js/CHANGELOG.md | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/federation-js/CHANGELOG.md b/federation-js/CHANGELOG.md
index 4302516d4..576f6092c 100644
--- a/federation-js/CHANGELOG.md
+++ b/federation-js/CHANGELOG.md
@@ -4,7 +4,7 @@
 
 > The changes noted within this `vNEXT` section have not been released yet.  New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development.  When a release is being prepared, a new header will be (manually) created below and the appropriate changes within that release will be moved into the new section.
 
--  _Nothing yet! Stay tuned!_
+- This change is mostly a set of follow-up changes for PR #622. Most of these changes are internal (renaming, etc.). Some noteworthy changes worth mentioning are: a switch to graphql-js's `stripIgnoredCharacters` during field set printing, an update to the `join__Enum` generation algorithm, and some additional assertions. [PR #656](https://github.com/apollographql/federation/pull/656)
 
 ## v0.23.0
 
diff --git a/query-planner-js/CHANGELOG.md b/query-planner-js/CHANGELOG.md
index ff93e9c48..eda3a7361 100644
--- a/query-planner-js/CHANGELOG.md
+++ b/query-planner-js/CHANGELOG.md
@@ -4,6 +4,8 @@
 
 > The changes noted within this `vNEXT` section have not been released yet.  New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development.  When a release is being prepared, a new header will be (manually) created below and the appropriate changes within that release will be moved into the new section.
 
+- This change is mostly a set of follow-up changes for PR #622. Most of these changes are internal (renaming, etc.). Some noteworthy changes worth mentioning are: the splitting of entity and value type metadata types and a conversion of GraphMap to an actual `Map` (which resulted in some additional assertions). [PR #656](https://github.com/apollographql/federation/pull/656)
+
 # v0.1.1
 
 - Remove unnecessary dependency on `@apollo/query-planner-wasm`