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.69k stars 3.17k forks source link

Issue with nested owned types and nullability #29868

Closed joelmdev closed 1 year ago

joelmdev commented 1 year ago

Recently we updated our projects from dotnet5 and efcore5 to dotnet6 and efcore6 and started receiving a new error during migration creation on an unchanged model:

Entity type 'Something' is an optional dependent using table sharing and containing other dependents without any required non shared property to identify whether the entity exists. If all nullable properties contain a null value in database then an object instance won't be created in the query causing nested dependent's values to be lost. Add a required property to create instances with null values for other properties or mark the incoming navigation as required to always create an instance.

This is a pretty dense error message and it's further complicated by the fact that it appears to be a shortcoming if not a bug. Here's the repro that can be dumped into the ApplicationDbContext.cs file of a new VS project template:

using Microsoft.AspNetCore.Identity.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore;

#nullable enable
namespace DeleteMe.Data;

public class ApplicationDbContext : IdentityDbContext
{
    public virtual DbSet<Foo> Foo { get; set; }
    public ApplicationDbContext(DbContextOptions<ApplicationDbContext> options)
        : base(options)
    {
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        base.OnModelCreating(modelBuilder);

        modelBuilder.Entity<Foo>().OwnsOne(f => f.Thing).OwnsOne(s => s.Nuther);
    }
}

public class Foo
{
    public int Id { get; set; }
    public string Name { get; set; }
    public Something? Thing { get; set; }
}

public class Something
{
    public Another Nuther { get; set; }
}

public class Another
{
    public int SomeNumber { get; set; }
}

If I fully understand the error message it is saying that in a scenario where Foo.Thing is instantiated but its properties are null there's no way to infer on rehydration whether or not Thing was ever instantiated so the 2nd level nested values will get nuked. If Another.SomeNumber was nullable that would be true- if Foo.Thing was instantiated but Foo.Thing.Nuther.SomeNumber was null there would be no way to determine whether Foo.Thing had been instantiated or not when the aggregate was rehydrated from the data store. However, the code in its current state does allow us to infer this by checking whether or not Foo.Thing.Nuther.SomeNumber has a value as SomeNumber must be null if Thing was not instantiated and SomeNumber must be not null if Thing has been instantiated. Am I missing anything here?

This affects both efcore 6 and 7. I started to file this as a bug but after reading the error message a few more times it appears that it may be by design, however as mentioned it was not an issue in efcore5. So is this a new safeguard that addresses an pitfall in earlier versions? Would the above logic patch it?

As an aside for anyone else who may run into this, the workaround for us was to add a dummy non-nullable primitive property to the 1st level owned type for the time being, similar to this:

public class Something
{
    public Another Nuther { get; set; }
    protected int Dummy { get; set; }
}

Finally, our real world class actually looks more like the code below:

public class Something
{
    public Another AWholeNuther { get; set; }
    public Another? ANullNuther { get; set; }
}

In the case that we only had Something.ANullNuther we would be back in the same problem with persistence and rehydration terms of ambiguous instantiation. However, as long as we have at least one non-nullable nested owned type with at least one non-nullable property, we should be able to infer the state of instantiation with the data that comes back from the data store.

ajcvickers commented 1 year ago

Would the above logic patch it?

Yes, but my understanding (I'm not an expert in the query pipeline code) is that this would be very complicated to implement.