fix: sorting with nested nullables

pdevito3 · pdevito3 · commit 7d122137e0cf · 2025-11-24T08:29:25.000-05:00
diff --git a/QueryKit.IntegrationTests/Tests/DatabaseSortingTests.cs b/QueryKit.IntegrationTests/Tests/DatabaseSortingTests.cs
@@ -6,7 +6,10 @@ namespace QueryKit.IntegrationTests.Tests;
 using QueryKit.Configuration;
 using QueryKit.Exceptions;
 using SharedTestingHelper.Fakes;
+using SharedTestingHelper.Fakes.Author;
+using SharedTestingHelper.Fakes.Recipes;
 using WebApiTestProject.Entities;
+using WebApiTestProject.Entities.Recipes;
 
 public class DatabaseSortingTests : TestBase
 {
@@ -361,4 +364,102 @@ public async Task can_sort_by_derived_property_using_complex_patient_like_scenar
         people[2].FirstName.Should().BeNull();
         people[2].LastName.Should().BeNull();
     }
+
+    [Fact]
+    public async Task can_sort_by_nullable_navigation_property()
+    {
+        // Arrange - This tests the scenario where a navigation property (Author) can be null
+        // Similar to the user's scenario: MatchPlayer.Player.LastName where Player is nullable
+        var testingServiceScope = new TestingServiceScope();
+        var uniqueId = Guid.NewGuid().ToString()[..8];
+
+        // Create recipe WITH author (name starting with "A")
+        var authorA = new FakeAuthorBuilder()
+            .WithName($"Alice_{uniqueId}")
+            .Build();
+        var recipeWithAuthorA = new FakeRecipeBuilder()
+            .WithTitle($"Recipe1_{uniqueId}")
+            .Build();
+        recipeWithAuthorA.SetAuthor(authorA);
+
+        // Create recipe WITH author (name starting with "Z")
+        var authorZ = new FakeAuthorBuilder()
+            .WithName($"Zoe_{uniqueId}")
+            .Build();
+        var recipeWithAuthorZ = new FakeRecipeBuilder()
+            .WithTitle($"Recipe2_{uniqueId}")
+            .Build();
+        recipeWithAuthorZ.SetAuthor(authorZ);
+
+        // Create recipe WITHOUT author (null navigation property)
+        var recipeWithoutAuthor = new FakeRecipeBuilder()
+            .WithTitle($"Recipe3_{uniqueId}")
+            .Build();
+        // Note: No SetAuthor call - Author will be null
+
+        await testingServiceScope.InsertAsync(recipeWithAuthorZ, recipeWithoutAuthor, recipeWithAuthorA);
+
+        // Act - Try to sort by nested navigation property Author.Name when Author can be null
+        var queryableRecipes = testingServiceScope.DbContext().Recipes
+            .Include(r => r.Author)
+            .Where(r => r.Title.Contains(uniqueId));
+
+        // This should handle null navigation properties gracefully
+        var appliedQueryable = queryableRecipes.ApplyQueryKitSort("Author.Name asc");
+        var recipes = await appliedQueryable.ToListAsync();
+
+        // Assert - Should sort without errors, with null authors sorted appropriately
+        recipes.Count.Should().Be(3);
+
+        // In PostgreSQL with ASC, nulls are sorted LAST by default
+        // So we expect: Alice, Zoe, then null
+        recipes[0].Title.Should().Be($"Recipe1_{uniqueId}"); // Alice
+        recipes[1].Title.Should().Be($"Recipe2_{uniqueId}"); // Zoe
+        recipes[2].Title.Should().Be($"Recipe3_{uniqueId}"); // null author
+        recipes[2].Author.Should().BeNull();
+    }
+
+    [Fact]
+    public async Task can_sort_descending_by_nullable_navigation_property()
+    {
+        // Arrange - Test DESC sorting with null navigation properties
+        var testingServiceScope = new TestingServiceScope();
+        var uniqueId = Guid.NewGuid().ToString()[..8];
+
+        var authorA = new FakeAuthorBuilder()
+            .WithName($"Alice_{uniqueId}")
+            .Build();
+        var recipeWithAuthorA = new FakeRecipeBuilder()
+            .WithTitle($"RecipeA_{uniqueId}")
+            .Build();
+        recipeWithAuthorA.SetAuthor(authorA);
+
+        var authorZ = new FakeAuthorBuilder()
+            .WithName($"Zoe_{uniqueId}")
+            .Build();
+        var recipeWithAuthorZ = new FakeRecipeBuilder()
+            .WithTitle($"RecipeZ_{uniqueId}")
+            .Build();
+        recipeWithAuthorZ.SetAuthor(authorZ);
+
+        var recipeWithoutAuthor = new FakeRecipeBuilder()
+            .WithTitle($"RecipeNull_{uniqueId}")
+            .Build();
+
+        await testingServiceScope.InsertAsync(recipeWithAuthorA, recipeWithoutAuthor, recipeWithAuthorZ);
+
+        // Act
+        var queryableRecipes = testingServiceScope.DbContext().Recipes
+            .Include(r => r.Author)
+            .Where(r => r.Title.Contains(uniqueId));
+
+        var appliedQueryable = queryableRecipes.ApplyQueryKitSort("Author.Name desc");
+        var recipes = await appliedQueryable.ToListAsync();
+
+        // Assert - In PostgreSQL with DESC, nulls are sorted FIRST by default
+        recipes.Count.Should().Be(3);
+        recipes[0].Title.Should().Be($"RecipeNull_{uniqueId}"); // null author first in DESC
+        recipes[1].Title.Should().Be($"RecipeZ_{uniqueId}"); // Zoe
+        recipes[2].Title.Should().Be($"RecipeA_{uniqueId}"); // Alice
+    }
 }
diff --git a/QueryKit.UnitTests/SortParserTests.cs b/QueryKit.UnitTests/SortParserTests.cs
@@ -301,16 +301,115 @@ private string GetMemberName<T>(Expression<Func<T, object>>? expr)
 
     private string GetMemberNameFromExpression(Expression expr)
     {
+        // Handle conditional expressions (generated by null-safe navigation)
+        // For x.A == null ? null : x.A.B, we want to extract "A.B"
+        if (expr is ConditionalExpression conditional)
+        {
+            // The IfFalse branch contains the actual property access chain
+            return GetMemberNameFromExpression(conditional.IfFalse);
+        }
+
+        // Handle Convert expressions that wrap the actual value
+        if (expr is UnaryExpression { NodeType: ExpressionType.Convert } convertExpr)
+        {
+            return GetMemberNameFromExpression(convertExpr.Operand);
+        }
+
         if (expr is MemberExpression memberExpr)
         {
-            if (memberExpr.Expression.NodeType == ExpressionType.MemberAccess)
+            if (memberExpr.Expression?.NodeType == ExpressionType.MemberAccess)
             {
                 return GetMemberNameFromExpression(memberExpr.Expression) + "." + memberExpr.Member.Name;
             }
 
+            // Check for nested conditional in the expression
+            if (memberExpr.Expression is ConditionalExpression nestedConditional)
+            {
+                var parentName = GetMemberNameFromExpression(nestedConditional);
+                return parentName + "." + memberExpr.Member.Name;
+            }
+
             return memberExpr.Member.Name;
         }
 
         throw new ArgumentException("Invalid expression type", nameof(expr));
     }
+
+    #region Nullable Navigation Property Tests
+
+    // Test entities for nullable navigation property scenarios
+    public class Player
+    {
+        public string? FirstName { get; set; }
+        public string? LastName { get; set; }
+    }
+
+    public class MatchPlayer
+    {
+        public string? FirstName { get; set; }
+        public string? LastName { get; set; }
+        public Player? Player { get; set; }  // Nullable navigation property
+    }
+
+    public class PlayerStat
+    {
+        public string Id { get; set; } = Guid.NewGuid().ToString();
+        public int Score { get; set; }
+        public MatchPlayer MatchPlayer { get; set; } = new();
+    }
+
+    [Fact]
+    public void in_memory_sort_handles_null_navigation_property_gracefully()
+    {
+        // Arrange - Create data where MatchPlayer.Player is null for some items
+        var stats = new List<PlayerStat>
+        {
+            new() { Score = 10, MatchPlayer = new MatchPlayer { FirstName = "Alice", Player = new Player { LastName = "Anderson" } } },
+            new() { Score = 20, MatchPlayer = new MatchPlayer { FirstName = "Bob", Player = null } }, // null Player!
+            new() { Score = 30, MatchPlayer = new MatchPlayer { FirstName = "Charlie", Player = new Player { LastName = "Clark" } } },
+        };
+
+        // Act - Sorting by MatchPlayer.Player.LastName should handle null Player gracefully
+        var sorted = stats.ApplyQueryKitSort("MatchPlayer.Player.LastName asc").ToList();
+
+        // Assert - Should not throw, nulls should sort first in ASC order
+        sorted.Count.Should().Be(3);
+        // Null values sort first, then alphabetically: null (Bob), Anderson (Alice), Clark (Charlie)
+        sorted[0].MatchPlayer.FirstName.Should().Be("Bob");     // null Player -> null LastName sorts first
+        sorted[1].MatchPlayer.FirstName.Should().Be("Alice");   // Anderson
+        sorted[2].MatchPlayer.FirstName.Should().Be("Charlie"); // Clark
+    }
+
+    [Fact]
+    public void in_memory_sort_works_with_derived_property_null_handling()
+    {
+        // Arrange - The user's workaround uses a derived property with null coalescing
+        var stats = new List<PlayerStat>
+        {
+            new() { Score = 10, MatchPlayer = new MatchPlayer { FirstName = "Alice", LastName = "Direct_A", Player = new Player { LastName = "Anderson" } } },
+            new() { Score = 20, MatchPlayer = new MatchPlayer { FirstName = "Bob", LastName = "Direct_B", Player = null } }, // null Player - should use MatchPlayer.LastName
+            new() { Score = 30, MatchPlayer = new MatchPlayer { FirstName = "Charlie", LastName = null, Player = new Player { LastName = "Clark" } } }, // null MatchPlayer.LastName - should use Player.LastName
+        };
+
+        var config = new QueryKitConfiguration(c =>
+        {
+            // User's workaround pattern: coalesce MatchPlayer.LastName with Player.LastName
+            c.DerivedProperty<PlayerStat>(x =>
+                x.MatchPlayer.LastName ??
+                (x.MatchPlayer.Player != null ? x.MatchPlayer.Player.LastName : "") ?? "")
+                .HasQueryName("playerLastName");
+        });
+
+        // Act - Should work without throwing
+        var sorted = stats.ApplyQueryKitSort("playerLastName asc", config).ToList();
+
+        // Assert
+        sorted.Count.Should().Be(3);
+        // Order: "Clark", "Direct_A", "Direct_B" (alphabetical by coalesced last name)
+        sorted[0].MatchPlayer.FirstName.Should().Be("Charlie"); // Clark
+        sorted[1].MatchPlayer.FirstName.Should().Be("Alice");   // Direct_A
+        sorted[2].MatchPlayer.FirstName.Should().Be("Bob");     // Direct_B
+    }
+
+    #endregion
 }
