Skip to content

explicitly set nodes for override hints. #1684

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 6, 2022
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions composition-js/src/merging/merge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ class FieldMergeContext {
return this._props[idx].unusedOverridden;
}

setUsedOverriden(idx: number) {
setUsedOverridden(idx: number) {
this._props[idx].usedOverridden = true;
}

Expand Down Expand Up @@ -1057,13 +1057,15 @@ class Merger {
subgraphsWithOverride.forEach((subgraphName) => {
const { overrideDirective } = subgraphMap[subgraphName];
const sourceSubgraphName = overrideDirective?.arguments()?.from;
const overridingSubgraphASTNode = overrideDirective?.sourceAST ? addSubgraphToASTNode(overrideDirective.sourceAST, subgraphName) : undefined;
if (!this.names.includes(sourceSubgraphName)) {
const suggestions = suggestionList(sourceSubgraphName, this.names);
const extraMsg = didYouMean(suggestions);
this.hints.push(new CompositionHint(
hintFromSubgraphDoesNotExist,
`Source subgraph "${sourceSubgraphName}" for field "${coordinate}" on subgraph "${subgraphName}" does not exist.${extraMsg}`,
coordinate,
overridingSubgraphASTNode,
));
} else if (sourceSubgraphName === subgraphName) {
this.errors.push(ERRORS.OVERRIDE_FROM_SELF_ERROR.err({
Expand All @@ -1080,6 +1082,7 @@ class Merger {
hintOverrideDirectiveCanBeRemoved,
`Field "${coordinate}" on subgraph "${subgraphName}" no longer exists in the from subgraph. The @override directive can be removed.`,
coordinate,
overridingSubgraphASTNode,
));
} else {
// check to make sure that there is no conflicting @provides, @requires, or @external directives
Expand All @@ -1102,27 +1105,31 @@ class Merger {
// if we get here, then the @override directive is valid
// 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;
if (this.isExternal(fromIdx, fromField)) {
// The from field is explcitely marked external by the user (which means it is "used" and cannot be completely
// 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.
this.hints.push(new CompositionHint(
hintOverrideDirectiveCanBeRemoved,
`Field "${coordinate}" on subgraph "${subgraphName}" is not resolved anymore by the from subgraph (it is marked "@external" in "${sourceSubgraphName}"). The @override directive can be removed.`,
coordinate,
overridingSubgraphASTNode,
));
} else if (this.metadata(fromIdx).isFieldUsed(fromField)) {
result.setUsedOverriden(fromIdx);
result.setUsedOverridden(fromIdx);
this.hints.push(new CompositionHint(
hintOverriddenFieldCanBeRemoved,
`Field "${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.`,
coordinate,
overriddenSubgraphASTNode,
));
} else {
result.setUnusedOverridden(fromIdx);
this.hints.push(new CompositionHint(
hintOverriddenFieldCanBeRemoved,
`Field "${coordinate}" on subgraph "${sourceSubgraphName}" is overridden. Consider removing it.`,
coordinate,
overriddenSubgraphASTNode,
));
}
}
Expand Down