Skip to content
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

Allow doubly-defined builtin directives #226

Merged
merged 1 commit into from
Jun 6, 2022

Conversation

benjaminjkraft
Copy link
Contributor

Normally we don't want to let you define a directive twice.* But if the
directive is builtin, and one copy of it is from your schema while the
other is from the prelude, then we have to, because the spec only says
that such directives "may" be excluded from SDL. (This happens in
practice when you generate a schema from introspection, which does
define all builtins, and your generator doesn't filter them out.) So now
we allow it, only for builtin directives.

*Fun fact: technically the spec doesn't say we shouldn't. But I think
that's an error in the spec, and it's reasonable and correct to check.
In general I think the schema validation portions of the spec are
pretty incomplete :| .

Fixes #223

Normally we don't want to let you define a directive twice.* But if the
directive is builtin, and one copy of it is from your schema while the
other is from the prelude, then we have to, because the spec only says
that such directives "may" be excluded from SDL. (This happens in
practice when you generate a schema from introspection, which does
define all builtins, and your generator doesn't filter them out.) So now
we allow it, only for builtin directives.

*Fun fact: technically the spec doesn't say we shouldn't. But I think
that's an error in the spec, and it's reasonable and correct to check.
In general I think the schema validation portions of the spec are
pretty incomplete :| .
@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 92.294% when pulling 375e680 on benjaminjkraft:builtin-directives into 6519773 on vektah:master.

@benjaminjkraft
Copy link
Contributor Author

@StevenACoffman see what you think of this. I don't love the fact that it doesn't validate that the directives match at all, but I don't see a great alternative, other than taking this problem back to the GraphQL Spec WG and seeing if they have thoughts. (The reason it's set up this way is described briefly in graphql/graphql-spec#815 (comment).) My feeling is having slightly loose validation here is better than rejecting spec-allowed schemas, but it's debatable.

@StevenACoffman
Copy link
Collaborator

Since this is limited to built-in directives, the likelihood that they differ is fairly small, and I think the benefits for the most common scenario outweigh any risks.

If I'm wrong about that, then I have some really awful, convoluted code to verify repeated directives are identical that I can try to clean up, but I'd rather not go there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gqlparser should be more flexible about built-in directives
3 participants