Some tweaks to latest changes for cost spec

composition-js/src/__tests__/compose.directiveArgumentMergeStrategies.test.ts

@@ -89,17 +89,19 @@ describe('composition of directive with non-trivial argument strategies', () =>
       t: 2, k: 1, b: 4,
     },
   }, {
-    name: 'sum',
-    type: (schema: Schema) => new NonNullType(schema.intType()),
-    compositionStrategy: ARGUMENT_COMPOSITION_STRATEGIES.SUM,
-    argValues: {
-      s1: { t: 3, k: 1 },
-      s2: { t: 2, k: 5, b: 4 },
-    },
-    resultValues: {
-      t: 5, k: 6, b: 4,
-    },
-  }, {
+  // NOTE: See the note for the SUM strategy in argumentCompositionStrategies.ts
+  // for more information on why this is commented out.
+  //   name: 'sum',
+  //   type: (schema: Schema) => new NonNullType(schema.intType()),
+  //   compositionStrategy: ARGUMENT_COMPOSITION_STRATEGIES.SUM,
+  //   argValues: {
+  //     s1: { t: 3, k: 1 },
+  //     s2: { t: 2, k: 5, b: 4 },
+  //   },
+  //   resultValues: {
+  //     t: 5, k: 6, b: 4,
+  //   },
+  // }, {
     name: 'intersection',
     type: (schema: Schema) => new NonNullType(new ListType(new NonNullType(schema.stringType()))),
     compositionStrategy: ARGUMENT_COMPOSITION_STRATEGIES.INTERSECTION,

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

@@ -414,5 +414,6 @@ describe('objectEquals tests', () => {
     expect(valueEquals({ foo: 'foo', bar: undefined }, { foo: 'foo' })).toBe(
       false,
     );
+    expect(valueEquals({}, null)).toBe(false);
   });
 });

internals-js/src/argumentCompositionStrategies.ts

