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: Graphql verbatim search terms #1174

Merged

Conversation

Harsgalt86
Copy link
Contributor

@Harsgalt86 Harsgalt86 commented Nov 3, 2023

==== Purpose of this PR ===
A PR to use filter search terms in a query "verbatim" without modification

Currently the backendService updateFilters() performs some manipulation of the columnFilter.searchTerms property. The flag on the column columnFilter.verbatimSearchTerms bypasses this manipulation and stringifies the search terms "as is".

Some examples of problematic manipulation are searching for columns that can be nullable

{ columnId: 'x', operator: 'in', searchTerms: 'null}
// Current: 
query{users(first:10, offset:0) { totalCount,nodes{ id,company,gender,name } }}
// withVerbatim:
query{users(first:10, offset:0, filterBy:[{field:gender, operator:EQ, value:"null"}]) { totalCount,nodes{ id,company,gender,name } }} 

Please see unit tests for more output differences.

=== Usage ===

{
    columnId: "gender",
    operator: OperatorType.in,
    searchTerms: this.genders, // supports ['male'], ['female'], ['male'/'female'], []    
    verbatimSearchTerms: true,
},

I did consider putting the flag on the backend service instead of the column, but it's possible that different backend end points may want to treat properties differently for backwards compatibility. By putting the flag on the column itself it allows for more flexibility.

=== TODO ===
Currently this flag is only respected on the graphqlService. This needs to be applied to other backend services.

@Harsgalt86 Harsgalt86 marked this pull request as draft November 3, 2023 03:35
Copy link

codecov bot commented Nov 3, 2023

Codecov Report

Merging #1174 (9de580e) into master (aa1939c) will not change coverage.
Report is 19 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1174   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          245       245           
  Lines        17246     17270   +24     
  Branches      6232      6241    +9     
=========================================
+ Hits         17246     17270   +24     
Files Coverage Δ
packages/common/src/filters/sliderFilter.ts 100.00% <100.00%> (ø)
packages/graphql/src/services/graphql.service.ts 100.00% <100.00%> (ø)
packages/odata/src/services/grid-odata.service.ts 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

@ghiscoding
Copy link
Owner

ghiscoding commented Nov 3, 2023

Currently the backendService updateFilters() performs some manipulation of the columnFilter.searchTerms property. The flag on the column columnFilter.verbatimSearchTerms bypasses this manipulation and stringifies the search terms "as is".

I'm not entirely sure what you mean but manipulation... is it because some null are changed to empty string or something similar? is that what this is about to leave some data untouched, like null, undefined and maybe number that are sometime transformed to string, is that this PR is about?

@Harsgalt86
Copy link
Contributor Author

I'm not entirely sure what you mean but manipulation... is it because some null are changed to empty string or something similar? is that what this is about to leave some data untouched, like null, undefined and maybe number that are sometime transformed to string, is that this PR is about?

Yeah, exactly. Specifically,null, "", [] are all getting removed from the query. (First 6 unit tests in the table).

Further, [null] and [""] becomes "". This is problematic for our backend server in regards to 1) a set being transformed to a string 2) two distinct values in a set being transformed into the same value.

So yes, this PR allow some columns to leave the searchTerms untouched.

@ghiscoding
Copy link
Owner

Note that there are some reasons why some of these are being transformed, it might not work with all type of Filters (some filters might require string only, for example an input value in theory only works with string). As long as you are aware of the reasons and that you're not introducing breaking changes, it should be fine and I have plenty of unit tests and E2E tests to cover them, so that should be fine, that's really where all these unit tests become handy :)

@Harsgalt86
Copy link
Contributor Author

Thanks again for reviewing this so quickly.

some filters might require string only, for example an input value in theory only works with string). As long as you are aware of the reasons

Yeah, that's acceptable. In those cases, if using verbatimSearchTerms it should now be the responsibility of the developer to sanitise the searchTerms array/value.

that you're not introducing breaking changes

For sure. Should be safe since the behaviour is opt-in.

that's really where all these unit tests become handy

Absolutely.

@Harsgalt86 Harsgalt86 marked this pull request as ready for review November 8, 2023 03:29
@@ -21,5 +21,5 @@ export interface CurrentFilter {
* When false, searchTerms may be manipulated to be functional with certain filters eg: string only filters.
* When true, JSON.stringify is used on the searchTerms and used in the query "as-is". It is then the responsibility of the developer to sanitise the `searchTerms` property if necessary.
*/
verbatimSearchTerms?: boolean;
useVerbatimSearchTerms?: boolean;
Copy link
Owner

@ghiscoding ghiscoding Nov 8, 2023

Choose a reason for hiding this comment

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

oh I think I didn't explain well, I wanted to keep this one as verbatimSearchTerms because this one here is not a flag but a prop, so can you rename it back to verbatimSearchTerms?

Copy link
Owner

@ghiscoding ghiscoding left a comment

Choose a reason for hiding this comment

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

left some comments for code change, thanks

@ghiscoding ghiscoding merged commit eadc5ef into ghiscoding:master Nov 9, 2023
5 checks passed
@ghiscoding
Copy link
Owner

looks good thanks, do you have anything planned? I'm currently working on the multiple-select-vanilla library, I should be done by the weekend, so I expect to be able to release a new version for both projects at that time

@Harsgalt86
Copy link
Contributor Author

No, nothing immediate.

I'm considering at some point looking into extending the pagination to allow for an infinite scrolling table so you could just scroll back up to see previous rows, rather than having to navigate back and forth through pages. Could be triggered by reaching the scroll bottom or a stand alone "fetch more" button.

@ghiscoding
Copy link
Owner

If you want a hint on how to do that, I decided long time ago that in my libs we'll only use the SlickGrid DataView, so with that in mind you can use the methods from the DataView to push data with addItem (or maybe even easier with Slickgrid-Universal GridService). That's the easy part but I'm not sure what you'll do when you reach the end, I guess the easy way would be to stop but I see many grids just make it infinite by redisplaying the first rows... not sure how you'll do that.

Anyway out of context, but similar approach, with Backend Services I clear the DataView data and insert data of the next page. Also another similar approach, in the past, I've done a lazy loading implementation where I was loading the first 500 rows so that the user can start using the grid right away and then keep pushing sets of 5000 in a lazy loading way... in summary, these 2 approaches are similar to how infinite scroll would work but also a little different because I don't know how to deal with the "when you reach the end"

@ghiscoding
Copy link
Owner

ghiscoding commented Nov 11, 2023

@Harsgalt86 this is now available in Slickgrid-Universal v3.5.0 and Aurelia-Slickgrid v6.5.0

Note also please note that I deprecated isWithCursor option in favor of a simpler useCursor, you can still use the previous flag but I'll remove it in the next major.

Thanks again for all your contributions 🚀

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.

2 participants