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(filters): add StartsWith/EndsWith (a*z) to OData/GraphQL #1532

Merged
merged 4 commits into from
May 18, 2024

Conversation

ghiscoding
Copy link
Owner

@ghiscoding ghiscoding commented May 17, 2024

OData Service

image

GraphQL Service

image

Copy link

stackblitz bot commented May 17, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

codecov bot commented May 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.8%. Comparing base (b800da3) to head (68fe740).

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1532     +/-   ##
========================================
+ Coverage    99.8%   99.8%   +0.1%     
========================================
  Files         198     198             
  Lines       21636   21661     +25     
  Branches     7227    6965    -262     
========================================
+ Hits        21575   21600     +25     
  Misses         61      61             

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ghiscoding
Copy link
Owner Author

ghiscoding commented May 17, 2024

@zewa666 here you go, the PR to add StartsWith/EndsWith to both OData and GraphQL, I think the query are ok (see above), but it would be nice to have confirmation and a review from you. Also a reminder that if you want to test without doing a git checkout, you can use the stackblitz link above

Couple of things to note:

  1. the previous PR where I introduced this new operator, I forgot to test Grid State & Preset and luckily the OData has that, so I was able to spot the error. Basically, the error was that even if we split the value for the filter to work with StartsWith+EndsWith, I still have to keep the search term untouched as ab*cd for the Grid State/Preset to work as intended.
  2. I found a bug in the OData example 9 (also exists in live demo), if you change the Name column filter and then refresh the page, after the reload the data isn't showing and the pagination is totally wrong showing like Page 3 of 1. I assume it's only a demo issue (since you never reported that kind of problem with OData + Grid Preset) and I will this in a separate PR.
  3. ohh while writing this post, I just remembered that we have 2 OData examples in universal and I forgot to update that 2nd one with OData+RxJS. I will update it tomorrow, time for bed now (though I'm off tomorrow :))

I don't think there's anything left to do and fix after that? Did you wanted to push something else before a new release?

@zewa666
Copy link
Contributor

zewa666 commented May 17, 2024

I'll give the PR a proper look tonight.

sorting had a couple of issues due to columnName vs columnId usage but I've fixed that a while ago in a PR, perhaps this is another bug.

and yep gridstate is something that I'll have to inspect in more detail anyways. For our usecase with ForeignKey columns I always display the referenced tables label col but write as value the id col. so I'd need the same for the filter persistence, meaning storing the whole object instead of only the value. will let you know once I dig into that

@ghiscoding
Copy link
Owner Author

ghiscoding commented May 18, 2024

So I found the pagination bug and fixed it in PR #1534 , it was related to Grid Preset of a page number that was out of boundaries (displayed as gif in the PR). It's probable that the issue might not have shown with a real backend server since typically the server will never return a page number that is out of boundaries (e.g. page size of 20, page number is set to 2, but we have less than 20 items so technically being in page 2 is out of bound and impossible, that only happened when loading Grid Preset)

and yep gridstate is something that I'll have to inspect in more detail anyways. For our usecase with ForeignKey columns I always display the referenced tables label col but write as value the id col. so I'd need the same for the filter persistence, meaning storing the whole object instead of only the value. will let you know once I dig into that

I designed Grid State/Preset to be easily stringified so that it can be saved in DB/LocalStorage and reused easily by parsing string to an object. Anyway you most probably know all of this already 😉

So I updated the OData with RxJS and I'm pretty much done with this PR and I think that I have addressed all other issues as well.

@zewa666
Copy link
Contributor

zewa666 commented May 18, 2024

sry, didnt manage to look at it yesterday but plan to do so tonight

@ghiscoding
Copy link
Owner Author

no worries, it's weekend after all... enjoy it 😉

@zewa666
Copy link
Contributor

zewa666 commented May 18, 2024

so what stood out while looking at the examples is that no matter what Operator I choose, as soon as I add a star and type on it will move to starts/endswith combo. I've realized though that you can disable the feature toggling off autoParseInputFilterOperator. Thats pretty neat.

Aside from that I think this PR looks totally fine, nice addition.


While looking at the example 14 though I came up with a quick example of what I thought the SQL Like style would be:

searchSQLLike(value, searchString) {
    // Replace * with .* to convert to regex pattern
    const regexPattern = '^' + searchString.replace(/\*/g, '.*') + '$';

    const regex = new RegExp(regexPattern); // add 'i' as second argument for insensitive

    return regex.test(value);
}

adding the insenstive operator would match ILIKE's behavior.

To quickly try it out I've added the additional condition in the filterPredicate of example 14

else if (searchFilterArgs.searchTerms[0]?.toString().includes('*')) {
  return this.searchSQLLike(cellValue, searchFilterArgs.searchTerms[0]);
}

So now writing something like a*0 would yield nothing but *a*0 would all Tasks ending with a Zero (0, 10).
Make it *a*0* and also 101, 102 ... would be included

Now if we'd like to have the same feature available in OData, one possible solution would be to use the matchesPattern string function. Note that ^ needs to be escaped with %5EA

searchBy = `matchesPattern(${fieldName}, '%5EA${searchValue.replace(/\*/g, '.*')}$')`;

@ghiscoding
Copy link
Owner Author

so what stood out while looking at the examples is that no matter what Operator I choose, as soon as I add a star and type on it will move to starts/endswith combo. I've realized though that you can disable the feature toggling off autoParseInputFilterOperator. Thats pretty neat.

hmm I even forgot that I added autoParseInputFilterOperator, it must have been a while ago lol... but yeah in theory these special symbols are always in effect even when compound operators are in play.

For the example 14, I have to mention again that was only meant for demo purposes (and to answer that SO question), though I do understand it might open ideas for other filter operators and others, the Example 14 remains an example which I don't really want to extend more than what it is.

I guess what could be done in future PR is to add an extra Compound Operator (maybe % since it must 1-2 char)

So now writing something like a*0 would yield nothing but *a*0 would all Tasks ending with a Zero (0, 10).
Make it *a*0* and also 101, 102 ... would be included

I not familiar with the ILIKE, is that related here? I mean the *a*0 seems more like %a%0 which is "a" anywhere and finishes with "0". That is also again going back to an original question/comment that I asked earlier which was to not support more than 1 * symbol at a time. This seems too complex for a regular input filter (I think supporting a*z, a* and *z is more than enough and anything else should be separate).

For now, I will merge the PR and anything else could be added in separate PRs

@ghiscoding ghiscoding merged commit 237d6a8 into master May 18, 2024
6 checks passed
@ghiscoding ghiscoding deleted the feat/starts-with-ends-backend-services branch May 18, 2024 19:57
@zewa666
Copy link
Contributor

zewa666 commented May 18, 2024

sorry for the confusion.

example 14 is absolutely fine as it is. My example function was more along the lines of mimicking the exact behavior of SQL LIKE, because in combo with matchesPattern, it's a simple mean for the backend to translate it back into SQL and that way receive %a%0 from ^*a*0$, when sent via OData

ILIKE is just the case insensitive form of LIKE

And yes you're right that the simple cases with a*, *z and a*z are absolutely enough. I'm just leaving this here as a reminder for how to get the multi wildcard feature supported, which I'm going to implement via the previously mentioned Operator.Custom

@ghiscoding
Copy link
Owner Author

ghiscoding commented May 18, 2024

ILIKE is just the case insensitive form of LIKE

ah ok cool, I wasn't aware, at least it's not another Apple stuff haha

And yes you're right that the simple cases with a*, _z and a_z are absolutely enough. I'm just leaving this here as a reminder for how to get the multi wildcard feature supported, which I'm going to implement via the previously mentioned Operator.Custom

if you get it all working as expected, I again think we could add it as a new compound operator that when selected (and only when selected) would parse SQL LIKE form just like with the * symbol but with the % instead. If implemented, it would have to bypass the current regex used by filter service. Like you said, we could get it working as SQL LIKE and that would make total sense since a backend service is expected to work with most often an SQL DB (and even if it's NoSQL, many people are used to SQL LIKE filtering).

@zewa666
Copy link
Contributor

zewa666 commented May 18, 2024

the only reason I'm holding off doing that right away is bc this is a very narrow use case thats only helpful if your OData is driving a SQL based DB backend. So while important for my app I feel like its too specific to be implemented as a standard feature for slickgrid universal itself. In contrast the a*z one is applicable for all types of OData backends

most often an SQL DB

well there is SFDC with the OData Wrapper, SAP, Sharepoint and a couple of others not exposing the DB directly ;)

@ghiscoding
Copy link
Owner Author

ghiscoding commented May 18, 2024

actually taking another look at this very own PR, the regex to be replaced is actually in the backend service (OData/GraphQL) and not the one from filter service (since that is meant to be local dataset)

the only reason I'm holding off doing that right away is bc this is a very narrow use case thats only helpful if your OData is driving a SQL based DB backend. So while important for my app I feel like its too specific to be implemented as a standard feature for slickgrid universal itself. In contrast the a*z one is applicable for all types of OData backends

perhaps some kind of backend plugin? Maybe some kind of filterPredicate function that could be added to the backend service? That seems like a nice alternative and easier to implement without requiring the user to extends the OData/GraphQL Services.

well there is SFDC with the OData Wrapper, SAP, Sharepoint and a couple of others not exposing the DB directly ;)

