efcore: SaveChanges circular dependency in unique unfiltered index

#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

About this issue

Original URL
State: closed
Created 2 years ago
Reactions: 6
Comments: 15 (8 by maintainers)

Commits related to this issue

Make unique index topological dependency optional for null values Fixes #29647 — committed to dotnet/efcore by AndriySvyryd 2 years ago
Make unique index SaveChanges topological dependency optional for null values Fixes #29647 — committed to dotnet/efcore by AndriySvyryd 2 years ago
Make unique index SaveChanges topological dependency optional for null values Fixes #29647 — committed to dotnet/efcore by AndriySvyryd 2 years ago
Make unique index SaveChanges topological dependency optional for null values Fixes #29647 — committed to dotnet/efcore by AndriySvyryd 2 years ago
Make unique index SaveChanges topological dependency optional for null values (#29921) Fixes #29647 — committed to dotnet/efcore by AndriySvyryd a year ago

Most upvoted comments

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.

AndriySvyryd on Dec 7, 2022

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.

AndriySvyryd on Dec 21, 2022

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 😄

stevendarby on Dec 20, 2022

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

grietine on Dec 20, 2022

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.

Devel484 on Dec 20, 2022

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.

grietine on Dec 20, 2022

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

AndriySvyryd on Mar 29, 2023