Creation of @OverRide directive along with relevant tests (#1484)

* Adding @OverRide for fields * only put external on overridden field if it is used (i.e. referenced by @provides, @requires) or referenced by an @key * if overridden directive does not have external, we want to filter it from consideration rather than allowing it to be used in the join * adding FieldMergeContext rather than filtering or adding external to subgraphs directly * adding lots of relevant tests Co-authored-by: Sylvain Lebresne <sylvain@apollographql.com>
apollographql · Apr 1, 2022 · 9050f03 · 9050f03
1 parent d2a5ae7
commit 9050f03
Show file tree

Hide file tree

Showing 31 changed files with 1,416 additions and 96 deletions.
diff --git a/composition-js/CHANGELOG.md b/composition-js/CHANGELOG.md
@@ -10,6 +10,7 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The
 - Optimize composition validation when many entities spans many subgraphs [PR #1653](https://github.com/apollographql/federation/pull/1653).
 - Support for Node 17 [PR #1541](https://github.com/apollographql/federation/pull/1541).
 - Adds Support for `@tag/v0.2`, which allows the `@tag` directive to be additionally placed on arguments, scalars, enums, enum values, input objects, and input object fields. [PR #1652](https://github.com/apollographql/federation/pull/1652).
+- Adds support for the `@override` directive on fields to indicate that a field should be moved from one subgraph to another. [PR #1484](https://github.com/apollographql/federation/pull/1484)
 
 ## v2.0.0-preview.8
 

diff --git a/composition-js/src/__tests__/compose.test.ts b/composition-js/src/__tests__/compose.test.ts
@@ -4,18 +4,18 @@ import gql from 'graphql-tag';
 import './matchers';
 import { print } from 'graphql';
 
-function assertCompositionSuccess(r: CompositionResult): asserts r is CompositionSuccess {
+export function assertCompositionSuccess(r: CompositionResult): asserts r is CompositionSuccess {
   if (r.errors) {
     throw new Error(`Expected composition to succeed but got errors:\n${r.errors.join('\n\n')}`);
   }
 }
 
-function errors(r: CompositionResult): [string, string][] {
+export function errors(r: CompositionResult): [string, string][] {
   return r.errors?.map(e => [e.extensions.code as string, e.message]) ?? [];
 }
 
 // Returns [the supergraph schema, its api schema, the extracted subgraphs]
-function schemas(result: CompositionSuccess): [Schema, Schema, Subgraphs] {
+export function schemas(result: CompositionSuccess): [Schema, Schema, Subgraphs] {
   // Note that we could user `result.schema`, but reparsing to ensure we don't lose anything with printing/parsing.
   const schema = buildSchema(result.supergraphSdl);
   expect(schema.isCoreSchema()).toBeTruthy();
@@ -74,7 +74,7 @@ describe('composition', () => {
         query: Query
       }
 
-      directive @join__field(graph: join__Graph!, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION
+      directive @join__field(graph: join__Graph!, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION
 
       directive @join__graph(name: String!, url: String!) on ENUM_VALUE
 
@@ -2025,7 +2025,7 @@ describe('composition', () => {
         query: Query
       }
 
-      directive @join__field(graph: join__Graph!, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION
+      directive @join__field(graph: join__Graph!, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION
 
       directive @join__graph(name: String!, url: String!) on ENUM_VALUE
 

diff --git a/composition-js/src/__tests__/composeFed1Subgraphs.test.ts b/composition-js/src/__tests__/composeFed1Subgraphs.test.ts
@@ -15,7 +15,7 @@ function errors(r: CompositionResult): [string, string][] {
 
 // Returns [the supergraph schema, its api schema, the extracted subgraphs]
 function schemas(result: CompositionSuccess): [Schema, Schema, Subgraphs] {
-  // Note that we could user `result.schema`, but reparsing to ensure we don't lose anything with printing/parsing.
+  // Note that we could user `result.schema`, but re-parsing to ensure we don't lose anything with printing/parsing.
   const schema = buildSchema(result.supergraphSdl);
   expect(schema.isCoreSchema()).toBeTruthy();
   return [schema, schema.toAPISchema(), extractSubgraphsFromSupergraph(schema)];

diff --git a/composition-js/src/__tests__/hints.test.ts b/composition-js/src/__tests__/hints.test.ts
@@ -20,6 +20,9 @@ import {
   hintInconsistentExecutionDirectiveLocations,
   hintInconsistentArgumentPresence,
   hintInconsistentDescription,
+  hintFromSubgraphDoesNotExist,
+  hintOverrideDirectiveCanBeRemoved,
+  hintOverriddenFieldCanBeRemoved,
 } from '../hints';
 import { MergeResult, mergeSubgraphs } from '../merging';
 
@@ -604,4 +607,126 @@ test('hints on inconsistent description for field', () => {
     + '  I don\'t know what I\'m doing\n'
     + '  """'
   );
-})
+});
+
+describe('hint tests related to the @override directive', () => {
+  it('hint when from subgraph does not exist', () => {
+    const subgraph1 = gql`
+      type Query {
+        a: Int
+      }
+
+      type T @key(fields: "id"){
+        id: Int
+        f: Int @override(from: "Subgraph3")
+      }
+    `;
+
+    const subgraph2 = gql`
+    type T @key(fields: "id"){
+      id: Int
+    }
+    `;
+    const result = mergeDocuments(subgraph1, subgraph2);
+    expect(result).toRaiseHint(
+      hintFromSubgraphDoesNotExist,
+      `Source subgraph "Subgraph3" for field "T.f" on subgraph "Subgraph1" does not exist. Did you mean "Subgraph1" or "Subgraph2"?`,
+    );
+  });
+
+  it('hint when @override directive can be removed', () => {
+    const subgraph1 = gql`
+      type Query {
+        a: Int
+      }
+
+      type T @key(fields: "id"){
+        id: Int
+        f: Int @override(from: "Subgraph2")
+      }
+    `;
+
+    const subgraph2 = gql`
+    type T @key(fields: "id"){
+      id: Int
+    }
+    `;
+    const result = mergeDocuments(subgraph1, subgraph2);
+    expect(result).toRaiseHint(
+      hintOverrideDirectiveCanBeRemoved,
+      `Field "T.f" on subgraph "Subgraph1" no longer exists in the from subgraph. The @override directive can be removed.`,
+    );
+  });
+
+  it('hint overridden field can be removed', () => {
+    const subgraph1 = gql`
+      type Query {
+        a: Int
+      }
+
+      type T @key(fields: "id"){
+        id: Int
+        f: Int @override(from: "Subgraph2")
+      }
+    `;
+
+    const subgraph2 = gql`
+    type T @key(fields: "id"){
+      id: Int
+      f: Int
+    }
+    `;
+    const result = mergeDocuments(subgraph1, subgraph2);
+    expect(result).toRaiseHint(
+      hintOverriddenFieldCanBeRemoved,
+      `Field "T.f" on subgraph "Subgraph2" is overridden. Consider removing it.`,
+    );
+  });
+
+  it('hint overridden field can be made external', () => {
+    const subgraph1 = gql`
+      type Query {
+        a: Int
+      }
+
+      type T @key(fields: "id"){
+        id: Int @override(from: "Subgraph2")
+      }
+    `;
+
+    const subgraph2 = gql`
+      type T @key(fields: "id"){
+        id: Int
+      }
+    `;
+    const result = mergeDocuments(subgraph1, subgraph2);
+    expect(result).toRaiseHint(
+      hintOverriddenFieldCanBeRemoved,
+      `Field "T.id" on subgraph "Subgraph2" is overridden. It is still used in some federation directive(s) (@key, @requires, and/or @provides) and/or to satisfy interface constraint(s), but consider marking it @external explicitly or removing it along with its references.`,
+    );
+  });
+
+  it('hint when @override directive can be removed because overridden field has been marked external', () => {
+    const subgraph1 = gql`
+      type Query {
+        a: Int
+      }
+
+      type T @key(fields: "id"){
+        id: Int @override(from: "Subgraph2")
+        f: Int
+      }
+    `;
+
+    const subgraph2 = gql`
+    type T @key(fields: "id"){
+      id: Int @external
+    }
+    `;
+    const result = mergeDocuments(subgraph1, subgraph2);
+    expect(result).toRaiseHint(
+      hintOverrideDirectiveCanBeRemoved,
+      `Field "T.id" on subgraph "Subgraph1" is not resolved anymore by the from subgraph (it is marked "@external" in "Subgraph2"). The @override directive can be removed.`,
+    );
+  });
+});