Simplification of cascade delete fluent API

[divega]

Note: we ended up not implementing the proposal below but what is described in #8654 (comment) and subsequent comments

While looking at customer reported issues related to OnDelete(DeleteBehavior) fluent API, we came to the conclusion that what we have today in EF Core is just too confusing:

DeleteBehavior conflates change tracking and database behavior in a single place and the behaviors resulting from the options available (Cascade, SetNull and Restrict) do not align with users' expectations.

There are reasons for the misalignment:

• When we originally designed the API, we naively expected we would make SetNull the default for nullable FKs and Restrict the default for non-nullable FKs. The change tracking and database behavior could align!
• However we hit a wall very quickly with that: in particular in SQL Server, setting multiple FKs to either SetNull or Cascade results frequently in cycles, which typically yield this error:

Introducing FOREIGN KEY constraint 'Foo' on table 'Bar' may cause cycles or multiple cascade paths. Specify ON DELETE NO ACTION or ON UPDATE NO ACTION, or modify other FOREIGN KEY constraints

The backup plan was to:

1. Make Restrict the default behavior in the database
2. Make the default behavior in change tracking always try to set keys to null regardless of whether Restrict or SetNull was specified.
3. Only DeleteBehavior.Cascade makes the change tracking and database behaviors align

Proposal:

1. Obsolete the OnDelete fluent API and the DeleteBehavior enum used in its argument

2. Create a new simple bool-based API that indicates only if deletes should cascade. This will control both the change tracking and (indirectly) the database behavior. Several naming alternatives are:

• WillCascadeOnDelete: this is what we had in EF6
• CascadeOnDelete: shortened version of the above
• OnDeleteCascade: closer name to current API in EF Core and closer to the SQL syntax

3. EF Core will continue to have current default behaviors, e.g. it will set it to false if the FK is nullable, true if the FK is non-nullable. When the API is called with no arguments, that means true. Otherwise we use whatever Boolean value is specified.

4. Relational can have its own API that represent more granularly what can go in the database's ON DELETE clause. We need to look at how easy it is to drop down to metadata to specify annotations in the model for this. Incidentally, we already have this enum that seems to cover all the necessary options in migrations:

namespace Microsoft.EntityFrameworkCore.Migrations
{
    public enum ReferentialAction
    {
        NoAction,
        Restrict,
        Cascade,
        SetNull,
        SetDefault
    }
}

5. When cascade deletes are disabled, the behavior in change tracking will continue to be setting nullable keys to null and tracking conceptual nulls for non-nullable keys.

6. Providers will be free to choose how to best match the default change tracking behavior. E.g. in the case of SQL Server, OnDeleteCascade(false) will result in ON DELETE NO ACTION, but on a different provider for a database which doesn't fail eagerly on cycles, it could produce ON DELETE SET NULL.

[divega]

@roji, @ErikEJ and everyone, we were wondering if you could provide any insights on the most appropriate default cascade behavior for nullable FKs across databases you are familiar with. There are two "schools of thought" on this:

1. The provider should match the change tracking behavior as closely as possible and use ON DELETE SET NULL. This probably introduces a performance penalty because it needs to effectively do an extra update, but it also probably removes the need to check that the constraint is met afterwards.
2. We should be conservative and keep ON DELETE RESTRICT as the default, not encourage any providers to deviate from that, etc. This means dependents not loaded in memory can cause the deletion of the principal to fail unexpectedly. But on the bright side it takes care of performance and cycle-related concerns.

[ErikEJ]

The SQL Compact engine only support CASCADE and NO ACTION https://technet.microsoft.com/en-us/library/ms173393(v=sql.110).aspx

[ajcvickers]

Add Default enum and make it the default. It will be the hybrid behavior--set null in the state manager, restrict in the database.
We will make Restrict work in the state manager even for nullable FKs, by re-using the conceptual null code.

[bricelam]

set null in the state manager, restrict in the database.

Is this just the behavior on SQL Server, or the semantic meaning of Default?

In other words, if a provider supports ON DELETE SET NULL without getting may cause cycles or multiple cascade paths errors, are they free to make SET NULL in the database the behavior of Default?

[ajcvickers]

I think that's an open question, but probably yes. However, it might be better for that provider to have a convention that makes SetNull the default.

[divega]

A few more thoughts on this

• Default may not be the name we are looking for:
  • This option represents only the default for nullable FKs. For non-nullable Fks the default is still Cascade. Correct?
  • In relational databases SET DEFAULT is one of the options for ON DELETE, so it is confusing
• Another tweak could be to just use SetNull by convention and have a SQL Server convention that overrides with Restrict (or NO ACTION). I think if the user explicitly asks for SetNull it should win over the SQL Server convention because of the configuration source. Makes sense?
• If we decide providers should try to do SET NULL, we should add a spec test (and override it for SQL Server and SQL CE)

[ajcvickers]

@divega How about this?

