Fix to #34760 - NullReferenceException for a custom ValueConverter in…

… EF Core 9 RC 1.

For AOT/precompiled queries we now do special processing (lifting) of all non-literal constants. We essentially provide a way to resolve the constant from well established entry points, e.g. model.
Problem happens for cases where we are projecting a property with a custom converter but do it in such a way that we lose the IProperty information (e.g. by projecting individual property, rather than as part of an entity)
Without IProperty information we can't reliably extract the proper type mapping for that property and as a result we don't know what custom converter to use. What we used to do is "guess" the type mapping based on CLR type of the property.
This is pretty much always wrong, and it also broke non-precompiled scenarios.
Fix is to just use ConvertFromProviderExpression expression irrespective of whether it contains closures or not. We only do that if we don't have the IProperty information.
This also requires disabling non-literal constant validation we do in the post processing, as now some queries may contain "unliftable" constants. We should re-enable the validation once the issue is properly addressed (#33517)

Fixes #34760

src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs

@@ -2743,9 +2743,11 @@ Expression valueExpression
                 // return incorrect mapping. So for that case we would prefer to incorporate the FromProvider lambda, like we used to do before AOT
                 // and only resort to unreliable TypeMappingSource lookup, if the converter expression captures "forbidden" constant
                 // see issue #33517 for more details
-                var requiresLiftableConstant =
-                    new ConstantValidator().RequiresLiftableConstant(converter.ConvertFromProviderExpression.Body);
-                if (property != null || requiresLiftableConstant)
+                // UPDATE: instead of guessing the type mapping in case where we don't have IProperty and converter uses non-literal constant,
+                // we just revert to the pre-AOT behavior, i.e. we still use converter.ConvertFromProviderExpression
+                // this will not work for precompiled query (which realistically was already broken for this scenario - type mapping we "guess"
+                // is pretty much always wrong), but regular case (not pre-compiled) will continue to work. 
+                if (property != null)
                 {
                     var typeMappingExpression = CreateTypeMappingExpression(
                         property, type, typeMapping, _parentVisitor.Dependencies.LiftableConstantFactory);
@@ -2929,29 +2931,6 @@ static Expression CreateTypeMappingExpression(
             }
         }
 
-        private sealed class ConstantValidator : ExpressionVisitor
-        {
-            private bool _requiresLiftableConstant;
-
-            public bool RequiresLiftableConstant(Expression expression)
-            {
-                _requiresLiftableConstant = false;
-                Visit(expression);
-
-                return _requiresLiftableConstant;
-            }
-
-            protected override Expression VisitConstant(ConstantExpression constantExpression)
-            {
-                if (!_requiresLiftableConstant && !LiftableConstantExpressionHelpers.IsLiteral(constantExpression.Value))
-                {
-                    _requiresLiftableConstant = true;
-                }
-
-                return constantExpression;
-            }
-        }
-
         private Expression CreateReadJsonPropertyValueExpression(
             ParameterExpression jsonReaderManagerParameter,
             IProperty property)

src/EFCore/Query/LiftableConstantProcessor.cs

@@ -256,11 +256,17 @@ protected override Expression VisitMember(MemberExpression memberExpression)
             ? memberExpression
             : base.VisitMember(memberExpression);
 
-    protected override Expression VisitConstant(ConstantExpression node)
-    {
-        _unsupportedConstantChecker.Check(node);
-        return node;
-    }
+    // issue #34760 - disabling the liftable constant verification because we sometimes are forced to
+    // use them (when type mapping has custom converter but we can't reliably get the correct type mapping
+    // when building the shaper) - if that converter uses a closure, we will embed it in the shaper
+    // we don't have a reasonalbe alternative currently
+    // Once #33517 is done, we should re-enable this check
+    //protected override Expression VisitConstant(ConstantExpression node)
+    //{
+    //    _unsupportedConstantChecker.Check(node);
+
+    //    return node;
+    //}
 #endif
 
     private sealed class UnsupportedConstantChecker(LiftableConstantProcessor liftableConstantProcessor) : ExpressionVisitor

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

@@ -1066,8 +1066,58 @@ where b.Id > 8
             errorAsserter: errors
                 => Assert.Equal(DesignStrings.QueryComprehensionSyntaxNotSupportedInPrecompiledQueries, errors.Single().Exception.Message));
 
