Skip to content

FIX Insert order bug when using TPC - Issue #35978 #38070

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.

Sign up for GitHub

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

Open
andrewraper-Sage wants to merge 5 commits into
base: main
Choose a base branch
from
+118 −34
Open

FIX Insert order bug when using TPC - Issue #35978 #38070

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a589fc1
fix insert order when using tpc
andrewraper-Sage Apr 9, 2026
f671169
add unit tests
andrewraper-Sage Apr 9, 2026
3fcf4b9
revert else removal
andrewraper-Sage Apr 13, 2026
518c646
Fix failing tests when mapped to json
andrewraper-Sage Apr 13, 2026
99bcc50
Address Copilot review: remove redundant GetMappedConstraints checks,…
andrewraper-Sage Apr 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 32 additions & 34 deletions src/EFCore.Relational/Update/Internal/CommandBatchPreparer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,7 +681,9 @@ private void AddForeignKeyEdges()
if (!CanCreateDependency(foreignKey, command, principal: true)
|| !IsModified(foreignKey.PrincipalKey.Properties, entry)
|| (command.Table != null
&& !IsStoreGenerated(entry, foreignKey.PrincipalKey)))
&& !IsStoreGenerated(entry, foreignKey.PrincipalKey)
&& (foreignKey.DeclaringEntityType.IsMappedToJson()
|| !foreignKey.DeclaringEntityType.GetTableMappings().Any())))
{
continue;
}
Expand Down Expand Up @@ -726,31 +728,29 @@ private void AddForeignKeyEdges()
}
}
}
else

foreach (var entry in command.Entries)
{
foreach (var entry in command.Entries)
foreach (var foreignKey in entry.EntityType.GetForeignKeys())
{
foreach (var foreignKey in entry.EntityType.GetForeignKeys())
if (!CanCreateDependency(foreignKey, command, principal: false)
Comment on lines +731 to +736
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the else means the model-level FK scanning loops now run even when command.Table != null. Although CanCreateDependency() filters out most FKs with mapped constraints, this still adds per-command work on common SaveChanges paths. If possible, consider an early skip/guard so the model-level path only runs when there are unmapped constraints for the given command (to avoid unnecessary iteration on large graphs).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

@andrewraper-Sage andrewraper-Sage Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes a valid point, however, the added complexity likely doesn't trade off with the optimization that would be gained.

Copy link
Copy Markdown
Member

@AndriySvyryd AndriySvyryd Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not only about perf, removing the else check also muddles this, already complicated logic. Currently there are two modes in the method:

  1. command.Table != null is the common case
  2. command.Table == null is used when there is not relational model, which should only be the case sometimes when applying seed data changes in migrations.

So, to keep the logic clear only change the first case. There, you can add another loop that goes over the FKs that are not mapped and would be between two different tables if mapped. Something like:

foreach (var foreignKey in principal
                ? entry.EntityType.GetReferencingForeignKeys()
                : (IEnumerable<IForeignKey>)entry.EntityType.GetForeignKeys())
{
    if (!foreignKey.GetMappedConstraints().Any()
        || (principal ? foreignKey.DeclaringEntityType : foreignKey.PrincipalEntityType).GetTableMappings().Any(m => m.Table == command.Table))
    {
        return true;
    }

    ...
}

The logic inside would indeed look a lot like the logic for the case without a relational model, but instead of trying to force the execution path to use it you can extract it to a local method that's used from both places.

Same applies for the changes below

|| !IsModified(foreignKey.Properties, entry))
{
if (!CanCreateDependency(foreignKey, command, principal: false)
|| !IsModified(foreignKey.Properties, entry))
{
continue;
}
continue;
}

var dependentKeyValue = foreignKey.GetDependentKeyValueFactory()
?.CreateDependentEquatableKey(entry, fromOriginalValues: true);
var dependentKeyValue = foreignKey.GetDependentKeyValueFactory()
?.CreateDependentEquatableKey(entry, fromOriginalValues: true);

if (dependentKeyValue != null)
if (dependentKeyValue != null)
{
if (!originalPredecessorsMap.TryGetValue(dependentKeyValue, out var predecessorCommands))
{
if (!originalPredecessorsMap.TryGetValue(dependentKeyValue, out var predecessorCommands))
{
predecessorCommands = [];
originalPredecessorsMap.Add(dependentKeyValue, predecessorCommands);
}

predecessorCommands.Add(command);
predecessorCommands = [];
originalPredecessorsMap.Add(dependentKeyValue, predecessorCommands);
}

predecessorCommands.Add(command);
}
}
}
Expand Down Expand Up @@ -825,25 +825,23 @@ private void AddForeignKeyEdges()
originalPredecessorsMap, principalKeyValue, command, foreignKey);
}
}
else

// ReSharper disable once ForCanBeConvertedToForeach
for (var entryIndex = 0; entryIndex < command.Entries.Count; entryIndex++)
{
// ReSharper disable once ForCanBeConvertedToForeach
for (var entryIndex = 0; entryIndex < command.Entries.Count; entryIndex++)
var entry = command.Entries[entryIndex];
foreach (var foreignKey in entry.EntityType.GetReferencingForeignKeys())
{
var entry = command.Entries[entryIndex];
foreach (var foreignKey in entry.EntityType.GetReferencingForeignKeys())
if (!CanCreateDependency(foreignKey, command, principal: true))
{
if (!CanCreateDependency(foreignKey, command, principal: true))
{
continue;
}

var principalKeyValue = foreignKey.GetDependentKeyValueFactory()
.CreatePrincipalEquatableKey(entry, fromOriginalValues: true);
Check.DebugAssert(principalKeyValue != null, "null principalKeyValue");
AddMatchingPredecessorEdge(
originalPredecessorsMap, principalKeyValue, command, foreignKey);
continue;
}

var principalKeyValue = foreignKey.GetDependentKeyValueFactory()
.CreatePrincipalEquatableKey(entry, fromOriginalValues: true);
Check.DebugAssert(principalKeyValue != null, "null principalKeyValue");
AddMatchingPredecessorEdge(
originalPredecessorsMap, principalKeyValue, command, foreignKey);
}
}
}
Expand Down
86 changes: 86 additions & 0 deletions test/EFCore.Relational.Tests/Update/CommandBatchPreparerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1328,4 +1328,90 @@ private class AnotherFakeEntity
public int Id { get; set; }
public int? AnotherId { get; set; }
}

[ConditionalFact]
public void BatchCommands_sorts_added_entities_with_TPC_abstract_principal()
{
var configuration = CreateContextServices(CreateTpcFKModel());
var stateManager = configuration.GetRequiredService<IStateManager>();

var principalEntry = stateManager.GetOrCreateEntry(
new ConcretePrincipal { Id = 1 });
principalEntry.SetEntityState(EntityState.Added);

var dependentEntry = stateManager.GetOrCreateEntry(
new TpcDependent { Id = 1, PrincipalId = 1 });
dependentEntry.SetEntityState(EntityState.Added);

var modelData = new UpdateAdapter(stateManager);

var batches = CreateBatches([dependentEntry, principalEntry], modelData);
var batch = Assert.Single(batches);

Assert.Equal(
[principalEntry, dependentEntry],
batch.ModificationCommands.Select(c => c.Entries.Single()));
}

[ConditionalFact]
public void BatchCommands_sorts_deleted_entities_with_TPC_abstract_principal()
{
var configuration = CreateContextServices(CreateTpcFKModel());
var stateManager = configuration.GetRequiredService<IStateManager>();

var principalEntry = stateManager.GetOrCreateEntry(
new ConcretePrincipal { Id = 1 });
principalEntry.SetEntityState(EntityState.Deleted);

var dependentEntry = stateManager.GetOrCreateEntry(
new TpcDependent { Id = 1, PrincipalId = 1 });
dependentEntry.SetEntityState(EntityState.Deleted);

var modelData = new UpdateAdapter(stateManager);

var batches = CreateBatches([principalEntry, dependentEntry], modelData);
var batch = Assert.Single(batches);

Assert.Equal(
[dependentEntry, principalEntry],
batch.ModificationCommands.Select(c => c.Entries.Single()));
}

private static IModel CreateTpcFKModel()
{
var modelBuilder = FakeRelationalTestHelpers.Instance.CreateConventionBuilder();

modelBuilder.Entity<AbstractPrincipal>()
.UseTpcMappingStrategy()
.ToTable((string)null)
.Property(e => e.Id)
.ValueGeneratedNever();

modelBuilder.Entity<ConcretePrincipal>()
.ToTable(nameof(ConcretePrincipal));

modelBuilder.Entity<TpcDependent>(b =>
{
b.HasOne<AbstractPrincipal>()
.WithMany()
.HasForeignKey(c => c.PrincipalId);
});

return modelBuilder.Model.FinalizeModel();
}

private abstract class AbstractPrincipal
{
public int Id { get; set; }
}

private class ConcretePrincipal : AbstractPrincipal
{
}

private class TpcDependent
{
public int Id { get; set; }
public int PrincipalId { get; set; }
}
}
Loading