Efcore: Update DeleteBehavior to be more consistent and understandable

[roji]

In https://github.com/npgsql/Npgsql.EntityFrameworkCore.PostgreSQL/issues/508, a user pointed out that Npgsql doesn't scaffold OnDelete actions correctly from existing PostgreSQL databases. Some investigation shows that in RelationalScaffoldingModelFactory, the database model's foreign key's ReferentialAction is translated to a DeleteBehavior, but that translation seems to lose some values: all actions apart from Cascade and SetNull get translated to ClientSetNull. I haven't dived yet to understand exactly what's going on, but this seems to make it impossible to roundtrip a database (scaffold and recreate from model). Can you guys please shed some light on why this is being done?

In addition, it seems that the DeleteBehavior enum does not have a SetDefault member, unlike the ReferentialAction enum which does. This makes it impossible to create foreign keys with SET DEFAULT actions, is this intended?

Finally, note that on PostgreSQL NO ACTION (the default) and RESTRICT aren't identical - they differ subtly in that RESTRICT isn't deferrable.

[ajcvickers]

@roji Unfortunately, the DeleteBehavior enum and its actions have become very confusing due to evolution of the simple thing we used to have into a more complex thing which tried not to break the simple thing. This doc is an attempt to explain it, but it's not very good either.

In a nutshell, Restrict and ClientSetNull are essentially the same in terms of database behavior, but ClientSetNull retains all the original fixup behavior. Restrict is intended to tell EF to do nothing for you--that is, no fixup. Except we later decided it will do some fixup, but it won't set an FK to null just because a navigation was set to null. And it has bugs as well.

I'm going to mark this for re-triage so we can come up with a plan.

[roji]

@ajcvickers I suspected that there was a mix of database-side logic and EF Core client-side entity tracking logic... Will look forward to understand better and try to help.

As a guiding principle, I think it's important to allow users to set up their database with whatever (supported) referential action they choose, i.e. not have EF Core block anything as it seems to be doing now. Although I do realize that there's important interaction with what happens client-side, and I can see how this whole question isn't trivial at all.

[ajcvickers]

Consider separating out the client and store parts of this.

[ajcvickers]

Current behavior

Top-level API calls look like:

```C#
modelBuilder
.Entity()
.HasMany(e => e.Posts)
.WithOne(e => e.Blog)
.OnDelete(DeleteBehavior.Cascade);

Values are:
* Cascade
  * Store: cascade delete
  * EF: cascade delete
  * Also delete orphans
  * Default for required relationships
* ClientSetNull
  * Store: Restrict
  * EF: full fixup
  * Default for optional relationships
* SetNull:
  * Store: SetNull
  * EF: full fixup
  * Hard to use effectively on SQL Server due to loops
* Restrict
  * Store: Restrict
  * EF: Will not null out FK on delete even if it is nullable
  * Usually means store will throw from constraint violation
  * Most people don't want this--EF6 never did it

### Goals

* Minimize breaking changes
* Guide people to Cascade and ClientSetNull behaviors above
* Guide people away from Restrict unless they know they want this behavior
* Allow client side and store side to be set differently
  * Helps with the SQL Server cycles case
* Allow orphan and delete behavior to be set differently

### Proposal A

Minor changes; less breaking.

* Obsolete Restrict because it is the most confusing value
  * It looks like it is used to set Restrict in the database. While it does do this, the reason to use it is to change EF fixup behavior, so it is usually used inappropriately.
  * Add "NoFixup" that indicates that EF will not attempt to do fixup when entities are deleted.
  * It will create Restrict in the database, but this is more a side-effect.
  * This is the same behavior as Restrict but with a less confusing name.
* Pair the others:
  * SetNull/ClientSetNull
  * Cascase/ClientCascade
* Add Default
  * Just do whatever ever EF would do for me if I wasn't even calling this
  * Not really needed, but may save people from trying to figure out what to do

```C#
public enum DeleteBehavior
{
    ClientSetNull,
    [Obsolete] Restrict,
    SetNull,
    Cascade,
    Default,
    ClientCascade,
    NoFixup = Restrict
}

Pros:

