Skip to content

Commit

Permalink
Include key values in the update topological sort (#21556)
Browse files Browse the repository at this point in the history 
Don't use entry sharing for entities with the same AK values
Only remove the entry with duplicate key value from the identity map if it's still there

Fixes #20870
  • Loading branch information
@AndriySvyryd
AndriySvyryd committed Jul 10, 2020
1 parent 30bb611 commit 78646d2
Show file tree
Hide file tree
Showing 5 changed files with 92 additions and 28 deletions.
98 changes: 75 additions & 23 deletions src/EFCore.Relational/Update/Internal/CommandBatchPreparer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@
using System.Linq;
using System.Text;
using JetBrains.Annotations;
using Microsoft.EntityFrameworkCore.ChangeTracking.Internal;
using Microsoft.EntityFrameworkCore.Diagnostics;
using Microsoft.EntityFrameworkCore.Infrastructure;
using Microsoft.EntityFrameworkCore.Metadata;
using Microsoft.EntityFrameworkCore.Metadata.Internal;
using Microsoft.EntityFrameworkCore.Storage;
using Microsoft.EntityFrameworkCore.Utilities;
using Microsoft.Extensions.DependencyInjection;
Expand Down Expand Up @@ -290,9 +292,8 @@ private void AddUnchangedSharingEntries(
// 2. Commands deleting rows or modifying the foreign key values must precede
// commands deleting rows or modifying the candidate key values (when supported) of rows
// that are currently being referenced by the former
// 3. Commands deleting rows or modifying the foreign key values must precede
// commands adding or modifying the foreign key values to the same values
// if foreign key is unique
// 3. Commands deleting rows or modifying unique constraint values must precede
// commands adding or modifying unique constraint values to the same values
/// <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
Expand Down Expand Up @@ -570,6 +571,12 @@ private void AddForeignKeyEdges(
{
foreach (var command in commandGraph.Vertices)
{
if (command.Predecessor != null)
{
// This is usually an implicit relationship between TPT rows for the same entity
commandGraph.AddEdge(command.Predecessor, command, null);
}

switch (command.EntityState)
{
case EntityState.Modified:
Expand Down Expand Up @@ -638,30 +645,26 @@ private static void AddMatchingPredecessorEdge(
IKeyValueIndex dependentKeyValue,
Multigraph<ModificationCommand, IAnnotatable> commandGraph,
ModificationCommand command,
IForeignKey foreignKey)
IAnnotatable edge)
{
if (predecessorsMap.TryGetValue(dependentKeyValue, out var predecessorCommands))
{
foreach (var predecessor in predecessorCommands)
{
if (predecessor != command)
{
commandGraph.AddEdge(predecessor, command, foreignKey);
commandGraph.AddEdge(predecessor, command, edge);
}
}
}
}

private void AddUniqueValueEdges(Multigraph<ModificationCommand, IAnnotatable> commandGraph)
{
Dictionary<IIndex, Dictionary<object[], ModificationCommand>> predecessorsMap = null;
Dictionary<IIndex, Dictionary<object[], ModificationCommand>> indexPredecessorsMap = null;
var keyPredecessorsMap = new Dictionary<IKeyValueIndex, List<ModificationCommand>>();
foreach (var command in commandGraph.Vertices)
{
if (command.Predecessor != null)
{
commandGraph.AddEdge(command.Predecessor, command, null);
}

if (command.EntityState != EntityState.Modified
&& command.EntityState != EntityState.Deleted)
{
Expand All @@ -682,11 +685,11 @@ private void AddUniqueValueEdges(Multigraph<ModificationCommand, IAnnotatable> c
var valueFactory = index.GetNullableValueFactory<object[]>();
if (valueFactory.TryCreateFromOriginalValues(entry, out var indexValue))
{
predecessorsMap ??= new Dictionary<IIndex, Dictionary<object[], ModificationCommand>>();
if (!predecessorsMap.TryGetValue(index, out var predecessorCommands))
indexPredecessorsMap ??= new Dictionary<IIndex, Dictionary<object[], ModificationCommand>>();
if (!indexPredecessorsMap.TryGetValue(index, out var predecessorCommands))
{
predecessorCommands = new Dictionary<object[], ModificationCommand>(valueFactory.EqualityComparer);
predecessorsMap.Add(index, predecessorCommands);
indexPredecessorsMap.Add(index, predecessorCommands);
}

if (!predecessorCommands.ContainsKey(indexValue))
Expand All @@ -695,19 +698,41 @@ private void AddUniqueValueEdges(Multigraph<ModificationCommand, IAnnotatable> c
}
}
}
}
}

if (predecessorsMap == null)
{
return;
if (command.EntityState != EntityState.Deleted)
{
continue;
}

foreach (var key in entry.EntityType.GetKeys().Where(k => k.GetMappedConstraints().Any()))
{
var principalKeyValue = _keyValueIndexFactorySource
.GetKeyValueIndexFactory(key)
.CreatePrincipalKeyValue(entry, null);

if (principalKeyValue != null)
{
if (!keyPredecessorsMap.TryGetValue(principalKeyValue, out var predecessorCommands))
{
predecessorCommands = new List<ModificationCommand>();
keyPredecessorsMap.Add(principalKeyValue, predecessorCommands);
}

predecessorCommands.Add(command);
}
}
}
}

foreach (var command in commandGraph.Vertices)
if (indexPredecessorsMap != null)
{
if (command.EntityState == EntityState.Modified
|| command.EntityState == EntityState.Added)
foreach (var command in commandGraph.Vertices)
{
if (command.EntityState == EntityState.Deleted)
{
continue;
}

foreach (var entry in command.Entries)
{
foreach (var index in entry.EntityType.GetIndexes().Where(i => i.IsUnique && i.GetMappedTableIndexes().Any()))
Expand All @@ -720,7 +745,7 @@ private void AddUniqueValueEdges(Multigraph<ModificationCommand, IAnnotatable> c

var valueFactory = index.GetNullableValueFactory<object[]>();
if (valueFactory.TryCreateFromCurrentValues(entry, out var indexValue)
&& predecessorsMap.TryGetValue(index, out var predecessorCommands)
&& indexPredecessorsMap.TryGetValue(index, out var predecessorCommands)
&& predecessorCommands.TryGetValue(indexValue, out var predecessor)
&& predecessor != command)
{
Expand All @@ -730,6 +755,33 @@ private void AddUniqueValueEdges(Multigraph<ModificationCommand, IAnnotatable> c
}
}
}

if (keyPredecessorsMap != null)
{
foreach (var command in commandGraph.Vertices)
{
if (command.EntityState != EntityState.Added)
{
continue;
}

foreach (var entry in command.Entries)
{
foreach (var key in entry.EntityType.GetKeys().Where(k => k.GetMappedConstraints().Any()))
{
var principalKeyValue = _keyValueIndexFactorySource
.GetKeyValueIndexFactory(key)
.CreatePrincipalKeyValue(entry, null);

if (principalKeyValue != null)
{
AddMatchingPredecessorEdge(
keyPredecessorsMap, principalKeyValue, commandGraph, command, key);
}
}
}
}
}
}
}
}
4 changes: 2 additions & 2 deletions src/EFCore.Relational/Update/Internal/IKeyValueIndexFactory.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,15 @@ public interface IKeyValueIndexFactory
/// 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>
IKeyValueIndex CreatePrincipalKeyValue([NotNull] IUpdateEntry entry, [NotNull] IForeignKey foreignKey);
IKeyValueIndex CreatePrincipalKeyValue([NotNull] IUpdateEntry entry, [CanBeNull] IForeignKey foreignKey);

/// <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>
IKeyValueIndex CreatePrincipalKeyValueFromOriginalValues([NotNull] IUpdateEntry entry, [NotNull] IForeignKey foreignKey);
IKeyValueIndex CreatePrincipalKeyValueFromOriginalValues([NotNull] IUpdateEntry entry, [CanBeNull] IForeignKey foreignKey);

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
Expand Down
2 changes: 1 addition & 1 deletion src/EFCore.Relational/Update/Internal/KeyValueIndex.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public sealed class KeyValueIndex<TKey> : IKeyValueIndex
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public KeyValueIndex(
[NotNull] IForeignKey foreignKey,
[CanBeNull] IForeignKey foreignKey,
[NotNull] TKey keyValue,
[NotNull] IEqualityComparer<TKey> keyComparer,
bool fromOriginalValues)
Expand Down
9 changes: 7 additions & 2 deletions src/EFCore/ChangeTracking/Internal/IdentityMap.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,8 @@ private void Add(TKey key, InternalEntityEntry entry, bool updateDuplicate)
}
}

if (!bothStatesEquivalent)
if (!bothStatesEquivalent
&& Key.IsPrimaryKey())
{
entry.SharedIdentityEntry = existingEntry;
existingEntry.SharedIdentityEntry = entry;
Expand Down Expand Up @@ -413,7 +414,11 @@ protected virtual void Remove([NotNull] TKey key, [NotNull] InternalEntityEntry

if (otherEntry == null)
{
_identityMap.Remove(key);
if (_identityMap.TryGetValue(key, out var existingEntry)
&& existingEntry == entry)
{
_identityMap.Remove(key);
}
}
else
{
Expand Down
7 changes: 7 additions & 0 deletions test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBaseOneToManyAk.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,13 @@ public virtual void Save_required_many_to_one_dependents_with_alternate_key(
new1d.Parent = null;
new1dd.Parent = null;
var old1 = root.RequiredChildrenAk.OrderBy(c => c.Id).Last();
// Test replacing entity with the same AK, but different PK
new1.AlternateId = old1.AlternateId;
context.Remove(old1);
context.RemoveRange(old1.Children);
context.RemoveRange(old1.CompositeChildren);
context.AddRange(new1, new1d, new1dd, new2a, new2d, new2dd, new2b, new2ca, new2cb);
}
Expand Down

0 comments on commit 78646d2

Please sign in to comment.