fix(composition): Add missing @link validations (#3430)

This PR adds `@link` validations to detect invalid alias names and alias/import conflicts. To maintain backwards compatibility, we do allow some exceptions, and we also auto-generate supergraph spec aliases when conflicts are detected.

Author: sachindshinde

.changeset/spotty-clocks-collect.md

@@ -0,0 +1,9 @@
+---
+"@apollo/federation-internals": minor
+---
+
+Add validations for `@link` usages to prevent name conflicts.
+
+This change helps to avoid ambiguity for downstream `@link`-consuming code, which previously may have found different results for a spec schema element depending on search order. If your composition fails after this change, please rename conflicting elements via `@link(import:)` and conflicting specs/features via `@link(as:)`.
+
+Note that if you were declaring `@link`s for the `https://specs.apollo.dev/tag` or `https://specs.apollo.dev/inaccessible` specs in your subgraph schema, you will need to instead import `@tag` and `@inaccessible` from the `https://specs.apollo.dev/federation` spec. This previous pattern only succeeded due to a now-fixed bug and is fragile/may lead to undesirable behavior.

composition-js/src/__tests__/__snapshots__/compose.composeDirective.test.ts.snap

@@ -4,7 +4,7 @@ exports[`composing custom core directives custom tag directive works when federa
 "schema
   @link(url: \\"https://specs.apollo.dev/link/v1.0\\")
   @link(url: \\"https://specs.apollo.dev/join/v0.3\\", for: EXECUTION)
-  @link(url: \\"https://specs.apollo.dev/tag/v0.3\\", import: [{name: \\"@tag\\", as: \\"@mytag\\"}])
+  @link(url: \\"https://specs.apollo.dev/tag/v0.3\\", as: \\"_0tag\\", import: [{name: \\"@tag\\", as: \\"@mytag\\"}])
   @link(url: \\"https://custom.dev/tag/v1.0\\", import: [\\"@tag\\"])
 {
   query: Query

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

@@ -79,10 +79,11 @@ const expectDirectiveDefinition = (schema: Schema, name: string, locations: Dire
   expect(directive?.arguments().map(arg => arg.name)).toEqual(args);
 };
 
-const expectCoreFeature = (schema: Schema, identity: string, version: string, imports: { [key: string]: string}[]) => {
+const expectCoreFeature = (schema: Schema, identity: string, version: string, imports: { [key: string]: string}[], nameInSchema?: string) => {
   const feature = schema.coreFeatures?.getByIdentity(identity);
   expect(feature?.url.toString()).toBe(`${identity}/v${version}`);
   expect(feature?.imports).toEqual(imports);
+  expect(feature?.nameInSchema).toEqual(nameInSchema ?? feature?.url?.name);
 };
 
 describe('composing custom core directives', () => {
@@ -791,23 +792,23 @@ describe('composing custom core directives', () => {
   it('composed directive must be the same original directive in all subgraphs', () => {
     const subgraphA = generateSubgraph({
       name: 'subgraphA',
-      linkText: '@link(url: "https://specs.apollo.dev/foo/v1.0", import: ["@foo"])',
-      composeText: '@composeDirective(name: "@foo")',
-      directiveText: 'directive @foo(name: String!) on FIELD_DEFINITION',
-      usage: '@foo(name: "a")',
+      linkText: '@link(url: "https://specs.apollo.dev/foo/v1.0", import: ["@baz"])',
+      composeText: '@composeDirective(name: "@baz")',
+      directiveText: 'directive @baz(name: String!) on FIELD_DEFINITION',
+      usage: '@baz(name: "a")',
     });
     const subgraphB = generateSubgraph({
       name: 'subgraphB',
-      linkText: '@link(url: "https://specs.apollo.dev/foo/v1.0", import: [{ name: "@bar", as: "@foo" }])',
-      composeText: '@composeDirective(name: "@foo")',
-      directiveText: 'directive @foo(name: String!) on FIELD_DEFINITION',
-      usage: '@foo(name: "a")',
+      linkText: '@link(url: "https://specs.apollo.dev/foo/v1.0", import: [{ name: "@bar", as: "@baz" }])',
+      composeText: '@composeDirective(name: "@baz")',
+      directiveText: 'directive @baz(name: String!) on FIELD_DEFINITION',
+      usage: '@baz(name: "a")',
     });
 
     const result = composeServices([subgraphA, subgraphB]);
     expect(hints(result)).toEqual([]);
     expect(errors(result)).toStrictEqual([
-      ['DIRECTIVE_COMPOSITION_ERROR', `Composed directive "@foo" does not refer to the same directive in every subgraph`],
+      ['DIRECTIVE_COMPOSITION_ERROR', `Composed directive "@baz" does not refer to the same directive in every subgraph`],
     ]);
   });
 
@@ -874,8 +875,8 @@ describe('composing custom core directives', () => {
     const result = composeServices([subgraphA, subgraphB]);
     expect(errors(result)).toStrictEqual([
       [
-        'DIRECTIVE_COMPOSITION_ERROR',
-        `Directive "${directive}" in subgraph "subgraphA" cannot be composed because it is not a member of a core feature`,
+        'INVALID_LINK_DIRECTIVE_USAGE',
+        `Cannot import "@foo" as "${directive}" from feature "https://specs.apollo.dev/foo" since it can be confused with a namespaced name from another linked feature "https://specs.apollo.dev/join". Please rename the import or feature to avoid conflicts via "as".`,
       ]
     ]);
   });
@@ -1004,7 +1005,7 @@ describe('composing custom core directives', () => {
       extend schema
         @link(url: "https://specs.apollo.dev/federation/v2.1", import: ["@key", "@composeDirective", "@tag"])
         @link(url: "https://specs.apollo.dev/link/v1.0")
-        @link(url: "https://custom.dev/tag/v1.0", import: [{ name: "@tag", as: "@mytag"}])
+        @link(url: "https://custom.dev/tag/v1.0", as: "mytag", import: [{ name: "@tag", as: "@mytag"}])
         @composeDirective(name: "@mytag")
 
         directive @mytag(name: String!, prop: String!) on FIELD_DEFINITION | OBJECT
@@ -1045,7 +1046,8 @@ describe('composing custom core directives', () => {
     expectDirectiveDefinition(schema, 'mytag', [DirectiveLocation.FIELD_DEFINITION, DirectiveLocation.OBJECT], ['name', 'prop']);
     expectDirectiveOnElement(schema, 'User.a', 'mytag', { name: 'a', prop: 'b' });
     expectDirectiveOnElement(schema, 'User.b', 'tag', { name: 'c' });
-    expectCoreFeature(schema, 'https://custom.dev/tag', '1.0', [{ name: '@tag', as: '@mytag' }]);
+    expectCoreFeature(schema, 'https://custom.dev/tag', '1.0', [{ name: '@tag', as: '@mytag' }], '_0tag');
+    expectCoreFeature(schema, 'https://specs.apollo.dev/tag', '0.3', []);
   });
 
   it('custom tag directive works when federation tag is renamed', () => {
@@ -1099,10 +1101,7 @@ describe('composing custom core directives', () => {
     expectDirectiveOnElement(schema, 'User.b', 'mytag', { name: 'c' });
 
     expectCoreFeature(schema, 'https://custom.dev/tag', '1.0', [{ name: '@tag' }]);
-    const feature = schema.coreFeatures?.getByIdentity('https://specs.apollo.dev/tag');
-    expect(feature?.url.toString()).toBe('https://specs.apollo.dev/tag/v0.3');
-    expect(feature?.imports).toEqual([{ name: '@tag', as: '@mytag' }]);
-    expect(feature?.nameInSchema).toEqual('tag');
+    expectCoreFeature(schema, 'https://specs.apollo.dev/tag', '0.3', [{ name: '@tag', as: '@mytag' }], '_0tag');
     expect(printSchema(schema)).toMatchSnapshot();
   });
 

composition-js/src/__tests__/composeFed1Subgraphs.test.ts

@@ -356,6 +356,56 @@ describe('validations', () => {
     expect(subgraphCAST.subgraph).toBe("subgraphC");
     expect(subgraphCAST.kind).toBe("ObjectTypeExtension");
   });
+
+  it('errors when trying to use supergraph tag spec', () => {
+    const subgraphA= {
+      typeDefs: gql`
+        schema
+          @link(url: "https://specs.apollo.dev/link/v1.0")
+          @link(url: "https://specs.apollo.dev/tag/v0.2")
+        {
+          query: Query
+        }
+
+        type Query {
+          q: Int
+        }
+      `,
+      name: 'subgraphA',
+    };
+
+    const result = composeServices([subgraphA]);
+
+    expect(result.errors).toBeDefined();
+    expect(errors(result)).toStrictEqual([
+      ['INVALID_LINK_DIRECTIVE_USAGE', '[subgraphA] Please import "@tag" from the feature "https://specs.apollo.dev/federation" instead of using "https://specs.apollo.dev/tag" to avoid potential unexpected behavior in the future.'],
+    ]);
+  });
+
+  it('errors when trying to use supergraph inaccessible spec', () => {
+    const subgraphA= {
+      typeDefs: gql`
+        schema
+          @link(url: "https://specs.apollo.dev/link/v1.0")
+          @link(url: "https://specs.apollo.dev/inaccessible/v0.2")
+        {
+          query: Query
+        }
+
+        type Query {
+          q: Int
+        }
+      `,
+      name: 'subgraphA',
+    };
+
+    const result = composeServices([subgraphA]);
+
+    expect(result.errors).toBeDefined();
+    expect(errors(result)).toStrictEqual([
+      ['INVALID_LINK_DIRECTIVE_USAGE', '[subgraphA] Please import "@inaccessible" from the feature "https://specs.apollo.dev/federation" instead of using "https://specs.apollo.dev/inaccessible" to avoid potential unexpected behavior in the future.'],
+    ]);
+  });
 });
 
 describe('shareable', () => {

composition-js/src/composeDirectiveManager.ts

@@ -286,8 +286,13 @@ export class ComposeDirectiveManager {
           if (featureDetails) {
             const identity = featureDetails.feature.url.identity;
 
-            // make sure that core feature is not blacklisted
-            if (DISALLOWED_IDENTITIES.includes(identity)) {
+            if (featureDetails.nameInFeature === null) {
+              this.pushError(ERRORS.DIRECTIVE_COMPOSITION_ERROR.err(
+                `Directive "@${name}" in subgraph "${sg.name}" cannot be composed because it is imported to a different name`,
+                { nodes: composeInstance.sourceAST },
+              ));
+            } else if (DISALLOWED_IDENTITIES.includes(identity)) {
+              // make sure that core feature is not blacklisted
               this.forFederationDirective(sg, composeInstance, directive);
             } else if (tagNamesInSubgraphs.includes(name)) {
               const subgraphs: string[] = [];

composition-js/src/merging/coreDirectiveCollector.ts

@@ -21,7 +21,7 @@ export function collectCoreDirectivesToCompose(
       const source = features.sourceFeature(directive);
       // We ignore directives that are not "core" ones, or the ones that are defined but unused (note that this
       // happen to ignore execution directives as a by-product)
-      if (!source || directive.applications().size === 0) {
+      if (!source || source.nameInFeature === null || directive.applications().size === 0) {
         continue;
       }