Fix skiptoken paging edge case when combined with involving nullable datetime property - #872
Conversation
0d0065c to
2943c38
Compare
| binaryOperator: BinaryOperatorKind.Equal, | ||
| left: property, | ||
| right: parameterizeConstant ? LinqParameterContainer.Parameterize(propertyType, null) : Expression.Constant(null), | ||
| right: parameterizeConstant ? LinqParameterContainer.Parameterize(property.Type, null) : Expression.Constant(null), |
There was a problem hiding this comment.
You reset propertyType using propertyType = property.Type;
why do you use property.Type again?
There was a problem hiding this comment.
@xuzhg Since the Edm doesn't support DateTime, we map DateTime to DateTimeOffset. When later we're creating the binary expression to compare the DateTimeOffset value to a DateTime property, we convert the value back to a DateTime in the CreateBinaryExpression method at this point
Now, when the value is of type DateTimeOffset? and the value is null, everything breaks down here. The parameterizedConstantValue is null, so parameterizedConstantValue as DateTimeOffset? returns null and consequently the type conversion from DateTimeOffset? to DateTime? doesn't happen and an exception gets thrown later as a result of trying to compare DateTime? (the type for the property) with DateTimeOffset? (the type for the value). I found this to be the easiest way to address the issue - by creating the binary expression with the actual property type when the value is null.
| } | ||
|
|
||
| IDictionary<string, Tuple<object, Type>> propertyValuePairs = PopulatePropertyValuePairs(skipTokenRawValue, context); | ||
| IDictionary<string, (object Value, Type Type)> propertyValuePairs = PopulatePropertyValuePairs(skipTokenRawValue, context); |
There was a problem hiding this comment.
what's the benefit of using this format(object Value, Type Type) in place of what was there?
There was a problem hiding this comment.
I'm not sure whether the Value and Type are meant to override the default field names item1, item2 etc. If that's the case then I'd suggest we have more descriptive names to tell someone which value is being referred to for example. etc
There was a problem hiding this comment.
what's the benefit of using this format
(object Value, Type Type)in place of what was there?
That is a ValueTuple. It's a struct and @habbes felt using it would help reduce allocations on the heap. Before I had Tuple<object, Type>.
There was a problem hiding this comment.
I'm not sure whether the
ValueandTypeare meant to override the default field namesitem1,item2etc. If that's the case then I'd suggest we have more descriptive names to tell someone which value is being referred to for example. etc
They are meant to override the default Item and Item2. I'll rename them to PropertyValue and PropertyType respectively.
999e380 to
d6448be
Compare
| left: property, | ||
| right: constant, | ||
| liftToNull: propertyIsNullable ? false : true, | ||
| liftToNull: !propertyIsNullable, |
There was a problem hiding this comment.
what is the meaning of this or why do we use the opposite of what the propertyIsNullable value is?
There was a problem hiding this comment.
We use in the BinaryExpression.IsLiftedToNull
A lifted operator allows an operator on a non-nullable type to be used with the nullable equivalent as well. See example here.
What we are doing here is setting liftToNull = true when the property in not nullable.
| left: property, | ||
| right: constant, | ||
| liftToNull: propertyIsNullable ? false : true, | ||
| liftToNull: !propertyIsNullable, |
There was a problem hiding this comment.
We use in the BinaryExpression.IsLiftedToNull
A lifted operator allows an operator on a non-nullable type to be used with the nullable equivalent as well. See example here.
What we are doing here is setting liftToNull = true when the property in not nullable.
| Contract.Assert(context != null); | ||
|
|
||
| IDictionary<string, Tuple<object, Type>> propertyValuePairs = new Dictionary<string, Tuple<object, Type>>(); | ||
| IDictionary<string, (object PropertyValue, Type PropertyType)> propertyValuePairs = new Dictionary<string, (object PropertyValue, Type PropertyType)>(); |
There was a problem hiding this comment.
Can we use a ValueTuple instead of an object?
Tuple - reference type
ValueTuple - value type
object - reference type
There was a problem hiding this comment.
@KenitoInc object is being used here because the property value can be of varying types. For that reason, we cannot swap object with ValueTuple
| binaryOperator: BinaryOperatorKind.Equal, | ||
| left: property, | ||
| right: parameterizeConstant ? LinqParameterContainer.Parameterize(propertyType, null) : Expression.Constant(null), | ||
| right: parameterizeConstant ? LinqParameterContainer.Parameterize(property.Type, null) : Expression.Constant(null), |
There was a problem hiding this comment.
I think the propertyType variable can be used here in place of property.Type since in line 308 you assigned property.Type to propertyType. In any case the code here will be executed only if value is not null. So there is actually no relation between what is assigned in line 308 and what is here.
There was a problem hiding this comment.
@ElizabethOkerio Use of property.Type here is deliberate due to the explanation provided here #872 (comment)
5 participants
Related to #859. Fixes skiptoken paging edge case when combined with involving nullable datetime property
In the case of a
DateTimeproperty, it is mapped toDateTimeOffsetsinceDateTimeis mapped toEdm.DateTimeOffsetin the Edm. This fix ensures proper handling of null when the skip token value of theDateTimeproperty is null. The PR also adds tests for the edge case.