[release/9.0-staging] Fix to #35393 - GroupJoin in EF Core 9 Returns Null for Joined Entities

Fixes #35393

Description
In EF9 we refactored some optimizations we performed on generated SQL - we are now applying them earlier (as part of SqlExpressionFactory. However, one of those optimizations is only correct for non-nullable arguments, and at the time we apply them we don't have full nullability information. Fix is to only apply this optimization when we know the argument is non-nullable.

Customer impact
Data corruption. Queries comparing nullable boolean property to a constant (true/false) produced incorrect results (data corruption). Workaround is to rewrite the query to add missing null checks, but this is not intuitive at all.

How found
Customer report on 9.0.0

Regression
Yes, from 8.0.

Testing
Added tests for the scenarios affected as well as test that brute-forces all combinations of comparisons between nullable arguments in variety of scenarios.

Risk
Low, we are just removing the optimization that was incorrect. After removal, the queries produced are the same as in EF8. Quirk added, just in case.

Author: maumar

src/EFCore.Relational/Query/SqlExpressionFactory.cs

@@ -9,6 +9,9 @@ namespace Microsoft.EntityFrameworkCore.Query;
 /// <inheritdoc />
 public class SqlExpressionFactory : ISqlExpressionFactory
 {
+    private static readonly bool UseOldBehavior35393 =
+        AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue35393", out var enabled35393) && enabled35393;
+
     private readonly IRelationalTypeMappingSource _typeMappingSource;
     private readonly RelationalTypeMapping _boolTypeMapping;
 
@@ -660,20 +663,45 @@ private SqlExpression Not(SqlExpression operand, SqlExpression? existingExpressi
             SqlBinaryExpression { OperatorType: ExpressionType.OrElse } binary
                 => AndAlso(Not(binary.Left), Not(binary.Right)),
 
-            // use equality where possible
+            // use equality where possible - we can only do this when we know a is not null
+            // at this point we are limited to constants, parameters and columns
+            // see issue #35393
             // !(a == true) -> a == false
             // !(a == false) -> a == true
             SqlBinaryExpression { OperatorType: ExpressionType.Equal, Right: SqlConstantExpression { Value: bool } } binary
+                when UseOldBehavior35393
+                => Equal(binary.Left, Not(binary.Right)),
+
+            SqlBinaryExpression
+            {
+                OperatorType: ExpressionType.Equal,
+                Right: SqlConstantExpression { Value: bool },
+                Left: SqlConstantExpression { Value: bool }
+                    or SqlParameterExpression { IsNullable: false }
+                    or ColumnExpression { IsNullable: false }
+            } binary
                 => Equal(binary.Left, Not(binary.Right)),
 
             // !(true == a) -> false == a
             // !(false == a) -> true == a
             SqlBinaryExpression { OperatorType: ExpressionType.Equal, Left: SqlConstantExpression { Value: bool } } binary
+                when UseOldBehavior35393
+                => Equal(Not(binary.Left), binary.Right),
+
+            SqlBinaryExpression
+            {
+                OperatorType: ExpressionType.Equal,
+                Left: SqlConstantExpression { Value: bool },
+                Right: SqlConstantExpression { Value: bool }
+                    or SqlParameterExpression { IsNullable: false }
+                    or ColumnExpression { IsNullable: false }
+            } binary
                 => Equal(Not(binary.Left), binary.Right),
 
             // !(a == b) -> a != b
             SqlBinaryExpression { OperatorType: ExpressionType.Equal } sqlBinaryOperand => NotEqual(
                 sqlBinaryOperand.Left, sqlBinaryOperand.Right),
+
             // !(a != b) -> a == b
             SqlBinaryExpression { OperatorType: ExpressionType.NotEqual } sqlBinaryOperand => Equal(
                 sqlBinaryOperand.Left, sqlBinaryOperand.Right),

src/EFCore.Relational/Query/SqlNullabilityProcessor.cs

@@ -19,6 +19,9 @@ namespace Microsoft.EntityFrameworkCore.Query;
 /// </summary>
 public class SqlNullabilityProcessor
 {
+    private static readonly bool UseOldBehavior35393 =
+        AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue35393", out var enabled35393) && enabled35393;
+
     private readonly List<ColumnExpression> _nonNullableColumns;
     private readonly List<ColumnExpression> _nullValueColumns;
     private readonly ISqlExpressionFactory _sqlExpressionFactory;
@@ -1343,7 +1346,7 @@ protected virtual SqlExpression VisitSqlBinary(
             // we assume that NullSemantics rewrite is only needed (on the current level)
             // if the optimization didn't make any changes.
             // Reason is that optimization can/will change the nullability of the resulting expression
-            // and that inforation is not tracked/stored anywhere
+            // and that information is not tracked/stored anywhere
             // so we can no longer rely on nullabilities that we computed earlier (leftNullable, rightNullable)
             // when performing null semantics rewrite.
             // It should be fine because current optimizations *radically* change the expression

test/EFCore.Relational.Specification.Tests/Query/NullSemanticsQueryTestBase.cs

@@ -92,22 +92,26 @@ public virtual async Task Rewrite_compare_int_with_int(bool async)
     public virtual async Task Rewrite_compare_bool_with_bool(bool async)
     {
         var bools = new[] { false, true };
+        var onetwothree = new[] { 1, 2, 3 };
 
         foreach (var neq in bools)
         {
             foreach (var negated in bools)
             {
                 foreach (var negateB in bools)
                 {
-                    foreach (var nullableA in bools)
+                    foreach (var nullableA in onetwothree)
                     {
                         foreach (var negateA in bools)
                         {
-                            foreach (var nullableB in bools)
+                            foreach (var nullableB in onetwothree)
                             {
+                                // filter out tests comparing two constants
+                                if (nullableA == 3 && nullableB == 3) continue;
+
                                 var queryBuilder = (ISetSource ss) =>
                                 {
-                                    var data = nullableA
+                                    var data = nullableA == 2
                                         ? ss.Set<NullSemanticsEntity1>().Select(
                                             e => new
                                             {
@@ -116,30 +120,47 @@ public virtual async Task Rewrite_compare_bool_with_bool(bool async)
                                                 e.BoolB,
                                                 e.NullableBoolB
                                             })
-                                        : ss.Set<NullSemanticsEntity1>().Select(
-                                            e => new
-                                            {
-                                                e.Id,
-                                                A = (bool?)e.BoolA,
-                                                e.BoolB,
-                                                e.NullableBoolB
-                                            });
-
-                                    var query = nullableB
+                                        : nullableA == 1
+                                            ? ss.Set<NullSemanticsEntity1>().Select(
+                                                e => new
+                                                {
+                                                    e.Id,
+                                                    A = (bool?)e.BoolA,
+                                                    e.BoolB,
+                                                    e.NullableBoolB
+                                                })
+                                            : ss.Set<NullSemanticsEntity1>().Select(
+                                                e => new
+                                                {
+                                                    e.Id,
+                                                    A = (bool?)true,
+                                                    e.BoolB,
+                                                    e.NullableBoolB
+                                                });
+
+                                    var query = nullableB == 2
                                         ? data.Select(
                                             e => new
                                             {
                                                 e.Id,
                                                 e.A,
                                                 B = e.NullableBoolB
                                             })
-                                        : data.Select(
-                                            e => new
-                                            {
-                                                e.Id,
-                                                e.A,
-                                                B = (bool?)e.BoolB
-                                            });
+                                        : nullableB == 1
+                                            ? data.Select(
+                                                e => new
+                                                {
+                                                    e.Id,
+                                                    e.A,
+                                                    B = (bool?)e.BoolB
+                                                })
+                                            : data.Select(
+                                                e => new
+                                                {
+                                                    e.Id,
+                                                    e.A,
+                                                    B = (bool?)true
+                                                });
 
                                     query = negateA
                                         ? query.Select(
@@ -2462,6 +2483,18 @@ await AssertQueryScalar(
             ss => ss.Set<NullSemanticsEntity1>().Where(e => true).Select(e => e.Id));
     }
 
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public virtual async Task Compare_constant_true_to_expression_which_evaluates_to_null(bool async)
+    {
+        var prm = default(bool?);
+
+        await AssertQueryScalar(
+            async,
+            ss => ss.Set<NullSemanticsEntity1>().Where(x => x.NullableBoolA != null
+                && !object.Equals(true, x.NullableBoolA == null ? null : prm)).Select(x => x.Id));
+    }
+
     // We can't client-evaluate Like (for the expected results).
     // However, since the test data has no LIKE wildcards, it effectively functions like equality - except that 'null like null' returns
     // false instead of true. So we have this "lite" implementation which doesn't support wildcards.

test/EFCore.Specification.Tests/Query/NorthwindJoinQueryTestBase.cs

@@ -654,6 +654,24 @@ join o in ss.Set<Order>().OrderBy(o => o.OrderID).Take(100) on c.CustomerID equa
                   from o in lo.Where(x => x.CustomerID.StartsWith("A"))
                   select new { c.CustomerID, o.OrderID });
 
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public virtual Task GroupJoin_on_true_equal_true(bool async)
+    => AssertQuery(
+        async,
+        ss => ss.Set<Customer>().GroupJoin(
+            ss.Set<Order>(),
+            x => true,
+            x => true,
+            (c, g) => new { c, g })
+        .Select(x => new { x.c.CustomerID, Orders = x.g }),
+        elementSorter: e => e.CustomerID,
+        elementAsserter: (e, a) =>
+        {
+            Assert.Equal(e.CustomerID, a.CustomerID);
+            AssertCollection(e.Orders, a.Orders, elementSorter: ee => ee.OrderID);
+        });
+
     [ConditionalTheory]
     [MemberData(nameof(IsAsyncData))]
     public virtual Task Inner_join_with_tautology_predicate_converts_to_cross_join(bool async)

test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs

@@ -5021,7 +5021,7 @@ INNER JOIN (
     FROM [Factions] AS [f]
     WHERE [f].[Name] = N'Swarm'
 ) AS [f0] ON [l].[Name] = [f0].[CommanderName]
-WHERE [f0].[Eradicated] = CAST(0 AS bit) OR [f0].[Eradicated] IS NULL
+WHERE [f0].[Eradicated] <> CAST(1 AS bit) OR [f0].[Eradicated] IS NULL
 """);
     }
 
@@ -5038,7 +5038,7 @@ LEFT JOIN (
     FROM [Factions] AS [f]
     WHERE [f].[Name] = N'Swarm'
 ) AS [f0] ON [l].[Name] = [f0].[CommanderName]
-WHERE [f0].[Eradicated] = CAST(0 AS bit) OR [f0].[Eradicated] IS NULL
+WHERE [f0].[Eradicated] <> CAST(1 AS bit) OR [f0].[Eradicated] IS NULL
 """);
     }
 
@@ -7592,7 +7592,7 @@ FROM [LocustLeaders] AS [l]
 INNER JOIN [Factions] AS [f] ON [l].[Name] = [f].[CommanderName]
 WHERE CASE
     WHEN [f].[Name] = N'Locust' THEN CAST(1 AS bit)
-END = CAST(0 AS bit) OR CASE
+END <> CAST(1 AS bit) OR CASE
     WHEN [f].[Name] = N'Locust' THEN CAST(1 AS bit)
 END IS NULL
 """);

test/EFCore.SqlServer.FunctionalTests/Query/NorthwindJoinQuerySqlServerTest.cs

@@ -992,6 +992,22 @@ ORDER BY [o].[OrderID]
 """);
     }
 
+    public override async Task GroupJoin_on_true_equal_true(bool async)
+    {
+        await base.GroupJoin_on_true_equal_true(async);
+
+        AssertSql(
+            """
+SELECT [c].[CustomerID], [o0].[OrderID], [o0].[CustomerID], [o0].[EmployeeID], [o0].[OrderDate]
+FROM [Customers] AS [c]
+OUTER APPLY (
+    SELECT [o].[OrderID], [o].[CustomerID], [o].[EmployeeID], [o].[OrderDate]
+    FROM [Orders] AS [o]
+) AS [o0]
+ORDER BY [c].[CustomerID]
+""");
+    }
+
     private void AssertSql(params string[] expected)
         => Fixture.TestSqlLoggerFactory.AssertBaseline(expected);