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 uniqueness of constructed graph types #242

Merged
merged 2 commits into from
Dec 20, 2022

Conversation

tlil
Copy link
Collaborator

@tlil tlil commented Dec 20, 2022

As discovered in #237, and after the addition of integrity checks in the parent library (graphql-dotnet/graphql-dotnet#3332), the Convention library has been shown to create duplicate instances of derived graph types in some cases due to insufficient caching logic.

Previously, this didn't flag since there were no integrity checks to look for this kind of behaviour. The types are derived deterministically, so although this miss caused usage with the latest version of the library to bark on query execution, there should be no logical changes in the way schemas are derived and evaluated (bar not triggering an error, obviously!) with this change.

As discovered in #237, and after the addition of integrity checks in the parent library (graphql-dotnet/graphql-dotnet#3332), the Convention library has been shown to create duplicate instances of derived graph types in some cases due to insufficient caching logic.

Previously, this didn't flag since there were no integrity checks to look for this kind of behaviour. The types are derived deterministically, so although this miss caused usage with the latest version of the library to bark on query execution, there should be no logical changes in the way schemas are derived and evaluated (bar not triggering an error, obviously!) with this change.
var request = Request.New(new QueryInput { QueryString = "{ __schema { types { name } } }" });
var result = await requestHandler.ProcessRequestAsync(request, null, dependencyInjector);

Assert.False(result.HasErrors);
Copy link
Collaborator Author

@tlil tlil Dec 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ FYI: This part would previously raise errors of the sort (in master):

Error executing document. A different instance of the GraphType 'OutputObjectGraphType<Author>' with the name 'Author' has already been registered within the schema. Please use the same instance for all references within the schema, or use GraphQLTypeReference to reference a type instantiated elsewhere.

and:

Error executing document. This graph type 'OutputObjectGraphType<Book>' with name 'Book' has already been initialized. Make sure that you do not use the same instance of a graph type in multiple schemas. It may be so if you registered this graph type as singleton; see https://graphql-dotnet.github.io/docs/getting-started/dependency-injection/ for more info.

@Shane32 Shane32 mentioned this pull request Dec 20, 2022
@tlil tlil merged commit ddf3306 into master Dec 20, 2022
@tlil tlil deleted the tlil-fix-uniqueness-of-constructed-graph-types branch December 20, 2022 19:03
@fgroen
Copy link

fgroen commented Dec 21, 2022

We have tested it and it appears to be working on our end! We will continue testing, but it looks like upgrading to GQL7 will not be a problem for us anymore.

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