diff --git a/.changeset/yellow-horses-protect.md b/.changeset/yellow-horses-protect.md new file mode 100644 index 000000000..2a710bc41 --- /dev/null +++ b/.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). diff --git a/composition-js/src/__tests__/hints.test.ts b/composition-js/src/__tests__/hints.test.ts index 9969132ca..c4824db7d 100644 --- a/composition-js/src/__tests__/hints.test.ts +++ b/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', () => { diff --git a/composition-js/src/hints.ts b/composition-js/src/hints.ts index d6dbf8b8e..11fafcf69 100644 --- a/composition-js/src/hints.ts +++ b/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, diff --git a/composition-js/src/merging/merge.ts b/composition-js/src/merging/merge.ts index 82da8fa90..2397e1187 100644 --- a/composition-js/src/merging/merge.ts +++ b/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, + )); } } }