diff --git a/src/GreenDonut/src/GreenDonut.Data.EntityFramework/Expressions/ExpressionHelpers.cs b/src/GreenDonut/src/GreenDonut.Data.EntityFramework/Expressions/ExpressionHelpers.cs index 850860e3d8a..cbb9e688651 100644 --- a/src/GreenDonut/src/GreenDonut.Data.EntityFramework/Expressions/ExpressionHelpers.cs +++ b/src/GreenDonut/src/GreenDonut.Data.EntityFramework/Expressions/ExpressionHelpers.cs @@ -154,6 +154,7 @@ private static Expression BuildEqualToKeyExpr( Expression cursorExpr) { var keyExpr = ReplaceParameter(cursorKey.Expression, parameter); + keyExpr = LiftValueTypeIfNullable(keyExpr, keyIsNullable); // Access the value of the key if it is a nullable value type. var keyValueExpr = cursorKey.Expression.ReturnType.IsValueType && keyIsNullable @@ -197,6 +198,7 @@ private static Expression BuildGreaterThanKeyExpr( Expression cursorExpr) { var keyExpr = ReplaceParameter(cursorKey.Expression, parameter); + keyExpr = LiftValueTypeIfNullable(keyExpr, keyIsNullable); // Access the value of the key if it is a nullable value type. var keyValueExpr = @@ -255,6 +257,7 @@ private static Expression BuildLessThanKeyExpr( Expression cursorExpr) { var keyExpr = ReplaceParameter(cursorKey.Expression, parameter); + keyExpr = LiftValueTypeIfNullable(keyExpr, keyIsNullable); // Access the value of the key if it is a nullable value type. var keyValueExpr = @@ -306,38 +309,37 @@ private static Expression BuildLessThanKeyExpr( private static bool IsNullable(LambdaExpression expression) { - if (expression.ReturnType.IsValueType) + // A nullable value-type return (for example an explicit cast to int?). + if (expression.ReturnType.IsValueType + && Nullable.GetUnderlyingType(expression.ReturnType) is not null) { - return Nullable.GetUnderlyingType(expression.ReturnType) is not null; + return true; } - var member = expression.Body switch + var current = StripConvert(expression.Body); + + // (a ?? b) is null only when the fallback b is null. + if (current is BinaryExpression { NodeType: ExpressionType.Coalesce, Right: var right }) { - MemberExpression { Member: PropertyInfo or FieldInfo } m => m.Member, - BinaryExpression - { - NodeType: ExpressionType.Coalesce, - Right: MemberExpression { Member: PropertyInfo or FieldInfo } m - } => m.Member, - _ => null - }; + current = StripConvert(right); + } - if (member is not null) + // The key value is nullable if the leaf or any intermediate navigation along + // the member-access path is nullable (for example x.Meter.Id is null when + // x.Meter is null). + while (current is MemberExpression { Member: PropertyInfo or FieldInfo } member) { - var state = member switch + if (IsMemberNullable(member)) { - PropertyInfo p => GetNullabilityInfoState(p), - FieldInfo f => GetNullabilityInfoState(f), - _ => throw new InvalidOperationException() - }; + return true; + } - return state switch + if (member.Expression is null) { - NullabilityState.Nullable => true, - // Unknown means the assembly was compiled without NRT annotations; - // treat as non-nullable (safe default). - _ => false - }; + break; + } + + current = StripConvert(member.Expression); } // For computed key expressions (method calls, concatenation, etc.) we cannot inspect @@ -623,6 +625,53 @@ private static Expression CreateAndConvertParameter(T value) return lambda.Body; } + /// + /// Determines whether a member access can yield null: a value-type member + /// only when it is , and a reference-type member according + /// to its nullable reference type annotation. + /// + private static bool IsMemberNullable(MemberExpression member) + { + if (member.Type.IsValueType) + { + return Nullable.GetUnderlyingType(member.Type) is not null; + } + + // Unknown means the assembly was compiled without NRT annotations; + // treat as non-nullable (safe default). + return member.Member switch + { + PropertyInfo p => GetNullabilityInfoState(p) == NullabilityState.Nullable, + FieldInfo f => GetNullabilityInfoState(f) == NullabilityState.Nullable, + _ => false + }; + } + + /// + /// Removes any or + /// wrappers from an expression and + /// returns the underlying operand. + /// + private static Expression StripConvert(Expression expression) + => expression is UnaryExpression + { + NodeType: ExpressionType.Convert or ExpressionType.ConvertChecked + } unary + ? StripConvert(unary.Operand) + : expression; + + /// + /// Lifts a non-nullable value-type key (for example int for + /// x.Meter.Id) to its form when the key is + /// nullable because of an intermediate navigation, so the key can be compared + /// against null. Keys that are already nullable or are reference types + /// are returned unchanged. + /// + private static Expression LiftValueTypeIfNullable(Expression keyExpr, bool keyIsNullable) + => keyIsNullable && keyExpr.Type.IsValueType && Nullable.GetUnderlyingType(keyExpr.Type) is null + ? Expression.Convert(keyExpr, typeof(Nullable<>).MakeGenericType(keyExpr.Type)) + : keyExpr; + private static Expression BuildEqualComparison( CursorKey cursorKey, Expression keyExpr, diff --git a/src/GreenDonut/test/GreenDonut.Data.EntityFramework.Tests/PagingHelperPostgreSqlNullableTests.cs b/src/GreenDonut/test/GreenDonut.Data.EntityFramework.Tests/PagingHelperPostgreSqlNullableTests.cs index 0a7c458e001..448c1cdafc2 100644 --- a/src/GreenDonut/test/GreenDonut.Data.EntityFramework.Tests/PagingHelperPostgreSqlNullableTests.cs +++ b/src/GreenDonut/test/GreenDonut.Data.EntityFramework.Tests/PagingHelperPostgreSqlNullableTests.cs @@ -313,6 +313,62 @@ await context.Records (await Assert.ThrowsAsync(Act)).Message); } + [Fact] + public async Task Paging_NullableNavigation_ValueTypeLeaf_Pages_Across_Null_Boundary() + { + // Arrange + var connectionString = CreateConnectionString(); + await SeedItemsAsync(connectionString); + await using var context = new NullableTestsContext(Provider.PostgreSql, connectionString); + + // Act + // Sort by a non-nullable value-type leaf (Detail.Number) behind a nullable + // navigation; the second page is requested after a null-navigation boundary. + var arguments = new PagingArguments(2) { NullOrdering = NullOrdering.NativeNullsLast }; + var page1 = await context.Items + .OrderBy(x => x.Detail!.Number) + .ThenBy(x => x.Id) + .ToPageAsync(arguments, Xunit.TestContext.Current.CancellationToken); + + arguments = arguments with { After = page1.CreateEndCursor() }; + var page2 = await context.Items + .OrderBy(x => x.Detail!.Number) + .ThenBy(x => x.Id) + .ToPageAsync(arguments, Xunit.TestContext.Current.CancellationToken); + + // Assert + Assert.Equal([1, 2], page1.Select(x => x.Id)); + Assert.Equal([3, 4], page2.Select(x => x.Id)); + } + + [Fact] + public async Task Paging_NullableNavigation_ReferenceLeaf_Pages_Across_Null_Boundary() + { + // Arrange + var connectionString = CreateConnectionString(); + await SeedItemsAsync(connectionString); + await using var context = new NullableTestsContext(Provider.PostgreSql, connectionString); + + // Act + // Sort by a non-nullable reference leaf (Detail.Name) behind a nullable + // navigation; the second page is requested after a null-navigation boundary. + var arguments = new PagingArguments(2) { NullOrdering = NullOrdering.NativeNullsLast }; + var page1 = await context.Items + .OrderBy(x => x.Detail!.Name) + .ThenBy(x => x.Id) + .ToPageAsync(arguments, Xunit.TestContext.Current.CancellationToken); + + arguments = arguments with { After = page1.CreateEndCursor() }; + var page2 = await context.Items + .OrderBy(x => x.Detail!.Name) + .ThenBy(x => x.Id) + .ToPageAsync(arguments, Xunit.TestContext.Current.CancellationToken); + + // Assert + Assert.Equal([1, 2], page1.Select(x => x.Id)); + Assert.Equal([3, 4], page2.Select(x => x.Id)); + } + private static async Task SeedAsync(string connectionString) { await using var context = new NullableTestsContext(Provider.PostgreSql, connectionString); @@ -359,4 +415,20 @@ private static async Task SeedAsync(string connectionString) await context.SaveChangesAsync(); } + + private static async Task SeedItemsAsync(string connectionString) + { + await using var context = new NullableTestsContext(Provider.PostgreSql, connectionString); + await context.Database.EnsureCreatedAsync(); + + // Only the first item has a navigation; the rest are null and sort last, + // so paging crosses a null-navigation boundary between page 1 and page 2. + context.Items.AddRange( + new Item { Id = 1, Detail = new Detail { Number = 10, Name = "a" } }, + new Item { Id = 2 }, + new Item { Id = 3 }, + new Item { Id = 4 }); + + await context.SaveChangesAsync(); + } } diff --git a/src/GreenDonut/test/GreenDonut.Data.EntityFramework.Tests/PagingHelperSqlServerNullableTests.cs b/src/GreenDonut/test/GreenDonut.Data.EntityFramework.Tests/PagingHelperSqlServerNullableTests.cs index 03efddc8bb2..02bcbfa7fb0 100644 --- a/src/GreenDonut/test/GreenDonut.Data.EntityFramework.Tests/PagingHelperSqlServerNullableTests.cs +++ b/src/GreenDonut/test/GreenDonut.Data.EntityFramework.Tests/PagingHelperSqlServerNullableTests.cs @@ -266,6 +266,64 @@ public async Task Paging_NullableReference_Descending_Cursor_Value_Null() snapshot.MatchMarkdownSnapshot(); } + [Fact] + public async Task Paging_NullableNavigation_ValueTypeLeaf_Pages_Across_Null_Boundary() + { + // Arrange + var connectionString = CreateConnectionString(); + await SeedItemsAsync(connectionString); + await using var context = new NullableTestsContext(Provider.SqlServer, connectionString); + + // Act + // Sort by a non-nullable value-type leaf (Detail.Number) behind a nullable + // navigation; with nulls first the second page is requested after a + // null-navigation boundary. + var arguments = new PagingArguments(2) { NullOrdering = NullOrdering.NativeNullsFirst }; + var page1 = await context.Items + .OrderBy(x => x.Detail!.Number) + .ThenBy(x => x.Id) + .ToPageAsync(arguments, Xunit.TestContext.Current.CancellationToken); + + arguments = arguments with { After = page1.CreateEndCursor() }; + var page2 = await context.Items + .OrderBy(x => x.Detail!.Number) + .ThenBy(x => x.Id) + .ToPageAsync(arguments, Xunit.TestContext.Current.CancellationToken); + + // Assert + Assert.Equal([2, 3], page1.Select(x => x.Id)); + Assert.Equal([4, 1], page2.Select(x => x.Id)); + } + + [Fact] + public async Task Paging_NullableNavigation_ReferenceLeaf_Pages_Across_Null_Boundary() + { + // Arrange + var connectionString = CreateConnectionString(); + await SeedItemsAsync(connectionString); + await using var context = new NullableTestsContext(Provider.SqlServer, connectionString); + + // Act + // Sort by a non-nullable reference leaf (Detail.Name) behind a nullable + // navigation; with nulls first the second page is requested after a + // null-navigation boundary. + var arguments = new PagingArguments(2) { NullOrdering = NullOrdering.NativeNullsFirst }; + var page1 = await context.Items + .OrderBy(x => x.Detail!.Name) + .ThenBy(x => x.Id) + .ToPageAsync(arguments, Xunit.TestContext.Current.CancellationToken); + + arguments = arguments with { After = page1.CreateEndCursor() }; + var page2 = await context.Items + .OrderBy(x => x.Detail!.Name) + .ThenBy(x => x.Id) + .ToPageAsync(arguments, Xunit.TestContext.Current.CancellationToken); + + // Assert + Assert.Equal([2, 3], page1.Select(x => x.Id)); + Assert.Equal([4, 1], page2.Select(x => x.Id)); + } + private static async Task SeedAsync(string connectionString) { await using var context = new NullableTestsContext(Provider.SqlServer, connectionString); @@ -312,4 +370,20 @@ private static async Task SeedAsync(string connectionString) await context.SaveChangesAsync(); } + + private static async Task SeedItemsAsync(string connectionString) + { + await using var context = new NullableTestsContext(Provider.SqlServer, connectionString); + await context.Database.EnsureCreatedAsync(); + + // Only the first item has a navigation; the rest are null and sort first, + // so paging crosses a null-navigation boundary between page 1 and page 2. + context.Items.AddRange( + new Item { Id = 1, Detail = new Detail { Number = 10, Name = "a" } }, + new Item { Id = 2 }, + new Item { Id = 3 }, + new Item { Id = 4 }); + + await context.SaveChangesAsync(); + } } diff --git a/src/GreenDonut/test/GreenDonut.Data.EntityFramework.Tests/TestContext/NullableTestsContext.cs b/src/GreenDonut/test/GreenDonut.Data.EntityFramework.Tests/TestContext/NullableTestsContext.cs index 67c10249666..707ce67782a 100644 --- a/src/GreenDonut/test/GreenDonut.Data.EntityFramework.Tests/TestContext/NullableTestsContext.cs +++ b/src/GreenDonut/test/GreenDonut.Data.EntityFramework.Tests/TestContext/NullableTestsContext.cs @@ -1,3 +1,4 @@ +using System.ComponentModel.DataAnnotations.Schema; using Microsoft.EntityFrameworkCore; namespace GreenDonut.Data.TestContext; @@ -20,6 +21,8 @@ protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder) } public DbSet Records { get; set; } = null!; + + public DbSet Items { get; set; } = null!; } public class Record @@ -30,6 +33,25 @@ public class Record public string? String { get; set; } } +public class Item +{ + [DatabaseGenerated(DatabaseGeneratedOption.None)] + public int Id { get; set; } + + public int? DetailId { get; set; } + + public Detail? Detail { get; set; } +} + +public class Detail +{ + public int Id { get; set; } + + public int Number { get; set; } + + public string Name { get; set; } = null!; +} + public enum Provider { PostgreSql,