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

Unable to use Hot Chocolate's builtin in filtering with type modules #6983

Closed
SeanTAllen opened this issue Mar 11, 2024 · 7 comments · Fixed by #7019
Closed

Unable to use Hot Chocolate's builtin in filtering with type modules #6983

SeanTAllen opened this issue Mar 11, 2024 · 7 comments · Fixed by #7019
Labels
🐛 bug Something isn't working 🌶️ hot chocolate

Comments

@SeanTAllen
Copy link
Contributor

Product

Hot Chocolate

Version

13.9.0

Link to minimal reproduction

https://github.com/SeanTAllen/hc-typemodule-filtering-bug

Steps to reproduce

  • dotnet restore
  • dotnet build
  • typemodule.web/bin/Debug/net8.0/typemodule.web

What is expected?

It is expected that the builtin filtering available to annotation-first schema additions in HotChocolate would work with types being added via the dynamic schema APIs.

What is actually happening?

A "No default filter convention found. Call AddFiltering() on the schema builder." is displayed. Note in the code in the reproduction that AddFiltering is called on the schema builder. So at minimum, the error message here is incorrect/misleading.

Relevant log output

fail: Microsoft.Extensions.Hosting.Internal.Host[11]
      Hosting failed to start
      HotChocolate.SchemaException: For more details look at the `Errors` property.
      
      1. No default filter convention found. Call `AddFiltering()` on the schema builder.
      
         at HotChocolate.Data.FilterDescriptorContextExtensions.<>c__DisplayClass1_0.<GetFilterConvention>b__0()
         at HotChocolate.Types.Descriptors.DescriptorContext.GetConventionOrDefault[T](Func`1 defaultConvention, String scope)
         at HotChocolate.Data.FilterDescriptorContextExtensions.GetFilterConvention(IDescriptorContext context, String scope)
         at HotChocolate.Types.FilterObjectFieldDescriptorExtensions.<>c__DisplayClass5_0.<UseFiltering>b__1(IDescriptorContext c, ObjectFieldDefinition definition)
         at HotChocolate.Types.Descriptors.DescriptorBase`1.<>c__DisplayClass19_0.<OnBeforeCreate>b__0(IDescriptorContext c, IDefinition d)
         at HotChocolate.Types.Descriptors.Definitions.CreateConfiguration.Configure(IDescriptorContext context)
         at HotChocolate.Types.Descriptors.DescriptorBase`1.CreateDefinition()
         at HotChocolate.Types.Descriptors.DescriptorExtensions.ToDefinition[T](IDescriptor`1 descriptor)
         at Program.CatProvider.makeCatsField(IDescriptorContext context) in /Users/sallen/Downloads/foo/typemodule/typemodule.web/Program.fs:line 71
         at Program.Pipe #1 input at line 78@78.MoveNext() in /Users/sallen/Downloads/foo/typemodule/typemodule.web/Program.fs:line 87
         at HotChocolate.Execution.RequestExecutorResolver.TypeModuleChangeMonitor.TypeModuleEnumerable.GetAsyncEnumerator(CancellationToken cancellationToken)+MoveNext()
         at HotChocolate.Execution.RequestExecutorResolver.TypeModuleChangeMonitor.TypeModuleEnumerable.GetAsyncEnumerator(CancellationToken cancellationToken)+System.Threading.Tasks.Sources.IValueTaskSource<System.Boolean>.GetResult()
         at HotChocolate.Execution.RequestExecutorResolver.CreateSchemaAsync(ConfigurationContext context, RequestExecutorSetup setup, RequestExecutorOptions executorOptions, IServiceProvider schemaServices, TypeModuleChangeMonitor typeModuleChangeMonitor, CancellationToken cancellationToken)
         at HotChocolate.Execution.RequestExecutorResolver.CreateSchemaAsync(ConfigurationContext context, RequestExecutorSetup setup, RequestExecutorOptions executorOptions, IServiceProvider schemaServices, TypeModuleChangeMonitor typeModuleChangeMonitor, CancellationToken cancellationToken)
         at HotChocolate.Execution.RequestExecutorResolver.CreateSchemaServicesAsync(ConfigurationContext context, RequestExecutorSetup setup, CancellationToken cancellationToken)
         at HotChocolate.Execution.RequestExecutorResolver.GetRequestExecutorNoLockAsync(String schemaName, CancellationToken cancellationToken)
         at HotChocolate.Execution.RequestExecutorResolver.GetRequestExecutorAsync(String schemaName, CancellationToken cancellationToken)
         at HotChocolate.AspNetCore.Warmup.ExecutorWarmupService.ExecuteAsync(CancellationToken stoppingToken)
         at Microsoft.Extensions.Hosting.Internal.Host.<StartAsync>b__15_1(IHostedService service, CancellationToken token)
         at Microsoft.Extensions.Hosting.Internal.Host.ForeachService[T](IEnumerable`1 services, CancellationToken token, Boolean concurrent, Boolean abortOnFirstException, List`1 exceptions, Func`3 operation)
Unhandled exception. HotChocolate.SchemaException: For more details look at the `Errors` property.

Additional context

We are attempting to use the builtin in filtering with the types that come from a type module. After extensive debugging (we weren't familiar with any of the HC code before hitting this problem), we believe the bug is from https://github.com/ChilliCream/graphql-platform/blob/main/src/HotChocolate/Core/src/Execution/RequestExecutorResolver.cs#L362 where a new descriptor context is built and supplied to the type module monitors. This context does not have any convention setup and there appears to be no way to correctly configure to use the existing filtering.

Based on discussions, we believe that it is possible to do a new driver that would work by recreating most of the filtering instead of using the builtin filtering. However, that isn't what we want to accomplish. We want to leverage the existing filtering but use it with types that are generated from a type module. See #6975 for more conversation related to this.

We have considered that this the functionality is working as expected and that the builtin filtering and custom filtering as detailed in the documentation is not intended to work with types generated in a type module. If that is the case, then we believe that the erroneous error message is still an issue to be resolved.

We are very new to using HC but are willing to assist as best we can. We doubt this means providing code to fix the issue as we do not have a sufficient level of familiarity with the codebase and know F# with only minimal C# knowledge. However, for any testing etc, we would be very willing to contribute.

Please let us know what we can do to assist.

@SeanTAllen SeanTAllen added the 🐛 bug Something isn't working label Mar 11, 2024
@michaelstaib
Copy link
Member

Why dont you just opt into our default convention?

@SeanTAllen
Copy link
Contributor Author

I tried that. Perhaps I did it wrong @michaelstaib. What is the proper way to do it?

@SeanTAllen
Copy link
Contributor Author

@michaelstaib can you clarify what "opt into our default convention" so I can verify that we did it and still encountered this issue?

@SeanTAllen
Copy link
Contributor Author

SeanTAllen commented Mar 14, 2024

I've been examining the differences between the builtin filtering and what Jim does in #6975 that allows for filtering with dynamic schemas.

It appears that a small change might solve this problem.

Our failure in our stack trace is coming from:

https://github.com/ChilliCream/graphql-platform/blob/main/src/HotChocolate/Data/src/Data/Filters/Extensions/FilterObjectFieldDescriptorExtensions.cs#L141

The convention that is looked up is only used in one of the 3 if statements that follow. In particular, at:

https://github.com/ChilliCream/graphql-platform/blob/main/src/HotChocolate/Data/src/Data/Filters/Extensions/FilterObjectFieldDescriptorExtensions.cs#L159

If the lookup of the convention was moved into the else if block for that code then the code at https://github.com/ChilliCream/graphql-platform/blob/main/src/HotChocolate/Data/src/Data/Filters/Extensions/FilterObjectFieldDescriptorExtensions.cs#L144 could be used to get the argumentTypeReference, without going through the convention lookup that fails.

If that were the case then it appears that the version of UseFiltering on line 73 https://github.com/ChilliCream/graphql-platform/blob/main/src/HotChocolate/Data/src/Data/Filters/Extensions/FilterObjectFieldDescriptorExtensions.cs#L73 could be used to allow filtering for dynamic schema types. As would the ones on line 100 https://github.com/ChilliCream/graphql-platform/blob/main/src/HotChocolate/Data/src/Data/Filters/Extensions/FilterObjectFieldDescriptorExtensions.cs#L100 and line 47 https://github.com/ChilliCream/graphql-platform/blob/main/src/HotChocolate/Data/src/Data/Filters/Extensions/FilterObjectFieldDescriptorExtensions.cs#L47

I have not tested this. It is possible that other errors might present themselves, but it certainly seems to be a promising avenue.

@SeanTAllen
Copy link
Contributor Author

@michaelstaib I have tested the aforementioned issue and we are now able to use filtering with dynamic schemas without resorting to the complete new middleware that was detailed as a solution by @jimitndiaye in #6975.

At the moment, we are using a "lightly patched" version of HotChocolate.Data that changes no existing functionality. It only moves some convention lookup calls for things like filtering and sorting into the block where they are used.

I'll open a PR sometime in the not so distant future.

@michaelstaib
Copy link
Member

Raise the PR and we will have a look. I will ping @PascalSenn once the PR is ready.

SeanTAllen added a commit to SeanTAllen/graphql-platform that referenced this issue Mar 26, 2024
When setting up filtering and sorting with UseFiltering and UseSorting,
there are 3 different code paths that are traversed. Prior to
this change, all 3 of them required that a convention be registered otherwise
an exception would occur. However, for both filtering and sorting, only 1 of
the code paths actually needs a convention. The other two do not use the
convention at all.

This commit changes the filtering and sorting object field descriptor extensions
to attempt to get a convention only within the block of code that requires a
convention. By making this small change, the other two code paths that do not
require a convention will no longer encounter an exception. Instead, filtering
and sorting can be set up as expected.

This commit partially addresses ChilliCream#6983. We are able locally, with these changes
applied, to use filtering and sorting from a type module by calling the versions
of `UseFiltering` and `UseSorting` that take a type. Without the changes in this
commit, we get the "no convention registered" error.

This change does not close ChilliCream#6983 as it is still possible to encounter the "no
convention registered" exception and its suggested remedy of
"Call `AddFiltering()` on the schema builder" doesn't work.

From what we can see, and have detailed in ChilliCream#6983, no conventions registered with
the schema builder either directly or the defaults provided by `AddFiltering`
and `AddSorting` are available at the time that a type module is run.

The result of this commit is to allow the usage `UseFiltering` and `UseSorting`
with types created via a type module. The other option we are aware of is
detailed by @jimitndiaye in ChilliCream#6975.
Jimit routes around the convention problem by not using a convention at all
in his custom middleware. A close examination of `UseFilteringInternal` at
ChilliCream#6975 (reply in thread)
will show this lack of convention usage as key to his fix. Instead of going
the custom middleware route, we made these two small changes to Hot Chocolate.
We prefer this approach.
SeanTAllen added a commit to SeanTAllen/graphql-platform that referenced this issue Mar 26, 2024
When setting up filtering and sorting with UseFiltering and UseSorting,
there are 3 different code paths that are traversed. Prior to
this change, all 3 of them required that a convention be registered otherwise
an exception would occur. However, for both filtering and sorting, only 1 of
the code paths actually needs a convention. The other two do not use the
convention at all.

This commit changes the filtering and sorting object field descriptor extensions
to attempt to get a convention only within the block of code that requires a
convention. By making this small change, the other two code paths that do not
require a convention will no longer encounter an exception. Instead, filtering
and sorting can be set up as expected.

This commit partially addresses ChilliCream#6983. We are able locally, with these changes
applied, to use filtering and sorting from a type module by calling the versions
of `UseFiltering` and `UseSorting` that take a type. Without the changes in this
commit, we get the "no convention registered" error.

This change does not close ChilliCream#6983 as it is still possible to encounter the "no
convention registered" exception and its suggested remedy of
"Call `AddFiltering()` on the schema builder" doesn't work.

From what we can see, and have detailed in ChilliCream#6983, no conventions registered with
the schema builder either directly or the defaults provided by `AddFiltering`
and `AddSorting` are available at the time that a type module is run.

The result of this commit is to allow the usage `UseFiltering` and `UseSorting`
with types created via a type module. The other option we are aware of is
detailed by @jimitndiaye in ChilliCream#6975.
Jimit routes around the convention problem by not using a convention at all
in his custom middleware. A close examination of `UseFilteringInternal` at
ChilliCream#6975 (reply in thread)
will show this lack of convention usage as key to his fix. Instead of going
the custom middleware route, we made these two small changes to Hot Chocolate.
We prefer this approach.
@SeanTAllen
Copy link
Contributor Author

@michaelstaib the PR is open: #7019.

michaelstaib pushed a commit that referenced this issue May 21, 2024
When setting up filtering and sorting with UseFiltering and UseSorting,
there are 3 different code paths that are traversed. Prior to
this change, all 3 of them required that a convention be registered otherwise
an exception would occur. However, for both filtering and sorting, only 1 of
the code paths actually needs a convention. The other two do not use the
convention at all.

This commit changes the filtering and sorting object field descriptor extensions
to attempt to get a convention only within the block of code that requires a
convention. By making this small change, the other two code paths that do not
require a convention will no longer encounter an exception. Instead, filtering
and sorting can be set up as expected.

This commit partially addresses #6983. We are able locally, with these changes
applied, to use filtering and sorting from a type module by calling the versions
of `UseFiltering` and `UseSorting` that take a type. Without the changes in this
commit, we get the "no convention registered" error.

This change does not close #6983 as it is still possible to encounter the "no
convention registered" exception and its suggested remedy of
"Call `AddFiltering()` on the schema builder" doesn't work.

From what we can see, and have detailed in #6983, no conventions registered with
the schema builder either directly or the defaults provided by `AddFiltering`
and `AddSorting` are available at the time that a type module is run.

The result of this commit is to allow the usage `UseFiltering` and `UseSorting`
with types created via a type module. The other option we are aware of is
detailed by @jimitndiaye in #6975.
Jimit routes around the convention problem by not using a convention at all
in his custom middleware. A close examination of `UseFilteringInternal` at
#6975 (reply in thread)
will show this lack of convention usage as key to his fix. Instead of going
the custom middleware route, we made these two small changes to Hot Chocolate.
We prefer this approach.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working 🌶️ hot chocolate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants