Skip to content

Fix skiptoken paging edge case when combined with involving nullable datetime property #872

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.

Sign up for GitHub

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

Merged
gathogojr merged 1 commit into from
Mar 31, 2023
Merged

Fix skiptoken paging edge case when combined with involving nullable datetime property #872

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 13 additions & 12 deletions src/Microsoft.AspNetCore.OData/Query/Query/DefaultSkipTokenHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ private static IQueryable ApplyToCore(IQueryable query, ODataQuerySettings query
directionMap = new Dictionary<string, OrderByDirection>();
}

IDictionary<string, Tuple<object, Type>> propertyValuePairs = PopulatePropertyValuePairs(skipTokenRawValue, context);
IDictionary<string, (object PropertyValue, Type PropertyType)> propertyValuePairs = PopulatePropertyValuePairs(skipTokenRawValue, context);

if (propertyValuePairs.Count == 0)
{
Expand All @@ -286,14 +286,14 @@ private static IQueryable ApplyToCore(IQueryable query, ODataQuerySettings query
Expression lastEquality = null;
bool firstProperty = true;

foreach (KeyValuePair<string, Tuple<object, Type>> item in propertyValuePairs)
foreach (KeyValuePair<string, (object PropertyValue, Type PropertyType)> item in propertyValuePairs)
{
string key = item.Key;
MemberExpression property = Expression.Property(param, key);

object value = item.Value.Item1;
object value = item.Value.PropertyValue;

Type propertyType = item.Value.Item2 ?? value.GetType();
Type propertyType = item.Value.PropertyType ?? value.GetType();
bool propertyIsNullable = propertyType.IsNullable();

Expression compare = null;
Expand All @@ -305,6 +305,7 @@ private static IQueryable ApplyToCore(IQueryable query, ODataQuerySettings query
else if (value is ODataNullValue)
{
value = null;
propertyType = property.Type;
Comment thread
ElizabethOkerio marked this conversation as resolved.
}

Expression constant = parameterizeConstant ? LinqParameterContainer.Parameterize(propertyType, value) : Expression.Constant(value);
Expand All @@ -316,7 +317,7 @@ private static IQueryable ApplyToCore(IQueryable query, ODataQuerySettings query
binaryOperator: BinaryOperatorKind.LessThan,
left: property,
right: constant,
liftToNull: propertyIsNullable ? false : true,
liftToNull: !propertyIsNullable,

@ElizabethOkerio ElizabethOkerio Mar 30, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the meaning of this or why do we use the opposite of what the propertyIsNullable value is?

@KenitoInc KenitoInc Mar 30, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

querySettings: querySettings);

if (propertyIsNullable && value != null)
Expand All @@ -329,7 +330,7 @@ private static IQueryable ApplyToCore(IQueryable query, ODataQuerySettings query
Expression condition = ExpressionBinderHelper.CreateBinaryExpression(
binaryOperator: BinaryOperatorKind.Equal,
left: property,
right: parameterizeConstant ? LinqParameterContainer.Parameterize(propertyType, null) : Expression.Constant(null),
right: parameterizeConstant ? LinqParameterContainer.Parameterize(property.Type, null) : Expression.Constant(null),

@xuzhg xuzhg Mar 27, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You reset propertyType using propertyType = property.Type;

why do you use property.Type again?

@gathogojr gathogojr Mar 28, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@ElizabethOkerio ElizabethOkerio Mar 30, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@gathogojr gathogojr Mar 30, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ElizabethOkerio Use of property.Type here is deliberate due to the explanation provided here #872 (comment)

liftToNull: false,
querySettings: querySettings);

Expand Down Expand Up @@ -364,7 +365,7 @@ private static IQueryable ApplyToCore(IQueryable query, ODataQuerySettings query
binaryOperator: BinaryOperatorKind.GreaterThan,
left: property,
right: constant,
liftToNull: propertyIsNullable ? false : true,
liftToNull: !propertyIsNullable,
querySettings: querySettings);
}
}
Expand All @@ -375,7 +376,7 @@ private static IQueryable ApplyToCore(IQueryable query, ODataQuerySettings query
binaryOperator: BinaryOperatorKind.Equal,
left: property,
right: constant,
liftToNull: propertyIsNullable ? false : true,
liftToNull: !propertyIsNullable,
querySettings: querySettings);
where = compare;
firstProperty = false;
Expand All @@ -390,7 +391,7 @@ private static IQueryable ApplyToCore(IQueryable query, ODataQuerySettings query
binaryOperator: BinaryOperatorKind.Equal,
left: property,
right: constant,
liftToNull: propertyIsNullable ? false : true,
liftToNull: !propertyIsNullable,
querySettings: querySettings));
}
}
Expand All @@ -405,11 +406,11 @@ private static IQueryable ApplyToCore(IQueryable query, ODataQuerySettings query
/// <param name="value">The skiptoken string value.</param>
/// <param name="context">The <see cref="ODataQueryContext"/> which contains the <see cref="IEdmModel"/> and some type information</param>
/// <returns>Dictionary with property name and property value in the skiptoken value.</returns>
internal static IDictionary<string, Tuple<object, Type>> PopulatePropertyValuePairs(string value, ODataQueryContext context)
internal static IDictionary<string, (object PropertyValue, Type PropertyType)> PopulatePropertyValuePairs(string value, ODataQueryContext context)
{
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)>();

@KenitoInc KenitoInc Mar 30, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use a ValueTuple instead of an object?

Tuple - reference type
ValueTuple - value type
object - reference type

@gathogojr gathogojr Mar 31, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KenitoInc object is being used here because the property value can be of varying types. For that reason, we cannot swap object with ValueTuple

IList<string> keyValuesPairs = ParseValue(value, CommaDelimiter);

IEdmStructuredType type = context.ElementType as IEdmStructuredType;
Expand All @@ -432,7 +433,7 @@ internal static IDictionary<string, Tuple<object, Type>> PopulatePropertyValuePa
}

