NullReferenceException for a custom ValueConverter in EF Core 9 RC 1.

[maliming]

The csproj and code to reproduce:

<Project Sdk="Microsoft.NET.Sdk">

    <PropertyGroup>
        <OutputType>Exe</OutputType>
        <TargetFramework>net9.0</TargetFramework>
        <ImplicitUsings>enable</ImplicitUsings>
        <Nullable>enable</Nullable>
    </PropertyGroup>

    <ItemGroup>
      <PackageReference Include="Microsoft.EntityFrameworkCore.Design" Version="9.0.0-rc.1.24451.1">
        <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
        <PrivateAssets>all</PrivateAssets>
      </PackageReference>
      <PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite" Version="9.0.0-rc.1.24451.1" />
    </ItemGroup>

</Project>

using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.Storage.ValueConversion;

var db = new BookContext();

await db.Books.AddAsync( new Book()
{
    CreationDate = DateTime.Now
});

await db.SaveChangesAsync();

var result =  await db.Books
    .GroupBy(t => t.Id)
    .Select(g => new
    {
        Day = g.Min(t => t.CreationDate)
    })
    .ToListAsync();

public class BookContext : DbContext
{
    public DbSet<Book> Books { get; set; }

    public string DbPath { get; }

    public BookContext()
    {
        var folder = Environment.SpecialFolder.LocalApplicationData;
        var path = Environment.GetFolderPath(folder);
        DbPath = Path.Join(path, "book.db");
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Book>().Property(e => e.CreationDate).HasConversion(new MyDateTimeValueConverter(new MyDatetimeConverter()));
    }

    protected override void OnConfiguring(DbContextOptionsBuilder options)
        => options.UseSqlite($"Data Source={DbPath}", builder => builder.UseQuerySplittingBehavior(QuerySplittingBehavior.SplitQuery)).EnableDetailedErrors();
}

public class MyDateTimeValueConverter : ValueConverter<DateTime, DateTime>
{
    public MyDateTimeValueConverter(MyDatetimeConverter myDatetimeConverter, ConverterMappingHints? mappingHints = null)
        : base(
            x => myDatetimeConverter.Normalize(x),
            x => myDatetimeConverter.Normalize(x),
            mappingHints)
    {
    }
}

public class Book
{
    public Guid Id { get; set; }

    public virtual DateTime CreationDate { get;  set; }
}


public class MyDatetimeConverter
{
    public virtual DateTime Normalize(DateTime dateTime)
    {
        return dateTime.Date;
    }
}

Unhandled exception. System.InvalidOperationException: An error occurred while reading a database value. The expected type was 'System.Nullable`1[System.DateTime]' but the actual value was null.
 ---> System.NullReferenceException: Object reference not set to an instance of an object.
   at lambda_method34(Closure, QueryContext, DbDataReader, ResultContext, SplitQueryResultCoordinator)
   --- End of inner exception stack trace ---
   at lambda_method34(Closure, QueryContext, DbDataReader, ResultContext, SplitQueryResultCoordinator)
   at Microsoft.EntityFrameworkCore.Query.Internal.SplitQueryingEnumerable`1.AsyncEnumerator.MoveNextAsync()
   at Microsoft.EntityFrameworkCore.EntityFrameworkQueryableExtensions.ToListAsync[TSource](IQueryable`1 source, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.EntityFrameworkQueryableExtensions.ToListAsync[TSource](IQueryable`1 source, CancellationToken cancellationToken)
   at Program.<Main>$(String[] args) in ConsoleApp\ConsoleApp\Program.cs:line 13
   at Program.<Main>(String[] args)

---

It works if I create the MyDatetimeConverter() object in the expression parameter.
And the all code works in EF Core 8.0.

public class MyDateTimeValueConverter : ValueConverter<DateTime, DateTime>
{
    public MyDateTimeValueConverter(MyDatetimeConverter myDatetimeConverter, ConverterMappingHints? mappingHints = null)
        : base(
            x => new MyDatetimeConverter().Normalize(x),
            x => new MyDatetimeConverter().Normalize(x),
            mappingHints)
    {
    }
}

[maumar]

@maliming you can workaround the problem by slightly modifying the converter:

    public class MyDateTimeValueConverter : ValueConverter<DateTime, DateTime>
    {
        public MyDateTimeValueConverter(ConverterMappingHints? mappingHints = null)
            : base(
                x => new MyDatetimeConverter().Normalize(x),
                x => new MyDatetimeConverter().Normalize(x),
                mappingHints)
        {
        }
    }

and the model configuration part:

            modelBuilder.Entity<Book>().Property(e => e.CreationDate).HasConversion(new MyDateTimeValueConverter());

Basically, rather than passing MyDatetimeConverter as closure variable in the MyDateTimeValueConverter ctor expressions, create them within the expression itself.

[maumar]

shaper we generate:

(queryContext, dataReader, resultContext, resultCoordinator) => 
{
    DateTime? value1;
    value1 = dataReader.IsDBNull(0) ? default(DateTime?) : (DateTime?)Invoke(((MyDateTimeValueConverter)[LIFTABLE Constant: SqlServerDateTimeTypeMapping | Resolver: c => (RelationalTypeMapping)c.Dependencies.TypeMappingSource.FindMapping(
        type: System.Nullable`1[System.DateTime], 
        model: c.Dependencies.Model, 
        elementMapping: null)].Converter).ConvertFromProviderTyped, dataReader.GetDateTime(0));
    return new { Day = (DateTime)value1 };
}

We use generic CLR DateTime to find mapping for the value, and that obviously doesn't have the converter. We can't use converter ctor expression because it contains an unliftable constant, so we "fall back" to the dummy way of doing things.
Here is a relevant piece of code documentation from CreateGetValueExpression:

// if IProperty is available, we can reliably get the converter from the model and then incorporate FromProvider(Typed) delegate
// into the expression. This way we have consistent behavior between precompiled and normal queries (same code path)
// however, if IProperty is not available, we could try to get TypeMapping from TypeMappingSource based on ClrType, but that may
// 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

This essentially is dupe of #33517

this is a breaking change, so we should document this, as the workaround is not intuitive at all @roji

[roji]

this is a breaking change

@maumar are you suggest we document this as a by-design regression? Isn't it a bug that we need to fix (and probably service)?

[maumar]

it's a bug/limitation of the current liftable constant approach. I'd service this but i'm worried how risky the fix is going to be.

[roji]

OK. It in any case seems like a bug (and a regression at that) rather than an intended change... Ideally we'd make sure this scenario works in regular mode even if it doesn't in precompiled/AOT mode (which is experimental anyway, etc.); I think it's OK for now if we'd throw an exception eagerly if a value converter with a captured variable is used with precompilation, as long as things work in normal mode... Let me know what you think.

BTW note that the workaround instantiates a converter instance each and every time a value is converted, which is a bit problematic perf-wise.

[maumar]

yeah, I thought about it over night and I do agree. I don't think we have a way to fix it right now in a way that would work for AOT (need #33517 for that), but we could revert to pre-AOT pattern in these specific scenarios.

[roji]

Yeah, that sounds right.

[maumar]

fixed in 8561f4e

[roji]

Also reported via #34934