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.62k stars 3.15k forks source link

Updating JSON column when query tracking is disabled throws #33862

Open vladislav-karamfilov opened 3 months ago

vladislav-karamfilov commented 3 months ago

File a bug

Setting a new value to a JSON-mapped column property (a non-collection or a collection one) when query tracking is disabled fails with InvalidOperationException. Enabling query tracking with optionsBuilder.UseQueryTrackingBehavior(QueryTrackingBehavior.TrackAll) fixes the issue but it is not an option for the project I'm working on.

PS All is working well when we have primitive collections like List<string> in the entity instead of JSON object(s).

Include your code

using Microsoft.EntityFrameworkCore;

await EnsureThingsInDbAsync();

// These method calls throw!!!
await UpdateThingAsync(isManualUpdate: false, updateSinglePart: false);
await UpdateThingAsync(isManualUpdate: false, updateSinglePart: true);

// These method calls doesn't throw but the update is not performed (the value is not updated in DB)!!!
await UpdateThingAsync(isManualUpdate: true, updateSinglePart: false);
await UpdateThingAsync(isManualUpdate: true, updateSinglePart: true);

static async Task UpdateThingAsync(bool isManualUpdate, bool updateSinglePart)
{
    using var db = new AppDbContext();
    var thing = await db.Things.FirstOrDefaultAsync();

    if (updateSinglePart)
    {
        thing!.SinglePart = new Part { Name = "updated single part " + Random.Shared.Next() };
    }
    else
    {
        thing!.Parts = [new Part { Name = "updated part " + Random.Shared.Next() }];
    }

    if (isManualUpdate)
    {
        var entry = db.Entry(thing);
        if (entry.State == EntityState.Detached)
        {
            db.Things.Attach(thing);
        }

        entry.State = EntityState.Modified;
    }
    else
    {
        db.Things.Update(thing);
    }

    await db.SaveChangesAsync();
}

static async Task EnsureThingsInDbAsync()
{
    using var db = new AppDbContext();
    await db.Database.EnsureCreatedAsync();

    if (await db.Things.AnyAsync())
    {
        return;
    }

    db.Things.Add(new Thing
    {
        Parts = [new Part { Name = "part " + Random.Shared.Next() }],
        SinglePart = new Part { Name = "single part " + Random.Shared.Next() },
    });

    await db.SaveChangesAsync();
}

public class AppDbContext : DbContext
{
    public DbSet<Thing> Things { get; set; }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
        => modelBuilder.Entity<Thing>()
            .OwnsOne(x => x.SinglePart, b => b.ToJson())
            .OwnsMany(x => x.Parts, b => b.ToJson());

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        base.OnConfiguring(optionsBuilder);

        optionsBuilder.UseSqlServer("Server=(localdb)\\mssqllocaldb;Database=ef-json-cols");

        optionsBuilder.UseQueryTrackingBehavior(QueryTrackingBehavior.NoTrackingWithIdentityResolution);
        // optionsBuilder.UseQueryTrackingBehavior(QueryTrackingBehavior.NoTracking); // Doesn't work too!!!
    }
}

public class Thing
{
    public int Id { get; set; }

    public List<Part> Parts { get; set; } = [];

    public Part? SinglePart { get; set; }
}

public class Part
{
    public required string Name { get; set; }
}

Include stack traces

