-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 GraphQL Explorer #15319
Fix GraphQL Explorer #15319
Conversation
@hyzx86 can you please review the changes in this PR? |
I did this change I believe (not even sure but seems familiar). And the comments states it is singleton, so the comment could be removed "if this is the correct thing to do". Have you tried to understand what the issue was with the resolution? It really can't be a singleton? |
@sebastienros I updated the description in the PR to show the detailed in the exception. I am guessing after the upgrade, we no longer need it to be singleton. And after the change everything seems to be working as expected. These types are resolved once during the schema building. |
src/OrchardCore/OrchardCore.ContentManagement.GraphQL/Queries/Types/TypedContentTypeBuilder.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.ContentManagement.GraphQL/Queries/Types/TypedContentTypeBuilder.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.Apis.GraphQL.Abstractions/ServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.Apis.GraphQL.Abstractions/ServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.Apis.GraphQL.Abstractions/ServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
Tried on my side and came to the same solution, using Transient though. Each type shouldn't be resolved multiple time during a request that builds the schema. |
@MikeAlhayek, @sebastienros I've just tried I only can see plenty of exceptions, when navigating from |
Note that by making the graph types
fails for complex types, because the |
Sorry, I didn't notice this PR. When I saw it in the message list, it was already merged and I turned it off. |
Fix #15304
@lampersky can you please checkout this branch and confirm that it fixes your issues?
The exception states