propValue = ODataUriUtils.ConvertFromUriLiteral(pieces[1], ODataVersion.V401, context.Model, propertyType);
propertyValuePairs.Add(pieces[0], Tuple.Create(propValue, propertyClrType));
propertyValuePairs.Add(pieces[0], (propValue, propertyClrType));
}
else
{
Expand Down
22 changes: 22 additions & 0 deletions test/Microsoft.AspNetCore.OData.E2E.Tests/ServerSidePaging/ServerSidePagingControllers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,4 +115,26 @@ public ActionResult<IEnumerable<SkipTokenPagingCustomer>> Get()
return customers;
}
}

public class SkipTokenPagingS3CustomersController : ODataController
{
private static readonly List<SkipTokenPagingCustomer> customers = new List<SkipTokenPagingCustomer>
{
new SkipTokenPagingCustomer { Id = 1, CustomerSince = null },
new SkipTokenPagingCustomer { Id = 2, CustomerSince = new DateTime(2023, 1, 2) },
new SkipTokenPagingCustomer { Id = 3, CustomerSince = null },
new SkipTokenPagingCustomer { Id = 4, CustomerSince = new DateTime(2023, 1, 30) },
new SkipTokenPagingCustomer { Id = 5, CustomerSince = null },
new SkipTokenPagingCustomer { Id = 6, CustomerSince = new DateTime(2023, 2, 4) },
new SkipTokenPagingCustomer { Id = 7, CustomerSince = new DateTime(2023, 1, 5) },
new SkipTokenPagingCustomer { Id = 8, CustomerSince = new DateTime(2023, 2, 19) },
new SkipTokenPagingCustomer { Id = 9, CustomerSince = new DateTime(2023, 1, 25) },
};

[EnableQuery(PageSize = 2)]
public ActionResult<IEnumerable<SkipTokenPagingCustomer>> Get()
{
return customers;
}
}
}
1 change: 1 addition & 0 deletions test/Microsoft.AspNetCore.OData.E2E.Tests/ServerSidePaging/ServerSidePagingDataModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,5 +38,6 @@ public class SkipTokenPagingCustomer
public int Id { get; set; }
public string Grade { get; set; }
public decimal? CreditLimit { get; set; }
public DateTime? CustomerSince { get; set; }
}
}
151 changes: 150 additions & 1 deletion test/Microsoft.AspNetCore.OData.E2E.Tests/ServerSidePaging/ServerSidePagingTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,8 @@ protected static void UpdateConfigureServices(IServiceCollection services)
IEdmModel model = GetEdmModel();
services.ConfigureControllers(
typeof(SkipTokenPagingS1CustomersController),
typeof(SkipTokenPagingS2CustomersController));
typeof(SkipTokenPagingS2CustomersController),
typeof(SkipTokenPagingS3CustomersController));
services.AddControllers().AddOData(opt => opt.Expand().OrderBy().SkipToken().AddRouteComponents("{a}", model));
}

