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

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(' ');
 }

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/__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 {

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}' : '';
 }