Skip to content

Commit

Permalink
Refine composition hints for progressive @override (#2922)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
trevor-scheer authored Feb 5, 2024
1 parent 33b937b commit 1f72f2a
Show file tree
Hide file tree
Showing 4 changed files with 101 additions and 14 deletions.
5 changes: 5 additions & 0 deletions .changeset/yellow-horses-protect.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/composition": patch
---

Introduce a new composition hint pertaining specifically to progressive `@override` usage (when a `label` argument is present).
58 changes: 58 additions & 0 deletions composition-js/src/__tests__/hints.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
7 changes: 7 additions & 0 deletions composition-js/src/hints.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
45 changes: 31 additions & 14 deletions composition-js/src/merging/merge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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)
Expand All @@ -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,
));
}
}
}
Expand Down

0 comments on commit 1f72f2a

Please sign in to comment.