Fix OData string comparisons (lt, gt, ge, le) #2386
Conversation
bahusoid
changed the title
| [TestCase("$filter=CustomerId le 'ANATR'",2 )] | ||
| [TestCase("$filter=startswith(CustomerId, 'ANATR')", 1)] | ||
| [TestCase("$filter=endswith(CustomerId, 'ANATR')", 1)] | ||
| [TestCase("$filter=indexof(CustomerId, 'ANATR') eq 0", 1)] |
There was a problem hiding this comment.
Since we do not register EndsWith, StartsWith or IndexOf overloads with StringComparison, does this means there is an inconsistency in OData implementation here? It would emit Compare calls with that argument, but it would omit it for these three other cases?
So I see three options:
- only register what is currently required by OData, as does this PR currently.
- also register other cases overloads in case OData changes on this, or in case another Linq generator library requires them.
- reject this as an external issue, to be fixed on OData side.
There was a problem hiding this comment.
does this means there is an inconsistency in OData implementation here?
Yeap. Seems to be.
also register other cases overloads in case OData changes on this, or in case another Linq generator library requires them.
I would choose this one as I see no difference with call to default string.Compare. It's not corelated in any way to with how DB is configured to make string comparisons. In fact StringComparison might be used to make LINQ to objects queries behave the same as DB (as I already mentioned in case of default SQL Server case insensitive collation)
There was a problem hiding this comment.
By just adding the missing overload we introduce another issue where we generate the same query when using StringComparison.XXX and StringComparison.XXXIgnoreCase, which would cause an unexpected result on a case sensitive collation. IMO we should at least try to mimic .NET behavior when using XXXIgnoreCase by using lower function on each side.
does this means there is an inconsistency in OData implementation here?
Yes, from the OData source code only StringComparison.Ordinal option is used for string.Compare, where for EndsWith, StartsWith and other methods the StringComparison overload is not used.
There was a problem hiding this comment.
By just adding the missing overload we introduce another issue where we generate the same query when using
StringComparison.XXXandStringComparison.XXXIgnoreCase, which would cause an unexpected result on a case sensitive collation.
The thing is, we do not try to support different StringComparison values, we just ignore them. The trouble is the same conversely, with case insensitive collations, when using a non ignoring case StringComparison. And for this converse case, there is no ".Net" side workaround.
The current state is, NHibernate bluntly states these overloads are not supported. That is fine for user generated expression. But when the user cannot control the generated expression, it is a blocker.
So I think accepting these overloads, while not actually supporting the StringComparison argument, is an acceptable trade-of. Maybe we should emit a NHibernate warning or information about the argument being ignored, though.
There was a problem hiding this comment.
And for this converse case, there is no ".Net" side workaround
Some databases support the COLLATE clause (SqlLite, SqlServer, Oracle, PostgreSql, MySql) within the query, which could be used to do case sensitive/insensitive comparison. But that's a matter for a separate PR.
So I think accepting these overloads, while not actually supporting the StringComparison argument, is an acceptable trade-of
It is fine by me.
|
Maybe we should check that the argument is |
I would lean more towards throwing an exception than just ignoring the argument, but I fine with both approaches. |
|
I don't understand why |
'instr' supported since SQLite 3.7.15. So maybe better be placed in separate class or in separate PR
Fixes #2362
Support StringComparison parameter with warning for string methods (StartsWith, EndsWith, IndexOf, Replace)