Preserve DeleteBehavior when scaffolding a database created by EF Core

[JonPSmith]

I have a test database where I set the DeleteBehaviour of one line to Restrict. When I reverse engineer it the DeleteBehaviour is set to ClientSetNull. As the foreign key isn't nullable ClientSetNull seems an odd result.

Steps to reproduce

My DbContext looks like this

public class EfCoreContext : DbContext
{
    private readonly Guid _userId;                                    

    public EfCoreContext(DbContextOptions<EfCoreContext> options,     
        IUserIdService userIdService = null)                          
        : base(options)
    {
        _userId = userIdService?.GetUserId()                          
                   ?? new ReplacementUserIdService().GetUserId();     
    }

    public DbSet<Book> Books { get; set; }
    public DbSet<Author> Authors { get; set; }
    public DbSet<PriceOffer> PriceOffers { get; set; }
    public DbSet<Order> Orders { get; set; }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<BookAuthor>().HasKey(x => new {x.BookId, x.AuthorId});

        modelBuilder.Entity<LineItem>()
            .HasOne(p => p.ChosenBook) 
            .WithMany()
            .OnDelete(DeleteBehavior.Restrict);

        modelBuilder.Entity<Book>().HasQueryFilter(p => !p.SoftDeleted);                     
                                                        
        modelBuilder.Entity<Order>() .HasQueryFilter(x => x.CustomerId == _userId);            
    } 
}

The part of the reverse engineered that configures the LineItem is

modelBuilder.Entity<LineItem>(entity =>
{
    entity.ToTable("LineItem");

    entity.HasIndex(e => e.BookId);

    entity.HasIndex(e => e.OrderId);

    entity.Property(e => e.BookPrice).HasColumnType("decimal(18, 2)");

    entity.HasOne(d => d.Book)
        .WithMany(p => p.LineItems)
        .HasForeignKey(d => d.BookId)
        .OnDelete(DeleteBehavior.ClientSetNull);

    entity.HasOne(d => d.Order)
        .WithMany(p => p.LineItems)
        .HasForeignKey(d => d.OrderId);
});

I did check the SQL for the LineItem below

CREATE TABLE [LineItem] (
    [LineItemId] int NOT NULL IDENTITY,
    [LineNum] tinyint NOT NULL,
    [NumBooks] smallint NOT NULL,
    [BookPrice] decimal(18,2) NOT NULL,
    [OrderId] int NOT NULL,
    [BookId] int NOT NULL,
    CONSTRAINT [PK_LineItem] PRIMARY KEY ([LineItemId]),
    CONSTRAINT [FK_LineItem_Books_BookId] FOREIGN KEY ([BookId]) REFERENCES [Books] ([BookId]) ON DELETE NO ACTION,
    CONSTRAINT [FK_LineItem_Orders_OrderId] FOREIGN KEY ([OrderId]) REFERENCES [Orders] ([OrderId]) ON DELETE CASCADE
);

Further technical details

EF Core version:
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET Core 5-preview5
Operating system: Windows
IDE: Visual Studio 2019 16.6

[ajcvickers]

@JonPSmith ClientSetNull is the default behavior that EF has always had for non-cascading relationships. It means that the context will still do fixup setting the FK to null when appropriate if the relationship is severed. There wasn't a way to configure EF6 or before to do anything other than this. And since this is a purely EF behavior, not a database behavior, it is never reflected in the schema, so from the database side it looks the same as Restrict.

[JonPSmith]

Yes, I can see your reasoning and it makes sense. I thought a non-nullable foreign key would be enough to decide on a better DeleteBehavior, but you don't know if its a Restrict or a ClientCascade. As you say, having the DeleteBehavior of ClientSetNull will fail because the foreign key is non-nullable, which means that the delete will fail as it should, but the exception message will be different.

Thanks for taking the time to explain that. I have marked this as closed.

[JonPSmith]

Sorry @ajcvickers , but this one was bugging me. I think if the the foreign key is non-nullable you should set the DeleteBehavior to Restrict. Yes, ClientSetNull will cause an exception, but to my mind Restrict is a better setting.

You have other, more important things to do so don't do anything about this, but I had to say that because it was going around in my head.

