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

Setting a Nullable Foreign Key property to Null triggers cascade delete #27174

Closed
failwyn opened this issue Jan 12, 2022 · 20 comments
Closed

Setting a Nullable Foreign Key property to Null triggers cascade delete #27174

failwyn opened this issue Jan 12, 2022 · 20 comments
Labels
area-change-tracking closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported Servicing-approved type-bug
Milestone

Comments

@failwyn
Copy link

failwyn commented Jan 12, 2022

Using Entity Framework Core 6.0.1

The following code should create a Module model with the HelpCard Foreign Key set, then set the Help Card Foreign Key to Null and save the model to the database, but instead the Cascade Delete is being triggered and the record is deleted from the database. This worked fine in Entity Framework Core 5;

Removing .OnDelete(DeleteBehavior.Cascade) from either of the Configurations does not affect this behavior; removing it from both of the Configurations makes it behave as expected (but I need the Cascade).

Edit: Nullable Reference Types are set to Disable for the project, according to documentation, this should maintain the existing behavior.

// create a model with the ForeignKey set
var m = new Models.Module() {
        Name = "Test",
        HelpCardId = DatabaseContext.HelpCard.Select(r => r.Id).First()
    };
    DatabaseContext.Add(m);
    DatabaseContext.SaveChanges(true);
    
    // set the ForeignKey to null
    m.HelpCardId = null;
    // the model state is now marked as Deleted
    var state = DatabaseContext.Entry(m).State;
    // after saving, the record is deleted from the database instead of being saved with the ForeignKey set to Null
    DatabaseContext.SaveChanges(true);

Models

public partial class Module : Entity, IEntity<Guid>
{
    private HelpCard _HelpCard;

    public Module()
    {
    }

    private Module(ILazyLoader lazyLoader) : this()
    {
        LazyLoader = lazyLoader;
    }

    private ILazyLoader LazyLoader { get; set; }

    [Key]
    [Required()]
    public virtual Guid Id { get; set; }

    [StringLength(100)]
    [Required()]
    public virtual string Name { get; set; }

    public virtual Guid? HelpCardId { get; set; }        

    public virtual HelpCard HelpCard
    {
        get
        {
            return LazyLoader?.Load(this, ref _HelpCard);
        }
        set
        {
            this._HelpCard = value;
        }
    }                
}
public partial class HelpCard : Entity, IEntity<Guid>, ICloneable 
{
    private IList<Module> _Modules;

    public HelpCard()
    {
        this._Modules = new List<Module>();
    }

    private HelpCard(ILazyLoader lazyLoader) : this()
    {
        LazyLoader = lazyLoader;
    }

    private ILazyLoader LazyLoader { get; set; }

    [Key]
    [Required()]
    public virtual Guid Id { get; set; }

    [StringLength(100)]
    [Required()]
    public virtual string Name { get; set; }

    public virtual IList<Module> Modules
    {
        get
        {
           return LazyLoader?.Load(this, ref _Modules);
        }
        set
        {
            this._Modules = value;
        }
    }
}

Configuration

public partial class ModuleConfiguration : IEntityTypeConfiguration<Module>
{
    public void Configure(EntityTypeBuilder<Module> builder)
    {
        builder.ToTable(@"Module", @"dbo");
        builder.Property(x => x.Id).HasColumnName(@"Id").HasColumnType(@"uniqueidentifier").IsRequired().ValueGeneratedOnAdd().HasDefaultValueSql(@"newid()");
        builder.Property(x => x.Name).HasColumnName(@"Name").HasColumnType(@"varchar(100)").IsRequired().ValueGeneratedNever().HasMaxLength(100);
        builder.Property(x => x.HelpCardId).HasColumnName(@"HelpCardId").HasColumnType(@"uniqueidentifier").ValueGeneratedNever();
        builder.HasKey(@"Id");
        builder.HasOne(x => x.HelpCard).WithMany(op => op.Modules).OnDelete(DeleteBehavior.Cascade).HasForeignKey(@"HelpCardId").IsRequired(false);          

        CustomizeConfiguration(builder);
    }
}
public partial class HelpCardConfiguration : IEntityTypeConfiguration<HelpCard>
{
    public void Configure(EntityTypeBuilder<HelpCard> builder)
    {
        builder.ToTable(@"HelpCard", @"dbo");
        builder.Property(x => x.Id).HasColumnName(@"Id").HasColumnType(@"uniqueidentifier").IsRequired().ValueGeneratedOnAdd().HasDefaultValueSql(@"newid()");
        builder.Property(x => x.Name).HasColumnName(@"Name").HasColumnType(@"varchar(100)").IsRequired().ValueGeneratedNever().HasMaxLength(100);
        builder.HasKey(@"Id");
        builder.HasMany(x => x.Modules).WithOne(op => op.HelpCard).OnDelete(DeleteBehavior.Cascade).HasForeignKey(@"HelpCardId").IsRequired(false);

        CustomizeConfiguration(builder);
    }
}

EF Core version: 6.00 & 6.01
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET 6.0
Operating system: Windows 10
IDE: Visual Studio 2022 17.0.2

@powermetal63
Copy link

This sounds similar to #26968, which is closed as "by design".

However, I verified (you can easily do the same with OP sample) that the same happens with NRT disabled, and even explicitly opting for optional relationship

builder.HasOne(x => x.HelpCard).WithMany(op => op.Modules).OnDelete(DeleteBehavior.Cascade).HasForeignKey(@"HelpCardId")
    .IsRequired(false);

builder.Navigation(x => x.HelpCard)
    .IsRequired(false);

The behavior is the same - the dependent entity is deleted rather than detached from parent.

P.S. Another SO post just asking the same - How to enable optional foreign key with cascade delete?

@failwyn
Copy link
Author

failwyn commented Jan 13, 2022

I Edited the initial report to note that Nullable Reference Types are set to Disable for the project, according to the documentation, this should maintain the existing behavior.

@ajcvickers
Copy link
Contributor

@failwyn The behavior prior to 6.0 was a bug--see #25360. This was fixed in 6.0, and the correct behavior (deleting the orphan) is now observed. If you don't want orphan deletion, but still want cascade deletes, then consider changing the delete orphan timing.

@failwyn
Copy link
Author

failwyn commented Jan 14, 2022

It's not an Orphan, the Module is the Primary Entity, the Help Card is just a property of the Module, why should setting a Nullable Property of a Primary Entity to Null Delete the Primary Entity?

@ajcvickers
Copy link
Contributor

@failwyn HelpCardId is the foreign key for the relationship. Setting it to null severs the relationship and orphans the dependent.

@failwyn
Copy link
Author

failwyn commented Jan 14, 2022

But a Module can legitimately exist without a HelpCard, the Module is the Primary Entity and needs to exist whether it's currently in use or not; is there at least a way to disable the Delete Orphan functionality, because this is a huge breaking change that isn't documented based on a ton of searching and views on StackOverflow.

@ajcvickers
Copy link
Contributor

@failwyn Yes; see the link I posted. You can set the timing to "Never". This will retain cascade deletes (although based on your description of the relationships it seems questionable that you want cascade deletes either) while preventing orphan deletion.

@failwyn
Copy link
Author

failwyn commented Jan 14, 2022

Can this be added to the Breaking Changes for Entity Framework Core 6.0?