/// <summary>
///     <para>
///         Indicates how a delete operation is applied to dependent entities in a relationship when the
///         principal is deleted or the relationship is severed.
///     </para>
///     <para>
///         Behaviors in the database are dependent on the database schema being created
///         appropriately. Using Entity Framework Migrations or <see cref="DatabaseFacade.EnsureCreated" />
///         will create the appropriate schema.
///     </para>
///     <para>
///         Note that the in-memory behavior for entities that are currently tracked by
///         the <see cref="DbContext" /> can be different from the behavior that happens in the database.
///         See the <see cref="Hybrid" /> behavior for more details.
///     </para>
/// </summary>
public enum DeleteBehavior
{
    /// <summary>
    ///     <para>
    ///         For entities being tracked by the <see cref="DbContext" />, the values of foreign key properties in
    ///         dependent entities are set to null. This helps keep the graph of entities in a consistent
    ///         state while they are being tracked, such that a fully consistent graph can then be written to
    ///         the database. If a property cannot be set to null because it is not a nullable type,
    ///         then an exception will be thrown when <see cref="DbContext.SaveChanges()" /> is called.
    ///         This is the same as the <see cref="SetNull" /> behavior.
    ///     </para>
    ///     <para>
    ///         If the database has been created from the model using Entity Framework Migrations or the
    ///         <see cref="DatabaseFacade.EnsureCreated" /> method, then the behavior in the database is a
    ///         sensible default for that database provider. Some databases cannot easily support the
    ///         <see cref="SetNull" /> type behavior described above for tracked entities, especially if there are
    ///         cycles in relationships. Therefore, for many database providers, the default behavior in the
    ///         database will be equivalent to a <see cref="Restrict" /> behavior where an error will be generated if a
    ///         foreign key constraint is violated.
    ///     </para>
    ///     <para>
    ///         This is the default for optional relationships. That is, for relationships that have
    ///         nullable foreign keys.
    ///     </para>
    /// </summary>
    Hybrid,

    /// <summary>
    ///     <para>
    ///         For entities being tracked by the <see cref="DbContext" />, the values of foreign key properties in
    ///         dependent entities are not changed. This can result in an inconsistent graph of entities
    ///         where the values of foreign key properties do not match the relationships in the
    ///         graph. If a property remains in this state when <see cref="DbContext.SaveChanges()" />
    ///         is called, then an exception will be thrown.
    ///     </para>
    ///     <para>
    ///         If the database has been created from the model using Entity Framework Migrations or the
    ///         <see cref="DatabaseFacade.EnsureCreated" /> method, then the behavior in the database
    ///         is to generate an error if a foreign key constraint is violated.
    ///     </para>
    /// </summary>
    Restrict,

    /// <summary>
    ///     <para>
    ///         For entities being tracked by the <see cref="DbContext" />, the values of foreign key properties in
    ///         dependent entities are set to null. This helps keep the graph of entities in a consistent
    ///         state while they are being tracked, such that a fully consistent graph can then be written to
    ///         the database. If a property cannot be set to null because it is not a nullable type,
    ///         then an exception will be thrown when <see cref="DbContext.SaveChanges()" /> is called.
    ///     </para>
    ///     <para>
    ///         If the database has been created from the model using Entity Framework Migrations or the
    ///         <see cref="DatabaseFacade.EnsureCreated" /> method, then the behavior in the database is
    ///         the same as is described above for tracked entities. Keep in mind that some databases cannot easily
    ///         support this behavior, especially if there are cycles in relationships.
    ///     </para>
    /// </summary>
    SetNull,

    /// <summary>
    ///     <para>
    ///         For entities being tracked by the <see cref="DbContext" />, the dependent entities
    ///         will also be deleted when <see cref="DbContext.SaveChanges()" /> is called.
    ///     </para>
    ///     <para>
    ///         If the database has been created from the model using Entity Framework Migrations or the
    ///         <see cref="DatabaseFacade.EnsureCreated" /> method, then the behavior in the database is
    ///         the same as is described above for tracked entities. Keep in mind that some databases cannot easily
    ///         support this behavior, especially if there are cycles in relationships.
    ///     </para>
    ///     <para>
    ///         This is the default for required relationships. That is, for relationships that have
    ///         non-nullable foreign keys.
    ///     </para>
    /// </summary>
    Cascade
}

[divega]

@ajcvickers when I made my previous comment I missed that Restrict will start being honored for nullable FKs in-memory, which makes it unsuitable for the default for nullable FKs with SQL Server. So I realize we do need a new option in the enum.

A few suggestions for your proposal above:

1. Define the behavior of the new "hybrid" option as just straight "set null in memory but restrict in the database" (rather than "set null in memory and then maybe set null in the database but perhaps no")

2. Then we can name it ClientSetNull or SetNullOnClient instead of Hybrid

3. I am still inclined to default nullable FKs to SetNull in relational but make SQL Server use ClientSetNull instead, but I think I can be convinced otherwise 😄. Whatever is chosen by convention should be overridable by user configuration.

4. Note that the SQL CE provider should also configure ClientSetNull by convention for nullable FKs but if the user configures SetNull explicitly it should throw because the database does not support it. In general if the database cannot honor a behavior it should start throwing (possibly in model validation but it might be acceptable for DDL SQL execution to fail) cc @ErikEJ

5. Since none of the option is really a universal default, there doesn't seem to be a compelling reason to put the new option at the beginning of the enum

6. Change "the behavior that happens in the database" to "the behavior applied in the database"

[bricelam]

In relational databases SET DEFAULT is one of the options for ON DELETE, so [Default] is confusing

I don't agree with this statement. The .NET idiom of an enum value named Default is well understood. Also, having a valued called SetNull tells me that if one of the values did support SET DEFAULT, it would be called SetDefault.

[divega]

@bricelam That was only the secondary argument. The primary argument is that there is actually no default value for this enum:

This option represents only the default for nullable FKs. For non-nullable Fks the default is still Cascade.