Skip to content

Commit

Permalink
Only null non-overlapping FK properties on delete/fixup, if possible (#…
Browse files Browse the repository at this point in the history 
…24538)

Fixes #23883

This is only relevant for overlapping composite foreign keys, where nulling all FK values can also sever other relationships. Since a composite FK is null if any part is null, we avoid nulling the properties that are also shared with another foreign key.
  • Loading branch information
@ajcvickers
ajcvickers authored Apr 3, 2021
1 parent df308c6 commit cff229a
Show file tree
Hide file tree
Showing 8 changed files with 649 additions and 14 deletions.
15 changes: 7 additions & 8 deletions src/EFCore/ChangeTracking/Internal/NavigationFixer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,8 @@ public virtual void NavigationReferenceChanged(
if (inverse != null
&& ReferenceEquals(oldTargetEntry[inverse], entry.Entity)
&& (entry.EntityType.GetNavigations().All(
n => n == navigation
|| !ReferenceEquals(oldTargetEntry.Entity, entry[n]))))
n => n == navigation
|| !ReferenceEquals(oldTargetEntry.Entity, entry[n]))))
{
SetNavigation(oldTargetEntry, inverse, null, fromQuery: false);
}
Expand Down Expand Up @@ -1194,21 +1194,20 @@ private void ConditionallyNullForeignKeyProperties(
InternalEntityEntry? principalEntry,
IForeignKey foreignKey)
{
var principalProperties = foreignKey.PrincipalKey.Properties;
var dependentProperties = foreignKey.Properties;
var hasOnlyKeyProperties = true;

var currentPrincipal = dependentEntry.StateManager.FindPrincipal(dependentEntry, foreignKey);
if (currentPrincipal != null
&& currentPrincipal != principalEntry)
{
return;
}

var hasOnlyKeyProperties = true;
foreignKey.GetPropertiesWithMinimalOverlapIfPossible(out var dependentProperties, out var principalProperties);

if (principalEntry != null
&& principalEntry.EntityState != EntityState.Detached)
{
for (var i = 0; i < foreignKey.Properties.Count; i++)
for (var i = 0; i < dependentProperties.Count; i++)
{
if (!PrincipalValueEqualsDependentValue(
principalProperties[i],
Expand All @@ -1225,7 +1224,7 @@ private void ConditionallyNullForeignKeyProperties(
}
}

for (var i = 0; i < foreignKey.Properties.Count; i++)
for (var i = 0; i < dependentProperties.Count; i++)
{
if (!dependentProperties[i].IsKey())
{
Expand Down
16 changes: 10 additions & 6 deletions src/EFCore/ChangeTracking/Internal/StateManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,8 @@ public virtual void StateChanging(InternalEntityEntry entry, EntityState newStat
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public virtual IModel Model => _model;
public virtual IModel Model
=> _model;

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
Expand Down Expand Up @@ -675,7 +676,7 @@ public virtual void Clear()
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
/// <param name="cancellationToken"> A <see cref="CancellationToken" /> to observe while waiting for the task to complete. </param>
/// <exception cref="OperationCanceledException"> If the <see cref="CancellationToken"/> is canceled. </exception>
/// <exception cref="OperationCanceledException"> If the <see cref="CancellationToken" /> is canceled. </exception>
public virtual Task ResetStateAsync(CancellationToken cancellationToken = default)
{
ResetState();
Expand Down Expand Up @@ -887,6 +888,7 @@ public virtual IEnumerable<IUpdateEntry> GetDependentsUsingRelationshipSnapshot(
}

return ((IEnumerable<object>)navigationValue)
// ReSharper disable once RedundantEnumerableCastCall
.Select(v => TryGetEntry(v, foreignKey.DeclaringEntityType)).Where(e => e != null).Cast<IUpdateEntry>();
}

Expand Down Expand Up @@ -1029,7 +1031,9 @@ public virtual void CascadeDelete(InternalEntityEntry entry, bool force, IEnumer
}
else if (!principalIsDetached)
{
foreach (var dependentProperty in fk.Properties)
fk.GetPropertiesWithMinimalOverlapIfPossible(out var fkProperties, out _);

foreach (var dependentProperty in fkProperties)
{
dependent.SetProperty(
dependentProperty, null, isMaterialization: false, setModified: true, isCascadeDelete: true);
Expand Down Expand Up @@ -1125,9 +1129,9 @@ protected virtual async Task<int> SaveChangesAsync(
public virtual int SaveChanges(bool acceptAllChangesOnSuccess)
=> Context.Database.AutoTransactionsEnabled
? Dependencies.ExecutionStrategyFactory.Create().Execute(
(StateManager: this, AcceptAllChangesOnSuccess: acceptAllChangesOnSuccess),
(_, t) => SaveChanges(t.StateManager, t.AcceptAllChangesOnSuccess),
null)
(StateManager: this, AcceptAllChangesOnSuccess: acceptAllChangesOnSuccess),
(_, t) => SaveChanges(t.StateManager, t.AcceptAllChangesOnSuccess),
null)
: SaveChanges(this, acceptAllChangesOnSuccess);

private static int SaveChanges(StateManager stateManager, bool acceptAllChangesOnSuccess)
Expand Down
49 changes: 49 additions & 0 deletions src/EFCore/Metadata/Internal/ForeignKeyExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Collections.Generic;
using System.Linq;
using Microsoft.EntityFrameworkCore.Diagnostics;

namespace Microsoft.EntityFrameworkCore.Metadata.Internal
Expand Down Expand Up @@ -234,5 +235,53 @@ public static IReadOnlyEntityType ResolveEntityTypeInHierarchy(
? foreignKey.DeclaringEntityType
: foreignKey.PrincipalEntityType;
}

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
/// the same compatibility standards as public APIs. It may be changed or removed without notice in
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public static void GetPropertiesWithMinimalOverlapIfPossible(
this IForeignKey foreignKey,
out IReadOnlyList<IProperty> foreignKeyProperties,
out IReadOnlyList<IProperty> principalKeyProperties)
{
// Finds the foreign key properties (and their associated principal key properties) of this foreign key where those
// properties are not overlapping with any other foreign key, or all properties of the foreign key if there is not
// a smaller set of non-overlapping properties.

foreignKeyProperties = foreignKey.Properties;
principalKeyProperties = foreignKey.PrincipalKey.Properties;

var count = foreignKeyProperties.Count;
if (count == 1)
{
return;
}

for (var i = 0; i < count; i++)
{
var dependentProperty = foreignKey.Properties[i];

if (dependentProperty.GetContainingForeignKeys().Count() > 1)
{
if (ReferenceEquals(foreignKeyProperties, foreignKey.Properties))
{
foreignKeyProperties = foreignKey.Properties.ToList();
principalKeyProperties = foreignKey.PrincipalKey.Properties.ToList();
}

((List<IProperty>)foreignKeyProperties).Remove(dependentProperty);
((List<IProperty>)principalKeyProperties).Remove(foreignKey.PrincipalKey.Properties[i]);
}
}

if (!foreignKeyProperties.Any())
{
foreignKeyProperties = foreignKey.Properties;
principalKeyProperties = foreignKey.PrincipalKey.Properties;
}
}
}
}
138 changes: 138 additions & 0 deletions test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,29 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
modelBuilder.Entity<Produce>()
.HasIndex(e => e.BarCode)
.IsUnique();

modelBuilder.Entity<SharedFkRoot>(builder =>
{
builder.HasMany(x => x.Dependants).WithOne(x => x.Root)
.HasForeignKey(x => new { x.RootId })
.HasPrincipalKey(x => x.Id)
.OnDelete(DeleteBehavior.Cascade);

builder.HasMany(x => x.Parents).WithOne(x => x.Root)
.HasForeignKey(x => new { x.RootId })
.HasPrincipalKey(x => x.Id)
.OnDelete(DeleteBehavior.Cascade);
});

modelBuilder.Entity<SharedFkParent>(builder =>
{
builder.HasOne(x => x.Dependant).WithOne(x => x!.Parent).IsRequired(false)
.HasForeignKey<SharedFkParent>(x => new { x.RootId, x.DependantId })
.HasPrincipalKey<SharedFkDependant>(x => new { x.RootId, x.Id })
.OnDelete(DeleteBehavior.ClientSetNull);
});

modelBuilder.Entity<SharedFkDependant>();
}

protected virtual object CreateFullGraph()
Expand Down Expand Up @@ -601,6 +624,21 @@ protected override void Seed(PoolableDbContext context)
new Poost { Id = 516, Bloog = bloog },
new Poost { Id = 517, Bloog = bloog });

var root = new SharedFkRoot();
context.Add(root);

var parent = new SharedFkParent
{
Root = root
};
context.Add(parent);

context.Add(new SharedFkDependant
{
Root = root,
Parent = parent
});

context.SaveChanges();
}

Expand Down Expand Up @@ -2954,6 +2992,106 @@ public Bloog Bloog
}
}

protected class SharedFkRoot : NotifyingEntity
{
private long _id;

private ICollection<SharedFkDependant> _dependants
= new ObservableHashSet<SharedFkDependant>(LegacyReferenceEqualityComparer.Instance);

private ICollection<SharedFkParent> _parents
= new ObservableHashSet<SharedFkParent>(LegacyReferenceEqualityComparer.Instance);

public long Id
{
get => _id;
set => SetWithNotify(value, ref _id);
}

public ICollection<SharedFkDependant> Dependants
{
get => _dependants;
set => SetWithNotify(value, ref _dependants);
}

public ICollection<SharedFkParent> Parents
{
get => _parents;
set => SetWithNotify(value, ref _parents);
}
}

protected class SharedFkParent : NotifyingEntity
{
private long _id;
private long? _dependantId;
private long _rootId;
private SharedFkRoot _root = null!;
private SharedFkDependant _dependant;

public long Id
{
get => _id;
set => SetWithNotify(value, ref _id);
}

public long? DependantId
{
get => _dependantId;
set => SetWithNotify(value, ref _dependantId);
}

public long RootId
{
get => _rootId;
set => SetWithNotify(value, ref _rootId);
}

public SharedFkRoot Root
{
get => _root;
set => SetWithNotify(value, ref _root);
}

public SharedFkDependant Dependant
{
get => _dependant;
set => SetWithNotify(value, ref _dependant);
}
}

protected class SharedFkDependant : NotifyingEntity
{
private long _id;
private long _rootId;
private SharedFkRoot _root = null!;
private SharedFkParent _parent = null!;

public long Id
{
get => _id;
set => SetWithNotify(value, ref _id);
}

public long RootId
{
get => _rootId;
set => SetWithNotify(value, ref _rootId);
}

public SharedFkRoot Root
{
get => _root;
set => SetWithNotify(value, ref _root);
}

public SharedFkParent Parent
{
get => _parent;
set => SetWithNotify(value, ref _parent);
}
}

protected class NotifyingEntity : INotifyPropertyChanging, INotifyPropertyChanged
{
protected void SetWithNotify<T>(T value, ref T field, [CallerMemberName] string propertyName = "")
Expand Down
Loading

0 comments on commit cff229a

Please sign in to comment.