-
Notifications
You must be signed in to change notification settings - Fork 181
Use the configured HandleNullPropagationOption #1538
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
base: main
Are you sure you want to change the base?
Use the configured HandleNullPropagationOption #1538
Conversation
|
@microsoft-github-policy-service agree |
|
/AzurePipelines run |
|
No pipelines are associated with this pull request. |
|
|
||
| private QueryBinderContext CreateSubContext(QueryBinderContext context, ComputeClause computeClause, Type clrElementType, | ||
| HandleNullPropagationOption option = HandleNullPropagationOption.True) | ||
| private QueryBinderContext CreateSubContext(QueryBinderContext context, ComputeClause computeClause, Type clrElementType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not pass the HandleNullPropagation in the caller. For example:
- In
SelectExpandBinder, you can pass as follows:// rest of code .. QueryBinderContext subContext = CreateSubContext(context, computeClause, clrElementType, settings.HandleNullPropagation); // rest of code ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @WanjohiSammy. The reason why I didn't pass HandleNullPropagation in the caller is because CreateSubContext originally clones the settings:
ODataQuerySettings newSettings = new ODataQuerySettings();
newSettings.CopyFrom(context.QuerySettings);The CopyFrom method does copy the HandleNullPropagation value:
internal void CopyFrom(ODataQuerySettings settings)
{
// ...
HandleNullPropagation = settings.HandleNullPropagation;
// ...
}I kept this approach because the original code also cloned context.QuerySettings, and I wanted to preserve that behavior unless there's a specific reason to move to the overload that takes HandleNullPropagation directly. If using the explicit overload is preferred for clarity, I can switch to that as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanielBertocci Could you add some tests for this fix.
Fix unexpected behavior when HandleNullPropagationOption is configured in ODataQuerySettings
Context
Although a developer can set the value of
context.QuerySettings.HandleNullPropagationOptionusingor using DI with
ODataQuerySettingsIn the current implementation, when execution enters
SelectExpandBinderthe setting is sometimes forced totrue, regardless of the developer's configuration.This is my first time contributing to this library, although I have used it extensively. I tried to trace the history of this behavior but couldn't find a clear reason for forcing the option. The oldest commit I found that introduced this change is Commit: Merge routing, formatter, query into one from five years ago, and the behavior appears to have been retained through subsequent refactorings.
@xuzhg — you authored that commit and have recently worked on this file, so perhaps you can help if I've missed something.
All the tests are green, and I don't think if it is valuable to introduce a new one to validate this behavior.
Current workaround (project side)
I register a custom
FilterBinderthat forces theHandleNullPropagationsetting:How I encountered this issue
I have a project that uses Entity Framework with PostgreSQL. I needed to build OData queries involving many-to-many relationships, for example:
nodes?$expand=children($filter=children/$count gt 0)customers?$expand=orders($filter=products/$count eq 0)customers?$expand=orders($filter=products/any(c: c/name eq 'Sample'))In case 1, even though
HandleNullPropagationOptionwas forced totrue, the expression that joined the same table did not cause issues. However, when joining different tables (cases 2 and 3), the generated expression included null checks that Entity Framework could not translate.