Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
benlongo opened this issue Apr 27, 2024 · 7 comments
Assignees
Labels
area-model-building customer-reported priority-bug Issues which requires API breaks and have bigger impact hence should be fixed earlier in the release type-bug
Milestone

Comments

@benlongo
Copy link

benlongo commented Apr 27, 2024

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 benlongo changed the title Recreation of unchanged foreign keys with different DeleteBehavior on every migration. Recreation of unchanged foreign keys with different DeleteBehavior on every migration with OwnsMany relationships Apr 27, 2024
@benlongo
Copy link
Author

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?

@roji
Copy link
Member

roji commented Apr 29, 2024

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.

@benlongo
Copy link
Author

benlongo commented Apr 29, 2024

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.

@roji
Copy link
Member

roji commented Apr 29, 2024

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

@ajcvickers ajcvickers removed their assignment May 1, 2024
@ajcvickers
Copy link
Member

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.

@AndriySvyryd
Copy link
Member

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

@AndriySvyryd AndriySvyryd added this to the Backlog milestone May 1, 2024
@AndriySvyryd AndriySvyryd changed the title Recreation of unchanged foreign keys with different DeleteBehavior on every migration with OwnsMany relationships Throw in validation if the DeleteBehavior or requiredness is changed for an ownership. May 1, 2024
@bbascarevic
Copy link

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.

@AndriySvyryd AndriySvyryd added the priority-bug Issues which requires API breaks and have bigger impact hence should be fixed earlier in the release label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-model-building customer-reported priority-bug Issues which requires API breaks and have bigger impact hence should be fixed earlier in the release type-bug
Projects
None yet
Development

No branches or pull requests

5 participants