+    [ConditionalFact]
+    public virtual Task Projecting_property_requiring_converter_with_closure_is_not_supported()
+        => Test(
+            "_ =  await context.Tags.Select(x => x.CreateDate)).ToListAsync();",
+            errorAsserter: errors
+                => Assert.Equal(
+"""
+Encountered a constant of unsupported type 'MyDatetimeConverter'. Only primitive constant nodes are supported.
+Microsoft.EntityFrameworkCore.Query.PrecompiledQueryRelationalTestBase+MyDatetimeConverter
+""",
+                    errors.Single().Exception.Message));
+
+
+    [ConditionalFact]
+    public virtual Task Projecting_expression_requiring_converter_with_closure_is_not_supported()
+        => Test(
+            """
+var result =  await context.Tags
+    .GroupBy(t => t.Id)
+    .Select(g => new
+    {
+        Day = g.Min(t => t.CreateDate)
+    })
+    .ToListAsync();
+""",
+            errorAsserter: errors
+                => Assert.Equal(
+"""
+Encountered a constant of unsupported type 'MyDatetimeConverter'. Only primitive constant nodes are supported.
+Microsoft.EntityFrameworkCore.Query.PrecompiledQueryRelationalTestBase+MyDatetimeConverter
+""",
+                    errors.Single().Exception.Message));
+
     #endregion Negative cases
 
+    [ConditionalFact]
+    public virtual Task Projecting_expression_requiring_converter_without_closure_works()
+        => Test(
+            """
+var result =  await context.Tags
+    .GroupBy(t => t.Id)
+    .Select(g => new
+    {
+        Day = g.Min(t => t.ModifyDate)
+    })
+    .ToListAsync();
+""");
+
+    [ConditionalFact]
+    public virtual Task Projecting_entity_with_property_requiring_converter_with_closure_works()
+        => Test("_ = await context.Tags.ToListAsync();");
+
     [ConditionalFact]
     public virtual Task Select_changes_type()
         => Test("_ = await context.Blogs.Select(b => b.Name).ToListAsync();");
@@ -1164,6 +1214,7 @@ public class PrecompiledQueryContext(DbContextOptions options) : DbContext(optio
     {
         public DbSet<Blog> Blogs { get; set; } = null!;
         public DbSet<Post> Posts { get; set; } = null!;
+        public DbSet<Tag> Tags { get; set; } = null!;
 
         protected override void OnModelCreating(ModelBuilder modelBuilder)
         {
@@ -1177,6 +1228,12 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
                 });
             modelBuilder.Entity<Blog>().HasMany(x => x.Posts).WithOne(x => x.Blog).OnDelete(DeleteBehavior.Cascade);
             modelBuilder.Entity<Post>().Property(x => x.Id).ValueGeneratedNever();
+            modelBuilder.Entity<Tag>(b =>
+            {
+                b.Property(x => x.Id).ValueGeneratedNever();
+                b.Property(x => x.CreateDate).HasConversion(new MyDateTimeValueConverterWithClosure(new MyDatetimeConverter()));
+                b.Property(x => x.ModifyDate).HasConversion(new MyDateTimeValueConverterWithoutClosure());
+            });
         }
     }
 
@@ -1267,5 +1324,39 @@ public class Post
         public Blog? Blog { get; set; }
     }
 
