Attribute for configuring DeleteBehavior - #27630
Conversation
…leteBehavior enum
|
|
There was a problem hiding this comment.
Here's a first round of comments, the others will likely have more.
Generally speaking, I think the delete behavior logically applies to the foreign key - or to the navigation - as a whole, and not on each property making up the foreign key. Specifically, the way this PR is currently implemented, it's possible to have multiple conflicting DeleteBehavior values on different properties making up the foreign key.
We should instead consider having the attribute on the navigation property from the dependent side. This would also support the case where the foreign key is a shadow property (but let's see what @AndriySvyryd says before making this change).
|
Okay, I'll fix those issues. |
I agree, FK properties could also be used by several FKs. |
…ect class cref on convention constructor
|
I've done smaller formatting fixes, simplified convention tests and made attribute being set on navigation property. @roji How should I go about forcing user to set the attribute on the navigation property. Should I throw an exception if the attribute was set on the individual property of the foreign key instead of the navigation property itself (for now i just ignore that fact)? If so what kind of exception? Next time I'm gonna have some time I'll do the property attribute tests and clean up the xml docs. @AndriySvyryd How would you select which entries in xml docs are important enough to keep them as links? Is there some rule of thumb that I should keep in mind? |
…instead of backing properties of it
…ing it on the navigation properties
Yes, I'd vote for throwing an InvalidOperationException with the message
The links here point to very general EF Core concepts (DbContext, SaveChanges) so I think they are quite low-value in general. Just mentioning "context" and |
…n something different than navigation property
|
@Vekz Have you completed all the requested changes such that this is ready for another review? |
|
Sorry I had a lot of work at studies. I still have to clean up XML comments and to add _overrides_configuration_from_convention_source, _does_not_override_configuration_from_explicit_source tests. I was going to do it this till end of the week. |
|
@AndriySvyryd I think I've managed to make all your requested changes. Although at the end I merged main branch finally to keep up to date with repo and I think I've broke something as it broke heck a lot of imports. Gonna look into it closer in few days. |
| modelBuilder.FinalizeModel()).Message | ||
| ); | ||
| } | ||
|
|
| namespace Microsoft.EntityFrameworkCore; | ||
|
|
||
| /// <summary> | ||
| /// Configures the property or field to indicate how a delete operation is applied to dependent entities |
There was a problem hiding this comment.
| /// Configures the property or field to indicate how a delete operation is applied to dependent entities | |
| /// Configures the navigation property on the dependent side of a relationship to indicate how a delete operation is applied to dependent entities |
d7ef011 to
8d9d189
Compare
(cherry picked from commit d7ef011) (cherry picked from commit 958f49613ac7d0ed1918620a3d6fb1b1ae81b156)
AndriySvyryd
left a comment
There was a problem hiding this comment.
You still need to resolve the conflict in CoreStrings.resx, you can rebase on latest main and squash your changes. Also make sure to "Run Custom Tool" on CoreStrings.Designer.tt in VS
|
Shoot, I use Rider daily at my private PC. So you'll have to wait for me to install VS ;) |
|
Okay, I've run the Custom Tool and I hope fixed tests that weren't passing on CIs. |
|
Thanks for your contribution! |
|
Thank you for your help with this issue. Especially that it was my first ever open source issue. It was really educative experience. |
Fixes #9621
I've added:
DeleteBehaviorAttribute - Stores DeleteBehavior set on the property.
DeleteBehaviorAttributeConvention - Triggers on ForeignKeyAdded, checks if it's properties contain DeleteBehavior. If so then sets DeleteBehavior of this foreign key.
DeleteBehaviorAttributeConventionTest - Tests cases for setting all possible DeleteBehavior for explicit Foreign Key defined by attribute. Also checks for composite key, for multiple different keys with differernt Behaviors, for Foreign Key defined by FluentAPI and for implicit Foreign Key.
Moved:
DeleteBehavior => EFCore.Abstractions and added TypeForward in EFCore
(Also I had to add EFCore.Abstractions reference in base classes for Migrations and Snapshot tests)