dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.77k stars 3.19k forks source link

False positive CoreEventId.NavigationLazyLoading exception thrown when FK is null #34604

Closed 2xRon closed 1 month ago

2xRon commented 2 months ago

File a bug

Setup:

Include your code

Reproduction

        static void Main(string[] args)
        {
            var options = new DbContextOptionsBuilder<TestContext>();
            options.UseSqlite("Data Source=LocalDatabase.db");
            options.UseLazyLoadingProxies();
            options.ConfigureWarnings(b => b.Throw(CoreEventId.NavigationLazyLoading));
            Setup(options.Options);
            Operate(options.Options);
        }

        private static void Setup(DbContextOptions<TestContext> options)
        {
            var context = new TestContext(options);
            context.Database.Migrate();
            var cet = new CommandExtensionType()
            {
                Description = "My value"
            };
            context.CommandExtensions.Add(cet);
            context.SaveChanges();

            //var c1 = new Command();
            //c1.CommandExtensionType = cet;
            //context.Commands.Add(c1);

            var c2 = new Command();
            context.Commands.Add(c2);

            context.SaveChanges();
        }

        private static void Operate(DbContextOptions<TestContext> options)
        {
            var context = new TestContext(options);
            context.CommandExtensions.Load();
            var command = context.Commands.First();
            // Throws NavigationLazyLoading exception even though the the navigation should be known to be null
            var ext = command.CommandExtensionType;
        }

Context

public class TestContext : DbContext
{
    public TestContext(DbContextOptions<TestContext> options) : base(options)
    {

    }

    public DbSet<Command> Commands { get; set; } = null!;
    public DbSet<CommandExtensionType> CommandExtensions { get; set; } = null!;

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        Command.Configure(modelBuilder.Entity<Command>());
        CommandExtensionType.Configure(modelBuilder.Entity<CommandExtensionType>());
    }
}

public class CommandExtensionType
{
    [Key]
    public int CommandExtensionTypeId { get; set; }

    public string Description { get; set; }

    [InverseProperty(nameof(Command.CommandExtensionType))]
    public virtual List<Command> Commands { get; set; }

    internal static void Configure(EntityTypeBuilder<CommandExtensionType> builder)
    {
        builder.HasKey(x => x.CommandExtensionTypeId);
        builder.Property(x => x.CommandExtensionTypeId).ValueGeneratedOnAdd();
    }
}

public class Command
{
    [Key]
    public int CommandId { get; set; }

    public int? CommandExtensionTypeId { get; set; }

    [ForeignKey(nameof(CommandExtensionTypeId))]
    public virtual CommandExtensionType CommandExtensionType { get; set; }

    internal static void Configure(EntityTypeBuilder<Command> builder)
    {
        builder.HasKey(x => x.CommandId);
        builder.HasOne(x => x.CommandExtensionType).WithMany(x => x.Commands);
        builder.Property(x => x.CommandId).ValueGeneratedOnAdd();
    }
}

Include provider and version information

EF Core version: Database provider: can replicate in Microsoft.EntityFrameworkCore.SqlServer and Sqlite Target framework: can replicate in .NET 8.0 and .NET 7.0 Operating system: Windows 11 IDE: Visual Studio 2022 17.4

NullLazyLoadTest.zip

Possible Resolution

For now we're thinking of replacing the LazyLoader with an identical implementation except for replacing ShouldLoad with

    private bool ShouldLoad(object entity, string navigationName, [NotNullWhen(true)] out NavigationEntry? navigationEntry)
    {
        if (!_detached && !IsLoaded(entity, navigationName))
        {
            if (_nonLazyNavigations == null
                || !_nonLazyNavigations.Contains(navigationName))
            {
                if (_disposed)
                {
                    Logger.LazyLoadOnDisposedContextWarning(Context, entity, navigationName);
                }
                else if (Context!.ChangeTracker.LazyLoadingEnabled)
                {
                    var entry = Context.Entry(entity);
                    navigationEntry = entry.Navigation(navigationName); // Will use local-DetectChanges, if enabled.
                    if (!navigationEntry.Metadata.IsCollection
                        &&((INavigation)navigationEntry.Metadata).IsOnDependent
                        && ((INavigation)navigationEntry.Metadata).ForeignKey.Properties.All(p => p.IsNullable && entry.Property(p).CurrentValue is null))
                    {
                        return false;
                    }
                    if (!navigationEntry.IsLoaded) // Check again because the nav may be loaded without the loader knowing
                    {
                        Logger.NavigationLazyLoading(Context, entity, navigationName);

                        return true;
                    }
                }
            }
        }

        navigationEntry = null;
        return false;
    }

Though I'm sure there is a better way to accomplish this.

AndriySvyryd commented 1 month ago

EF can't know beforehand that a given navigation will be null when loaded. In a situation like this you can supply this information manually:

if (command.CommandExtensionTypeId == null)
{
    context.Entry(command).Reference(c => c.CommandExtensionType).IsLoaded = true;
}