[CSHARP-527] Custom Gt/Gte/Lt/Lte filter builders for unsigned integer fields represented using signed integers Created: 14/Jul/12 Updated: 19/Oct/16 Resolved: 18/Oct/16 |
|
| Status: | Closed |
| Project: | C# Driver |
| Component/s: | None |
| Affects Version/s: | 1.5 |
| Fix Version/s: | 2.4 |
| Type: | Bug | Priority: | Critical - P2 |
| Reporter: | Alexander Nagy | Assignee: | Robert Stam |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Environment: |
All |
||
| Description |
|
The new typed query builder in 1.5 does not correctly convert queries when dealing with signed <-> unsigned conversions and GTE, LTE, GT, LT operations. Consider: class Foo } Query<Foo>.GTE(f => f.Bar, (uint)int.MaxValue + 1) // Error, needs to execute $lt : 0, but instead executes $gte : -2147483648, which is always true Marked this critical because while folks can work around this, it is counter intuitive and assuming the bug is fixed properly, will cause breaking changes that silently lead to corruption or undefined state for anyone that starts using these APIs like this. |
| Comments |
| Comment by Githook User [ 18/Oct/16 ] | ||
|
Author: {u'username': u'rstam', u'name': u'rstam', u'email': u'robert@robertstam.org'}Message: | ||
| Comment by Robert Stam [ 12/Oct/16 ] | ||
|
Note: this issue was reported against the 1.x driver and is being resolved in the 2.x driver. We will not be backporting this change to the 1.x driver. | ||
| Comment by Alexander Nagy [ 15/Jul/12 ] | ||
|
That sounds right - depending on the direction of the operation and comparand, one or two clauses may be needed. Some minor optimization might be possible via switching and -> not or depending on the optimizer, but I'm not not aware of any arithmetic or bit-twiddling hacks as long as the encoded representation is a direct cast of the input type. Certainly in any case where possible, only one query clause should be executed. | ||
| Comment by Craig Wilson [ 14/Jul/12 ] | ||
|
Ok, I see now what is going on. It's actually more likely that simply $lt : 0 is not enough in a majority of cases. Robert pointed out that we actually need extra conditions.
should render to this query: { bar : { $lt : 0, $gt : -2147483638 }} Likewise...
...this should render even slightly different: { $or : [ { bar : { $gt : 5 }}, { bar : { $lt : 0 }} ] } Does that seem right to you? If so, this is not a trivial problem. We'll keep you in the loop as to what we come up with. | ||
| Comment by Alexander Nagy [ 14/Jul/12 ] | ||
|
The AllowOverflow in the example is more of a distraction - $lt : 0 for an int32 should be logically equivalent to $gte 0x80000000 for an uint32, assuming the uint32 is represented as an int32. The problem is that query being executed isn't respecting the semantics of the input type, even though it is at the encoding level, which is inconsistent with the rest of the typed query API (one could argue the whole reason why the effort is made with the serialization info inspection). I.e. Query.GTE("bar", 0x80000000U) will always return nothing because there will be a type mismatch (int32 vs int64), but Query<Foo>(q => q.Bar, 0x80000000U) one would expect to execute correctly because the typed version knows the encoding semantics of the type. Also notably, it is correct that -2147483648 be serialized to the database - but not in a query like this. This is definitely not an academic case, we checked in several mitigations on our side today due to this. EDIT: grammar, improve example | ||
| Comment by Craig Wilson [ 14/Jul/12 ] | ||
|
@Alex: why do you think it should resolve to 0? AllowOverflow doesn't mean (no negative numbers). Rather, it indicates whether going over the maximum value is allowed and, if so, we do what the .NET framework does natively. This isn't a typed builder problem either. If you look at the UInt32Serializer, you'll see we defer to the RepresentationSerializationOptions to give us a value. Furthermore, if you tried to save this:
It would save with -2147483648. Hence, the typed builders render the query in the same way that it would get serialized. On a separate not, was this purely academic or do you have a class like this? |