+    public class Tag
+    {
+        public int Id { get; set; }
+        public string? Name { get; set; }
+        public DateTime CreateDate { get; set; }
+        public DateTime ModifyDate { get; set; }
+    }
+
+    public class MyDateTimeValueConverterWithClosure : ValueConverter<DateTime, DateTime>
+    {
+        public MyDateTimeValueConverterWithClosure(MyDatetimeConverter myDatetimeConverter)
+            : base(
+                x => myDatetimeConverter.Normalize(x),
+                x => myDatetimeConverter.Normalize(x))
+        {
+        }
+    }
+
+    public class MyDateTimeValueConverterWithoutClosure : ValueConverter<DateTime, DateTime>
+    {
+        public MyDateTimeValueConverterWithoutClosure()
+            : base(
+                x => new MyDatetimeConverter().Normalize(x),
+                x => new MyDatetimeConverter().Normalize(x))
+        {
+        }
+    }
+
+    public class MyDatetimeConverter
+    {
+        public virtual DateTime Normalize(DateTime dateTime)
+            => dateTime.Date;
+    }
+
     public static IEnumerable<object[]> IsAsyncData = new object[][] { [false], [true] };
 }

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

@@ -692,4 +692,123 @@ public class Dog : Pet
     }
 
     #endregion
+
+    #region 34760
+
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public virtual async Task Projecting_property_with_converter_with_closure(bool async)
+    {
+        var contextFactory = await InitializeAsync<Context34760>(seed: c => c.SeedAsync());
+        using var context = contextFactory.CreateContext();
+
+        var query = context.Books.Select(x => x.PublishDate);
+
+        var result = await query.ToListAsync();
+        Assert.Equal(2, result.Count);
+    }
+
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public virtual async Task Projecting_expression_with_converter_with_closure(bool async)
+    {
+        var contextFactory = await InitializeAsync<Context34760>(seed: c => c.SeedAsync());
+        using var context = contextFactory.CreateContext();
+
+        var query = context.Books
+            .GroupBy(t => t.Id)
+            .Select(g => new
+            {
+                Day = g.Min(t => t.PublishDate)
+            });
+
+        var result = await query.ToListAsync();
+        Assert.Equal(2, result.Count);
+    }
+
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public virtual async Task Projecting_property_with_converter_without_closure(bool async)
+    {
+        var contextFactory = await InitializeAsync<Context34760>(seed: c => c.SeedAsync());
+        using var context = contextFactory.CreateContext();
+
+        var query = context.Books
+            .GroupBy(t => t.Id)
+            .Select(g => new
+            {
+                Day = g.Min(t => t.AudiobookDate)
+            });
+
+        var result = await query.ToListAsync();
+        Assert.Equal(2, result.Count);
+    }
+
+    protected class Context34760(DbContextOptions options) : DbContext(options)
+    {
+        public DbSet<Book> Books { get; set; }
+
+        protected override void OnModelCreating(ModelBuilder modelBuilder)
+        {
+            modelBuilder.Entity<Book>().Property(e => e.Id).ValueGeneratedNever();
+            modelBuilder.Entity<Book>().Property(e => e.PublishDate).HasConversion(new MyDateTimeValueConverterWithClosure(new MyDatetimeConverter()));
+            modelBuilder.Entity<Book>().Property(e => e.PublishDate).HasConversion(new MyDateTimeValueConverterWithoutClosure());
+        }
+
+        public Task SeedAsync()
+        {
+            AddRange(
+                new Book {
+                    Id = 1,
+                    Name = "The Blade Itself",
+                    PublishDate = new DateTime(2006, 5, 4, 11, 59, 59),
+                    AudiobookDate = new DateTime(2015, 9, 8, 23, 59, 59)
+                },
+                new Book {
+                    Id = 2,
+                    Name = "Red Rising",
+                    PublishDate = new DateTime(2014, 1, 27, 23, 59, 59),
+                    AudiobookDate = new DateTime(2014, 1, 27, 23, 59, 59),
+                });
+
+            return SaveChangesAsync();
+        }
+
+        public class Book
+        {
+            public int Id { get; set; }
+            public string Name { get; set; }
+
+            public virtual DateTime PublishDate { get; set; }
+            public virtual DateTime AudiobookDate { get; set; }
+        }
+
+        public class MyDateTimeValueConverterWithClosure : ValueConverter<DateTime, DateTime>
+        {
+            public MyDateTimeValueConverterWithClosure(MyDatetimeConverter myDatetimeConverter)
+                : base(
+                    x => myDatetimeConverter.Normalize(x),
+                    x => myDatetimeConverter.Normalize(x))
+            {
+            }
+        }
+
+        public class MyDateTimeValueConverterWithoutClosure : ValueConverter<DateTime, DateTime>
+        {
+            public MyDateTimeValueConverterWithoutClosure()
+                : base(
+                    x => new MyDatetimeConverter().Normalize(x),
+                    x => new MyDatetimeConverter().Normalize(x))
+            {
+            }
+        }
+
+        public class MyDatetimeConverter
+        {
+            public virtual DateTime Normalize(DateTime dateTime)
+                => dateTime.Date;
+        }
+    }
+
+    #endregion
 }

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

