Fix value comparison, argument merging, and link feature addition/extraction in composition (#3134)

While reviewing recent code changes that released in `2.9.0`, I noticed
a few bugs in the code, so I've pushed some fixes for them in this PR
(along with some fixes for more ancient bugs).

I'll comment on the code with more details, but to summarize the fixes:
- Value comparison/default value application has been updated to handle
`null`s/`undefined`s better.
- Composition argument strategy code has been refactored to handle
nullability more cleanly (and fix some bugs around nullability).
- Note I've commented out the unused `SUM` strategy, as using it
currently wouldn't yield expected results since directive applications
are deduped before being merged.
- The logic that applies `@link` applications to the supergraph schema
for supergraph specs needed by subgraph directives has been updated to
handle specs that have multiple directives that need composing (the
behavior before was adding spurious directive definitions to the
supergraph schema).
- Some ancient logic around computing directive/type names for
`@core`/`@link` was off.

Author: sachindshinde

.changeset/proud-days-press.md

@@ -0,0 +1,7 @@
+---
+"@apollo/federation-internals": patch
+"@apollo/gateway": patch
+"@apollo/composition": patch
+---
+
+Fix bugs in composition when merging nulls in directive applications and when handling renames.

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,
@@ -219,7 +221,7 @@ describe('composition of directive with non-trivial argument strategies', () =>
     const s = result.schema;
 
     expect(directiveStrings(s.schemaDefinition, name)).toStrictEqual([
-      `@link(url: "https://specs.apollo.dev/${name}/v0.1", import: ["@${name}"])`
+      `@link(url: "https://specs.apollo.dev/${name}/v0.1")`
     ]);
 
     const t = s.type('T') as ObjectType;

composition-js/src/merging/merge.ts

@@ -31,7 +31,6 @@ import {
   CompositeType,
   Subgraphs,
   JOIN_VERSIONS,
-  INACCESSIBLE_VERSIONS,
   NamedSchemaElement,
   errorCauses,
   isObjectType,
@@ -69,7 +68,6 @@ import {
   CoreSpecDefinition,
   FeatureVersion,
   FEDERATION_VERSIONS,
-  InaccessibleSpecDefinition,
   LinkDirectiveArgs,
   sourceIdentity,
   FeatureUrl,
@@ -81,6 +79,10 @@ import {
   isNullableType,
   isFieldDefinition,
   Post20FederationDirectiveDefinition,
+  DirectiveCompositionSpecification,
+  FeatureDefinition,
+  CoreImport,
+  inaccessibleIdentity,
 } from "@apollo/federation-internals";
 import { ASTNode, GraphQLError, DirectiveLocation } from "graphql";
 import {
@@ -345,6 +347,12 @@ interface OverrideArgs {
   label?: string;
 }
 
+interface MergedDirectiveInfo {
+  definition: DirectiveDefinition;
+  argumentsMerger?: ArgumentMerger;
+  staticArgumentTransform?: StaticArgumentsTransform;
+}
+
 class Merger {
   readonly names: readonly string[];
   readonly subgraphsSchema: readonly Schema[];
@@ -353,7 +361,8 @@ class Merger {
   readonly merged: Schema = new Schema();
   readonly subgraphNamesToJoinSpecName: Map<string, string>;
   readonly mergedFederationDirectiveNames = new Set<string>();
-  readonly mergedFederationDirectiveInSupergraph = new Map<string, { definition: DirectiveDefinition, argumentsMerger?: ArgumentMerger, staticArgumentTransform?: StaticArgumentsTransform }>();
+  readonly mergedFederationDirectiveInSupergraphByDirectiveName =
+    new Map<string, MergedDirectiveInfo>();
   readonly enumUsages = new Map<string, EnumTypeUsage>();
   private composeDirectiveManager: ComposeDirectiveManager;
   private mismatchReporter: MismatchReporter;
@@ -364,7 +373,7 @@ class Merger {
   }[];
   private joinSpec: JoinSpecDefinition;
   private linkSpec: CoreSpecDefinition;
-  private inaccessibleSpec: InaccessibleSpecDefinition;
+  private inaccessibleDirectiveInSupergraph?: DirectiveDefinition;
   private latestFedVersionUsed: FeatureVersion;
   private joinDirectiveIdentityURLs = new Set<string>();
   private schemaToImportNameToFeatureUrl = new Map<Schema, Map<string, FeatureUrl>>();
@@ -375,7 +384,6 @@ class Merger {
     this.latestFedVersionUsed = this.getLatestFederationVersionUsed();
     this.joinSpec = JOIN_VERSIONS.getMinimumRequiredVersion(this.latestFedVersionUsed);
     this.linkSpec = LINK_VERSIONS.getMinimumRequiredVersion(this.latestFedVersionUsed);
-    this.inaccessibleSpec = INACCESSIBLE_VERSIONS.getMinimumRequiredVersion(this.latestFedVersionUsed);
     this.fieldsWithFromContext = this.getFieldsWithFromContextDirective();
     this.fieldsWithOverride = this.getFieldsWithOverrideDirective();
 
@@ -470,59 +478,127 @@ class Merger {
     assert(errors.length === 0, "We shouldn't have errors adding the join spec to the (still empty) supergraph schema");
 
     const directivesMergeInfo = collectCoreDirectivesToCompose(this.subgraphs);
-    for (const mergeInfo of directivesMergeInfo) {
-      this.validateAndMaybeAddSpec(mergeInfo);
-    }
-
+    this.validateAndMaybeAddSpecs(directivesMergeInfo);
     return this.joinSpec.populateGraphEnum(this.merged, this.subgraphs);
   }
 
-  private validateAndMaybeAddSpec({url, name, definitionsPerSubgraph, compositionSpec}: CoreDirectiveInSubgraphs) {
-    // Not composition specification means that it shouldn't be composed.
-    if (!compositionSpec) {
-      return;
-    }
-
-    let nameInSupergraph: string | undefined;
-    for (const subgraph of this.subgraphs) {
-      const directive = definitionsPerSubgraph.get(subgraph.name);
-      if (!directive) {
-        continue;
+  private validateAndMaybeAddSpecs(directivesMergeInfo: CoreDirectiveInSubgraphs[]) {
+    const supergraphInfoByIdentity = new Map<
+      string,
+      {
+        specInSupergraph: FeatureDefinition;
+        directives: {
+          nameInFeature: string;
+          nameInSupergraph: string;
+          compositionSpec: DirectiveCompositionSpecification;
+        }[];
       }
+    >;
 
-      if (!nameInSupergraph) {
-        nameInSupergraph = directive.name;
-      } else if (nameInSupergraph !== directive.name) {
-        this.mismatchReporter.reportMismatchError(
-          ERRORS.LINK_IMPORT_NAME_MISMATCH,
-          `The "@${name}" directive (from ${url}) is imported with mismatched name between subgraphs: it is imported as `,
-          directive,
-          sourcesFromArray(this.subgraphs.values().map((s) => definitionsPerSubgraph.get(s.name))),
-          (def) => `"@${def.name}"`,
-        );
+    for (const {url, name, definitionsPerSubgraph, compositionSpec} of directivesMergeInfo) {
+      // No composition specification means that it shouldn't be composed.
+      if (!compositionSpec) {
         return;
       }
+
+      let nameInSupergraph: string | undefined;
+      for (const subgraph of this.subgraphs) {
+        const directive = definitionsPerSubgraph.get(subgraph.name);
+        if (!directive) {
+          continue;
+        }
+
+        if (!nameInSupergraph) {
+          nameInSupergraph = directive.name;
+        } else if (nameInSupergraph !== directive.name) {
+          this.mismatchReporter.reportMismatchError(
+            ERRORS.LINK_IMPORT_NAME_MISMATCH,
+            `The "@${name}" directive (from ${url}) is imported with mismatched name between subgraphs: it is imported as `,
+            directive,
+            sourcesFromArray(this.subgraphs.values().map((s) => definitionsPerSubgraph.get(s.name))),
+            (def) => `"@${def.name}"`,
+          );
+          return;
+        }
+      }
+
+      // If we get here with `nameInSupergraph` unset, it means there is no usage for the directive at all and we
+      // don't bother adding the spec to the supergraph.
+      if (nameInSupergraph) {
+        const specInSupergraph = compositionSpec.supergraphSpecification(this.latestFedVersionUsed);
+        let supergraphInfo = supergraphInfoByIdentity.get(specInSupergraph.url.identity);
+        if (supergraphInfo) {
+          assert(
+            specInSupergraph.url.equals(supergraphInfo.specInSupergraph.url),
+            `Spec ${specInSupergraph.url} directives disagree on version for supergraph`,
+          );
+        } else {
+          supergraphInfo = {
+            specInSupergraph,
+            directives: [],
+          };
+          supergraphInfoByIdentity.set(specInSupergraph.url.identity, supergraphInfo);
+        }
+        supergraphInfo.directives.push({
+          nameInFeature: name,
+          nameInSupergraph,
+          compositionSpec,
+        });
+      }
     }
 
-    // If we get here with `nameInSupergraph` unset, it means there is no usage for the directive at all and we
-    // don't bother adding the spec to the supergraph.
-    if (nameInSupergraph) {
-      const specInSupergraph = compositionSpec.supergraphSpecification(this.latestFedVersionUsed);
-      const errors = this.linkSpec.applyFeatureAsLink(this.merged, specInSupergraph, specInSupergraph.defaultCorePurpose, [{ name, as: name === nameInSupergraph ? undefined : nameInSupergraph }], );
-      assert(errors.length === 0, "We shouldn't have errors adding the join spec to the (still empty) supergraph schema");
-      const feature = this.merged?.coreFeatures?.getByIdentity(specInSupergraph.url.identity);
+    for (const { specInSupergraph, directives } of supergraphInfoByIdentity.values()) {
+      const imports: CoreImport[] = [];
+      for (const { nameInFeature, nameInSupergraph } of directives) {
+        const defaultNameInSupergraph = CoreFeature.directiveNameInSchemaForCoreArguments(
+          specInSupergraph.url,
+          specInSupergraph.url.name,
+          [],
+          nameInFeature,
+        );
+        if (nameInSupergraph !== defaultNameInSupergraph) {
+          imports.push(nameInFeature === nameInSupergraph
+            ? { name: `@${nameInFeature}` }
+            : { name: `@${nameInFeature}`, as: `@${nameInSupergraph}` }
+          );
+        }
+      }
+      const errors = this.linkSpec.applyFeatureToSchema(
+        this.merged,
+        specInSupergraph,
+        undefined,
+        specInSupergraph.defaultCorePurpose,
+        imports,
+      );
+      assert(
+        errors.length === 0,
+        "We shouldn't have errors adding the join spec to the (still empty) supergraph schema"
+      );
+      const feature = this.merged.coreFeatures?.getByIdentity(specInSupergraph.url.identity);
       assert(feature, 'Should have found the feature we just added');
-      const argumentsMerger = compositionSpec.argumentsMerger?.call(null, this.merged, feature);
-      if (argumentsMerger instanceof GraphQLError) {
-        // That would mean we made a mistake in the declaration of a hard-coded directive, so we just throw right away so this can be caught and corrected.
-        throw argumentsMerger;
-      }
-      this.mergedFederationDirectiveNames.add(nameInSupergraph);
-      this.mergedFederationDirectiveInSupergraph.set(name, {
-        definition: this.merged.directive(nameInSupergraph)!,
-        argumentsMerger,
-        staticArgumentTransform: compositionSpec.staticArgumentTransform,
-      });
+      for (const { nameInFeature, nameInSupergraph, compositionSpec } of directives) {
+        const argumentsMerger = compositionSpec.argumentsMerger?.call(null, this.merged, feature);
+        if (argumentsMerger instanceof GraphQLError) {
+          // That would mean we made a mistake in the declaration of a hard-coded directive,
+          // so we just throw right away so this can be caught and corrected.
+          throw argumentsMerger;
+        }
+        this.mergedFederationDirectiveNames.add(nameInSupergraph);
+        this.mergedFederationDirectiveInSupergraphByDirectiveName.set(nameInSupergraph, {
+          definition: this.merged.directive(nameInSupergraph)!,
+          argumentsMerger,
+          staticArgumentTransform: compositionSpec.staticArgumentTransform,
+        });
+        // If we encounter the @inaccessible directive, we need to record its
+        // definition so certain merge validations that care about @inaccessible
+        // can act accordingly.
+        if (
+          specInSupergraph.identity === inaccessibleIdentity
+            && nameInFeature === specInSupergraph.url.name
+        ) {
+          this.inaccessibleDirectiveInSupergraph = this.merged.directive(nameInSupergraph)!;
+        }
+      }
     }
   }
 
@@ -2464,8 +2540,8 @@ class Merger {
     this.recordAppliedDirectivesToMerge(valueSources, value);
     this.addJoinEnumValue(valueSources, value);
 
-    const inaccessibleInSupergraph = this.mergedFederationDirectiveInSupergraph.get(this.inaccessibleSpec.inaccessibleDirectiveSpec.name);
-    const isInaccessible = inaccessibleInSupergraph && value.hasAppliedDirective(inaccessibleInSupergraph.definition);
+    const isInaccessible = this.inaccessibleDirectiveInSupergraph
+      && value.hasAppliedDirective(this.inaccessibleDirectiveInSupergraph);
     // The merging strategy depends on the enum type usage:
     //  - if it is _only_ used in position of Input type, we merge it with an "intersection" strategy (like other input types/things).
     //  - if it is _only_ used in position of Output type, we merge it with an "union" strategy (like other output types/things).
@@ -2562,8 +2638,6 @@ class Merger {
   }
 
   private mergeInput(inputSources: Sources<InputObjectType>, dest: InputObjectType) {
-    const inaccessibleInSupergraph = this.mergedFederationDirectiveInSupergraph.get(this.inaccessibleSpec.inaccessibleDirectiveSpec.name);
-
     // Like for other inputs, we add all the fields found in any subgraphs initially as a simple mean to have a complete list of
     // field to iterate over, but we will remove those that are not in all subgraphs.
     const added = this.addFieldsShallow(inputSources, dest);
@@ -2572,7 +2646,8 @@ class Merger {
       // compatibility between definitions and 2) we actually want to see if the result is marked inaccessible or not and it makes
       // that easier.
       this.mergeInputField(subgraphFields, destField);
-      const isInaccessible = inaccessibleInSupergraph && destField.hasAppliedDirective(inaccessibleInSupergraph.definition);
+      const isInaccessible = this.inaccessibleDirectiveInSupergraph
+        && destField.hasAppliedDirective(this.inaccessibleDirectiveInSupergraph);
       // Note that if the field is manually marked @inaccessible, we can always accept it to be inconsistent between subgraphs since
       // it won't be exposed in the API, and we don't hint about it because we're just doing what the user is explicitely asking.
       if (!isInaccessible && someSources(subgraphFields, field => !field)) {
@@ -2840,8 +2915,7 @@ class Merger {
   // is @inaccessible, which is necessary to exist in the supergraph for EnumValues to properly
   // determine whether the fact that a value is both input / output will matter
   private recordAppliedDirectivesToMerge(sources: Sources<SchemaElement<any, any>>, dest: SchemaElement<any, any>) {
-    const inaccessibleInSupergraph = this.mergedFederationDirectiveInSupergraph.get(this.inaccessibleSpec.inaccessibleDirectiveSpec.name);
-    const inaccessibleName = inaccessibleInSupergraph?.definition.name;
+    const inaccessibleName = this.inaccessibleDirectiveInSupergraph?.name;
     const names = this.gatherAppliedDirectiveNames(sources);
 
     if (inaccessibleName && names.has(inaccessibleName)) {
@@ -2905,7 +2979,7 @@ class Merger {
       return;
     }
 
-    const directiveInSupergraph = this.mergedFederationDirectiveInSupergraph.get(name);
+    const directiveInSupergraph = this.mergedFederationDirectiveInSupergraphByDirectiveName.get(name);
 
     if (dest.schema().directive(name)?.repeatable) {
       // For repeatable directives, we simply include each application found but with exact duplicates removed
@@ -2945,7 +3019,7 @@ class Merger {
       if (differentApplications.length === 1) {
         dest.applyDirective(name, differentApplications[0].arguments(false));
       } else {
-        const info = this.mergedFederationDirectiveInSupergraph.get(name);
+        const info = this.mergedFederationDirectiveInSupergraphByDirectiveName.get(name);
         if (info && info.argumentsMerger) {
           const mergedArguments = Object.create(null);
           const applicationsArguments = differentApplications.map((a) => a.arguments(true));

gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts

@@ -149,7 +149,7 @@ describe('lifecycle hooks', () => {
     // the supergraph (even just formatting differences), this ID will change
     // and this test will have to updated.
     expect(secondCall[0]!.compositionId).toMatchInlineSnapshot(
-      `"4aa2278e35df345ff5959a30546d2e9ef9e997204b4ffee4a42344b578b36068"`,
+      `"6dc1bde2b9818fabec62208c5d8825abaa1bae89635fa6f3a5ffea7b78fc6d82"`,
     );
     // second call should have previous info in the second arg
     expect(secondCall[1]!.compositionId).toEqual(expectedFirstId);

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