Allow compatibility when using introspection derived client schema's #145
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
When using introspection based schema generators, the server schema is strictly explicit causing a clash with `gqlparser.LoadSchema` due to it's schema prelude source accommodating implicitly defined schema's.
suessflorian
requested review from
benjaminjkraft,
csilvers,
dnerdy and
StevenACoffman
as code owners
This was referenced Oct 23, 2021
csilvers
reviewed
Oct 24, 2021
There was a problem hiding this comment.
This makes sense to me! But I'd like to wait a few days to give @benjaminjkraft a chance to take a look.
|
Also, if you haven't already, can you sign the CLA? https://www.khanacademy.org/r/cla |
|
@csilvers, thanks! Also, CLA signed 👍 |
This was referenced Oct 25, 2021
|
This looks perfect, thanks! |
benjaminjkraft
approved these changes
Oct 27, 2021
benjaminjkraft
added a commit
that referenced
this pull request
Jun 6, 2022
GraphQL schemas have some builtin types, like `String`. The spec says your SDL must not include those, but in practice some schemas do. (This is probably because introspection must include them, and some tools that create SDL from introspection don't know they're supposed to filter them out.) Anyway, we've since #145 had logic to handle this; we just parse with and without the prelude that defines them and see which works. The problem is that this makes for very confusing error messages if you have an invalid schema. (Or if you have a schema that you think is valid but gqlparser doesn't, which is the more common case in the wild; see for example #200.) Right now if both ways error we take the without-prelude error, which if you didn't define the builtins is just `undefined type String`; if we took the with-prelude error then if you did define the builtins you'd just get `type String defined twice`. So we actually have to be smart if we want good error messages for everyone. So in this commit we are smart: we check if your schema defines `String`, and include the prelude only if it does not. To do this I basically inlined `gqlparser.LoadSchema` (twice), so that in between parsing and validation we can check if you have `String` and if not add the prelude. This should in theory be both more efficient (we don't have to do everything twice) and give better error messages, although it's a bit more code. Test plan: make check
6 tasks
benjaminjkraft
added a commit
that referenced
this pull request
Jun 6, 2022
GraphQL schemas have some builtin types, like `String`. The spec says your SDL must not include those, but in practice some schemas do. (This is probably because introspection must include them, and some tools that create SDL from introspection don't know they're supposed to filter them out.) Anyway, we've since #145 had logic to handle this; we just parse with and without the prelude that defines them and see which works. The problem is that this makes for very confusing error messages if you have an invalid schema. (Or if you have a schema that you think is valid but gqlparser doesn't, which is the more common case in the wild; see for example #200.) Right now if both ways error we take the without-prelude error, which if you didn't define the builtins is just `undefined type String`; if we took the with-prelude error then if you did define the builtins you'd just get `type String defined twice`. So we actually have to be smart if we want good error messages for everyone. So in this commit we are smart: we check if your schema defines `String`, and include the prelude only if it does not. To do this I basically inlined `gqlparser.LoadSchema` (twice), so that in between parsing and validation we can check if you have `String` and if not add the prelude. This should in theory be both more efficient (we don't have to do everything twice) and give better error messages, although it's a bit more code. Fixes #175. Test plan: make check
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.
During generation we parse the schema using
gqlparser.LoadSchemaover here. As you can seegqlparser.LoadSchemauses it's sub directory declaredvalidator.LoadSchema- but prepends a schema with typical implicit declared types. Full server schema introspection exposes the entire schema explicitly, hence causing a clash with this prelude schema rendering introspection derived schemas to fail.Here I just introduce a two stage schema parsing step to accomodate both implicit and explicit sdl's.
--
I have also added to the FAQ as per #4 how we can use introspection to fetch the schema when using
genqlient.