Reverse engineering: relationship delete behaviour is incorrect

[stevendarby]

A non-nullable foreign key is reversed engineered into a relationship where the delete behaviour is ClientSetNull

Steps to reproduce

Applicant and ApplicationForm tables:

Reverse engineered into this:

entity.HasOne(d => d.Applicant)
    .WithMany(p => p.ApplicationForm)
    .HasForeignKey(d => new { d.TenantId, d.ApplicantId })
    .OnDelete(DeleteBehavior.ClientSetNull)
    .HasConstraintName("FK_ApplicationForm_Applicant");

ClientSetNull summary mentions: This is the default for optional relationships. That is, for relationships that have nullable foreign keys.

Should it actually be Cascade, which says: This is the default for required relationships. That is, for relationships that have non-nullable foreign keys.

Unless I'm missing something. Thanks

Further technical details

EF Core Power Tools version: Version 2.4.217.0

Database engine: SQL Server

Visual Studio version: 2019 16.7.2

[ErikEJ]

Not sure, but you should probably ask in the EF Core repo

@bricelam ?

[bricelam]

Reverse engineering will only use Cascade if the FK in the database uses ON DELETE CASCADE. In other words, your database doesn't use EF's default, so it explicitly configured it to match the database.

[stevendarby]

Ok, so my suggestion it should be Cascade in the EF confit is wrong, but isn’t there still an issue with using ClientSetNull? Because it knows it’s not nullable. Isn’t Restrict or NoAction more appropriate?

edit: No Action seems to be what is configured in the DB, if I'm checking the right thing

[bricelam]

ClientSetNull means that EF (the client) will set any loaded foreign keys currently being tracked to null. But the database will continue to restrict (more or less the same thing as "no action") if you try to save the entity marked for delete. Having the client cascade the deletion into loaded entities can help make managing graphs of objects easier for scenarios like re-parenting.

Also worth mentioning that SQL Server doesn't actually support RESTRICT, so EF always uses NO ACTION instead.

See also dotnet/efcore#21252 (comment). I agree we need to review this area of the code.

[stevendarby]

Not sure now where is best to comment, but one of your replies in the other thread is:

“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 can give you a use case but it does feel like a simple point that the reverse engineering should just accurately model the database.

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.

[ErikEJ]

Closing as there is not really anything to do for Power Tools here.