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
Comments
Why dont you just opt into our default convention? |
I tried that. Perhaps I did it wrong @michaelstaib. What is the proper way to do it? |
@michaelstaib can you clarify what "opt into our default convention" so I can verify that we did it and still encountered this issue? |
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: The convention that is looked up is only used in one of the 3 if statements that follow. In particular, at: If the lookup of the convention was moved into the 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. |
@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. |
Raise the PR and we will have a look. I will ping @PascalSenn once the PR is ready. |
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.
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.
@michaelstaib the PR is open: #7019. |
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.
Product
Hot Chocolate
Version
13.9.0
Link to minimal reproduction
https://github.com/SeanTAllen/hc-typemodule-filtering-bug
Steps to reproduce
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 thatAddFiltering
is called on the schema builder. So at minimum, the error message here is incorrect/misleading.Relevant log output
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.
The text was updated successfully, but these errors were encountered: