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 schema built from SDL to return null for unknown types #1068

Merged
merged 8 commits into from
Feb 15, 2022

Conversation

vhenzl
Copy link
Contributor

@vhenzl vhenzl commented Feb 13, 2022

Fixes #997 (and unblocks #998 and #999).

History background

The implementation of BuildSchema, SchemaExtender and ASTDefinitionBuilder roughly matches their counterparts from graphql-js v14.0.

However, during the lifetime of v14, the JS code underwent significant changes. In the final version 14.7, buildASTSchema(), extendSchema() and ASTDefinitionBuilder looks much different then they did in v14.0. For the context of this PR, the most important changes (graphql/graphql-js@afc6b7c) were that ASTDefinitionBuilder's constructor didn't accept typeDefinitionsMap param any more, and that the original single method buildType() was replaced with a new buildType() and getNamedType(). Each was used in a different context, and only the latter could throw the "Unknown type" error.

These changes to the reference implementation have never been reflected in graphql-php.

Implementation notes

The context is everything. Sometimes it's okay for the builder to return null instead of throwing "Unknown type" error – like in this case when it's requested to build a type that isn't defined in the SDL schema.

To handle this explicitly, I added the new maybeBuildType(string $name): ?Type method to the builder.

graphql-js (v14.2+) builds typeMap with all types from SDL ahead. Then calling typeMap[typeName] returns either a type or undefined.

The new maybeBuildType() provides similar functionality for the lazy environment where we don't have such a map and where the individual types are built from the SDL schema on demand.

Additional notes

In v15, graphql-js has undergone even more significant changes and ASTDefinitionBuilder has been removed completely (graphql/graphql-js@3948aa2).
To ensure future compatibility with the reference implementation, it would be worth considering reimplementing schema builder and extender completely.

@spawnia spawnia merged commit 5b41769 into webonyx:master Feb 15, 2022
@spawnia
Copy link
Collaborator

spawnia commented Feb 15, 2022

Awesome work! This solution is really simple and powerful.

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.

Schema::getType() shoud always return null for non-existent type
2 participants