Expand All @@ -144,6 +145,7 @@ protected static IEdmModel GetEdmModel()
ODataModelBuilder builder = new ODataConventionModelBuilder();
builder.EntitySet<SkipTokenPagingCustomer>("SkipTokenPagingS1Customers");
builder.EntitySet<SkipTokenPagingCustomer>("SkipTokenPagingS2Customers");
builder.EntitySet<SkipTokenPagingCustomer>("SkipTokenPagingS3Customers");

return builder.GetEdmModel();
}
Expand Down Expand Up @@ -427,5 +429,152 @@ public async Task VerifySkipTokenPagingOrderedByNullablePropertyThenByNonNullabl
Assert.Equal(55, (pageResult[0] as JObject)["CreditLimit"].ToObject<decimal?>());
Assert.Null(content.GetValue("@odata.nextLink"));
}
[Fact]
public async Task VerifySkipTokenPagingOrderedByNullableDateTimeProperty()
{
HttpClient client = CreateClient();
HttpRequestMessage request;
HttpResponseMessage response;
JObject content;
JArray pageResult;

// NOTE: Using a loop in this test (as opposed to parameterized tests using xunit Theory attribute)
// is intentional. The next-link in one response is used in the next request
// so we need to control the execution order (unlike Theory attribute where order is random)
var skipTokenTestData = new List<Tuple<int, DateTime?, int, DateTime?>>
{
Tuple.Create<int, DateTime?, int, DateTime?> (1, null, 3, null),
Tuple.Create<int, DateTime?, int, DateTime?> (5, null, 2, new DateTime(2023, 1, 2)),
Tuple.Create<int, DateTime?, int, DateTime?> (7, new DateTime(2023, 1, 5), 9, new DateTime(2023, 1, 25)),
Tuple.Create<int, DateTime?, int, DateTime?>(4, new DateTime(2023, 1, 30), 6, new DateTime(2023, 2, 4))
};

string requestUri = "/prefix/SkipTokenPagingS3Customers?$orderby=CustomerSince";

foreach (var testData in skipTokenTestData)
{
int idAt0 = testData.Item1;
DateTime? customerSinceAt0 = testData.Item2;
int idAt1 = testData.Item3;
DateTime? customerSinceAt1 = testData.Item4;

// Arrange
request = new HttpRequestMessage(HttpMethod.Get, requestUri);
string skipTokenStart = string.Concat(
"$skiptoken=CustomerSince-",
customerSinceAt1 != null ? customerSinceAt1.Value.ToString("yyyy-MM-dd") : "null");
string skipTokenEnd = string.Concat(",Id-", idAt1);

// Act
response = await client.SendAsync(request);
content = await response.Content.ReadAsObject<JObject>();

// Assert
Assert.Equal(HttpStatusCode.OK, response.StatusCode);

pageResult = content["value"] as JArray;
Assert.NotNull(pageResult);
Assert.Equal(2, pageResult.Count);
Assert.Equal(idAt0, (pageResult[0] as JObject)["Id"].ToObject<int>());
Assert.Equal(customerSinceAt0, (pageResult[0] as JObject)["CustomerSince"].ToObject<DateTime?>());
Assert.Equal(idAt1, (pageResult[1] as JObject)["Id"].ToObject<int>());
Assert.Equal(customerSinceAt1, (pageResult[1] as JObject)["CustomerSince"].ToObject<DateTime?>());

string nextPageLink = content["@odata.nextLink"].ToObject<string>();
Assert.NotNull(nextPageLink);
Assert.Contains("/prefix/SkipTokenPagingS3Customers?$orderby=CustomerSince&" + skipTokenStart, nextPageLink);
Assert.EndsWith(skipTokenEnd, nextPageLink);

requestUri = nextPageLink;
}

// Fetch last page
request = new HttpRequestMessage(HttpMethod.Get, requestUri);
response = await client.SendAsync(request);

content = await response.Content.ReadAsObject<JObject>();

// Assert
Assert.Equal(HttpStatusCode.OK, response.StatusCode);

pageResult = content["value"] as JArray;
Assert.NotNull(pageResult);
Assert.Single(pageResult);
Assert.Equal(8, (pageResult[0] as JObject)["Id"].ToObject<int>());
Assert.Equal(new DateTime(2023, 2, 19), (pageResult[0] as JObject)["CustomerSince"].ToObject<DateTime?>());
Assert.Null(content.GetValue("@odata.nextLink"));
}

