Breaking behavior of LoadAsync() in EF 8 preview

[bkoelman]

File a bug

After updating to EF Core 8 preview 6, our existing code that uses ReferenceEntry.LoadAsync() no longer works.

The repro code below is an extremely simplified version; our framework JsonApiDotNetCore allows developers to plug in their own DbContext, so in reality, our code is pretty dynamic. We also aim to execute a minimal amount of queries to the database, for performance. For more background on our framework, see the intro at #27436 (comment).

Include your code

The code below succeeds when adding a NuGet reference to Microsoft.EntityFrameworkCore.Sqlite v7.0.9. Replacing the version with 8.0.0-preview.6.23329.4 makes the LoadAsync() method call throw.

using System.Diagnostics;
using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.ChangeTracking;

// Arrange
var order1 = new Order
{
    Shipment = new Shipment()
};

var order2 = new Order();

await using (var appDbContext = new AppDbContext(new DbContextOptions<AppDbContext>()))
{
    await appDbContext.Database.EnsureDeletedAsync();
    await appDbContext.Database.EnsureCreatedAsync();

    appDbContext.Orders.AddRange(order1, order2);
    await appDbContext.SaveChangesAsync();
}

// Act
await using (var appDbContext = new AppDbContext(new DbContextOptions<AppDbContext>()))
{
    // Handle logic for JSON:API request: PATCH /orders/2/relationships/shipment
    // {
    //    type: "shipments"
    //    id: 1
    // }
    // Which means: Assign the Shipment (currently attached to order1) to order2, effectively removing it from order1.
    long orderToUpdateId = order2.Id;
    long shipmentToAssignId = order1.Shipment.Id;

    Order orderToUpdate = await appDbContext.Orders
        .Include(order => order.Shipment)
        .Where(order => order.Id == orderToUpdateId)
        .SingleAsync();

    Shipment trackedShipmentToAssign = (Shipment)MakeTracked(new Shipment() { Id = shipmentToAssignId }, appDbContext);

    // Because we have a 1-to-1 relationship, we must ensure that order1.Shipment gets set to NULL
    // to avoid a FK unique constraint violation.
    ReferenceEntry inverseToLoad = appDbContext.Entry(trackedShipmentToAssign).Reference("Order");
    await inverseToLoad.LoadAsync(); // throws on EF 8
    
    orderToUpdate.Shipment = trackedShipmentToAssign;
    await appDbContext.SaveChangesAsync();
}

// Assert
await using (var appDbContext = new AppDbContext(new DbContextOptions<AppDbContext>()))
{
    var storedOrder1 = await appDbContext.Orders
        .Include(order => order.Shipment)
        .Where(order => order.Id == order1.Id)
        .SingleAsync();

    var storedOrder2 = await appDbContext.Orders
        .Include(order => order.Shipment)
        .Where(order => order.Id == order2.Id)
        .SingleAsync();

    Debug.Assert(storedOrder1.Shipment == null);
    Debug.Assert(storedOrder2.Shipment.Id == order1.Shipment.Id);
}

static object MakeTracked(object entity, DbContext dbContext)
{
    dbContext.Entry(entity).State = EntityState.Unchanged;
    return entity;
}

public sealed class Shipment
{
    public long Id { get; set; }
    
    public DateTimeOffset ShippedAt { get; set; }

    public Order Order { get; set; } = null!;
}

public sealed class Order
{
    public long Id { get; set; }

    public decimal Amount { get; set; }

    public Shipment Shipment { get; set; } = null!;
}

public sealed class AppDbContext : DbContext
{
    public DbSet<Order> Orders => Set<Order>();
    public DbSet<Shipment> Shipments => Set<Shipment>();

    public AppDbContext(DbContextOptions<AppDbContext> options)
        : base(options)
    {
    }

    protected override void OnConfiguring(DbContextOptionsBuilder builder)
    {
        builder.UseSqlite("Data Source=SampleDb.db;Pooling=False");
    }