@@ -13,87 +13,105 @@ export type ArgumentCompositionStrategy = {
 function supportFixedTypes(types: (schema: Schema) => InputType[]): TypeSupportValidator {
   return (schema, type) => {
     const supported = types(schema);
-    if (!supported.some((t) => sameType(t, type))) {
-      return { valid: false, supportedMsg: `type(s) ${supported.join(', ')}` };
-    }
-    return { valid: true };
+    return supported.some((t) => sameType(t, type))
+      ? { valid: true }
+      : { valid: false, supportedMsg: `type(s) ${supported.join(', ')}` };
   };
 }
 
 function supportAnyNonNullArray(): TypeSupportValidator {
-  return (_, type) => {
-    if (!isNonNullType(type) || !isListType(type.ofType)) {
-      return { valid: false, supportedMsg: 'non nullable list types of any type'};
-    }
-    return { valid: true };
+  return (_, type) => isNonNullType(type) && isListType(type.ofType)
+    ? { valid: true }
+    : { valid: false, supportedMsg: 'non nullable list types of any type' }
+}
+
+function supportAnyArray(): TypeSupportValidator {
+  return (_, type) => isListType(type) || (isNonNullType(type) && isListType(type.ofType))
+    ? { valid: true }
+    : { valid: false, supportedMsg: 'list types of any type' };
+}
+
+// NOTE: This function makes the assumption that for the directive argument
+// being merged, it is not "nullable with non-null default" in the supergraph
+// schema (this kind of type/default combo is confusing and should be avoided,
+// if possible). This assumption allows this function to replace null with
+// undefined, which makes for a cleaner supergraph schema.
+function mergeNullableValues<T>(
+  mergeValues: (values: T[]) => T
+): (values: (T | null | undefined)[]) => T | undefined {
+  return (values: (T | null | undefined)[]) => {
+    const nonNullValues = values.filter((v) => v !== null && v !== undefined) as T[];
+    return nonNullValues.length > 0
+      ? mergeValues(nonNullValues)
+      : undefined;
   };
 }
 
+function unionValues(values: any[]): any {
+  return values.reduce((acc, next) => {
+    const newValues = next.filter((v1: any) => !acc.some((v2: any) => valueEquals(v1, v2)));
+    return acc.concat(newValues);
+  }, []);
+}
+
 export const ARGUMENT_COMPOSITION_STRATEGIES = {
   MAX: {
     name: 'MAX',
     isTypeSupported: supportFixedTypes((schema: Schema) => [new NonNullType(schema.intType())]),
-    mergeValues: (values: any[]) => Math.max(...values),
+    mergeValues: (values: number[]) => Math.max(...values),
   },
   MIN: {
     name: 'MIN',
     isTypeSupported: supportFixedTypes((schema: Schema) => [new NonNullType(schema.intType())]),
-    mergeValues: (values: any[]) => Math.min(...values),
-  },
-  SUM: {
-    name: 'SUM',
-    isTypeSupported: supportFixedTypes((schema: Schema) => [new NonNullType(schema.intType())]),
-    mergeValues: (values: any[]) => values.reduce((acc, val) => acc + val, 0),
+    mergeValues: (values: number[]) => Math.min(...values),
   },
+  // NOTE: This doesn't work today because directive applications are de-duped
+  // before being merged, we'd need to modify merge logic if we need this kind
+  // of behavior.
+  // SUM: {
+  //   name: 'SUM',
+  //   isTypeSupported: supportFixedTypes((schema: Schema) => [new NonNullType(schema.intType())]),
+  //   mergeValues: (values: any[]) => values.reduce((acc, val) => acc + val, 0),
+  // },
   INTERSECTION: {
     name: 'INTERSECTION',
     isTypeSupported: supportAnyNonNullArray(),
-    mergeValues: (values: any[]) => values.reduce((acc, val) => acc.filter((v1: any) => val.some((v2: any) => valueEquals(v1, v2))), values[0]),
+    mergeValues: (values: any[]) => values.reduce((acc, next) => {
+      if (acc === undefined) {
+        return next;
+      } else {
+        return acc.filter((v1: any) => next.some((v2: any) => valueEquals(v1, v2)));
+      }  
+    }, undefined) ?? [],
   },
   UNION: {
     name: 'UNION',
     isTypeSupported: supportAnyNonNullArray(),
-    mergeValues: (values: any[]) =>
-      values.reduce((acc, val) => {
-        const newValues = val.filter((v1: any) => !acc.some((v2: any) => valueEquals(v1, v2)));
-        return acc.concat(newValues);
-      }, []),
+    mergeValues: unionValues,
   },
   NULLABLE_AND: {
     name: 'NULLABLE_AND',
-    isTypeSupported: supportFixedTypes((schema: Schema) => [schema.booleanType()]),
-    mergeValues: (values: (boolean | null | undefined)[]) => values.reduce((acc, next) => {
-      if (acc === null || acc === undefined) {
-        return next;
-      } else if (next === null || next === undefined) {
-        return acc;
-      } else {
-        return acc && next;
-      }
-    }, undefined),
+    isTypeSupported: supportFixedTypes((schema: Schema) => [
+      schema.booleanType(),
+      new NonNullType(schema.booleanType())
+    ]),
+    mergeValues: mergeNullableValues(
+      (values: boolean[]) => values.every((v) => v)
+    ),
   },
   NULLABLE_MAX: {
     name: 'NULLABLE_MAX',
-    isTypeSupported: supportFixedTypes((schema: Schema) => [schema.intType(), new NonNullType(schema.intType())]),
-    mergeValues: (values: any[]) => values.reduce((a: any, b: any) => a !== undefined && b !== undefined ? Math.max(a, b) : a ?? b, undefined),
+    isTypeSupported: supportFixedTypes((schema: Schema) => [
+      schema.intType(),
+      new NonNullType(schema.intType())
+    ]),
+    mergeValues: mergeNullableValues(
+      (values: number[]) => Math.max(...values)
+    )
   },
   NULLABLE_UNION: {
     name: 'NULLABLE_UNION',
-    isTypeSupported: (_: Schema, type: InputType) => ({ valid: isListType(type) }),
-    mergeValues: (values: any[]) => {
-      if (values.every((v) => v === undefined)) {
-        return undefined;
-      }
-
-      const combined = new Set();
-      for (const subgraphValues of values) {
-        if (Array.isArray(subgraphValues)) {
-          for (const value of subgraphValues) {
-            combined.add(value);
-          }
-        }
-      }
-      return Array.from(combined);
-    }
+    isTypeSupported: supportAnyArray(),
+    mergeValues: mergeNullableValues(unionValues),
   }
 }

internals-js/src/definitions.ts

@@ -1064,7 +1064,7 @@ export class CoreFeatures {
       const feature = this.byAlias.get(splitted[0]);
       return feature ? {
         feature,
-        nameInFeature: splitted[1],
+        nameInFeature: splitted.slice(1).join('__'),
         isImported: false,
       } : undefined;
     } else {
@@ -1076,7 +1076,7 @@ export class CoreFeatures {
           if ((as ?? name) === importName) {
             return {
               feature,
-              nameInFeature: name.slice(1),
+              nameInFeature: isDirective ? name.slice(1) : name,
               isImported: true,
             };
           }
@@ -1088,8 +1088,8 @@ export class CoreFeatures {
       if (directFeature && isDirective) {
         return {
           feature: directFeature,
-          nameInFeature: directFeature.imports.find(imp => imp.as === `@${element.name}`)?.name.slice(1) ?? element.name,
-          isImported: true,
+          nameInFeature: element.name,
+          isImported: false,
         };
       }