chore: typescript-redux followups

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 { graphNameToEnumValueName } = getJoinDefinitions(
       fixturesWithQuestionableServiceNames,
     );
 
@@ -33,12 +33,12 @@ 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',
+      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',
     });
   })

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 sanitizedServiceNames: 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);
+
+    const existingEntry = sanitizedNameToServiceDefinitions.get(sanitized);
+    if (existingEntry) {
+      sanitizedNameToServiceDefinitions.set(sanitized, [
+        ...existingEntry,
+        service,
+      ]);
+    } else {
+      sanitizedNameToServiceDefinitions.set(sanitized, [service]);
+    }
+  }
 
-    // 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);
-      sanitizedServiceNames[name] = uniquified;
-      return uniquified;
+  // 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 {
-      nameMap.set(toUpper, 1);
-      sanitizedServiceNames[name] = toUpper;
-      return toUpper;
+      for (const [index, service] of services.entries()) {
+        generatedNameToServiceDefinition[`${name}_${index + 1}`] = service;
+      }
     }
   }
 
+  const entries = Object.entries(generatedNameToServiceDefinition);
   return {
-    sanitizedServiceNames,
+    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 }]),
       ),
     }),
   };
@@ -115,8 +139,8 @@ function getJoinOwnerDirective(JoinGraphEnum: GraphQLEnumType) {
   });
 }
 
-export function getJoins(serviceList: ServiceDefinition[]) {
-  const { sanitizedServiceNames, JoinGraphEnum } = getJoinGraphEnum(serviceList);
+export function getJoinDefinitions(serviceList: ServiceDefinition[]) {
+  const { graphNameToEnumValueName, JoinGraphEnum } = getJoinGraphEnum(serviceList);
   const JoinFieldDirective = getJoinFieldDirective(JoinGraphEnum);
   const JoinOwnerDirective = getJoinOwnerDirective(JoinGraphEnum);
 
@@ -135,7 +159,7 @@ export function getJoins(serviceList: ServiceDefinition[]) {
   });
 
   return {
-    sanitizedServiceNames,
+    graphNameToEnumValueName,
     FieldSetScalar,
     JoinTypeDirective,
     JoinFieldDirective,

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 = {
   /**
@@ -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,8 +72,8 @@ export function printSupergraphSdl(
     JoinOwnerDirective,
     JoinGraphEnum,
     JoinGraphDirective,
-    sanitizedServiceNames,
-  } = getJoins(serviceList);
+    graphNameToEnumValueName,
+  } = getJoinDefinitions(serviceList);
 
   schema = new GraphQLSchema({
     ...config,
@@ -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}`,
     );
   }
 

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;
 

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

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 {