    protected override void OnModelCreating(ModelBuilder builder)
    {
        // By default, Entity Framework Core generates an identifying foreign key for a required 1-to-1 relationship.
        // This means no foreign key column is generated, instead the primary keys point to each other directly.
        // That mechanism does not make sense for JSON:API, because patching a relationship would result in also
        // changing the identity of a resource. Naming the foreign key explicitly fixes the problem by forcing to
        // create a foreign key column.
        builder.Entity<Order>()
            .HasOne(order => order.Shipment)
            .WithOne(shipment => shipment.Order)
            .HasForeignKey<Shipment>("OrderId");
    }
}

Include stack traces

System.InvalidOperationException
  HResult=0x80131509
  Message=The navigation 'Shipment.Order' cannot be loaded because one or more of the key or foreign key properties are shadow properties and the entity is not being tracked. Relationships using shadow values can only be loaded for tracked entities.
  Source=Microsoft.EntityFrameworkCore
  StackTrace:
   at Microsoft.EntityFrameworkCore.Internal.EntityFinder`1.GetLoadValues(INavigation navigation, InternalEntityEntry entry)
   at Microsoft.EntityFrameworkCore.Internal.EntityFinder`1.<LoadAsync>d__27.MoveNext()
   at Program.<<Main>$>d__0.MoveNext() in D:\Bart\Source\Projects\LoadAsyncBugRepro\LoadAsyncBugRepro\Program.cs:line 44
   at Program.<<Main>$>d__0.MoveNext() in D:\Bart\Source\Projects\LoadAsyncBugRepro\LoadAsyncBugRepro\Program.cs:line 48

Include provider and version information