diff --git a/QueryKit/SortParser.cs b/QueryKit/SortParser.cs
@@ -96,25 +96,64 @@ private static SortExpressionInfo<T> CreateSortExpression<T>(string sortClause,
             return parameterReplacer.Visit(derivedPropertyInfo.DerivedExpression);
         }
 
-        // Handle regular properties
+        // Handle regular properties with null-safe navigation
         var propertyPath = config?.GetPropertyPathByQueryName(propertyName) ?? propertyName;
         var propertyNames = propertyPath.Split('.');
 
-        var propertyExpression = propertyNames.Aggregate(parameter, (expr, propName) =>
+        var result = CreateNullSafePropertyExpression(parameter, propertyNames, 0);
+        if (result == null)
+            throw new SortParsingException(propertyName);
+
+        return result;
+    }
+
+    private static Expression? CreateNullSafePropertyExpression(Expression currentExpr, string[] propertyNames, int index)
+    {
+        if (index >= propertyNames.Length)
+            return currentExpr;
+
+        var propName = propertyNames[index];
+        var propertyInfo = GetPropertyInfo(currentExpr.Type, propName);
+        if (propertyInfo == null)
+            return null;
+
+        var propertyAccess = Expression.PropertyOrField(currentExpr, propertyInfo.Name);
+
+        // If this is the last property in the chain, just return the access
+        if (index == propertyNames.Length - 1)
+            return propertyAccess;
+
+        // Get the rest of the property chain
+        var restOfChain = CreateNullSafePropertyExpression(propertyAccess, propertyNames, index + 1);
+        if (restOfChain == null)
+            return null;
+
+        // Check if the current property type is a reference type (could be null)
+        var propertyType = propertyInfo.PropertyType;
+        var isNullableType = !propertyType.IsValueType || Nullable.GetUnderlyingType(propertyType) != null;
+
+        if (isNullableType)
         {
-            var propertyInfo = GetPropertyInfo(expr.Type, propName);
-            if (propertyInfo == null)
-            {
-                return null;
-            }
-            var actualPropertyName = propertyInfo?.Name ?? propName;
-            return Expression.PropertyOrField(expr, actualPropertyName);
-        });
+            // It's a reference type or nullable value type - add null check
+            // Generate: property == null ? (TargetType)null : <rest of chain>
+            var targetType = restOfChain.Type;
+            var nullableTargetType = targetType.IsValueType && Nullable.GetUnderlyingType(targetType) == null
+                ? typeof(Nullable<>).MakeGenericType(targetType)
+                : targetType;
+
+            var nullCheck = Expression.Equal(propertyAccess, Expression.Constant(null, propertyType));
+            var nullValue = Expression.Constant(null, nullableTargetType);
+
+            // Convert rest of chain to nullable if needed
+            var convertedRest = targetType != nullableTargetType
+                ? Expression.Convert(restOfChain, nullableTargetType)
+                : restOfChain;
+
+            return Expression.Condition(nullCheck, nullValue, convertedRest);
+        }
 
-        if(propertyExpression == null)
-            throw new SortParsingException(propertyName);
-        
-        return propertyExpression;
+        // Non-nullable value type - continue without null check
+        return restOfChain;
     }
 
     private static PropertyInfo? GetPropertyInfo(Type type, string propertyName)