what I meant was that SQL LIKE is a standard that many people already know 😄

@zewa666
Copy link
Contributor

zewa666 commented May 18, 2024

actually taking another look at this very own PR, the regex to be replaced is actually in the backend service (OData/GraphQL) and not the one from filter service (since that is meant to be local dataset)

yep thats what I did for trying it out

perhaps some kind of backend plugin? Maybe some kind of filterPredicate function that could be added to the backend service? That seems like a nice alternative and easier to implement without requiring the user to extends the OData/GraphQL Services.

thats a great idea. while I already have subclassed the odata service for other reasons, it would still allow not having to override the whole updateFilters method but merely provide a callback to extend filtering behavior.

@ghiscoding
Copy link
Owner Author

ghiscoding commented May 18, 2024

thats a great idea. while I already have subclassed the odata service for other reasons, it would still allow not having to override the whole updateFilters method but merely provide a callback to extend filtering behavior.

indeed, so I'll wait for your upcoming PR then 😉 that could be added later anyway, it's just meant to be easier for the end user. We should probably include @jr01 in the discussion as well since he's another great user of the platform 😺

@jr01
Copy link
Collaborator

jr01 commented May 21, 2024

@ghiscoding yes, would be cool if there was an easier way for extending filter behavior. This thread reminded me that I still have this in place: ghiscoding/Angular-Slickgrid#808 (comment)

@ghiscoding
Copy link
Owner Author

ghiscoding commented May 21, 2024

@jr01 thanks for the feedback, you can probably imagine that I would certainly accept any PR if there's a way to support your use case in a generic way then I'm more than happy to receive a PR to have it built-in. Otherwise, like discussed above, maybe a filterPredicate, similar to what I just added for local JSON dataset in PR #1531, might be good alternative.

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.

None yet

3 participants