[Fact]
public async Task VerifySkipTokenPagingOrderedByNullableDateTimePropertyDescending()
{
HttpClient client = CreateClient();
HttpRequestMessage request;
HttpResponseMessage response;
JObject content;
JArray pageResult;

// NOTE: Using a loop in this test (as opposed to parameterized tests using xunit Theory attribute)
// is intentional. The next-link in one response is used in the next request
// so we need to control the execution order (unlike Theory attribute where order is random)
var skipTokenTestData = new List<Tuple<int, DateTime?>>
{
Tuple.Create<int, DateTime ?> (6, new DateTime(2023, 2, 4)),
Tuple.Create<int, DateTime?> (9, new DateTime(2023, 1, 25)),
Tuple.Create<int, DateTime?> (2, new DateTime(2023, 1, 2)),
Tuple.Create<int, DateTime?>(3, null)
};

string requestUri = "/prefix/SkipTokenPagingS3Customers?$orderby=CustomerSince desc";

foreach (var testData in skipTokenTestData)
{
int idAt1 = testData.Item1;
DateTime? customerSinceAt1 = testData.Item2;

// Arrange
request = new HttpRequestMessage(HttpMethod.Get, requestUri);
string skipTokenStart = string.Concat(
"$skiptoken=CustomerSince-",
customerSinceAt1 != null ? customerSinceAt1.Value.ToString("yyyy-MM-dd") : "null");
string skipTokenEnd = string.Concat(",Id-", idAt1);

// Act
response = await client.SendAsync(request);
content = await response.Content.ReadAsObject<JObject>();

// Assert
Assert.Equal(HttpStatusCode.OK, response.StatusCode);

pageResult = content["value"] as JArray;
Assert.NotNull(pageResult);
Assert.Equal(2, pageResult.Count);
Assert.Equal(idAt1, (pageResult[1] as JObject)["Id"].ToObject<int>());
Assert.Equal(customerSinceAt1, (pageResult[1] as JObject)["CustomerSince"].ToObject<DateTime?>());

string nextPageLink = content["@odata.nextLink"].ToObject<string>();
Assert.NotNull(nextPageLink);
Assert.Contains("/prefix/SkipTokenPagingS3Customers?$orderby=CustomerSince%20desc&" + skipTokenStart, nextPageLink);
Assert.EndsWith(skipTokenEnd, nextPageLink);

requestUri = nextPageLink;
}

// Fetch last page
request = new HttpRequestMessage(HttpMethod.Get, requestUri);
response = await client.SendAsync(request);

content = await response.Content.ReadAsObject<JObject>();

// Assert
Assert.Equal(HttpStatusCode.OK, response.StatusCode);

pageResult = content["value"] as JArray;
Assert.NotNull(pageResult);
Assert.Single(pageResult);
Assert.Equal(5, (pageResult[0] as JObject)["Id"].ToObject<int>());
Assert.Null((pageResult[0] as JObject)["CustomerSince"].ToObject<DateTime?>());
Assert.Null(content.GetValue("@odata.nextLink"));
}
}
}