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

invalid oData queries with operators in input filter #650

Closed
jr01 opened this issue Dec 17, 2020 · 7 comments · Fixed by #656
Closed

invalid oData queries with operators in input filter #650

jr01 opened this issue Dec 17, 2020 · 7 comments · Fixed by #656

Comments

@jr01
Copy link
Collaborator

jr01 commented Dec 17, 2020

I'm submitting a Bug report

Your Environment

Software Version(s)
Angular 11.0
Angular-Slickgrid 2.24.1
TypeScript 4.0.5

Describe the Bug

Invalid oData queries are sent when using operators in an input filter.

Steps to Reproduce

case 1
fieldType: number
enter in a number field: >50.
oData query: $filter=(amount gt 50.)
Server returns 400

case 2
fieldType: number
enter in a number field: 50..100
oData query: $filter=(amount gt 50 and amount lt 100)

case 3
fieldType: number
enter in a number field: 50..
oData query: $filter=(amount gt 50 and amount lt)
Server returns 400

case 4
fieldType: string
enter in a string field: Bob..Jane
oData query: $filter=(name gt Bob and name lt Jane)
Server returns 400

case 5
fieldType: string
enter in a string field: Bob..
oData query: $filter=(name gt Bob and name lt)
Server returns 400

Expected Behavior

case 1
oData query: $filter=(amount gt 50) or $filter=(amount gt 50.0)

case 2
oData query: $filter=(amount ge 50) or $filter=(amount ge 50.0)
In my world (financial accounting) most users expect .. to mean range-inclusive, so ge. Also .. means range inclusive in most programming languages.

case 3
oData query: $filter=(amount ge 50)

case 4
oData query: $filter=(name ge 'Bob' and name le 'Jane')
The strings Bob and Jane are escaped and range-inclusive is used.

case5
oData query: $filter=(name ge Bob)

Current Behavior

case 1
Server returns 400

case 2
records with amount=50 and amount=100 aren't returned.

case 3-5
Server returns 400

Possible Solution

case 1
strip .

case 3
Escape strings when using range operator in string fields.

case 2-5
Change the meaning of .. to be range-inclusive and use ge and le. See the arguments above.
About range-exclusive: in my mind there is no need to support that at all, a user can just enter the next/previous number. So 50..100 should deliver 50,51,....,99,100 and if the user doesn't want 50 or 100 he'll type 51..99.

Code Sample

For cases 1 - https://ghiscoding.github.io/angular-slickgrid-demos/#/editor - enter 10..13 in % Complete - only 1 record is showing - enter 10..14 now 3 records are showing and 2 of them have value 13.

For cases 2 to 5 - https://ghiscoding.github.io/angular-slickgrid-demos/#/odata

@ghiscoding
Copy link
Owner

ghiscoding commented Dec 17, 2020

I see your point and I would require your help to fix it, you can take a look grid-odata.service.ts, that is where everything happens and the logic was all written by hand. Because it was written by hand, I'm sure that we've missed a few use cases like yours. We also only used OData for 1 project and we moved on to GraphQL some time ago, I also don't have access to an OData Server anymore, so if you could please help that would be really great. Thank you

@jr01
Copy link
Collaborator Author

jr01 commented Dec 17, 2020

Yes, I now see the magic in grid-odata.service.ts - I'll work on a fix.

@ghiscoding
Copy link
Owner

ghiscoding commented Dec 17, 2020

I read your description of range inclusive with .., so I didn't know that it meant inclusive, the way I've programmed it, was to use the .. for any range (default to exclusive) and if you want inclusive/exclusive then you (the developer) specify it with the operator and the user can't change it.

filter: {
      model: Filters.sliderRange,
      maxValue: 100, // or you can use the filterOptions as well
      operator: OperatorType.rangeInclusive, // defaults to exclusive
      filterOptions: { min: 0, step: 5 } as JQueryUiSliderOption // you can also optionally pass any option of the jQuery UI Slider
}

So my question to you, does the 3 dots ... mean exclusive then? or there's just no way of defining an exclusive operator?
It just happened that I decided to make the default as range exclusive when seeing the range with .., which is the inverse of what you said lol... As you can see, I'm not a Finance guy :P

I could switch the default to be inclusive instead of exclusive, it might affect some users but I would prefer to follow the standard defined in the Finance world (Excel). Another thing I could maybe do is to add "Range Inclusive" and "Range Exclusive" in the Compound Filter dropdowns but that would assume the users enters a cell value including dots x..y (might not be a good idea, as it might confuse people more than anything else).

@jr01
Copy link
Collaborator Author

jr01 commented Dec 17, 2020

I didn't notice the filter.operator, but it won't help in my use-case. I would like to support any operation in a single input field. So the user could type for example >40 or 40..100.

3 dots ... - some programming languages use 2 dots, some have 3, but it always means range inclusive (AFAIK).

Range exclusive operator: In 'Swift' programming language there exists the concept of a half-open range 2..<3, but it only exists at the end and 2>..3 and 2>..<3 are not supported. In functional programming [1..10] is range-inclusive, (1..10) is range-exclusive and [1..10) and (1..10] can also be used.

In Excel (good you mention it!) a range is also inclusive: A1:F1. Thinking about other data-types and use cases, when I book a holiday I book it from X to Y and those dates are inclusive.

So to sum up I think it makes more sense to have range inclusive as a default. Also I think a range exclusive option in the input field isn't really needed and it can be confusing that '..' could mean exclusive/inclusive based on filter.operator...

@ghiscoding
Copy link
Owner

ghiscoding commented Dec 17, 2020

So to sum up I think it makes more sense to have range inclusive as a default.

Ok I can change it to inclusive by default in all Filters

Also I think a range exclusive option in the input field isn't really needed

No I don't want to remove it, I needed to change it which is why I added this option, it's fully tested and it would also require lot of code refactoring to remove it in the code and in the unit tests so I'll keep the option, in your case you won't use it and that's fine.

@ghiscoding
Copy link
Owner

ghiscoding commented Dec 17, 2020

@jr01
Today is my last working day of the year, so I made a PR to switch the default to inclusive before I forget... it does change couple of lines in the OData Service code and associated unit tests, so you might want to get latest code before creating a PR with a fix. I also forgot that I had a global grid option for that as well, so you could change it on your local even without having latest code/version (probably better to make it global in your ngModule)

this.gridOptions = {
  defaultFilterRangeOperator: OperatorType.rangeInclusive,
};

Cheers ⭐ 🎄

@ghiscoding
Copy link
Owner

This is now fixed and released in the new version 2.25.0, see Release Note.

Thanks a lot for your contribution in fixing this 😉

Cheers ⭐

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants