SaveChanges circular dependency in unique unfiltered index

[roji]

#28065 raised a 7.0 SaveChanges circular dependency issue with unique indexes; two repros were posted there - one with a filtered index, one without. The fix for that issue (#28923) seems to have fixed only the filtered index case, but the unfiltered one still fails on 7.0.

Repro

using System;
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.Logging;

var db = new TestDbContext();
await db.Database.EnsureDeletedAsync();
await db.Database.EnsureCreatedAsync();

var t1 = new TestEntity();

db.Tests.Add(t1);

await db.SaveChangesAsync();

var dependent = new TestDependentEntity()
{
    TestEntityId = t1.Id,
    UniqueOn = null,
    ToChange = 1,
};

var dependent2 = new TestDependentEntity()
{
    TestEntityId = t1.Id,
    UniqueOn = null,
    ToChange = 2,
};

db.TestDependents.Add(dependent);
db.TestDependents.Add(dependent2);

await db.SaveChangesAsync();

dependent.ToChange = null;
dependent2.ToChange = null;

await db.SaveChangesAsync();

public class TestDbContext : DbContext
{
    public DbSet<TestEntity> Tests { get; set; }
    public DbSet<TestDependentEntity> TestDependents { get; set; }
    
    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            .UseSqlite(@"Data Source=foo.sqlite")
            .LogTo(Console.WriteLine, LogLevel.Information)
            .EnableSensitiveDataLogging();

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<TestDependentEntity>().HasIndex(e => new { e.TestEntityId, e.UniqueOn })
            .IsUnique();
    }
}

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

public class TestDependentEntity
{
    public int Id { get; set; }
    public int TestEntityId { get; set; }

    public bool? UniqueOn { get; set; }
    public int? ToChange { get; set; }
}

/cc @AndriySvyryd

[AndriySvyryd]

A workaround for this is to define a filter:

modelBuilder.Entity<TestDependentEntity>().HasIndex(e => new { e.TestEntityId, e.UniqueOn })
            .IsUnique().HasFilter("true");

This doesn't repro on SQL Server because the provider defines a filter whenever the index contains a nullable column.

[grietine]

Will this change be released with 7.0.x patch? If yes, is there a planned date / version?

[AndriySvyryd]

Will this change will be released with 7.0.x patch? If yes, is there a planned date / version?

No, the fix is too risky to be shipped in a patch. Are you not able to use the workaround mentioned above?

[grietine]

Will this change will be released with 7.0.x patch? If yes, is there a planned date / version?

No, the fix is too risky to be shipped in a patch. Are you not able to use the workaround mentioned above?

Maybe then this fix will reach any of 7.x version? Or is it planned only for 8.0.0 version? 😄 We can use the workaround, but as an outcome it requires us to re-create the indexes on a database which we want to avoid.

[Devel484]

Will this change will be released with 7.0.x patch? If yes, is there a planned date / version?

No, the fix is too risky to be shipped in a patch. Are you not able to use the workaround mentioned above?

I need the fix too and can not use the workaround due to the fact that it would require too much downtime and is too risky.

We (me and my company) would skip 7 entirely if its not fixed.

[alexmpa]

It's a serious bug and without this fix EF7 is unusable imo. Very sad to see how this develops.

[AndriySvyryd]

We can use the workaround, but as an outcome it requires us to re-create the indexes on a database which we want to avoid.

You can edit the generated migration and remove the commands that recreate the indexes. The filters don't need to be in the database, EF just needs to assume that the index is filtered.

We (me and my company) would skip 7 entirely if its not fixed.

Another workaround is to use the daily builds. The 8.0 branch currently has no new features and has more bug fixes. This should be mostly the case until preview 1.

[stevendarby]

Another workaround is to use the daily builds. The 8.0 branch currently has no new features and has more bug fixes. This should be mostly the case until preview 1.

That's not true, is it? https://github.com/dotnet/efcore/issues?q=is%3Aissue+milestone%3A8.0.0+label%3Atype-enhancement+is%3Aclosed

I find the support strategy quite bizarre. EF 7 is only 1 month into its 18 months of support but there are already bugs that are too risky to patch? Isn't it better to fix known bugs which are already there rather than be very conservative against unknown bugs?* If you accidentally introduce a bug, then there is good news - there are still 17 months of support in which you can fix the new bug too :)

*probably a bad example because 5.0.1 introduced a bad bug :D

[AndriySvyryd]

That's not true, is it?

I meant no new big features that could still be buggy.

If you accidentally introduce a bug, then there is good news - there are still 17 months of support in which you can fix the new bug too :)

We'll discuss this again. The issue is that it might take up to two months since the bug is reported for the fix to be shipped in a patch, depending on factors we don't control.

[ajcvickers]

Note from triage: we will take a patch to Tactics with the understanding that it is not low risk and so may be rejected.

[jmalczak]

Hi guys. I have tried 7.03 and simple test like that fails for me with the same exception:

  // https://github.com/dotnet/efcore/issues/29647
    [Fact]
    public void SaveChangesShouldNotThrow()
    {
        var context = new TestDbContext();

        var e = new TestEntity { Id = Guid.NewGuid(), Name = "n1" };
        var e2 = new TestEntity { Id = Guid.NewGuid(), Name = "n2" };

        context.TestEntities.Attach(e);
        context.TestEntities.Attach(e2);

        context.Entry(e).Property(p => p.Name).IsModified = true;
        context.Entry(e2).Property(p => p.Name).IsModified = true;

        context.SaveChanges();
    }

    public class TestDbContext : DbContext
    {
        public DbSet<TestEntity> TestEntities { get; set; }

        protected override void OnConfiguring(
            DbContextOptionsBuilder optionsBuilder)
        {
            if (!optionsBuilder.IsConfigured || optionsBuilder.Options.Extensions.Count() <= 1)
            {
                optionsBuilder.UseSqlServer("Server=.;Initial Catalog=test;User Id=sa;Password=test;TrustServerCertificate=true");

                base.OnConfiguring(optionsBuilder);
            }
        }

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

            modelBuilder.Entity<TestEntity>()
                .HasKey(t => t.Id);

            modelBuilder.Entity<TestEntity>()
                .HasIndex(t => new { t.Id1, t.Id2 })
                .IsUnique();
        }
    }

    public class TestEntity
    {
        public Guid Id { get; set; }
        public string Name { get; set; }
        public Guid Id1 { get; set; }
        public Guid Id2 { get; set; }
    }

Exception:

ystem.InvalidOperationException
Unable to save changes because a circular dependency was detected in the data to be saved: 'TestEntity [Modified] <-
Index { 'Id1', 'Id2' } TestEntity [Modified] <-
Index { 'Id1', 'Id2' } TestEntity [Modified]To show additional information call 'DbContextOptionsBuilder.EnableSensitiveDataLogging'.'.
   at Microsoft.EntityFrameworkCore.Utilities.Multigraph`2.ThrowCycle(List`1 cycle, Func`2 formatCycle, Func`2 formatException)
   at Microsoft.EntityFrameworkCore.Utilities.Multigraph`2.TopologicalSortCore(Boolean withBatching, Func`4 tryBreakEdge, Func`2 formatCycle, Func`2 formatException)
   at Microsoft.EntityFrameworkCore.Utilities.Multigraph`2.BatchingTopologicalSort(Func`4 tryBreakEdge, Func`2 formatCycle, Func`2 formatException)
   at Microsoft.EntityFrameworkCore.Update.Internal.CommandBatchPreparer.TopologicalSort(IEnumerable`1 commands)
   at Microsoft.EntityFrameworkCore.Update.Internal.CommandBatchPreparer.BatchCommands(IList`1 entries, IUpdateAdapter updateAdapter)+MoveNext()
   at Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.Execute(IEnumerable`1 commandBatches, IRelationalConnection connection)
   at Microsoft.EntityFrameworkCore.Storage.RelationalDatabase.SaveChanges(IList`1 entries)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChanges(IList`1 entriesToSave)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChanges(StateManager stateManager, Boolean acceptAllChangesOnSuccess)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.<>c.<SaveChanges>b__107_0(DbContext _, ValueTuple`2 t)
   at Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal.SqlServerExecutionStrategy.Execute[TState,TResult](TState state, Func`3 operation, Func`3 verifySucceeded)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChanges(Boolean acceptAllChangesOnSuccess)
   at Microsoft.EntityFrameworkCore.DbContext.SaveChanges(Boolean acceptAllChangesOnSuccess)
   at Microsoft.EntityFrameworkCore.DbContext.SaveChanges()**
**

I assume that updated with only name field should be generated. It was working fine prior in ef core 6.*.

Thanks

[AndriySvyryd]

@jmalczak In your case the Id1 and Id2 properties are not null, so the exception is by-design

[jmalczak]

Hey, but I don't mark them as modified. What sense does it make to even check them if they won't be part of final update statement?

[AndriySvyryd]

Indeed. Please file a new issue for this.

[jmalczak]

@AndriySvyryd thanks, here it is #30601.