-
Notifications
You must be signed in to change notification settings - Fork 248
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): Prevent @tag and @inaccessible usages on fields marked @external #869
Conversation
f55eab7
to
295ac98
Compare
7b518fc
to
10a7b57
Compare
295ac98
to
492ae7b
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.
The logic looks good to me, just a small comment about naming and the difference between directive definitions and usages.
|
||
| Code | Description | | ||
|---|---| | ||
| `TAG_USED_WITH_EXTERNAL` | Fields marked as `@external` cannot also have `@tag` usages. | |
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.
This seems fine for now, but I wonder if we should generalize this to other directives.
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.
@martijnwalraven @trevor-scheer should we add the same handling for@inaccessible
here as well -- even if we don't generalize just yet?
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.
also how will we handle @tag
and @inaccessible
on value types referenced only from @external
fields, like this example?
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.
@@ -39,7 +39,7 @@ export interface FederationField { | |||
requires?: ReadonlyArray<SelectionNode>; | |||
provides?: ReadonlyArray<SelectionNode>; | |||
belongsToValueType?: boolean; | |||
appliedDirectives?: DirectiveNode[] | |||
otherKnownDirectives?: DirectiveNode[] |
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.
Note that these are directive usages / applications, not directive definitions. (The naming in graphql-js
is confusing, but GraphQLDirective
is a directive definition, while DirectiveNode
is a usage.)
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.
Got it! Went through and clarified the naming a bit further - everything is either named usage or definition accordingly. A bit verbose but helpful I think.
10a7b57
to
89f7eda
Compare
Also rename 'appliedDirectives' to 'otherKnownDirectives'
This reverts most of the previous implementation in favor of a new composition error which prevents users from confusingly using both @tag and @external together. For now, @tag is expected to have implications with schema contracts and in the context of an external field, it's not well-defined what that would actually mean. We may revisit and relax this in the future, but for now have decided to just prevent the possible confusion.
a68ab81
to
884da2c
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 we'll figure out handling of value types in a follow up PR, this looks good to me!
Overridden by #880 |
This PR introduces new composition errors to prevent the usage of
@tag
and@inaccessible
on a field that is marked@external
. Since@tag
is part of Studio's API, we're going to block this usage for now to prevent confusion. Similarly,@inaccessible
usage on@external
doesn't really mean anything, so allowing it will probably lead to confusion as users may not know what to expect.This may be relaxed in the future in order to accommodate an actual semantic usage of the combination or possibly just a warning (warnings don't exist yet, but this could be a good candidate for one if we choose to go down that path).
This PR also handles the rename of
appliedDirectives
recommended in feedback on #859.Fixes: #867