ghiscoding / slickgrid-universal

Slickgrid-Universal is a monorepo which includes all Editors, Filters, Extensions, Services related to SlickGrid usage and is also Framework Agnostic
https://ghiscoding.github.io/slickgrid-universal/
MIT License
83 stars 26 forks source link

feat(filters): add StartsWith/EndsWith (`a*z`) to OData/GraphQL #1532

Closed ghiscoding closed 4 months ago

ghiscoding commented 4 months ago

OData Service

image

GraphQL Service

image

stackblitz[bot] commented 4 months ago

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

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ghiscoding commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

ghiscoding commented 4 months ago

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

zewa666 commented 4 months ago

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 commented 4 months ago

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

zewa666 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

@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: https://github.com/ghiscoding/Angular-Slickgrid/issues/808#issuecomment-880605321

ghiscoding commented 4 months ago

@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.