Unhandled exception. System.InvalidOperationException: The value of shadow key property 'Thing.Parts#Part (Part).Id' is unknown when attempting to save changes. This is because shadow property values cannot be preserved when the entity is not being tracked. Consider adding the property to the entity's .NET type. See https://aka.ms/efcore-docs-owned-collections for more information.
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.<PrepareToSave>g__CheckForUnknownKey|111_0(IProperty property, <>c__DisplayClass111_0&)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.PrepareToSave()
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.GetEntriesToSave(Boolean cascadeChanges)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChangesAsync(StateManager stateManager, Boolean acceptAllChangesOnSuccess, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal.SqlServerExecutionStrategy.ExecuteAsync[TState,TResult](TState state, Func`4 operation, Func`4 verifySucceeded, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.DbContext.SaveChangesAsync(Boolean acceptAllChangesOnSuccess, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.DbContext.SaveChangesAsync(Boolean acceptAllChangesOnSuccess, CancellationToken cancellationToken)
   at Program.<<Main>$>g__UpdateThingAsync|0_0(Boolean isManualUpdate, Boolean updateSinglePart) in C:\Users\Vladislav\source\repos\ConsoleApp9\Program.cs:line 42
   at Program.<Main>$(String[] args) in C:\Users\Vladislav\source\repos\ConsoleApp9\Program.cs:line 8
   at Program.<Main>(String[] args)

Include provider and version information

EF Core version: 8.0.6 Database provider: Microsoft.EntityFrameworkCore.SqlServer Target framework: .NET 8.0 Operating system: Windows 11 IDE: Visual Studio 2022 17.10.1

vladislav-karamfilov commented 2 months ago

@maumar , I see that you have self-assigned yourself a couple of days ago. Have you had the chance to investigate this bug in details?

I have just tested on .NET 7 and it still fails. I hope that you'll fix it for some EF Core 8 patch version or at worst for the .NET 9 release.

maumar commented 2 months ago

@vladislav-karamfilov no, I didn't investigate this issue yet. JSON is generally my area to look after, so I just assigned myself for now, so that other team members don't need to read through the issue. I will post my findings here when I get to it, but it may take some time - we have a big backlog of uninvestigated issues at the moment.

maumar commented 1 month ago

workaround is to do manual update by adding the entry and setting the state to unchanged. This forces generation of all the shadow values.

            var entry = db.Entry(thing);
            if (entry.State == EntityState.Detached)
            {
                db.Things.Add(thing);
            }

            entry.State = EntityState.Unchanged;
vladislav-karamfilov commented 1 month ago

@maumar , the project I'm working on (where I found this issue) is massive and we use the approach with manual update from the issue description (because of various reasons not related to this issue):

var entry = db.Entry(thing);
if (entry.State == EntityState.Detached)
{
    db.Things.Attach(thing);
}

entry.State = EntityState.Modified;

What are the consequences of using the new suggested manual update approach? What behavioral changes will we observe? Is there a way to narrow down the entry objects which have JSON columns (skipping the ones with primitive collections) so we can apply the new update only on a subset of all updates that we do in the app?

PS I see that you have put this in Backlog milestone. Does that mean that you don't plan to fix this for EF Core 9? And if the answer is "Yes", is there any specific reason (it seems like 2 major features of EF Core cannot cooperate because of a bug and thus I would consider this as important bug to fix)?

maumar commented 1 month ago

looping in @ajcvickers and @AndriySvyryd, who are the domain experts, wrt consequences and subtleties of this approach vs the original one.

As to your other question, Backlog milestone unfortunately means we are not planning to fix this bug in 9. Reason being, there is not much development time left before we close the release, the fix is likely non-trivial/risky, and there is a reasonable workaround. All those factors push this issue down in the priority for us, compared to other items the team is working on.

ajcvickers commented 3 weeks ago

This seems by-design to me. Marking for discussion by team.

vladislav-karamfilov commented 3 weeks ago

@ajcvickers , I'm not sure I understand. Aren't these major EF features (no query tracking + JSON columns) supposed to work together? Our team's expectation is to have this scenario working out of the box just like the query tracking ON + JSON columns are working nicely together.

ajcvickers commented 3 weeks ago

@vladislav-karamfilov If you're explicitly tracking entities and using shadow key properties, then you need to make sure that these shadow properties have been set appropriately. Alternatively, don't use shadow keys, which means that all the values will travel with the detached entity.

vladislav-karamfilov commented 3 weeks ago

@ajcvickers , we are not explicitly tracking entities and we are not doing anything specific with shadow properties and everything is working well for us. But recently we saw opportunity to use JSON columns for some of our entity props and we hit this issue. We don't know how to overcome it and IMHO it seems like something that should work perfectly out of the box. I hope you are going to fix it as soon as possible so we can leverage this EF feature soon.

PS If you have any suggestions about a workaround, please share it with us. How will the one suggested @maumar affect our EF usage having in mind how we are using it?

ajcvickers commented 3 weeks ago

@vladislav-karamfilov Unfortunately, using owned entities results in this kind of issue. They are not a great match for JSON, since they are still entity types, just with hidden identity. Please vote for Add relational JSON mapping support for complex types, which should make this much better.

we are not explicitly tracking entities

I mean calling Attach or Update on entities queried by a different context instance. See Explicitly Tracking Entities.

vladislav-karamfilov commented 2 weeks ago

@vladislav-karamfilov Unfortunately, using owned entities results in this kind of issue. They are not a great match for JSON, since they are still entity types, just with hidden identity. Please vote for Add relational JSON mapping support for complex types, which should make this much better.

I have already upvoted this issue and my colleagues will upvote too. I see that this is the most requested feature in the open issues list. Is there any chance for this issue to make it for the .NET 9 release?

we are not explicitly tracking entities

I mean calling Attach or Update on entities queried by a different context instance. See Explicitly Tracking Entities.

I have read this article in the past but I will read it again to search for more info about this workaround. Thanks, @ajcvickers!