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

Fix Panic on Introspection of Schema using ToJSON #328

Conversation

dackroyd
Copy link
Contributor

@dackroyd dackroyd commented May 8, 2019

Introspection via ToJSON works differently when used via the Schema.ToJSON function than when making an introspection query from a client such as GraphiQL. In the later case, introspection uses the Schema's registered resolver, while in the former case, a resolvable.Schema is faked

Without the Meta being set on this resolvable.Schema (following the recent changes to address race conditions around the Meta schema), this resulted in a panic when using this form of schema introspection.

@dackroyd
Copy link
Contributor Author

dackroyd commented May 8, 2019

Fixes #327

@bmhatfield
Copy link

I was able to confirm that this PR resolves my issue by adding this fork as a remote and checking out this branch:

[bhatfield graphql-go]% git remote add dackroyd https://github.com/dackroyd/graphql-go.git
[bhatfield graphql-go]% git fetch dackroyd
remote: Enumerating objects: 9, done.
remote: Counting objects: 100% (9/9), done.
remote: Compressing objects: 100% (6/6), done.
remote: Total 9 (delta 3), reused 9 (delta 3), pack-reused 0
Unpacking objects: 100% (9/9), done.
From github.com:dackroyd/graphql-go
 * [new branch]      bug/fix-data-race-on-schema-parsing   -> dackroyd/bug/fix-data-race-on-schema-parsing
 * [new branch]      bug/fix-panic-on-introspection-tojson -> dackroyd/bug/fix-panic-on-introspection-tojson
 * [new branch]      experimental-lexer                    -> dackroyd/experimental-lexer
 * [new branch]      master                                -> dackroyd/master
 * [new branch]      new-resolvers                         -> dackroyd/new-resolvers
[bhatfield graphql-go]% git checkout dackroyd/bug/fix-panic-on-introspection-tojson
Note: checking out 'dackroyd/bug/fix-panic-on-introspection-tojson'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b <new-branch-name>

HEAD is now at e571531 Fix Panic on Introspection of Schema using ToJSON

Introspection via ToJSON works differently when used via the
 `Schema.ToJSON` function than when making an introspection query from a
 client such as GraphiQL. In the later case, introspection uses the
 Schema's registered resolver, while in the former case, a
 `resolvable.Schema` is faked

Without the `Meta` being set on this `resolvable.Schema` (following the
 recent changes to address race conditions around the Meta schema), this
 resulted in a panic when using this form of schema introspection.
@dackroyd dackroyd force-pushed the bug/fix-panic-on-introspection-tojson branch from e571531 to c126f0e Compare May 8, 2019 03:19
@bmhatfield
Copy link

Out of curiosity, is there any particular reason why this hasn't been merged yet?

@dackroyd
Copy link
Contributor Author

I assume that @pavelnikolov has not yet had time to revisit this since I made the corrections for schemas without resolvers

@pavelnikolov
Copy link
Member

Thanks, Dave! It looks awesome! 🎉

@pavelnikolov pavelnikolov merged commit 7ff14cf into graph-gophers:master May 11, 2019
@dackroyd dackroyd deleted the bug/fix-panic-on-introspection-tojson branch October 4, 2019 00:36
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.

3 participants