EF Core version: 8.0.0-preview.6.23329.4
Database provider: Sqlite (originally PostgreSQL, but there's no compatible preview-6 release yet)
Target framework: .NET 8 preview6
Operating system: Windows 11
IDE: Visual Studio 2022 Community v17.7.0 Preview 3

[bkoelman]

@ajcvickers @roji I've created this issue in response to #30306 (comment).

[ajcvickers]

@bkoelman What's the intention of calling LoadAsync here? The FK value has not been set, and so in EF7 an attempt would be made to load the principal with ID zero, but there should never be a principal with ID zero otherwise EF value generation won't work correctly.

[kevinchalet]

@bkoelman this breaking change was reverted in preview7: #31104.

[bkoelman]

@ajcvickers You're absolutely right. In EF7, LoadAsync() executes:

Executed DbCommand (1ms) [Parameters=[@__p_0='0'], CommandType='Text', CommandTimeout='30']
SELECT "o"."Id", "o"."Amount"
FROM "Orders" AS "o"
WHERE "o"."Id" = @__p_0

And I agree that doesn't make any sense. It is a pointless extra query that our code should not cause. We didn't realize this until now, because earlier EF Core versions didn't produce an error.

It turns out that I've mixed up two distinct scenarios in the repro code. The alternative scenario is where the FK is defined on the other side of the relationship. Only in that case, LoadAsync() is needed.

I've updated the repro code (new version at the bottom) to cover both scenarios; just comment out the FK_IN_SHIPMENT conditional to get the second scenario. I've added logging and made both navigations optional. Most important is that the LoadAsync() call is now conditional, based on where the FK is defined:

if (RequiresInverseLoad(appDbContext, typeof(Order), "Shipment"))
{
    // ...LoadAsync() as before
}

static bool RequiresInverseLoad(DbContext dbContext, Type leftType, string propertyName)
{
    IEntityType? leftEntityType = dbContext.Model.FindEntityType(leftType);
    INavigation? navigation = leftEntityType?.FindNavigation(propertyName);

    return navigation != null && navigation.ForeignKey.DeclaringEntityType.ClrType == leftType;
}

This works in both EF 7 and 8. I'm going to update our project code accordingly. Thanks for pointing out the flaw on our side.

Updated full repro including the fix

#define FK_IN_SHIPMENT

using System.Diagnostics;
using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.ChangeTracking;
using Microsoft.EntityFrameworkCore.Metadata;
using Microsoft.Extensions.Logging;

// Arrange
var order1 = new Order
{
    Shipment = new Shipment()
};

var order2 = new Order();

await using (var appDbContext = new AppDbContext(new DbContextOptions<AppDbContext>()))
{
    await appDbContext.Database.EnsureDeletedAsync();
    await appDbContext.Database.EnsureCreatedAsync();

    appDbContext.Orders.AddRange(order1, order2);
    await appDbContext.SaveChangesAsync();
}

// Act
await using (var appDbContext = new AppDbContext(new DbContextOptions<AppDbContext>()))
{
    // Handle logic for JSON:API request: PATCH /orders/2/relationships/shipment
    // {
    //    type: "shipments"
    //    id: 1
    // }
    // Which means: Assign the Shipment (currently attached to order1) to order2, effectively removing it from order1.
    long orderToUpdateId = order2.Id;
    long shipmentToAssignId = order1.Shipment.Id;

    Order orderToUpdate = await appDbContext.Orders
        .Include(order => order.Shipment)
        .Where(order => order.Id == orderToUpdateId)
        .SingleAsync();

    Shipment trackedShipmentToAssign = (Shipment)MakeTracked(new Shipment() { Id = shipmentToAssignId }, appDbContext);

    if (RequiresInverseLoad(appDbContext, typeof(Order), "Shipment"))
    {
        // Because we have a 1-to-1 relationship, we must ensure that order1.Shipment gets set to NULL
        // to avoid a FK unique constraint violation.
        ReferenceEntry inverseToLoad = appDbContext.Entry(trackedShipmentToAssign).Reference("Order");
        await inverseToLoad.LoadAsync();
    }
    
    orderToUpdate.Shipment = trackedShipmentToAssign;
    await appDbContext.SaveChangesAsync();
}

// Assert
await using (var appDbContext = new AppDbContext(new DbContextOptions<AppDbContext>()))
{
    var storedOrder1 = await appDbContext.Orders
        .Include(order => order.Shipment)
        .Where(order => order.Id == order1.Id)
        .SingleAsync();

    var storedOrder2 = await appDbContext.Orders
        .Include(order => order.Shipment)
        .Where(order => order.Id == order2.Id)
        .SingleAsync();

    Debug.Assert(storedOrder1.Shipment == null);
    Debug.Assert(storedOrder2.Shipment!.Id == order1.Shipment.Id);
}

static object MakeTracked(object entity, DbContext dbContext)
{
    dbContext.Entry(entity).State = EntityState.Unchanged;
    return entity;
}

static bool RequiresInverseLoad(DbContext dbContext, Type leftType, string propertyName)
{
    IEntityType? leftEntityType = dbContext.Model.FindEntityType(leftType);
    INavigation? navigation = leftEntityType?.FindNavigation(propertyName);

    return navigation != null && navigation.ForeignKey.DeclaringEntityType.ClrType == leftType;
}

public sealed class Shipment
{
    public long Id { get; set; }
    
    public DateTimeOffset ShippedAt { get; set; }

    public Order? Order { get; set; }
}

public sealed class Order
{
    public long Id { get; set; }

    public decimal Amount { get; set; }

    public Shipment? Shipment { get; set; }
}

public sealed class AppDbContext : DbContext
{
    public DbSet<Order> Orders => Set<Order>();
    public DbSet<Shipment> Shipments => Set<Shipment>();

    public AppDbContext(DbContextOptions<AppDbContext> options)
        : base(options)
    {
    }

    protected override void OnConfiguring(DbContextOptionsBuilder builder)
    {
        builder.UseSqlite("Data Source=SampleDb.db;Pooling=False");
        builder.EnableSensitiveDataLogging();
        builder.LogTo(text => Debug.WriteLine(text), LogLevel.Information);
    }

    protected override void OnModelCreating(ModelBuilder builder)
    {
        // By default, Entity Framework Core generates an identifying foreign key for a required 1-to-1 relationship.
        // This means no foreign key column is generated, instead the primary keys point to each other directly.
        // That mechanism does not make sense for JSON:API, because patching a relationship would result in also
        // changing the identity of a resource. Naming the foreign key explicitly fixes the problem by forcing to
        // create a foreign key column.
        builder.Entity<Order>()
            .HasOne(order => order.Shipment)
            .WithOne(shipment => shipment.Order)
#if FK_IN_SHIPMENT
            .HasForeignKey<Shipment>("OrderId");
#else
            .HasForeignKey<Order>("ShipmentId");
#endif
    }
}

[bkoelman]

@kevinchalet I'm aware of that. However, this issue does not concern the binary breaking change, but breaking behavior.