Skip to content

Warn on merging inconsistent non-repeatable directive applications in… #1840

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 1, 2022
Merged

Warn on merging inconsistent non-repeatable directive applications in… #1840

merged 1 commit into from
Jul 1, 2022

Conversation

pcmanus
Copy link
Contributor

@pcmanus pcmanus commented May 10, 2022

…stead of erroring

When, for a given element, a non-repeatable directive having arguments
appeared with inconsistent arguments in different subgraph, we used to
error out. Instead, this commit picks one of the application for the
supergraph and issue a warning that the other applications were ignored.

The version picked follows the same algorithm that for the similar case
with description: if a given application is repeated in multiple
subgraphs, more than other applications, we pick that version.
Otherwise, we pick the application of the first subgraph in alphabetical
order.

Additionally, this commit ensures that we treat an non-provided argument
with a default the same than that if that argument is provided but uses
exactly the default value.

Fixes #1804

@pcmanus pcmanus mentioned this pull request May 10, 2022
Closed
@netlify
Copy link

netlify bot commented May 10, 2022 •
edited
Loading

👷 Deploy request for apollo-federation-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit c9c14d1

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 10, 2022 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@pcmanus pcmanus force-pushed the 1804-merge-directive-warning branch from 7284307 to a43d8c4 Compare May 17, 2022 09:21
@pcmanus pcmanus added this to the 2.1.0 milestone May 19, 2022
@pcmanus pcmanus self-assigned this May 23, 2022
benweatherman
benweatherman approved these changes Jun 24, 2022
View reviewed changes
Copy link
Contributor

@benweatherman benweatherman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐐

docs/source/errors.md Outdated
@@ -103,6 +102,7 @@ The following error codes have been removed and are no longer generated by the m
| `KEY_MISSING_ON_BASE` | Each subgraph is now free to declare a key only if it needs it. |
| `KEY_NOT_SPECIFIED` | Each subgraph can declare key independently of any other subgraph. |
| `MULTIPLE_KEYS_ON_EXTENSION` | Every subgraph can have multiple keys, as necessary. |
| `NON_REPEATABLE_DIRECTIVE_ARGUMENTS_MISMATCH` | Since federation 2.10, the case this error used to cover is now a warning (with code `INCONSISTEN_NON_REPEATABLE_DIRECTIVE_ARGUMENTS`) instead of an error |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| `NON_REPEATABLE_DIRECTIVE_ARGUMENTS_MISMATCH` | Since federation 2.10, the case this error used to cover is now a warning (with code `INCONSISTEN_NON_REPEATABLE_DIRECTIVE_ARGUMENTS`) instead of an error |
| `NON_REPEATABLE_DIRECTIVE_ARGUMENTS_MISMATCH` | Since federation 2.1.0, the case this error used to cover is now a warning (with code `INCONSISTEN_NON_REPEATABLE_DIRECTIVE_ARGUMENTS`) instead of an error |

