feat(Filter): add StringComparison parameter #7449
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideAdds optional StringComparison support to filter expression generation and QueryPageOptions helpers, propagating the parameter through the lambda-building pipeline, updating string Contains handling accordingly, and extending unit tests plus a small DynamicObject helper refinement. Sequence diagram for QueryPageOptions ToFilterFunc with StringComparisonsequenceDiagram
actor Caller
participant QPO as QueryPageOptions
participant QPOExt as QueryPageOptionsExtensions
participant FKA as FilterKeyValueAction
participant LE as LambdaExtensions
Caller->>QPOExt: ToFilterFunc~TItem~(QPO, comparison)
QPOExt->>QPOExt: ToFilter(QPO)
QPOExt-->>FKA: returns FilterKeyValueAction
QPOExt->>LE: GetFilterLambda~TItem~(FKA, comparison)
activate LE
LE->>LE: GetFilterLambda~TItem~(filters, logic, comparison)
loop each FilterKeyValueAction
LE->>LE: GetInnerFilterLambda~TItem~(filter, comparison)
LE->>LE: GetFilterExpression~TItem~(filter, prop, fieldExpression, parameter, comparison)
alt string Contains or NotContains
LE->>LE: GetExpression(filter, left, comparison)
alt comparison has value
LE->>LE: ContainsWidthComparison(left, right, comparison)
else no comparison
LE->>LE: ContainsWithoutComparison(left, right)
end
else other FilterAction
LE->>LE: GetExpression(filter, left, comparison)
end
end
LE-->>QPOExt: Expression~Func~TItem,bool~~
deactivate LE
QPOExt->>QPOExt: Compile Expression to Func
QPOExt-->>Caller: Func~TItem,bool~
Class diagram for StringComparison-aware filter generationclassDiagram
class QueryPageOptions {
}
class FilterKeyValueAction {
+FilterLogic FilterLogic
+List~FilterKeyValueAction~ Filters
+string FieldKey
+object FieldValue
+FilterAction FilterAction
}
class LambdaExtensions {
<<static>>
+Func~TItem,bool~ GetFilterFunc~TItem~(FilterKeyValueAction filter, StringComparison? comparison)
+Expression~Func~TItem,bool~~ GetFilterLambda~TItem~(FilterKeyValueAction filter, StringComparison? comparison)
-Expression~Func~TItem,bool~~ GetFilterLambda~TItem~(IEnumerable~FilterKeyValueAction~ filters, FilterLogic logic, StringComparison? comparison)
-Expression~Func~TItem,bool~~ GetInnerFilterLambda~TItem~(FilterKeyValueAction filter, StringComparison? comparison)
-Expression~Func~TItem,bool~~ GetFilterExpression~TItem~(FilterKeyValueAction filter, PropertyInfo prop, Expression fieldExpression, ParameterExpression parameter, StringComparison? comparison)
-Expression GetExpression(FilterKeyValueAction filter, Expression left, StringComparison? comparison)
-BinaryExpression Contains(Expression left, Expression right, StringComparison? comparison)
-BinaryExpression ContainsWithoutComparison(Expression left, Expression right)
-BinaryExpression ContainsWidthComparison(Expression left, Expression right, StringComparison comparison)
}
class QueryPageOptionsExtensions {
<<static>>
+FilterKeyValueAction ToFilter(QueryPageOptions option)
+Func~TItem,bool~ ToFilterFunc~TItem~(QueryPageOptions option, StringComparison? comparison)
+Expression~Func~TItem,bool~~ ToFilterLambda~TItem~(QueryPageOptions option, StringComparison? comparison)
}
QueryPageOptionsExtensions --> QueryPageOptions : uses
QueryPageOptionsExtensions --> FilterKeyValueAction : builds
QueryPageOptionsExtensions ..> LambdaExtensions : calls
LambdaExtensions --> FilterKeyValueAction : reads filters
FilterKeyValueAction --> FilterKeyValueAction : nested Filters
Flow diagram for Contains helper with optional StringComparisonflowchart TD
A["Start Contains(left, right, comparison)"] --> B{"comparison has value?"}
B -- Yes --> C["Call ContainsWidthComparison(left, right, comparison.Value)"]
B -- No --> D["Call ContainsWithoutComparison(left, right)"]
C --> E["Build Expression: left != null && left.Contains(right, comparison)"]
D --> F["Build Expression: left != null && left.Contains(right)"]
E --> G["Return BinaryExpression"]
F --> G["Return BinaryExpression"]
G --> H["End"]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7449 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 749 749
Lines 32911 32919 +8
Branches 4573 4574 +1
=========================================
+ Hits 32911 32919 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The new helper name
ContainsWidthComparisonappears to be a typo and is confusing; consider renaming it to something likeContainsWithComparisonfor clarity and consistency. - The XML docs on the new
comparisonparameters say they do not support EF CoreWherequeries, butToFilterLambdais still documented as recommended for EF Core; consider clarifying this contract (or enforcing it at runtime) so consumers don’t accidentally pass aStringComparisoninto EF scenarios that cannot translate it.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new helper name `ContainsWidthComparison` appears to be a typo and is confusing; consider renaming it to something like `ContainsWithComparison` for clarity and consistency.
- The XML docs on the new `comparison` parameters say they do not support EF Core `Where` queries, but `ToFilterLambda` is still documented as recommended for EF Core; consider clarifying this contract (or enforcing it at runtime) so consumers don’t accidentally pass a `StringComparison` into EF scenarios that cannot translate it.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Extensions/LambdaExtensions.cs:236` </location>
<code_context>
return Expression.AndAlso(Expression.NotEqual(left, Expression.Constant(null)), Expression.Call(left, method, right));
}
+ private static BinaryExpression ContainsWidthComparison(this Expression left, Expression right, StringComparison comparison)
+ {
+ var method = typeof(string).GetMethod("Contains", [typeof(string), typeof(StringComparison)])!;
</code_context>
<issue_to_address>
**nitpick (typo):** Method name `ContainsWidthComparison` appears to have a typo and should likely be `ContainsWithComparison`.
Renaming it to `ContainsWithComparison` will better reflect its purpose and reduce confusion for future readers. Since it’s private, the change is localized to this file.
Suggested implementation:
```csharp
private static BinaryExpression Contains(this Expression left, Expression right, StringComparison? comparison) => comparison.HasValue
? ContainsWithComparison(left, right, comparison.Value)
: ContainsWithoutComparison(left, right);
```
```csharp
private static BinaryExpression ContainsWithComparison(this Expression left, Expression right, StringComparison comparison)
```
</issue_to_address>
### Comment 2
<location> `test/UnitTest/Extensions/LambadaExtensionsTest.cs:13-39` </location>
<code_context>
public class LambadaExtensionsTest : BootstrapBlazorTestBase
{
+ [Fact]
+ public void GetFilterFunc_Comparison()
+ {
+ var filter = new FilterKeyValueAction()
+ {
+ FieldKey = "Name",
+ FilterAction = FilterAction.Contains,
+ FieldValue = "T"
+ };
+
+ var foos = new Foo[]
+ {
+ new() { Name = "Test1" },
+ new() { Name = "test2" },
+ };
+
+ var items = foos.Where(filter.GetFilterFunc<Foo>());
+ Assert.Single(items);
+
+ // 忽略大小写
+ items = foos.Where(filter.GetFilterFunc<Foo>(StringComparison.OrdinalIgnoreCase));
+ Assert.Equal(2, items.Count());
+ }
+
[Fact]
</code_context>
<issue_to_address>
**suggestion (testing):** Extend StringComparison tests to cover NotContains and null string values.
Please also cover:
- `FilterAction.NotContains` with a specified `StringComparison`, asserting it returns only items that do *not* contain the value when case-insensitive.
- A case where `Name` is `null`, confirming the null-safe `Contains` path behaves identically with and without `StringComparison`.
This will better exercise the new comparison-aware branch and its null handling.
```suggestion
public class LambadaExtensionsTest : BootstrapBlazorTestBase
{
[Fact]
public void GetFilterFunc_Comparison()
{
var filter = new FilterKeyValueAction()
{
FieldKey = "Name",
FilterAction = FilterAction.Contains,
FieldValue = "T"
};
var foos = new Foo[]
{
new() { Name = "Test1" },
new() { Name = "test2" },
new() { Name = "Foo" },
new() { Name = null },
};
// FilterAction.Contains (default comparison)
var items = foos.Where(filter.GetFilterFunc<Foo>());
Assert.Single(items);
// 忽略大小写 - FilterAction.Contains
items = foos.Where(filter.GetFilterFunc<Foo>(StringComparison.OrdinalIgnoreCase));
Assert.Equal(2, items.Count());
// FilterAction.NotContains with StringComparison - should return only items
// that do NOT contain the value when evaluated case-insensitively
filter.FilterAction = FilterAction.NotContains;
var notContainsItems = foos.Where(filter.GetFilterFunc<Foo>(StringComparison.OrdinalIgnoreCase)).ToArray();
Assert.Single(notContainsItems);
Assert.All(notContainsItems, i => Assert.Equal("Foo", i.Name));
// Null-safe Contains: ensure entries with Name == null are treated identically
// with and without a specified StringComparison
filter.FilterAction = FilterAction.Contains;
var itemsDefaultComparison = foos.Where(filter.GetFilterFunc<Foo>()).ToArray();
var itemsWithComparison = foos.Where(filter.GetFilterFunc<Foo>(StringComparison.OrdinalIgnoreCase)).ToArray();
Assert.Equal(
itemsDefaultComparison.Any(i => i.Name is null),
itemsWithComparison.Any(i => i.Name is null)
);
}
[Fact]
```
</issue_to_address>
### Comment 3
<location> `test/UnitTest/Extensions/QueryPageOptionsExtensionsTest.cs:42-45` </location>
<code_context>
+ var items = foos.Where(filter.GetFilterFunc<Foo>());
+ Assert.Single(items);
+
+ // 忽略大小写
+ items = foos.Where(filter.GetFilterFunc<Foo>(StringComparison.OrdinalIgnoreCase));
+ Assert.Equal(2, items.Count());
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for ToFilterLambda with StringComparison and verify behavior parity.
To fully exercise the new API surface and ensure parity, please also add tests that:
- Use `option.ToFilterLambda<Foo>(StringComparison.OrdinalIgnoreCase)` and apply it via `.Where(expr.Compile())`, asserting it matches `ToFilterFunc` results for string field searches.
- Verify that `comparison: null` for both `ToFilterFunc` and `ToFilterLambda` behaves identically to the previous overloads without a comparison parameter (backward compatibility).
This will confirm both overloads behave consistently with the new `StringComparison` support.
Suggested implementation:
```csharp
expected = _foos.Where(predicate);
Assert.Equal(2, expected.Count());
// 忽略大小写
predicate = option.ToFilterFunc<Foo>(StringComparison.OrdinalIgnoreCase);
expected = _foos.Where(predicate);
Assert.Equal(4, expected.Count());
// ToFilterLambda with StringComparison parity with ToFilterFunc
var exprIgnoreCase = option.ToFilterLambda<Foo>(StringComparison.OrdinalIgnoreCase);
var lambdaIgnoreCaseResult = _foos.Where(exprIgnoreCase.Compile());
Assert.Equal(expected.Count(), lambdaIgnoreCaseResult.Count());
// comparison: null behaves the same as the overload without comparison for ToFilterFunc
var predicateWithNullComparison = option.ToFilterFunc<Foo>(null);
var expectedWithNullComparison = _foos.Where(predicateWithNullComparison);
var expectedWithoutComparison = _foos.Where(option.ToFilterFunc<Foo>());
Assert.Equal(expectedWithoutComparison.Count(), expectedWithNullComparison.Count());
// comparison: null behaves the same as the overload without comparison for ToFilterLambda
var exprWithNullComparison = option.ToFilterLambda<Foo>(null);
var exprWithoutComparison = option.ToFilterLambda<Foo>();
var lambdaWithNullComparisonResult = _foos.Where(exprWithNullComparison.Compile());
var lambdaWithoutComparisonResult = _foos.Where(exprWithoutComparison.Compile());
Assert.Equal(lambdaWithoutComparisonResult.Count(), lambdaWithNullComparisonResult.Count());
option.Searches.Clear();
option.Searches.Add(new SearchFilterAction("Name", "Mock"));
predicate = option.ToFilterFunc<Foo>();
```
If `System.Linq.Expressions` is not already imported at the top of `QueryPageOptionsExtensionsTest.cs`, add:
`using System.Linq.Expressions;`
so that `ToFilterLambda` returning an expression compiles cleanly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds support for case-insensitive string filtering by introducing an optional StringComparison parameter to filter methods. This enhancement allows consumers to specify string comparison behavior when filtering data collections, addressing issue #7441.
Key changes:
- Added optional
StringComparisonparameter to filter extension methods (GetFilterFunc,GetFilterLambda,ToFilterFunc,ToFilterLambda) - Implemented support for
StringComparisonin theContainsfilter action through new helper methods - Added comprehensive test coverage for case-insensitive filtering scenarios
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/BootstrapBlazor/Extensions/LambdaExtensions.cs | Core implementation: Added StringComparison parameter throughout the filter expression building pipeline and created helper methods to handle string contains operations with comparison options |
| src/BootstrapBlazor/Extensions/QueryPageOptionsExtensions.cs | Added StringComparison parameter to public API methods ToFilterFunc and ToFilterLambda with documentation noting EFCore compatibility |
| test/UnitTest/Extensions/LambadaExtensionsTest.cs | Added new test GetFilterFunc_Comparison to verify case-insensitive filtering behavior and improved code quality with TryGetValue pattern |
| test/UnitTest/Extensions/QueryPageOptionsExtensionsTest.cs | Extended existing test with case-insensitive scenario, verifying 4 results when ignoring case vs 2 with case-sensitive comparison |
| src/BootstrapBlazor/BootstrapBlazor.csproj | Version bumped from 10.2.0 to 10.2.1-beta01 for this feature addition |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static Func<TItem, bool> ToFilterFunc<TItem>(this QueryPageOptions option, StringComparison? comparison = null) => option.ToFilterLambda<TItem>(comparison).Compile(); | ||
|
|
||
| /// <summary> | ||
| /// 将 QueryPageOptions 过滤条件转换为 <see cref="Expression{TDelegate}"/> 表达式"/> 推荐 EFCore <see cref="IQueryable"/> 使用 |
There was a problem hiding this comment.
The documentation comment contains a typo with an extra closing quote after the closing tag. The text should be: '将 QueryPageOptions 过滤条件转换为 表达式 推荐 EFCore 使用' (removing the extra "/> after 表达式).
| return Expression.AndAlso(Expression.NotEqual(left, Expression.Constant(null)), Expression.Call(left, method, right)); | ||
| } | ||
|
|
||
| private static BinaryExpression ContainsWidthComparison(this Expression left, Expression right, StringComparison comparison) |
There was a problem hiding this comment.
The method name has a typo: "ContainsWidthComparison" should be "ContainsWithComparison". "Width" refers to dimensions/size, while "With" is the correct preposition indicating inclusion.
|
|
||
| private static BinaryExpression Contains(this Expression left, Expression right) | ||
| private static BinaryExpression Contains(this Expression left, Expression right, StringComparison? comparison) => comparison.HasValue | ||
| ? ContainsWidthComparison(left, right, comparison.Value) |
There was a problem hiding this comment.
The method name has a typo: "ContainsWidthComparison" should be "ContainsWithComparison". "Width" refers to dimensions/size, while "With" is the correct preposition indicating inclusion.
Comment on lines
+37
to
+38
| /// <param name="comparison"><see cref="StringComparison"/> 实例，此方法不支持 EFCore Where 查询</param> | ||
| public static Func<TItem, bool> GetFilterFunc<TItem>(this FilterKeyValueAction filter, StringComparison? comparison = null) => filter.GetFilterLambda<TItem>(comparison).Compile(); |
There was a problem hiding this comment.
Missing return value documentation. The parameter documentation was added, but the <returns> section was removed. Add: /// <returns></returns> to maintain complete API documentation.
| /// <param name="filter"></param> | ||
| /// <returns></returns> | ||
| public static Expression<Func<TItem, bool>> GetFilterLambda<TItem>(this FilterKeyValueAction filter) | ||
| /// <param name="comparison"><see cref="StringComparison"/> 实例，此方法不支持 EFCore Where 查询</param> |
There was a problem hiding this comment.
Missing return value documentation. The parameter documentation was added, but the <returns> section was removed. Add: /// <returns></returns> to maintain complete API documentation.
Suggested change
| /// <param name="comparison"><see cref="StringComparison"/> 实例，此方法不支持 EFCore Where 查询</param> | |
| /// <param name="comparison"><see cref="StringComparison"/> 实例，此方法不支持 EFCore Where 查询</param> | |
| /// <returns></returns> |
| expected = _foos.Where(predicate); | ||
| Assert.Equal(2, expected.Count()); | ||
|
|
||
| // 忽略大小写 |
There was a problem hiding this comment.
The comment is in Chinese and should be translated to English for consistency with the rest of the codebase. Consider changing to: "// Case-insensitive comparison" or "// Ignore case"
Suggested change
| // 忽略大小写 | |
| // Case-insensitive comparison |
| var items = foos.Where(filter.GetFilterFunc<Foo>()); | ||
| Assert.Single(items); | ||
|
|
||
| // 忽略大小写 |
There was a problem hiding this comment.
The comment is in Chinese and should be translated to English for consistency with the rest of the codebase. Consider changing to: "// Case-insensitive comparison" or "// Ignore case"
Suggested change
| // 忽略大小写 | |
| // Ignore case |
| } | ||
|
|
||
| /// <summary> | ||
| /// 将 QueryPageOptions 过滤条件转换为 where 条件中的参数 <see cref="Func{T, TResult}"/>"/> 推荐 Linq 使用 |
There was a problem hiding this comment.
The documentation comment contains a typo with an extra closing quote and forward slash. The text should be: '将 QueryPageOptions 过滤条件转换为 where 条件中的参数 推荐 Linq 使用' (removing the extra closing quote after the closing tag).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Link issues
fixes #7441
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add optional StringComparison support to filter-building helpers and propagate it through query option extensions for more flexible string matching.
New Features:
Enhancements:
Tests: