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.

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

Jump to bottom

Don't overwrite reference when querying for old principal #28395

Merged
merged 1 commit into from
Jul 9, 2022
Merged

Don't overwrite reference when querying for old principal #28395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 20 additions & 5 deletions src/EFCore/ChangeTracking/Internal/NavigationFixer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -781,6 +781,7 @@ private void InitialFixup(
{
var toDependent = foreignKey.PrincipalToDependent;
if (CanOverrideCurrentValue(entry, toDependent, dependentEntry, fromQuery)
&& (!fromQuery || CanOverrideCurrentValue(dependentEntry, foreignKey.DependentToPrincipal, entry, fromQuery))
&& !IsAmbiguous(dependentEntry))
{
SetNavigation(entry, toDependent, dependentEntry, fromQuery);
Expand All @@ -792,7 +793,8 @@ private void InitialFixup(
{
foreach (InternalEntityEntry dependentEntry in dependents)
{
if (!IsAmbiguous(dependentEntry))
if (!IsAmbiguous(dependentEntry)
&& (!fromQuery || CanOverrideCurrentValue(dependentEntry, foreignKey.DependentToPrincipal, entry, fromQuery)))
{
SetNavigation(dependentEntry, foreignKey.DependentToPrincipal, entry, fromQuery);
AddToCollection(entry, foreignKey.PrincipalToDependent, dependentEntry, fromQuery);
Expand Down Expand Up @@ -1440,14 +1442,27 @@ private static bool CanOverrideCurrentValue(
InternalEntityEntry value,
bool fromQuery)
{
if (fromQuery)
var existingValue = navigation == null ? null : entry[navigation];
if (existingValue == null
|| existingValue == value.Entity)
{
return true;
}

var existingValue = navigation == null ? null : entry[navigation];
return existingValue == null
|| existingValue == value.Entity;
if (!fromQuery)
{
return false;
}

var existingEntry = entry.StateManager.TryGetEntry(existingValue, throwOnNonUniqueness: false);
if (existingEntry == null)
{
return true;
}

SetForeignKeyProperties(entry, existingEntry, ((INavigation)navigation!).ForeignKey, setModified: true, fromQuery);

return false;
}

private void SetNavigation(InternalEntityEntry entry, INavigationBase? navigation, InternalEntityEntry? value, bool fromQuery)
Expand Down
98 changes: 98 additions & 0 deletions test/EFCore.Specification.Tests/LoadTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ public virtual void Attached_references_to_principal_are_marked_as_loaded(Entity
AlternateId = "Root",
SinglePkToPk = new SinglePkToPk { Id = 707 },
Single = new Single { Id = 21 },
RequiredSingle = new RequiredSingle { Id = 21 },
SingleAk = new SingleAk { Id = 42 },
SingleShadowFk = new SingleShadowFk { Id = 62 },
SingleCompositeKey = new SingleCompositeKey { Id = 62 }
Expand Down Expand Up @@ -78,6 +79,7 @@ public virtual void Attached_references_to_dependents_are_marked_as_loaded(Entit
AlternateId = "Root",
SinglePkToPk = new SinglePkToPk { Id = 707 },
Single = new Single { Id = 21 },
RequiredSingle = new RequiredSingle { Id = 21 },
SingleAk = new SingleAk { Id = 42 },
SingleShadowFk = new SingleShadowFk { Id = 62 },
SingleCompositeKey = new SingleCompositeKey { Id = 62 }
Expand All @@ -91,6 +93,7 @@ public virtual void Attached_references_to_dependents_are_marked_as_loaded(Entit

context.Entry(parent.SinglePkToPk).State = state;
context.Entry(parent.Single).State = state;
context.Entry(parent.RequiredSingle).State = state;
context.Entry(parent.SingleAk).State = state;
context.Entry(parent.SingleShadowFk).State = state;
context.Entry(parent.SingleCompositeKey).State = state;
Expand All @@ -101,6 +104,7 @@ public virtual void Attached_references_to_dependents_are_marked_as_loaded(Entit

Assert.True(context.Entry(parent.SinglePkToPk).Reference(e => e.Parent).IsLoaded);
Assert.True(context.Entry(parent.Single).Reference(e => e.Parent).IsLoaded);
Assert.True(context.Entry(parent.RequiredSingle).Reference(e => e.Parent).IsLoaded);
Assert.True(context.Entry(parent.SingleAk).Reference(e => e.Parent).IsLoaded);
Assert.True(context.Entry(parent.SingleShadowFk).Reference(e => e.Parent).IsLoaded);
Assert.True(context.Entry(parent.SingleCompositeKey).Reference(e => e.Parent).IsLoaded);
Expand Down Expand Up @@ -5798,12 +5802,69 @@ public virtual void Query_reference_to_dependent_using_string_for_detached_throw
Assert.Throws<InvalidOperationException>(() => referenceEntry.Query()).Message);
}

[ConditionalFact] // Issue #27497
public virtual void Fixup_reference_after_FK_change_without_DetectChanges()
{
using var context = CreateContext();

var child = context.Attach(new Child { Id = 274, ParentId = 707 }).Entity;
var newParent = context.Attach(new Parent { Id = 497 }).Entity;

child.Parent = newParent;

var oldParent = context.Set<Parent>().Single(e => e.Id == 707);

Assert.Same(newParent, child.Parent);
Assert.Equal(497, child.ParentId);
}

[ConditionalFact] // Issue #27497
public virtual void Fixup_one_to_one_reference_after_FK_change_without_DetectChanges()
{
using var context = CreateContext();

var child = context.Attach(new Single { Id = 274, ParentId = 707 }).Entity;
var newParent = context.Attach(new Parent { Id = 497 }).Entity;

child.Parent = newParent;

var oldParent = context.Set<Parent>().Single(e => e.Id == 707);

Assert.Same(newParent, child.Parent);
Assert.Equal(497, child.ParentId);
}

[ConditionalFact]
public virtual void Setting_navigation_to_null_is_detected_by_local_DetectChanges() // Issue #26937
{
using var context = CreateContext();
context.ChangeTracker.AutoDetectChangesEnabled = false;

var child = context.Attach(new RequiredSingle { Id = 274 }).Entity;
var newParent = new Parent { Id = 497 };
child.Parent = newParent;

var childEntry = context.Entry(child);
childEntry.DetectChanges();

Assert.Same(newParent, child.Parent);
Assert.Equal(newParent.Id, child.ParentId);
Assert.Equal(EntityState.Modified, childEntry.State);

child.Parent = null;
childEntry.DetectChanges();

Assert.Null(child.Parent);
Assert.Equal(EntityState.Deleted, childEntry.State);
}

protected class Parent
{
private readonly Action<object, string> _loader;
private IEnumerable<Child> _children;
private SinglePkToPk _singlePkToPk;
private Single _single;
private RequiredSingle _requiredSingle;
private IEnumerable<ChildAk> _childrenAk;
private SingleAk _singleAk;
private IEnumerable<ChildShadowFk> _childrenShadowFk;
Expand Down Expand Up @@ -5843,6 +5904,12 @@ public Single Single
set => _single = value;
}

public RequiredSingle RequiredSingle
{
get => _loader.Load(this, ref _requiredSingle);
set => _requiredSingle = value;
}

public IEnumerable<ChildAk> ChildrenAk
{
get => _loader.Load(this, ref _childrenAk);
Expand Down Expand Up @@ -5956,6 +6023,32 @@ public Parent Parent
}
}

protected class RequiredSingle
{
private readonly Action<object, string> _loader;
private Parent _parent;

public RequiredSingle()
{
}

public RequiredSingle(Action<object, string> lazyLoader)
{
_loader = lazyLoader;
}

[DatabaseGenerated(DatabaseGeneratedOption.None)]
public int Id { get; set; }

public int ParentId { get; set; }

public Parent Parent
{
get => _loader.Load(this, ref _parent);
set => _parent = value;
}
}

protected class ChildAk
{
private readonly Action<object, string> _loader;
Expand Down Expand Up @@ -6287,6 +6380,10 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
.WithOne(e => e.Parent)
.HasForeignKey<Single>(e => e.ParentId);

b.HasOne<RequiredSingle>(nameof(Parent.RequiredSingle))
.WithOne(e => e.Parent)
.HasForeignKey<RequiredSingle>(e => e.ParentId);

b.HasMany<ChildAk>(nameof(Parent.ChildrenAk))
.WithOne(e => e.Parent)
.HasPrincipalKey(e => e.AlternateId)
Expand Down Expand Up @@ -6341,6 +6438,7 @@ protected override void Seed(PoolableDbContext context)
Children = new List<Child> { new() { Id = 11 }, new() { Id = 12 } },
SinglePkToPk = new SinglePkToPk { Id = 707 },
Single = new Single { Id = 21 },
RequiredSingle = new RequiredSingle { Id = 21 },
ChildrenAk = new List<ChildAk> { new() { Id = 31 }, new() { Id = 32 } },
SingleAk = new SingleAk { Id = 42 },
ChildrenShadowFk = new List<ChildShadowFk> { new() { Id = 51 }, new() { Id = 52 } },
Expand Down
42 changes: 42 additions & 0 deletions test/EFCore.Tests/ChangeTracking/ChangeTrackerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2338,6 +2338,28 @@ public void Can_remove_dependent_identifying_one_to_many(bool saveEntities)
Assert.Empty(order.OrderDetails);
}

[ConditionalFact] // Issue #26827
public void Setting_dependent_to_null_for_client_cascaded_optional_is_not_overwritten_by_DetectChanges()
{
using var context = new EarlyLearningCenter();
var bobby = context.Add(new Bobby { Buggy = new() }).Entity;

context.SaveChanges();
context.ChangeTracker.Clear();

bobby = context.Set<Bobby>().Include(e => e.Buggy).Single(e => e.Id == bobby.Id);

Assert.NotNull(bobby.Buggy);

bobby.Buggy = null;

Assert.Null(bobby.Buggy);

context.ChangeTracker.DetectChanges();

Assert.Null(bobby.Buggy);
}

[ConditionalFact]
public void Keyless_type_negative_cases()
{
Expand Down Expand Up @@ -3703,6 +3725,17 @@ private class DependentGN
public PrincipalGN? PrincipalGN { get; set; }
}

public class Bobby
{
public int Id { get; set; }
public Buggy? Buggy { get; set; }
}

public class Buggy
{
public int Id { get; set; }
}

private class EarlyLearningCenter : DbContext
{
private readonly IInterceptor[] _interceptors;
Expand Down Expand Up @@ -3800,6 +3833,15 @@ protected internal override void OnModelCreating(ModelBuilder modelBuilder)
});

modelBuilder.Entity<DependentGN>().Property(e => e.Id).ValueGeneratedNever();

modelBuilder.Entity<Buggy>(entity =>
{
entity.Property<int?>("BobbyId");
entity.HasOne<Bobby>()
.WithOne(p => p.Buggy)
.HasForeignKey<Buggy>("BobbyId")
.OnDelete(DeleteBehavior.Cascade);
});
}

private class DummyValueGenerator : ValueGenerator<int>
Expand Down