internals-js/src/error.ts Outdated
@@ -529,4 +523,6 @@ export const REMOVED_ERRORS = [
['VALUE_TYPE_UNION_TYPES_MISMATCH', 'Subgraph definitions for an union are now merged by composition'],
['PROVIDES_FIELDS_SELECT_INVALID_TYPE', '@provides can now be used on field of interface, union and list types'],
['RESERVED_FIELD_USED', 'This error was previously not correctly enforced: the _service and _entities, if present, were overridden; this is still the case'],

['NON_REPEATABLE_DIRECTIVE_ARGUMENTS_MISMATCH', 'Since federation 2.10, the case this error used to cover is now a warning (with code `INCONSISTEN_NON_REPEATABLE_DIRECTIVE_ARGUMENTS`) instead of an error'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
['NON_REPEATABLE_DIRECTIVE_ARGUMENTS_MISMATCH', 'Since federation 2.10, the case this error used to cover is now a warning (with code `INCONSISTEN_NON_REPEATABLE_DIRECTIVE_ARGUMENTS`) instead of an error'],
['NON_REPEATABLE_DIRECTIVE_ARGUMENTS_MISMATCH', 'Since federation 2.1.0, the case this error used to cover is now a warning (with code `INCONSISTEN_NON_REPEATABLE_DIRECTIVE_ARGUMENTS`) instead of an error'],

composition-js/src/merging/merge.ts Outdated
@@ -587,6 +597,7 @@ class Merger {
);
}

// Note meant to be used directly: use `reportMismatchError` or `reportMismatchHint` instead.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick Sorry to nitpick this one, but it does change the meaning a bit (though I think it's fairly easy to see what the intent is)

Suggested change
// Note meant to be used directly: use `reportMismatchError` or `reportMismatchHint` instead.
// Not meant to be used directly: use `reportMismatchError` or `reportMismatchHint` instead.

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

return {
message: () => "You're negating a negative method? Instead of using `toRaiseHint`? Do you want to talk about it?",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😂

composition-js/src/merging/merge.ts Outdated
@@ -322,8 +322,7 @@ class Merger {
}

private joinSpecName(subgraphIndex: number): string {
return this.subgraphNamesToJoinSpecName.get(this.names[subgraphIndex])!;
}
return this.subgraphNamesToJoinSpecName.get(this.names[subgraphIndex])!; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick I think this might've been an accidental change

Suggested change
return this.subgraphNamesToJoinSpecName.get(this.names[subgraphIndex])!; }
return this.subgraphNamesToJoinSpecName.get(this.names[subgraphIndex])!;
}

composition-js/src/merging/merge.ts Outdated
}

if (dest.schema().directive(name)?.repeatable) {
// For repeatable directives, we simply include each application found but with exact duplicates removed (which is done by `removeDirective`).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick

This should say something about checking sameDirectiveApplication instead of removeDirective, right? Or maybe leave that part out since it's susceptible to code changes and is readable otherwise.

Suggested change
// For repeatable directives, we simply include each application found but with exact duplicates removed (which is done by `removeDirective`).
// For repeatable directives, we simply include each application found but with exact duplicates removed.

composition-js/src/merging/merge.ts Outdated
// Note: we currently "only" merge together applications that have the same arguments (with defaults expanded however).
// But when argument is an input object type, should we ignore those fields that will not be included in the supergraph due
// the intersection merging of input types?
//
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question Did you mean to separate these questions now? Or is this an actual open question?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to remove the 2nd part of the comment because it was actually incorrect, but forgot apparently (which is why that 2nd part was looking weird). Fixed it, and actually turned the "Note" here to a "TODO" because I think we may have a small bug here. But I need to write a test to validate if that's true, and I'll open a separate issue if that' is indeed true since it's unrelated to the issue at end.

Warn on merging inconsistent non-repeatable directive applications in…
c9c14d1 
…stead of erroring

When, for a given element, a non-repeatable directive having arguments
appeared with inconsistent arguments in different subgraph, we used to
error out. Instead, this commit picks one of the application for the
supergraph and issue a warning that the other applications were ignored.

The version picked follows the same algorithm that for the similar case
with description: if a given application is repeated in multiple
subgraphs, more than other applications, we pick that version.
Otherwise, we pick the application of the first subgraph in alphabetical
order.

Additionally, this commit ensures that we treat an non-provided argument
with a default the same than that if that argument is provided but uses
exactly the default value.

Fixes apollographql#1804
@pcmanus pcmanus force-pushed the 1804-merge-directive-warning branch from a43d8c4 to c9c14d1 Compare July 1, 2022 13:31
@pcmanus pcmanus merged commit 368a1df into apollographql:main Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benweatherman benweatherman benweatherman approved these changes

Assignees

@pcmanus pcmanus

Labels
None yet
Projects
None yet
Milestone
2.1.0
Development

Successfully merging this pull request may close these issues.

Composing subgraphs with different @deprecated args should succeed with a hint instead of error
2 participants
@pcmanus @benweatherman