Adds @shareable, fed1 schema upgrades and @link to identify fed2 schema #1510
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
clenfest
approved these changes
Feb 11, 2022
clenfest
reviewed
Feb 21, 2022
| const isDirective = element instanceof DirectiveDefinition; | ||
| const splitted = element.name.split('__'); | ||
| if (splitted.length > 1) { | ||
| return this.byAlias.get(splitted[0]); |
There was a problem hiding this comment.
Should this throw if it's not in the map?
There was a problem hiding this comment.
If it did, this would throw if a user decided to use __ within a name but without meaning to use anything core schema related, and since __ in the middle of names is not invalid graphQL, I don't know if we want that (note that I wouldn't mind terribly making __ in names "reserved", but I'm not particular in favor either because I don't think we need such restriction).
|
|
||
| export type CoreOrLinkDirectiveArgs = CoreDirectiveArgs | LinkDirectiveArgs; | ||
|
|
||
| export type CoreImport = { |
There was a problem hiding this comment.
You're suggesting a change of name for the import parameter in the spec of @link, not just a renaming specific to this PR, right?
Assuming that, we can have that discussion, but I don't have strong opinions. Personally, I'm ok with import (but I'm also not completely sharing the concerns against @import) but I'd be fine with use as well.
internals-js/src/coreSpec.ts
Outdated
| const importArg = args.import; | ||
| const imports: CoreImport[] = importArg ? importArg.map((a) => typeof a === 'string' ? { name: a } : a) : []; | ||
| for (const i of imports) { | ||
| if (i.as && i.name.charAt(0) === '@' && i.as.charAt(0) !== '@') { |
There was a problem hiding this comment.
Do we also want to throw if i.name.charAt(0) is something other than @?
There was a problem hiding this comment.
Assuming you mean throwing if i.as starts with @ but not i.name? Which I agree with and added a case.
pcmanus
force-pushed
the
fed2-next
branch
4 times, most recently
from
e5ced83
to
0de1c85
Compare
added 17 commits
February 25, 2022 14:36
This is meant to avoid recomputation of federation related metadata. Only contains external-related information as of this patch but more is planned to be added by future patches.
This commit introduces code to automatically migrate (whenever possible) fed1 schema into fed2 ones. That code is run automatically during composition on fed1 schema, so that internally, only fed2 schema are composed. The code however make sure that composition errors points to the "original" schema, the one the user inputed, not the automagically migrated one. The migration code is written is such a way that a separate tool to convert fed1 schema into fed2 ones could be easily written (and provide insights into the conversion), but such tool is not part of this commit. Importantly, as of this commit, we need to be able to identify fed1 vs fed2 schema, and this commit recognize fed2 schema as those "linking" to a federation "core" feature with something like: ``` extend schema @link(url: "https://specs.apollo.dev/federation/v2.0/") ```
This commit make it so that schema are only considered fed2 if they have a @link to the federation spec. Otherwise, they are considered fed1 schema and automaticaly upgraded using the `schemaUpgrader` before composition. In doing so, this commit allows composition to accept the renaming of federation directives, which is responsible for a lot of the changes in this commit, requiring some amount of refactoring.
This commit allow the schema upgrader (that converts fed1 to fed2 schema) to: - Removes @provides applied to non-composite fields. Those were not rejected but are non-sensical so did nothing. - Removes @key, @provides and @requires on interfaces. Those were also accepted but had no effect whatsoever (note that we plan to allow @key on interfaces in fed2 as a quick follow-up to GA, but even when we add it, it will continue to make more sense to remove key on interfaces on upgrades since it is the correct "backward compatible" behavior).
Those tests have been re-enabled after not being run for a while and need to be adapted since the tests fixtures have been converted to fed2 schema.
Unsure why I added a validation to reject it in the first place, I must have assume this wasn't supported by federation 1, but it is as far as I can tell and in any case it works just fine. So this commit removes the rejection and adds a few tests for the case.
This commit: - rejects @external on interface fields in fed2 schema. - removes @external on inteface fields in fed1 schema when auto-migrating. The rational is that for fed2, @external is about fields that are not resolved by the subgraph, but interface fields are not resolved in the first place so @external makes no sense on them and "should" be rejected (or rather, accepting them would just muddy the water; note that they were mostly rejected already due to rejecting unused @external and not allowing federation directives on interface, but the new message is a clearer). But fed1 required some interface fields to be marked @external (without that impacting any runtime behavior), so we make sure migration get rid of it.
Default values where not properly validated and when an invalid value was inputed for an enum, we were particularly not handling that gracefully and were generating invalid graphQL when printing back, which lead to hard-to-debug errors. This commit fixes both issues (ensuring we print back syntactically valid graphQL by using a string, and otherwise throwing a meaningful error).
Earlier versions have a bug with graphql-js 16 whereby calling `toString` on errors generate an infinite loop and we really want to avoid it, so forcing at least 0.2.2.
The code that, during validation, avoided considering some useless/nonsensical paths for root types was hardcoded to `Query` and was thus not working properly for other root types (mostly `Mutation`). The result was that a number of trivially-equivalent duplicate paths were considered, and while this was harmless when validation succeeds, when there is a legitimate validation error, it made the error message repeats the same explanation lines once per-subgraph.
The schema upgrader rely on the `sharing` computation on fed1 schema as a shortcut to decide if a field is part of a key (any key in the subgraph). The idea is that the only non-external fields are that are shareable by default are the one that are part of a key, and since fed1 schema don't have @Shareable at all, then the sharing computation does, for fed1 schema, essentially identify fields part of a key. The only small issue is that the code was assuming that fields that are part of @provides were always external, which is true for fed2 schema (which has validation to reject non-external fields in @provides), but not necessarily for fed1 schema. As a result, the schema upgraded wasn't adding @Shareable for some non-external field even if they needed it (because they were also provided by another subgraph).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR has 3 main additions:
@shareabledirective and implementation.@linklinking to the federation spec (along with support of renaming federation directives using theimportargument of@link).The reason all of those are bundled in the same PR is that they are a bit hard to untangle properly in that
@shareableessentially create a compatibility break between fed1 and fed2 schema, so we need the fed1-upgrade code to avoid having to break or disable many tests, and to be able to auto-upgrade fed1 schema, we need to know how to distinguish between a fed1 and fed2 schema (hence the@linksupport).I'll also note that the addition of
@linkimplies a number of changes to the code as federation directives names cannot be hard-coded as they previously were and so it entails a bit of refactoring. The result is not perfect and could be further improved (but it feels more involved so I'd prefer pushing further, more involved cleanup to later).