• Not very breaking; all existing values supported; one obsoletion
• Removes the confusion around Restrict
• Allows client-side cascade to be configured; cycles issue

Cons:

• Not a clean break between client/store behavior
  
  
  
  • Not possible to specify, for example, Restrict verses No Action in the model
  
  
  • But, not clear if this is really needed
  
  
  • Also, not clear that all providers would need the same set of values here
  
  
  • Could it be done through provider-specific API?
  
  
• Does not allow orphan config to be different from cascade config
  
  
  
  • But similar thing could be achieved with configuration of timing
  
  
  • Could introduce CascadeNoOrphans and ClientCascadeNoOrphans to cover this
  
  

Propsal B

More breaking; may be cleaner.

Introduce:

```C#
public enum ClientDeleteBehavior
{
Default,
SetNulls,
Cascade
CascadeExcludeOrphans, // Needs a better name!
NoFixup
}

```C#
public enum StoreDeleteBehavior // Same as ReferentialAction
{
    NoAction,
    Restrict,
    Cascade,
    SetNull,
    SetDefault
}

Obsolete existing APIs and instead allow, for example:

C# modelBuilder .Entity<Blog>() .HasMany(e => e.Posts) .WithOne(e => e.Blog) .OnDelete(ClientDeleteBehavior.Cascade, StoreDeleteBehavior.Restrict);

Pros:

• Clean distinction between client and store behaviors
• Allows orphan behavior to be configured separately

Cons:

• Very breaking
  
  
  
  • Note that lots of core metadata and internal APIs currently work with DeleteBehavior, so obseleting or removing this is a big change.
  
  

[ajcvickers]

Notes from the design meeting:

We will go with:
C# public enum DeleteBehavior { SetNull, // Cascading nulls on client and in database ClientSetNull, // Cascading nulls on client only (default for optional) Cascade, // Cascading deletes on client and in database (default for required) ClientCascade, // Cascading deletes on client only NoAction, // No action in the database; default behavior on the client ClientNoAction, // No action on the client and on the database (current Restrict behavior) Restrict, // No action in the database, default behavior on the client }
Notes:

• Enum ordering changed here for clarity--we don't actually intend on changing existing values.
• While the pairs of values are not completely consistent, we think they will direct people to choose an appropriate behavior
• The behavior of Restrict changes to just make the FK restrict in the database without changing the EF behavior. This is break, but unlikely to impact many people.
• NoAction is the same as Restrict, except that it will flow into the migrations as NoAction instead of Restrict
• ClientNoAction is the unusual one. This means don't delete a dependent or null out its FK on the client because the principal has been deleted. It's unusual to want this, but is useful for people who need to maintain the current behavior of Restrict.
• We'll make the API nullable to allow the default to be re-asserted.
• Configuration of different orphans behavior will be through a new API
• We discussed changing the default for optional/required relationships, and while we had ideas we decided that non of them were worth making a break for.

[ajcvickers]

See #10066 for delete orphans configuration.

[divega]

@ajcvickers is this a breaking change in preview 4 that still needs to be documented?

[ajcvickers]

No, it's not merged yet and won't be in preview4.

[markusschaber]

We're just upgrading from EF Core 2.1 to 3.1, and this breaking change broke our app. How could you come up with "Restrict" not actually restricting any more?
I'm not against having an option for this behaviour, but shipping it under the name "Restrict" which used to really restrict in previous versions is kinda perfidious.

[tekTutorialsHub]

NoAction, // No action in the database; default behavior on the client

The default behavior is Cascade if Foreign key is Not Nullable & ClientSetNull if the Foreign key is nullable. But it is always using ClientSetNull

[rockstardev]

@markusschaber so it seems that ClientNoAction is ACTUALLY Restrict?

If so, then yes - this is truly terrible design decision... would've made so much more sense to name it RestrictClientSetToNull. What did you end up using in the end?

[AndriySvyryd]

Here's the code that translates DeleteBehavior to the database action: https://github.com/dotnet/efcore/blob/b4baed658262a6bd889066186db18054518a02f4/src/EFCore.Relational/Metadata/Internal/RelationalModel.cs#L1081