Is there a preferred way to configure DeleteOrphansTiming with DbContextOptionsBuilder or in the Context class (It can't be done in the constructor since ChangeTracker is virtual)? Right now the best way I can find to configure it is using OnActivated when registering it with AutoFac.

@failwyn
Copy link
Author

failwyn commented Jan 14, 2022

Setting 'context.ChangeTracker.DeleteOrphansTiming = CascadeTiming.Never' throws an exception. Nullable Reference Types is Disabled, the Foreign Key property is Guid?, and the Relationship in the configuration has .IsRequired(false). Why is Entity Framework saying that it's required?

The association between entity types 'HelpCard' and 'Module' has been severed, but the relationship is either marked as required or is implicitly required because the foreign key is not nullable. If the dependent/child entity should be deleted when a required relationship is severed, configure the relationship to use cascade deletes.

@ajcvickers
Copy link
Contributor

@failwyn

Setting 'context.ChangeTracker.DeleteOrphansTiming = CascadeTiming.Never' throws an exception.

I will investigate this.

@powermetal63
Copy link

powermetal63 commented Jan 15, 2022

@ajcvickers Why cascade delete / delete orphans options affect the operation of the dependent entity? This is counterintuitive to everything we know about cascade delete - defines what to do with dependents when deleting the principal. Here we are not deleting the principal. We are updating the dependent. No relational database deletes the record when you update the nullable cascade delete enforced FK to NULL.

Also does not match the behavior explained in Optional relationships

By default for optional relationships, the foreign key value is set to null. This means that the dependent/child is no longer associated with any principal/parent.
...
Notice that the post is not marked as Deleted. It is marked as Modified so that the FK value in the database will be set to null when SaveChanges is called.

This is exactly what one would expect for optional dependent, regardless of the cascade delete option.

So not sure what was fixed in 6.0, but the current behavior definitely sounds like bug to me.

And the worse is that there is no workaround - the orphan timing option you are suggesting is irrelevant, as we are not trying to reparent the entity, but simply disassociate it from the parent (after all, it is possible to be created without parent and should be able to be updated to have no parent).

@mt-akar
Copy link

mt-akar commented Jan 16, 2022

I agree with everything @powermetal63 said. This does seem like a regression.

I am using the following workaround.

public class User
{
    public string Id { get; set; }
    public string? DepartmentId { get; set; }
    public Department? Department { get; set; }
}

public class Department
{
    public string Id { get; set; }
    public List<User>? Users { get; set; }
}

and in the OnModelCreating

builder.Entity<User>()
  .HasOne(e => e.Department)
  .WithMany(e => e.Users)
  .IsRequired(false) // This line has no effect
  .OnDelete(DeleteBehavior.Cascade);

This correctly sets up an optional FK relationship with cascade delete enabled.

However, when I set myUser.Department = null, the user state becomes Deleted. What I do is just to set the state of the entity back to Modified manually.

user.Department = null;
_db.Entry(user).State = EntityState.Modified;
await _db.SaveChangesAsync();

At the end of the day, I feel like this behavior is confusing. When an optional foreign key with cascade delete enabled is set to null, the dependant entity state is set to Deleted, when I think it should be set to Modified.

@ajcvickers
Copy link
Contributor

@failwyn @m-azyoksul @powermetal63 I've been pondering on this over the weekend, and I think you folks are right. Cascade delete is typically used with required relationships, where a dependent cannot exist in the database without a principal. In this case, delete orphans behavior makes sense. However, if cascade delete is used with optional relationships, then the dependent can still exist without the principal, which means it probably doesn't make sense to delete orphans automatically.

In addition, setting orphan timing to Never definitely should not throw when attempting to save a null FK for optional relationships.

I will investigate and discuss with the team what we can do here. We can likely revert this particular change in a patch (although that will require Director-level approval, so I can't promise), but we also need to be conscious of making breaking changes here if other behaviors around orphan deletion are changed.

@mm-ryo
Copy link

mm-ryo commented Jan 19, 2022

we also faced this issue.
@ajcvickers can we revert these changes asap? we get stack with this .net6 upgrading :'(

@ajcvickers
Copy link
Contributor

PR #27216 reverts the 6.0 change. We will seek permission to ship this in the 6.0.3 patch; the window for fixes in 6.0.2 has already closed.

@failwyn
Copy link
Author

failwyn commented Jan 19, 2022

@ajcvickers Any idea on the timeline for 6.0.2 and 6.0.3? I have clients waiting to go live and there are no usable workarounds to make this work while we wait.

@ajcvickers
Copy link
Contributor

6.0.2 is scheduled for February; 6.0.3 will likely ship in March. Unfortunately, shipping patches requires a considerable amount of testing, validation, and sign-off work which is coordinated centrally, and is a very slow process. We are investigating ways to make this faster for components like Entity Framework, which are not part of the SDK, but this won't happen immediately.

@failwyn You should be able to use the workaround outlined by @m-azyoksul in #27174 (comment)

@failwyn
Copy link
Author

failwyn commented Jan 19, 2022

That's not a feasible workaround for large applications; Were you able to find anything on why setting DeleteOrphansTiming to Never is throwing an exception for optional relationships?

@ajcvickers
Copy link
Contributor

@failwyn

That's not a feasible workaround for large applications

Why so?

Were you able to find anything on why setting DeleteOrphansTiming to Never is throwing an exception for optional relationships?

I filed #27218.

@failwyn
Copy link
Author

failwyn commented Jan 19, 2022

Why so?

I'd have to add that code to hundreds of locations, test to be sure it doesn't create any new issues, then remove it all in March and retest everything again. DeleteOrphansTiming Never would have been a great workaround.

Thanks for filing #27218 , will that also be addressed in 6.0.3?

@ajcvickers ajcvickers modified the milestones: 6.0.x, 6.0.3 Jan 27, 2022
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jan 27, 2022
@ajcvickers ajcvickers removed their assignment Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-change-tracking closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported Servicing-approved type-bug
Projects
None yet
Development

No branches or pull requests

5 participants