yahoo / elide

Elide is a Java library that lets you stand up a GraphQL/JSON-API web service with minimal effort.
https://elide.io
Other
1k stars 227 forks source link

Support case insensitive filtering (RSQL) #387

Closed DennisMcWherter closed 6 years ago

DennisMcWherter commented 7 years ago

RSQL should support a case insensitive filtering syntax. A ticket has been opened here with the RSQL repository, but we can also consider contributing our own scheme back to avoid stalling too much on this feature.

NOTE: After further investigating the FIQL Spec (a subset of RSQL), it looks like searches should generally be case insensitive. Consequently, we maybe should update our RSQL implementation and look into case sensitive support.

DennisMcWherter commented 7 years ago

My proposal here is to introduce a custom operator (supported by RSQL spec) known as like-- a case sensitive regex search. RSQL is default case insensitive by the FIQL spec it appears. Consequently, the custom operator will be case sensitive.

This would look something like =like=*CaSeSeNsItIvESeArCh*.

Any oppositions to this syntax?

micheljung commented 7 years ago

So that means that ==caseINSENSITIVEsearch would be, well, case insensitive and =like= would be a case sensitive regex? IMO regex searches should be case sensitive, so I'm fine with that, but looking at q-builders, it states that rsql-parser supports =re= for regular expression (even though I wasn't able to find that quickly) which is why I think Elide should use =re= as well (and maybe =rei= or =ire= for a case insensitive regex, if desired?)

DennisMcWherter commented 7 years ago

This is a good point. Perhaps =re= would be sufficient to support full regex. Maybe we can implement both forms? =like= I believe was already merged in here.

=re= could be generally useful, however. This would just require datastores to be more cognizant of their handling of this operation though. For instance, MySQL's REGEX doesn't work with unicode :(

micheljung commented 7 years ago

@DeathByTape So we're using Elide 3.0.2 now but: https://api.dev.faforever.com/data/game?include=playerStats&page[size]=10&filter=playerStats.player.login==downlord vs https://api.dev.faforever.com/data/game?include=playerStats&page[size]=10&filter=playerStats.player.login==Downlord

Shouldn't it be case insensitive?

DennisMcWherter commented 7 years ago

== is an exact match if no wildcards are present. We may want to think about a way to denote a case insensitive exact match.

micheljung commented 7 years ago

Didn't we conclude that == should be case insensitive, as per the FIQL spec?

Simple text comparisons allow for case insensitive [...] A match occurs when the text in question is character-by-character equivalent with the argument, after observing the following rules, in order: [...] Both strings MUST have locale-independent case folding applied

DennisMcWherter commented 7 years ago

That looks right to me. It looks like the PR above only took into account the context we were discussing at the time (namely, a like operator).

@clayreimann @aklish do you think this is a reasonable change to just bump our minor version? Behavior is changed, but nothing necessarily "breaks." If anything, a greater subset of values would be returned. Thoughts?

DennisMcWherter commented 6 years ago

Fixed in Elide 4.0 by #539