@@ -305,6 +305,41 @@ ORDER BY [c].[Id]
 ) AS [s]
 LEFT JOIN [As] AS [a] ON [s].[AId] = [a].[Id]
 ORDER BY [s].[Id]
+""");
+    }
+
+    public override async Task Projecting_property_with_converter_with_closure(bool async)
+    {
+        await base.Projecting_property_with_converter_with_closure(async);
+
+        AssertSql(
+"""
+SELECT [b].[PublishDate]
+FROM [Books] AS [b]
+""");
+    }
+
+    public override async Task Projecting_expression_with_converter_with_closure(bool async)
+    {
+        await base.Projecting_expression_with_converter_with_closure(async);
+
+        AssertSql(
+"""
+SELECT MIN([b].[PublishDate]) AS [Day]
+FROM [Books] AS [b]
+GROUP BY [b].[Id]
+""");
+    }
+
+    public override async Task Projecting_property_with_converter_without_closure(bool async)
+    {
+        await base.Projecting_property_with_converter_without_closure(async);
+
+        AssertSql(
+"""
+SELECT MIN([b].[AudiobookDate]) AS [Day]
+FROM [Books] AS [b]
+GROUP BY [b].[Id]
 """);
     }
 }

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

@@ -1864,8 +1864,45 @@ public override async Task Query_syntax_is_not_supported()
         AssertSql();
     }
 
+    public override async Task Projecting_property_requiring_converter_with_closure_is_not_supported()
+    {
+        await base.Projecting_property_requiring_converter_with_closure_is_not_supported();
+
+        AssertSql();
+    }
+
+    public override async Task Projecting_expression_requiring_converter_with_closure_is_not_supported()
+    {
+        await base.Projecting_expression_requiring_converter_with_closure_is_not_supported();
+
+        AssertSql();
+    }
+
     #endregion Negative cases
 
+    public override async Task Projecting_expression_requiring_converter_without_closure_works()
+    {
+        await base.Projecting_expression_requiring_converter_without_closure_works();
+
+        AssertSql(
+            """
+SELECT MIN([t].[ModifyDate]) AS [Day]
+FROM [Tags] AS [t]
+GROUP BY [t].[Id]
+""");
+    }
+
+    public override async Task Projecting_entity_with_property_requiring_converter_with_closure_works()
+    {
+        await base.Projecting_entity_with_property_requiring_converter_with_closure_works();
+
+        AssertSql(
+"""
+SELECT [t].[Id], [t].[CreateDate], [t].[ModifyDate], [t].[Name]
+FROM [Tags] AS [t]
+""");
+    }
+
     public override async Task Select_changes_type()
     {
         await base.Select_changes_type();