Repository navigation
feat(mapper): add case-insensitive per-map override - #336
Conversation
PR Summary
|
There was a problem hiding this comment.
Pull request overview
This PR adds a per-map override to control case-insensitive filtering behavior, allowing specific mapped fields to opt in/out independently of the global CaseInsensitiveFiltering configuration.
Changes:
- Extends mapping metadata (
IGMap) to carry an optional per-mapCaseInsensitiveoverride. - Threads the per-map override into query building so string filtering can be forced case-sensitive/insensitive per mapping.
- Adds documentation and a dedicated test suite covering per-map override behavior for
stringandList<string>mappings.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Gridify.Tests/IssueTests/CaseInsensitivePerMapTests.cs | Adds tests validating per-map case sensitivity overrides for string and list-of-string mappings. |
| src/Gridify/IGridifyMapper.cs | Adds optional caseInsensitive parameter to selected AddMap overloads. |
| src/Gridify/IGMap.cs | Adds CaseInsensitive property to maps to support per-map override behavior. |
| src/Gridify/GridifyMapper.cs | Persists the per-map CaseInsensitive override when creating GMap instances via AddMap. |
| src/Gridify/GMap.cs | Stores per-map CaseInsensitive override on concrete map implementation. |
| src/Gridify/CompositeGMap.cs | Stores per-map CaseInsensitive override on composite map implementation. |
| src/Gridify/Builder/LinqQueryBuilder.cs | Passes per-map override into nested-expression query building. |
| src/Gridify/Builder/BaseQueryBuilder.cs | Incorporates per-map override when determining case-insensitive handling for string filtering. |
| docs/pages/guide/gridifyMapper.md | Documents the new caseInsensitive per-map override and provides usage example. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @moxplod, thanks for the PR, I'll review it soon, meanwhile if you have time, please check the Copilot comments and let me know if you think they are irrelevant (happens a lot :)) |
|
@alirezanet yea - i went through and added my responses to the copilot comments. Let me know if you agree with my comments. If you are on the same page, I will make all the changes at once. Thanks! |
This comment was marked as outdated.
This comment was marked as outdated.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@alirezanet Updated all the changes requested. Please take a look. |
alirezanet
left a comment
There was a problem hiding this comment.
Hi @moxplod, thanks again for the PR! I spent some time reviewing this and I like the feature but because this touches the public mapper interfaces/API, I’m planning to target it for v3, so I’m not worried about the breaking interface/signature changes here.
I also thought more about your point around /i having the highest precedence. I agree that it makes sense if we treat /i as the client’s explicit intent and mapper/global settings as defaults.
However, I think for this feature we also need a way for the API/mapper author to enforce case behavior for specific maps. The original use case mentions columns that should stay case-sensitive for performance reasons. If /i always wins, then caseInsensitive: false is not really an override; it is a default that clients can bypass.
Since this is going into v3, I think we may want to model this more explicitly with an enum instead of bool?, and use the same concept for global config, mapper config, and map config:
public enum GridifyCaseSensitivity
{
Default,
CaseSensitive,
CaseInsensitive
}The meaning would be:
Default= do not force behavior; allow lower-priority settings or query-level/ito decideCaseSensitive= force case-sensitiveCaseInsensitive= force case-insensitive
Then resolution could be:
- Map setting
- Mapper setting
- Global setting
- Query-level
/i - Built-in default case-sensitive behavior
This way, users who want to respect /i can leave the setting as Default, while users who need server-side enforcement can choose CaseSensitive or CaseInsensitive.
I also found a few things I think we should address before merging:
- Preserve the setting when composing nested mappers
In AddNestedMapperInternal, the nested map setting is currently not carried over:
AddMap(compositeKey, composedExpression, nestedMap.Convertor, overrideIfExists);So a nested map configured as case-sensitive could lose that behavior when composed into the parent mapper. I think we should pass the nested map’s case-sensitivity value through here. Composite nested maps may need the same treatment.
- Make the API support consistent
Since this is v3, I think the feature should either be supported consistently across map creation paths or the limitations should be documented. For example:
- indexed/keyed
AddMapoverloads AddCompositeMap- possibly
IQueryBuilder.AddMap, if that should mirror mapper behavior
- Add a few more tests
Suggested coverage:
- map-level
CaseSensitivebeats query-level/i - map-level setting beats mapper/global setting
- nested mapper preserves case-sensitive/case-insensitive settings
- composite map behavior
- indexed/keyed map behavior if supported
Overall, I like the direction but feel like it still need a bit more refining... please let me know what do you think
|
Hey @alirezanet I personally prefer having the Regarding the nested mapper - I have not used that feature, so I was not aware of testing that out. But it makes sense to follow the same pattern with that. I can look for the tests that cover that and add this feature there. Also what about CompositeMap (see comment below)? Regarding the more tests - Happy to add them! Regarding Make the API support consistent - This one might be a bit tricky. I can see wanting to add this to CompositeMap as well. The CompositeMap interface would need to change to support this and will be a breaking change as it takes multiple expressions and we could have case-sensitive per expression. |
|
Hi @moxplod,
actually I also prefer
I was thinking to adding another overload, instead of the optional parameter, could easily solve it, but yeah, I'm not sure. Regarding the multiple expressions in a CompositeMap, I don't think we should worry about it too much. I consider the whole CompositeMap a single map in the Gridify API, so, if a user defines a composite map instead of a regular map, they can only provide a single case-sensitivity config for it (which we can internally reuse per map). |
|
Hey @alirezanet - got busy with work and just getting back to this. I agree that the current design is quite convoluted and hard to track and maintain. When are you thinking of doing v3? I would like to get this merged into v2 or v3 as you prefer as I am currently using my updated version from a local build. |
|
Hey @alirezanet, Any chance we could just get this into v2 as-is for a minor release (like 2.20), and leave the bigger enum/redesign stuff for v3?
It's tested, working great in production for me for the last many months, and safe to ship. What do you think about getting it in for v2? |
|
Hi @moxplod, |
|
Thank you Would be great to get the local nuget packet removed and use the official ones again. |
|
Just release the v2.20.0. |
Description
I am working with some large tables with a lot of columns, I have had to specifically decide which columns a user should be allowed to query in a case-insensitive manner. We do not want the user to have to enter /i in the queries.
Some columns in our case are needed to be Case-Sensitive for performance reasons. Exact strings matching is much faster in postgres and in clickhouse with large data. Than case-insensitive matching.
Type of change
Checklist