-
Notifications
You must be signed in to change notification settings - Fork 257
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
fix(federation): Skip missing types while iterating over field directive usages #868
Conversation
47715e1
to
7b518fc
Compare
7b518fc
to
10a7b57
Compare
name: 'inventory', | ||
typeDefs: gql` | ||
extend type Product @key(fields: "id") { | ||
id: ID! @external @tag(name: "hi from inventory") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this error out because of #869?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it explicitly should not error out, which it was doing before. #869 updates this test with the new composition error that's returned (it's based against this branch for simplicity). So I'll leave this as is since it's properly addressed in the next PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Now that we have a constructed schema, we can filter this map based on types | ||
// that made it into the schema successfully. Types that didn't make it into the | ||
// schema will have related composition errors. | ||
const filteredTypeNameToFieldDirectivesMap = filterMap( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of filtering here, wouldn't it be easier to make addFederationMetadataToSchemaNodes
skip types in typeNameToFieldDirectivesMap
that don't exist in the schema? That avoids the need for filterMap
and recreating a Map
even if nothing gets removed. It seems to me the code here could use a check.
10a7b57
to
89f7eda
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming the feedback is addressed/responded to accordingly, this should move forward to unblock work. Please file follow up
Issues for anything we should circle back to before preview/GA. Thanks!
With the addition of
@tag
and@inaccessible
directives, we inadvertently began adding invalid types to ourtypeNameToFieldDirectivesMap
. These "invalid" types exist in the map (but not the schema) when a type is extended but never actually defined anywhere.We can simply check the existence of each type in the schema before performing any work there and skip any iterations where a type is not found.
Fixes #866