[ajcvickers]

@JonPSmith I believe that the behavior of Restrict and ClientSetNull will be the same for non-cascading required relationships. I'm going to re-open this to discuss whether we should use Restrict instead since it will be functionally the same.

[JonPSmith]

Hi @ErikEJ,

I wondered if you give your feedback on this issue I raised scaffolding.

Super-quick summary: If the SQL Delete constraint is NO ACTION and the foreign key is non-nullable, then I think the DeleteBehavior in the scaffolded config should be Restrict, but at the moment it is ClientSetNull.

Do you have a view on the right state, and more importantly if this was changed would it cause a problem to anyone already using scaffolding? As you and your company use scaffolding in production I expect you will have a view on this :)

[ErikEJ]

@JonPSmith Given that the foreign keys and their delete behaviour is defined in the database, I fail to see how this makes much difference if any if scaffolding from an existing database. (I never use CASCADE anyway - too much dangerous magic)

[ajcvickers]

Putting this on the backlog to scaffold the same value that the database schema indicates whenever possible.

[bricelam]

Current flow of values:

RevEng	Model	Migrations
NO ACTION	ClientSetNull	RESTRICT
RESTRICT	ClientSetNull	RESTRICT
CASCADE	Cascade	CASCADE
SET NULL	SetNull	SET NULL
SET DEFAULT	ClientSetNull	RESTRICT
	NoAction	NO ACTION
	ClientNoAction	NO ACTION
	Restrict	RESTRICT
	ClientCascade	RESTRICT
		SET DEFAULT

Note: SQL Server doesn't support RESTRICT so we use NO ACTION instead

There seems to be a lot of room for improvement. 🙂

[stevendarby]

I have built a system around checking dependencies before deleting an entity so that:
a) dependents with NoAction delete behaviour are counted and reported back, to block the delete and inform the user with details of the dependencies so they can manually manage it.
b) dependents with cascade, set null etc. can be loaded into the client so that their deletion is audited before any database action deletes/updates them and also so that any client-side delete behaviour can be performed by EF.

With reverse engineering I would expect the modelled delete behaviour to match the database. If I desire some client behaviour such as ClientCascade, I can modify as such and the delete routine will load and audit them for deletion.

In the given example (unfortunately one of many) the user will delete an Applicant, be given no warning about dependent ApplicationForms, then EF will throw an exception trying to set the FK to null. Because I have built a system on the assumption the metadata EF holds about the database is accurate, and where it isn’t it’s because I’ve manually modified it. But currently it’s not accurate off the bat.

[stevendarby]

Can I ask for clarity over delete behaviour in EF Core 3.1 please.

This documentation for Restrict states None for effect on dependent entities in memory.

But the enum documentation for Restrict says "For entities being tracked by the DbContext, the values of foreign key properties independent entities are set to null when the related principal is deleted."

Can I correctly assume that the first documentation is correct and the enum one wrong?

As a side note, NoAction also has the same enum comment as Restrict, but doesn't feature in the first documentation. I'm making an assumption that they have the same behaviour client side and both exist just to match different database options among different providers. But it'd be nice not to have to make assumptions about any of this.

[ajcvickers]

@Snappyfoo The cascade delete docs are out-of-date. This issue is tracking updating the docs: dotnet/EntityFramework.Docs#473

I believe the enum documentation is correct, since it was updated when the behavior was updated before 3.0. Note that each value describes both client and database behaviors. This means that there isn't a 1:1 mapping between the database value and the EF value. However, there should not be a case where the database has a different behavior from that which is reverse engineered.

[bricelam]

Here are my proposed changes:

Scaffolding

Database	Model
NO ACTION	ClientSetNull
RESTRICT	ClientSetNull Restrict
CASCADE	Cascade
SET NULL	SetNull
SET DEFAULT	ClientSetNull (☹️ no SetDefault)

Migraitons

Model	Database
ClientSetNull	RESTRICT NO ACTION
Restrict	RESTRICT
SetNull	SET NULL
Cascade	CASCADE
ClientCascade	RESTRICT NO ACTION
NoAction	NO ACTION
ClientNoAction	NO ACTION
(😕 edit migration)	SET DEFAULT