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

feat(odata): add case insensitive option for OData search filter #761

Conversation

suvirbhargav
Copy link

From SO question, here is some changes for making odata search

@@ -345,6 +345,9 @@ export interface GridOption {
/** Do we want to enable pagination? Currently only works with a Backend Service API */
enablePagination?: boolean;

/** Odata case insensitive search */
caseInsensitiveOdataSearch?: boolean;
Copy link
Owner

@ghiscoding ghiscoding Sep 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather have you move the flag to the OData options, in the OdataOption interface since this is not a common grid option but rather an OData only option, the interface this should belong to is the one shown below

https://github.com/ghiscoding/slickgrid-universal/blob/master/packages/odata/src/interfaces/odataOption.interface.ts

and since that interface is related to OData, we can rename the flag to simply be caseInsensitive

@@ -428,7 +428,16 @@ export class GridOdataService implements BackendService {
} else if (operator === OperatorType.rangeExclusive || operator === OperatorType.rangeInclusive) {
// example:: (Name >= 'Bob' and Name <= 'Jane')
searchBy = this.filterBySearchTermRange(fieldName, operator, searchTerms);
} else if ((operator === '' || operator === OperatorType.contains || operator === OperatorType.notContains) &&
} else if (this._gridOptions && (this._gridOptions.caseInsensitiveOdataSearch || !this._gridOptions.hasOwnProperty('caseInsensitiveOdataSearch'))) {
Copy link
Owner

@ghiscoding ghiscoding Sep 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it necessary to add the flag check in the if condition, I would rather see it in a simpler way, something along the code shown below (untested)

+ if (this.options.caseInsensitive) {
+   searchBy = odataVersion >= 4 ? `contains(tolower(${fieldName}, ${searchValue}))` : `substringof(tolower(${searchValue}, ${fieldName}))`;
+ else {
    searchBy = odataVersion >= 4 ? `contains(${fieldName}, ${searchValue})` : `substringof(${searchValue}, ${fieldName})`;
+ }

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it also looks like the linter failed as well, make sure to have the correct indentation in your code. I use the Editorconfig VSCode extension and also auto-format on save when I run the project. We also need unit tests to cover the change, if you need help let me know, here's 1 of the test for substringof, I would expect to see at least 2 new unit tests covering your change for both contains(tolower(...)) and ``substringof(tolower(...))`

it('should return a query to filter a search value between an exclusive range of dates using 2 search terms and the "RangeExclusive" operator', () => {
const expectation = `$top=10&$filter=(substringof('abc', Company) and (UpdatedDate gt DateTime'2001-01-20T00:00:00Z' and UpdatedDate lt DateTime'2001-02-28T00:00:00Z'))`;
const mockColumnCompany = { id: 'company', field: 'company' } as Column;
const mockColumnUpdated = { id: 'updatedDate', field: 'updatedDate', type: FieldType.date } as Column;
const mockColumnFilters = {
company: { columnId: 'company', columnDef: mockColumnCompany, searchTerms: ['abc'], operator: 'Contains', type: FieldType.string },
updatedDate: { columnId: 'updatedDate', columnDef: mockColumnUpdated, searchTerms: ['2001-01-20', '2001-02-28'], operator: 'RangeExclusive', type: FieldType.dateIso },
} as ColumnFilters;
service.init(serviceOptions, paginationOptions, gridStub);
service.updateFilters(mockColumnFilters, false);
const query = service.buildQuery();
expect(query).toBe(expectation);
});

@ghiscoding
Copy link
Owner

it would be nice to get comments/reviews from @jr01, if possible, since he contributed some nice features in the past.

@ghiscoding ghiscoding changed the title added intial changes for case insensitiveness for odata search functi… feat(odata): ad case insensitive for odata search filter Sep 19, 2022
@ghiscoding ghiscoding changed the title feat(odata): ad case insensitive for odata search filter feat(odata): add case insensitive option for OData search filter Sep 19, 2022
@jr01
Copy link
Collaborator

jr01 commented Sep 20, 2022

it would be nice to get comments/reviews from @jr01, if possible, since he contributed some nice features in the past.

Our oData API (backed by SQL server) returns the same data for $filter=(contains(name, 'So') and $filter=(contains(name, 'so') ...

Usually case-insensitiveness is handled by the server. E.g., if using a SQL database you usually set case insensitive collation at the database level and (if you really have to) make exceptions at column level. In my business domain my users always want to search case insensitive and I have never encountered an exception.

A nice read: https://learn.microsoft.com/en-us/ef/core/miscellaneous/collations-and-case-sensitivity

So I wonder what the real use case is and if this PR is really needed. @suvirbhargav can you elaborate?

@suvirbhargav
Copy link
Author

@jr01 thanks for your reply. I will read it your link..yes, i have the oData, .net 4.7 and sql..no idea why my backend does not work with case insensitiveness. I will read a bit and update here.

@ghiscoding
Copy link
Owner

ghiscoding commented Sep 20, 2022

@suvirbhargav
What @jr01 wrote is totally valid, but I saw you wrote an extra comment on your original SO question (shown below) and if that was your way of dealing with case insensitive because you can't get your backend to do it or you simply don't have access to the SQL server, then I think it would still be better to add a new flag (because using Grid State to get filter keyup and change query seems horrible on the performance side of it). Adding a new flag is just couple lines of code and for most users the flag would simply remain disabled and not use it but if it really helps in fixing your problem then go ahead with the PR, but make sure to do the code changes I asked though.

Is it possible to get some callback everytime i type in filter area in my column ? Then I can just take care of casing in callback itself.

My answer to this in SO was: "you can get that via the Grid State"

@jr01 thanks for the comments and feedback, it's really helpful since I haven't used OData in a while 😉

Also a note for everyone, I'm currently working in the next major version of all SlickGrid libs (which will remove jQueryUI and replace it with SortableJS) and I'm expecting it to land in the next couple weeks (I just have to schedule it with the SlickGrid fork maintainer). See PR #756 ... so if you want to go ahead with this PR here, you better be quick at it because whenever I land a new major version, I stop supporting older versions

@suvirbhargav
Copy link
Author

it would be nice to get comments/reviews from @jr01, if possible, since he contributed some nice features in the past.

Our oData API (backed by SQL server) returns the same data for $filter=(contains(name, 'So') and $filter=(contains(name, 'so') ...

Usually case-insensitiveness is handled by the server. E.g., if using a SQL database you usually set case insensitive collation at the database level and (if you really have to) make exceptions at column level. In my business domain my users always want to search case insensitive and I have never encountered an exception.

A nice read: https://learn.microsoft.com/en-us/ef/core/miscellaneous/collations-and-case-sensitivity

So I wonder what the real use case is and if this PR is really needed. @suvirbhargav can you elaborate?

Yes, I have been reading the backend code this morning. I think @jr01 has done some needed changes in database at column level to make it case insensitive in response because this works for me /Customers?$count=true&$filter=(IsActive%20eq%20true%20and%20contains(Name,%20%27So%27)) but this one does not /Customers?$count=true&$filter=(IsActive%20eq%20true%20and%20contains(Name,%20%27so%27)). So I am missing something my sql side. I will see a bit more today in backend to see if i can fix it. Otherwise i will update here with frontend changes.

@suvirbhargav
Copy link
Author

By the way, as a side note: You both are awesome and thanks a ton to support such a nice library.

@ghiscoding
Copy link
Owner

ghiscoding commented Oct 18, 2022

@suvirbhargav

I just released Slickgrid-Universal v2.0.0 and also the external Angular & Aurelia-Slickgrid v5.0.0

Also I don't think this PR is required anymore so I decided to just close it. If you feel that it is still necessary for you, then feel free to reopen but you'll have to add unit tests, I praise on the fact that all my slickgrid libs have 100% unit tested code coverage and that took quite a lot of time and effort, so if you create a new PR, make sure to have unit tests 😉

Cheers

@ghiscoding ghiscoding closed this Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants