Refine composition hints for progressive @override (#2922)

Composition hints w.r.t. `@override` usage don't _quite_ apply now that
fields could be in a "in migration" state when progressive `@override`
is in use.

This change introduces a different hint for progressive `@override`
usage which advises completing the migration before suggesting the
removal of the original field.

Author: trevor-scheer

.changeset/yellow-horses-protect.md

@@ -0,0 +1,5 @@
+---
+"@apollo/composition": patch
+---
+
+Introduce a new composition hint pertaining specifically to progressive `@override` usage (when a `label` argument is present).

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

@@ -900,6 +900,64 @@ describe('hint tests related to the @override directive', () => {
       'T.id'
     );
   });
+
+  it('hint when progressive @override migration is in progress', () => {
+    const subgraph1 = gql`
+      type Query {
+        a: Int
+      }
+
+      type T @key(fields: "id"){
+        id: Int
+        f: Int @override(from: "Subgraph2", label: "percent(1)")
+      }
+    `;
+
+    const subgraph2 = gql`
+    type T @key(fields: "id"){
+      id: Int
+      f: Int
+    }
+    `;
+    const result = mergeDocuments(subgraph1, subgraph2);
+
+    // We don't want to see the related hint for non-progressive overrides that
+    // suggest removing the original field.
+    expect(result.hints).toHaveLength(1);
+    expect(result).toRaiseHint(
+      HINTS.OVERRIDE_MIGRATION_IN_PROGRESS,
+      `Field "T.f" is currently being migrated with progressive @override. Once the migration is complete, remove the field from subgraph "Subgraph2".`,
+      'T.f',
+    );
+  });
+
+  it('hint when progressive @override migration is in progress (for a referenced field)', () => {
+    const subgraph1 = gql`
+      type Query {
+        a: Int
+      }
+
+      type T @key(fields: "id"){
+        id: Int @override(from: "Subgraph2", label: "percent(1)")
+      }
+    `;
+
+    const subgraph2 = gql`
+      type T @key(fields: "id"){
+        id: Int
+      }
+    `;
+    const result = mergeDocuments(subgraph1, subgraph2);
+
+    // We don't want to see the related hint for non-progressive overrides that
+    // suggest removing the original field.
+    expect(result.hints).toHaveLength(1);
+    expect(result).toRaiseHint(
+      HINTS.OVERRIDE_MIGRATION_IN_PROGRESS,
+      `Field "T.id" on subgraph "Subgraph2" is currently being migrated via progressive @override. It is still used in some federation directive(s) (@key, @requires, and/or @provides) and/or to satisfy interface constraint(s). Once the migration is complete, consider marking it @external explicitly or removing it along with its references.`,
+      'T.id'
+    );
+  });
 });
 
 describe('on non-repeatable directives used with incompatible arguments', () => {

composition-js/src/hints.ts

@@ -163,6 +163,12 @@ const OVERRIDE_DIRECTIVE_CAN_BE_REMOVED = makeCodeDefinition({
   description: 'Field with @override directive no longer exists in source subgraph, the directive can be safely removed',
 });
 
+const OVERRIDE_MIGRATION_IN_PROGRESS = makeCodeDefinition({
+  code: 'OVERRIDE_MIGRATION_IN_PROGRESS',
+  level: HintLevel.INFO,
+  description: 'Field is currently being migrated with progressive @override. Once the migration is complete, remove the field from the original subgraph.',
+});
+
 const UNUSED_ENUM_TYPE = makeCodeDefinition({
   code: 'UNUSED_ENUM_TYPE',
   level: HintLevel.DEBUG,
@@ -228,6 +234,7 @@ export const HINTS = {
   FROM_SUBGRAPH_DOES_NOT_EXIST,
   OVERRIDDEN_FIELD_CAN_BE_REMOVED,
   OVERRIDE_DIRECTIVE_CAN_BE_REMOVED,
+  OVERRIDE_MIGRATION_IN_PROGRESS,
   UNUSED_ENUM_TYPE,
   INCONSISTENT_NON_REPEATABLE_DIRECTIVE_ARGUMENTS,
   MERGED_NON_REPEATABLE_DIRECTIVE_ARGUMENTS,

composition-js/src/merging/merge.ts

@@ -1304,6 +1304,8 @@ class Merger {
           // if the field being overridden is used, then we need to add an @external directive
           assert(fromField, 'fromField should not be undefined');
           const overriddenSubgraphASTNode = fromField.sourceAST ? addSubgraphToASTNode(fromField.sourceAST, sourceSubgraphName) : undefined;
+          const overrideLabel = overrideDirective.arguments().label;
+          const overriddenFieldIsReferenced = !!this.metadata(fromIdx).isFieldUsed(fromField);
           if (this.isExternal(fromIdx, fromField)) {
             // The from field is explicitly marked external by the user (which means it is "used" and cannot be completely
             // removed) so the @override can be removed.
@@ -1313,26 +1315,30 @@ class Merger {
               dest,
               overridingSubgraphASTNode,
             ));
-          } else if (this.metadata(fromIdx).isFieldUsed(fromField)) {
+          } else if (overriddenFieldIsReferenced) {
             result.setUsedOverridden(fromIdx);
-            this.hints.push(new CompositionHint(
-              HINTS.OVERRIDDEN_FIELD_CAN_BE_REMOVED,
-              `Field "${dest.coordinate}" on subgraph "${sourceSubgraphName}" 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.`,
-              dest,
-              overriddenSubgraphASTNode,
-            ));
+            if (!overrideLabel) {
+              this.hints.push(new CompositionHint(
+                HINTS.OVERRIDDEN_FIELD_CAN_BE_REMOVED,
+                  `Field "${dest.coordinate}" on subgraph "${sourceSubgraphName}" 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.`,
+                  dest,
+                  overriddenSubgraphASTNode,
+                )
+              );
+            }
           } else {
             result.setUnusedOverridden(fromIdx);
-            this.hints.push(new CompositionHint(
-              HINTS.OVERRIDDEN_FIELD_CAN_BE_REMOVED,
-              `Field "${dest.coordinate}" on subgraph "${sourceSubgraphName}" is overridden. Consider removing it.`,
-              dest,
-              overriddenSubgraphASTNode,
-            ));
+            if (!overrideLabel) {
+              this.hints.push(new CompositionHint(
+                HINTS.OVERRIDDEN_FIELD_CAN_BE_REMOVED,
+                `Field "${dest.coordinate}" on subgraph "${sourceSubgraphName}" is overridden. Consider removing it.`,
+                dest,
+                overriddenSubgraphASTNode,
+              ));
+            }
           }
 
           // capture an override label if it exists
-          const overrideLabel = overrideDirective.arguments().label;
           if (overrideLabel) {
             const labelRegex = /^[a-zA-Z][a-zA-Z0-9_\-:./]*$/;
             // Enforce that the label matches the following pattern: percent(x)
@@ -1358,6 +1364,17 @@ class Merger {
                 { nodes: overridingSubgraphASTNode }
               ));
             }
+
+            const message = overriddenFieldIsReferenced
+              ? `Field "${dest.coordinate}" on subgraph "${sourceSubgraphName}" is currently being migrated via progressive @override. It is still used in some federation directive(s) (@key, @requires, and/or @provides) and/or to satisfy interface constraint(s). Once the migration is complete, consider marking it @external explicitly or removing it along with its references.`
+              : `Field "${dest.coordinate}" is currently being migrated with progressive @override. Once the migration is complete, remove the field from subgraph "${sourceSubgraphName}".`;
+
+            this.hints.push(new CompositionHint(
+              HINTS.OVERRIDE_MIGRATION_IN_PROGRESS,
+              message,
+              dest,
+              overriddenSubgraphASTNode,
+            ));
           }
         }
       }