From d7c1b5f389a13b357802c0a3ebd2fb5b75842b61 Mon Sep 17 00:00:00 2001 From: John Gathogo Date: Tue, 4 Apr 2023 21:08:42 +0300 Subject: [PATCH] Fix skiptoken edge case where property is nullable on CLR type but not nullable on Edm type (#881) --- .../Properties/SRResources.Designer.cs | 9 + .../Properties/SRResources.resx | 3 + .../Query/Query/DefaultSkipTokenHandler.cs | 6 +- .../ServerSidePagingControllers.cs | 18 ++ .../ServerSidePagingDataModel.cs | 6 + .../ServerSidePaging/ServerSidePagingTests.cs | 180 +++++++++++++++--- 6 files changed, 192 insertions(+), 30 deletions(-) diff --git a/src/Microsoft.AspNetCore.OData/Properties/SRResources.Designer.cs b/src/Microsoft.AspNetCore.OData/Properties/SRResources.Designer.cs index 38e667b70..0233d2208 100644 --- a/src/Microsoft.AspNetCore.OData/Properties/SRResources.Designer.cs +++ b/src/Microsoft.AspNetCore.OData/Properties/SRResources.Designer.cs @@ -1716,6 +1716,15 @@ internal static string SkipTokenParseError { } } + /// + /// Looks up a localized string similar to Unable to get property values from the skiptoken value.. + /// + internal static string SkipTokenProcessingError { + get { + return ResourceManager.GetString("SkipTokenProcessingError", resourceCulture); + } + } + /// /// Looks up a localized string similar to The limit of '{0}' for {1} query has been exceeded. The value from the incoming request is '{2}'.. /// diff --git a/src/Microsoft.AspNetCore.OData/Properties/SRResources.resx b/src/Microsoft.AspNetCore.OData/Properties/SRResources.resx index 91a0110b8..c2acf3b92 100644 --- a/src/Microsoft.AspNetCore.OData/Properties/SRResources.resx +++ b/src/Microsoft.AspNetCore.OData/Properties/SRResources.resx @@ -732,4 +732,7 @@ 'select' and 'expand' cannot be empty or whitespace. Omit the parameter from the query if it is not used. + + Unable to get property values from the skiptoken value. + \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.OData/Query/Query/DefaultSkipTokenHandler.cs b/src/Microsoft.AspNetCore.OData/Query/Query/DefaultSkipTokenHandler.cs index 065e44a7f..43112d2af 100644 --- a/src/Microsoft.AspNetCore.OData/Query/Query/DefaultSkipTokenHandler.cs +++ b/src/Microsoft.AspNetCore.OData/Query/Query/DefaultSkipTokenHandler.cs @@ -8,6 +8,7 @@ using System; using System.Collections.Generic; using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Diagnostics.Contracts; using System.Globalization; using System.Linq; @@ -250,6 +251,7 @@ private static IQueryable ApplyToImplementation(IQueryable query, SkipTokenQuery /// The which contains the and some type information /// The raw string value of the skiptoken query parameter. /// + [SuppressMessage("Microsoft.Maintainability", "CA1506:AvoidExcessiveClassCoupling", Justification = "Class coupling acceptable.")] private static IQueryable ApplyToCore(IQueryable query, ODataQuerySettings querySettings, IList orderByNodes, ODataQueryContext context, string skipTokenRawValue) { Contract.Assert(query != null); @@ -270,7 +272,7 @@ private static IQueryable ApplyToCore(IQueryable query, ODataQuerySettings query if (propertyValuePairs.Count == 0) { - throw Error.InvalidOperation("Unable to get property values from the skiptoken value."); + throw Error.InvalidOperation(SRResources.SkipTokenProcessingError); } bool parameterizeConstant = querySettings.EnableConstantParameterization; @@ -294,7 +296,7 @@ private static IQueryable ApplyToCore(IQueryable query, ODataQuerySettings query object value = item.Value.PropertyValue; Type propertyType = item.Value.PropertyType ?? value.GetType(); - bool propertyIsNullable = propertyType.IsNullable(); + bool propertyIsNullable = property.Type.IsNullable(); Expression compare = null; if (value is ODataEnumValue enumValue) diff --git a/test/Microsoft.AspNetCore.OData.E2E.Tests/ServerSidePaging/ServerSidePagingControllers.cs b/test/Microsoft.AspNetCore.OData.E2E.Tests/ServerSidePaging/ServerSidePagingControllers.cs index ce20c2ea7..5736c5c52 100644 --- a/test/Microsoft.AspNetCore.OData.E2E.Tests/ServerSidePaging/ServerSidePagingControllers.cs +++ b/test/Microsoft.AspNetCore.OData.E2E.Tests/ServerSidePaging/ServerSidePagingControllers.cs @@ -137,4 +137,22 @@ public ActionResult> Get() return customers; } } + + public class SkipTokenPagingEdgeCase1CustomersController : ODataController + { + private static readonly List customers = new List + { + new SkipTokenPagingEdgeCase1Customer { Id = 2, CreditLimit = 2 }, + new SkipTokenPagingEdgeCase1Customer { Id = 4, CreditLimit = 30 }, + new SkipTokenPagingEdgeCase1Customer { Id = 6, CreditLimit = 35 }, + new SkipTokenPagingEdgeCase1Customer { Id = 7, CreditLimit = 5 }, + new SkipTokenPagingEdgeCase1Customer { Id = 9, CreditLimit = 25 }, + }; + + [EnableQuery(PageSize = 2)] + public ActionResult> Get() + { + return customers; + } + } } diff --git a/test/Microsoft.AspNetCore.OData.E2E.Tests/ServerSidePaging/ServerSidePagingDataModel.cs b/test/Microsoft.AspNetCore.OData.E2E.Tests/ServerSidePaging/ServerSidePagingDataModel.cs index b2f5b80d9..7a4f19c23 100644 --- a/test/Microsoft.AspNetCore.OData.E2E.Tests/ServerSidePaging/ServerSidePagingDataModel.cs +++ b/test/Microsoft.AspNetCore.OData.E2E.Tests/ServerSidePaging/ServerSidePagingDataModel.cs @@ -40,4 +40,10 @@ public class SkipTokenPagingCustomer public decimal? CreditLimit { get; set; } public DateTime? CustomerSince { get; set; } } + + public class SkipTokenPagingEdgeCase1Customer + { + public int Id { get; set; } + public decimal? CreditLimit { get; set; } + } } diff --git a/test/Microsoft.AspNetCore.OData.E2E.Tests/ServerSidePaging/ServerSidePagingTests.cs b/test/Microsoft.AspNetCore.OData.E2E.Tests/ServerSidePaging/ServerSidePagingTests.cs index a3d6af34e..e25ce7786 100644 --- a/test/Microsoft.AspNetCore.OData.E2E.Tests/ServerSidePaging/ServerSidePagingTests.cs +++ b/test/Microsoft.AspNetCore.OData.E2E.Tests/ServerSidePaging/ServerSidePagingTests.cs @@ -7,14 +7,18 @@ using System; using System.Collections.Generic; +using System.IO; using System.Net; using System.Net.Http; +using System.Text; using System.Text.Json; using System.Threading.Tasks; +using System.Xml; using Microsoft.AspNetCore.OData.E2E.Tests.Extensions; using Microsoft.AspNetCore.OData.TestCommon; using Microsoft.Extensions.DependencyInjection; using Microsoft.OData.Edm; +using Microsoft.OData.Edm.Csdl; using Microsoft.OData.ModelBuilder; using Newtonsoft.Json.Linq; using Xunit; @@ -162,12 +166,12 @@ public async Task VerifySkipTokenPagingOrderedByNullableProperty() // 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> + var skipTokenTestData = new List<(int, decimal?, int, decimal?)> { - Tuple.Create (1, null, 3, null), - Tuple.Create (5, null, 2, 2), - Tuple.Create (7, 5, 9, 25), - Tuple.Create(4, 30, 6, 35) + (1, null, 3, null), + (5, null, 2, 2), + (7, 5, 9, 25), + (4, 30, 6, 35) }; string requestUri = "/prefix/SkipTokenPagingS1Customers?$orderby=CreditLimit"; @@ -234,12 +238,12 @@ public async Task VerifySkipTokenPagingOrderedByNullablePropertyDescending() // 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> + var skipTokenTestData = new List<(int, decimal?)> { - Tuple.Create (6, 35), - Tuple.Create (9, 25), - Tuple.Create (2, 2), - Tuple.Create(3, null) + (6, 35), + (9, 25), + (2, 2), + (3, null) }; string requestUri = "/prefix/SkipTokenPagingS1Customers?$orderby=CreditLimit desc"; @@ -302,11 +306,11 @@ public async Task VerifySkipTokenPagingOrderedByNonNullablePropertyThenByNullabl // 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> + var skipTokenTestData = new List<(int, string, decimal?)> { - Tuple.Create (2, "B", null), - Tuple.Create (6, "C", null), - Tuple.Create (11, "F", 35), + (2, "B", null), + (6, "C", null), + (11, "F", 35), }; string requestUri = "/prefix/SkipTokenPagingS2Customers?$orderby=Grade,CreditLimit"; @@ -372,11 +376,11 @@ public async Task VerifySkipTokenPagingOrderedByNullablePropertyThenByNonNullabl // 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> + var skipTokenTestData = new List<(int, string, decimal?)> { - Tuple.Create (6, "C", null), - Tuple.Create (5, "A", 30), - Tuple.Create (10, "D", 50), + (6, "C", null), + (5, "A", 30), + (10, "D", 50), }; string requestUri = "/prefix/SkipTokenPagingS2Customers?$orderby=CreditLimit,Grade"; @@ -441,12 +445,12 @@ public async Task VerifySkipTokenPagingOrderedByNullableDateTimeProperty() // 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> + var skipTokenTestData = new List<(int, DateTime?, int, DateTime?)> { - Tuple.Create (1, null, 3, null), - Tuple.Create (5, null, 2, new DateTime(2023, 1, 2)), - Tuple.Create (7, new DateTime(2023, 1, 5), 9, new DateTime(2023, 1, 25)), - Tuple.Create(4, new DateTime(2023, 1, 30), 6, new DateTime(2023, 2, 4)) + (1, null, 3, null), + (5, null, 2, new DateTime(2023, 1, 2)), + (7, new DateTime(2023, 1, 5), 9, new DateTime(2023, 1, 25)), + (4, new DateTime(2023, 1, 30), 6, new DateTime(2023, 2, 4)) }; string requestUri = "/prefix/SkipTokenPagingS3Customers?$orderby=CustomerSince"; @@ -517,12 +521,12 @@ public async Task VerifySkipTokenPagingOrderedByNullableDateTimePropertyDescendi // 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> + var skipTokenTestData = new List<(int, DateTime?)> { - Tuple.Create (6, new DateTime(2023, 2, 4)), - Tuple.Create (9, new DateTime(2023, 1, 25)), - Tuple.Create (2, new DateTime(2023, 1, 2)), - Tuple.Create(3, null) + (6, new DateTime(2023, 2, 4)), + (9, new DateTime(2023, 1, 25)), + (2, new DateTime(2023, 1, 2)), + (3, null) }; string requestUri = "/prefix/SkipTokenPagingS3Customers?$orderby=CustomerSince desc"; @@ -577,4 +581,124 @@ public async Task VerifySkipTokenPagingOrderedByNullableDateTimePropertyDescendi Assert.Null(content.GetValue("@odata.nextLink")); } } + + public class SkipTokenPagingEdgeCaseTests : WebApiTestBase + { + public SkipTokenPagingEdgeCaseTests(WebApiTestFixture fixture) + : base(fixture) + { + } + + // following the Fixture convention. + protected static void UpdateConfigureServices(IServiceCollection services) + { + IEdmModel model = GetEdmModel(); + services.ConfigureControllers( + typeof(SkipTokenPagingEdgeCase1CustomersController)); + services.AddControllers().AddOData(opt => opt.Expand().OrderBy().SkipToken().AddRouteComponents("{a}", model)); + } + + protected static IEdmModel GetEdmModel() + { + var csdl = "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + // Property is nullable on CLR type + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + ""; + + IEdmModel model; + + using (var memoryStream = new MemoryStream(Encoding.UTF8.GetBytes(csdl))) + using (var reader = XmlReader.Create(memoryStream)) + { + model = CsdlReader.Parse(reader); + } + + return model; + } + + [Fact] + public async Task VerifySkipTokenPagingForPropertyNullableOnClrTypeButNotNullableOnEdmType() + { + 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<(int, decimal?, int, decimal?)> + { + (2, 2, 7, 5), + (9, 25, 4, 30), + }; + + string requestUri = "/prefix/SkipTokenPagingEdgeCase1Customers?$orderby=CreditLimit"; + + foreach (var testData in skipTokenTestData) + { + int idAt0 = testData.Item1; + decimal? creditLimitAt0 = testData.Item2; + int idAt1 = testData.Item3; + decimal? creditLimitAt1 = testData.Item4; + + // Arrange + request = new HttpRequestMessage(HttpMethod.Get, requestUri); + string skipToken = string.Concat("$skiptoken=CreditLimit-", creditLimitAt1 != null ? creditLimitAt1.ToString() : "null", ",Id-", idAt1); + + // Act + response = await client.SendAsync(request); + content = await response.Content.ReadAsObject(); + + // 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()); + Assert.Equal(creditLimitAt0, (pageResult[0] as JObject)["CreditLimit"].ToObject()); + Assert.Equal(idAt1, (pageResult[1] as JObject)["Id"].ToObject()); + Assert.Equal(creditLimitAt1, (pageResult[1] as JObject)["CreditLimit"].ToObject()); + + string nextPageLink = content["@odata.nextLink"].ToObject(); + Assert.NotNull(nextPageLink); + Assert.EndsWith("/prefix/SkipTokenPagingEdgeCase1Customers?$orderby=CreditLimit&" + skipToken, nextPageLink); + + requestUri = nextPageLink; + } + + // Fetch last page + request = new HttpRequestMessage(HttpMethod.Get, requestUri); + response = await client.SendAsync(request); + + content = await response.Content.ReadAsObject(); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + + pageResult = content["value"] as JArray; + Assert.NotNull(pageResult); + Assert.Single(pageResult); + Assert.Equal(6, (pageResult[0] as JObject)["Id"].ToObject()); + Assert.Equal(35, (pageResult[0] as JObject)["CreditLimit"].ToObject()); + Assert.Null(content.GetValue("@odata.nextLink")); + } + } }