efcore icon indicating copy to clipboard operation
efcore copied to clipboard

Throw in validation if the DeleteBehavior or requiredness is changed for an ownership.

Open benlongo opened this issue 9 months ago • 6 comments

Every time I run a migration, the same few foreign keys are recreated with DeleteBehavior.Restrict in Up() and ReferentialAction.Cascade in Down(). If I just run dotnet ef migrations add Test<n> repeatedly, I get the same migration code every time with the same snapshot. ~I don't see any immediately obvious commonalities between the set of foreign keys that is problematic.~ Upon closer inspection, it looks like this has to do with OwnMany relationships. All of the foreign keys that are getting recreated appear to be Owns relationships.

Are owned entity relationships assumed to be always cascading delete behavior? I could see why this might be the case.

I have the following bit of code running in OnModelCreating which restricts all foreign keys to Restrict by default.

public static class DeleteBehaviorExtensions {
    public static void RestrictAllCascadingDeletes( this ModelBuilder modelBuilder ) {
        var cascadeFKs = modelBuilder.Model.GetEntityTypes()
            .SelectMany( t => t.GetForeignKeys() );

        foreach( var fk in cascadeFKs ) {
            fk.DeleteBehavior = DeleteBehavior.Restrict;
        }
    }
}

I believe this is fundamentally the same issue as #3751 (dupes #3761, #3851, #3940, #3976, #3979, #4166, #4213, #4288, #4325, #5061 and probably others). There are a few relevant stack overflow issues as well (https://stackoverflow.com/questions/34082913/ef-migrations-add-always-recreates-foreign-keys-in-the-new-migration https://stackoverflow.com/questions/36824101/ef7-code-first-keeps-dropping-and-recreating-foreign-keys). Searching around the internet hasn't yielded any recent experiences since the fixes for those issues landed.

I apologize that I have a large proprietary schema and haven't had time to dissect out a minimal reproduction yet.

A redacted example of what the migrations look like:

public partial class TestMigration : Migration {
    protected override void Up(MigrationBuilder migrationBuilder) {
          migrationBuilder.DropForeignKey(
              name: "fk_redacted",
              schema: "redacted_schema",
              table: "redacted_table");
              
          migrationBuilder.AddForeignKey(
              name: "fk_redacted",
              schema: "redacted_schema",
              table: "other_redacted_table",
              column: "redacted_column",
              principalSchema: "redacted_schema",
              principalTable: "redacted_table",
              principalColumn: "id",
              onDelete: ReferentialAction.Restrict);
    }
    
    protected override void Down(MigrationBuilder migrationBuilder) {
          migrationBuilder.DropForeignKey(
              name: "fk_redacted",
              schema: "redacted_schema",
              table: "redacted_table");          
        
        migrationBuilder.AddForeignKey(
              name: "fk_redacted",
              schema: "redacted_schema",
              table: "other_redacted_table",
              column: "redacted_column",
              principalSchema: "redacted_schema",
              principalTable: "redacted_table",
              principalColumn: "id",
              onDelete: ReferentialAction.Cascade);
    }
}

Include provider and version information

EF Core version: 8.0.2 Database provider: Npgsql.EntityFrameworkCore.PostgreSQL @ `8.0.2 Target framework: .NET 8 Operating system: Windows

benlongo avatar Apr 27 '24 21:04 benlongo

Adding .Where( fk => !fk.IsOwnership ) appears to have fixed at least the appearance of the recreations. However, I'm not sure this solution really makes sense - is it not valid to have restrictive delete behavior on owned entities?

benlongo avatar Apr 27 '24 21:04 benlongo

I apologize that I have a large proprietary schema and haven't had time to dissect out a minimal reproduction yet.

Can you please try to do that? Given that you're seeing the bug actually happening, you're in a much better place than us to put together such a minimal repro.

roji avatar Apr 29 '24 11:04 roji

Turns out this was caused by a combination of my mistakes - I don't believe there is actually a bug in EF Core. I was running RestrictAllCascadingDeletes before I executed ApplyConfigurationsFromAssembly. This means new foreign keys were added which were not restricted as I had hoped. Additionally, I wasn't filtering out 'owned' relationships.

If I understand correctly, EF Core doesn't allow for non-cascading deletes with owned entities. If this is actually not intended then I think this would be a bug. In such a case, it might make sense to emit warnings when encountering foreign keys that are part of owned relationships which are marked with this cascade behavior.

edit: Upon further thought, I think it would be nice to have guardrails that explicitly prevent changing the deletion behavior of owned-relationship foreign keys. It should always be true that creating a migration with no changes to the snapshot should not result in any changes. It being possible to end up in that state is undesirable IMO and the developer shouldn't be able to land themselves into that position in the first place.

In terms of reproduction, I believe it might be sufficient (haven't tested this myself) to manually set DeleteBehavior = DeleteBehavior.Restrict on a foreign key that has IsOwnership == true.

benlongo avatar Apr 29 '24 16:04 benlongo

Leaving open to let @AndriySvyryd and @ajcvickers comment on the above (or close).

roji avatar Apr 29 '24 17:04 roji

Might be worth adding some logging/warnings, but not sure the value is high enough. I'll leave it to @AndriySvyryd to either close or backlog.

ajcvickers avatar May 01 '24 14:05 ajcvickers

We should throw in validation if the DeleteBehavior or requiredness is changed for an ownership.

AndriySvyryd avatar May 01 '24 21:05 AndriySvyryd

My team is affected by this issue too. Similar to OP, we're explicitly setting the DeleteBehavior of all foreign keys to ClientNoAction in OnModelCreating. We want deletes to fail if dependent entities were not explicitly and deliberately deleted, and this goes for owned entities as well.

In many systems a delete is a whole ceremony and not something that should be done automatically by the data layer. If EF Core's concept of owned entities requires that deletes must cascade then please add that to the docs, as that's an important consideration and likely an adoption blocker for some.

bbascarevic avatar Jun 03 '24 08:06 bbascarevic