Stop shredding graph when FK value and navigation property don't agree

Fixes #15819 We had previously decided in #1246 on a behavior for resolving this, but that behavior results in not tracking the graph passed to us. On reflection, that decision was likely wrong, so reverting it here to avoid discarding entities in the graph.
dotnet · Jul 11, 2019 · 0599eda · 0599eda
1 parent 05b80c7
commit 0599eda
Show file tree

Hide file tree

Showing 3 changed files with 124 additions and 34 deletions.
diff --git a/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs b/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs
@@ -587,11 +587,17 @@ private void InitialFixup(
  var principalEntry = stateManager.FindPrincipal(entry, foreignKey);
  if (principalEntry != null)
  {
- // Set navigation to principal based on FK properties
- SetNavigation(entry, foreignKey.DependentToPrincipal, principalEntry);
+ var navigation = foreignKey.DependentToPrincipal;
+ var existingPrincipal = navigation == null ? null : entry[navigation];
+ if (existingPrincipal == null
+ || existingPrincipal == principalEntry.Entity)
+ {
+ // Set navigation to principal based on FK properties
+ SetNavigation(entry, navigation, principalEntry);
 
- // Add this entity to principal's collection, or set inverse for 1:1
- ToDependentFixup(entry, principalEntry, foreignKey);
+ // Add this entity to principal's collection, or set inverse for 1:1
+ ToDependentFixup(entry, principalEntry, foreignKey);
+ }
  }
  }
  }

diff --git a/test/EFCore.Tests/ChangeTracking/Internal/FixupTest.cs b/test/EFCore.Tests/ChangeTracking/Internal/FixupTest.cs
@@ -4,8 +4,11 @@
 using System;
 using System.Collections.Generic;
 using System.ComponentModel.DataAnnotations;
+using System.ComponentModel.DataAnnotations.Schema;
 using System.Linq;
+using Microsoft.EntityFrameworkCore.Diagnostics;
 using Microsoft.EntityFrameworkCore.Infrastructure;
+using Microsoft.EntityFrameworkCore.Metadata.Internal;
 using Microsoft.EntityFrameworkCore.Storage;
 using Xunit;
 
@@ -2750,6 +2753,64 @@ protected internal override void OnConfiguring(DbContextOptionsBuilder optionsBu
  public DbSet<Level2> Level2s { get; set; }
  }
 
+ [ConditionalFact]
+ public void Detached_entity_is_not_replaced_by_tracked_entity()
+ {
+ using (var context = new BadBeeContext(nameof(BadBeeContext)))
+ {
+ var b1 = new EntityB
+ {
+ EntityBId = 1
+ };
+ context.BEntities.Attach(b1);
+
+ var b2 = new EntityB
+ {
+ EntityBId = 1
+ };
+
+ var a = new EntityA
+ {
+ EntityAId = 1, EntityB = b2
+ };
+
+ Assert.Equal(
+ CoreStrings.IdentityConflict(
+ nameof(EntityB),
+ $"{{'{nameof(EntityB.EntityBId)}'}}"),
+ Assert.Throws<InvalidOperationException>(() => context.Add(a)).Message);
+ }
+ }
+
+ private class EntityB
+ {
+ public int EntityBId { get; set; }
+ public EntityA EntityA { get; set; }
+ }
+
+ private class EntityA
+ {
+ public int EntityAId { get; set; }
+ public int? EntityBId { get; set; }
+ public EntityB EntityB { get; set; }
+ }
+
+ private class BadBeeContext : DbContext
+ {
+ private readonly string _databaseName;
+
+ public BadBeeContext(string databaseName)
+ {
+ _databaseName = databaseName;
+ }
+
+ protected internal override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
+ => optionsBuilder.UseInMemoryDatabase(_databaseName);
+
+ public DbSet<EntityA> AEntities { get; set; }
+ public DbSet<EntityB> BEntities { get; set; }
+ }
+
  protected virtual void AssertAllFixedUp(DbContext context)
  {
  foreach (var entry in context.ChangeTracker.Entries<Product>())

diff --git a/test/EFCore.Tests/DbContextTrackingTest.cs b/test/EFCore.Tests/DbContextTrackingTest.cs
@@ -1498,32 +1498,37 @@ public void Can_attach_with_inconsistent_FK_dependent_first_fully_fixed_up_with_
  Id = 1,
  Name = "Beverages"
  };
+
  var product = new Product
  {
  Id = 1,
  CategoryId = 7,
  Name = "Marmite",
  Category = category
  };
+
  category.Products = new List<Product>
  {
  product
  };
 
  context.Attach(product);
 
- Assert.Equal(7, product.CategoryId);
+ Assert.Equal(1, product.CategoryId);
  Assert.Same(product, category.Products.Single());
- Assert.Same(category7, product.Category);
- Assert.Same(product, category7.Products.Single());
- Assert.Equal(EntityState.Detached, context.Entry(category).State);
+ Assert.Same(category, product.Category);
+ Assert.Empty(category7.Products);
+ Assert.Equal(EntityState.Unchanged, context.Entry(category7).State);
+ Assert.Equal(EntityState.Unchanged, context.Entry(category).State);
  Assert.Equal(EntityState.Unchanged, context.Entry(product).State);
 
  context.Attach(category);
 
  Assert.Equal(1, product.CategoryId);
  Assert.Same(product, category.Products.Single());
  Assert.Same(category, product.Category);
+ Assert.Empty(category7.Products);
+ Assert.Equal(EntityState.Unchanged, context.Entry(category7).State);
  Assert.Equal(EntityState.Unchanged, context.Entry(category).State);
  Assert.Equal(EntityState.Unchanged, context.Entry(product).State);
  }
@@ -1546,30 +1551,34 @@ public void Can_attach_with_inconsistent_FK_principal_first_collection_not_fixed
  Id = 1,
  Name = "Beverages"
  };
+
  var product = new Product
  {
  Id = 1,
  CategoryId = 7,
  Name = "Marmite",
  Category = category
  };
+
  category.Products = new List<Product>();
 
  context.Attach(category);
 
  Assert.Equal(7, product.CategoryId);
  Assert.Empty(category.Products);
  Assert.Same(category, product.Category);
- Assert.Empty(category7.Products);
+ Assert.Empty(category.Products);
+ Assert.Equal(EntityState.Unchanged, context.Entry(category7).State);
  Assert.Equal(EntityState.Unchanged, context.Entry(category).State);
  Assert.Equal(EntityState.Detached, context.Entry(product).State);
 
  context.Attach(product);
 
- Assert.Equal(7, product.CategoryId);
- Assert.Empty(category.Products);
- Assert.Same(category7, product.Category);
- Assert.Same(product, category7.Products.Single());
+ Assert.Equal(1, product.CategoryId);
+ Assert.Same(product, category.Products.Single());
+ Assert.Same(category, product.Category);
+ Assert.Empty(category7.Products);
+ Assert.Equal(EntityState.Unchanged, context.Entry(category7).State);
  Assert.Equal(EntityState.Unchanged, context.Entry(category).State);
  Assert.Equal(EntityState.Unchanged, context.Entry(product).State);
  }
@@ -1592,30 +1601,34 @@ public void Can_attach_with_inconsistent_FK_dependent_first_collection_not_fixed
  Id = 1,
  Name = "Beverages"
  };
+
  var product = new Product
  {
  Id = 1,
  CategoryId = 7,
  Name = "Marmite",
  Category = category
  };
+
  category.Products = new List<Product>();
 
  context.Attach(product);
 
- Assert.Equal(7, product.CategoryId);
- Assert.Empty(category.Products);
- Assert.Same(category7, product.Category);
- Assert.Same(product, category7.Products.Single());
- Assert.Equal(EntityState.Detached, context.Entry(category).State);
+ Assert.Equal(1, product.CategoryId);
+ Assert.Same(product, category.Products.Single());
+ Assert.Same(category, product.Category);
+ Assert.Empty(category7.Products);
+ Assert.Equal(EntityState.Unchanged, context.Entry(category7).State);
+ Assert.Equal(EntityState.Unchanged, context.Entry(category).State);
  Assert.Equal(EntityState.Unchanged, context.Entry(product).State);
 
  context.Attach(category);
 
- Assert.Equal(7, product.CategoryId);
- Assert.Empty(category.Products);
- Assert.Same(category7, product.Category);
- Assert.Same(product, category7.Products.Single());
+ Assert.Equal(1, product.CategoryId);
+ Assert.Same(product, category.Products.Single());
+ Assert.Same(category, product.Category);
+ Assert.Empty(category7.Products);
+ Assert.Equal(EntityState.Unchanged, context.Entry(category7).State);
  Assert.Equal(EntityState.Unchanged, context.Entry(category).State);
  Assert.Equal(EntityState.Unchanged, context.Entry(product).State);
  }
@@ -1782,13 +1795,15 @@ public void Can_set_set_to_Unchanged_with_inconsistent_FK_dependent_first_fully_
  Id = 1,
  Name = "Beverages"
  };
+
  var product = new Product
  {
  Id = 1,
  CategoryId = 7,
  Name = "Marmite",
  Category = category
  };
+
  category.Products = new List<Product>
  {
  product
@@ -1798,8 +1813,9 @@ public void Can_set_set_to_Unchanged_with_inconsistent_FK_dependent_first_fully_
 
  Assert.Equal(7, product.CategoryId);
  Assert.Same(product, category.Products.Single());
- Assert.Same(category7, product.Category);
- Assert.Same(product, category7.Products.Single());
+ Assert.Same(category, product.Category);
+ Assert.Empty(category7.Products);
+ Assert.Equal(EntityState.Unchanged, context.Entry(category7).State);
  Assert.Equal(EntityState.Detached, context.Entry(category).State);
  Assert.Equal(EntityState.Unchanged, context.Entry(product).State);
 
@@ -1808,6 +1824,8 @@ public void Can_set_set_to_Unchanged_with_inconsistent_FK_dependent_first_fully_
  Assert.Equal(1, product.CategoryId);
  Assert.Same(product, category.Products.Single());
  Assert.Same(category, product.Category);
+ Assert.Empty(category7.Products);
+ Assert.Equal(EntityState.Unchanged, context.Entry(category7).State);
  Assert.Equal(EntityState.Unchanged, context.Entry(category).State);
  Assert.Equal(EntityState.Unchanged, context.Entry(product).State);
  }
@@ -1850,10 +1868,11 @@ public void Can_set_set_to_Unchanged_with_inconsistent_FK_principal_first_collec
 
  context.Entry(product).State = EntityState.Unchanged;
 
- Assert.Equal(7, product.CategoryId);
- Assert.Empty(category.Products);
- Assert.Same(category7, product.Category);
- Assert.Same(product, category7.Products.Single());
+ Assert.Equal(1, product.CategoryId);
+ Assert.Same(product, category.Products.Single());
+ Assert.Same(category, product.Category);
+ Assert.Empty(category7.Products);
+ Assert.Equal(EntityState.Unchanged, context.Entry(category7).State);
  Assert.Equal(EntityState.Unchanged, context.Entry(category).State);
  Assert.Equal(EntityState.Unchanged, context.Entry(product).State);
  }
@@ -1876,30 +1895,34 @@ public void Can_set_set_to_Unchanged_with_inconsistent_FK_dependent_first_collec
  Id = 1,
  Name = "Beverages"
  };
+
  var product = new Product
  {
  Id = 1,
  CategoryId = 7,
  Name = "Marmite",
  Category = category
  };
+
  category.Products = new List<Product>();
 
  context.Entry(product).State = EntityState.Unchanged;
 
  Assert.Equal(7, product.CategoryId);
  Assert.Empty(category.Products);
- Assert.Same(category7, product.Category);
- Assert.Same(product, category7.Products.Single());
+ Assert.Same(category, product.Category);
+ Assert.Empty(category7.Products);
  Assert.Equal(EntityState.Detached, context.Entry(category).State);
  Assert.Equal(EntityState.Unchanged, context.Entry(product).State);
+ Assert.Equal(EntityState.Unchanged, context.Entry(category7).State);
 
  context.Entry(category).State = EntityState.Unchanged;
 
- Assert.Equal(7, product.CategoryId);
- Assert.Empty(category.Products);
- Assert.Same(category7, product.Category);
- Assert.Same(product, category7.Products.Single());
+ Assert.Equal(1, product.CategoryId);
+ Assert.Same(product, category.Products.Single());
+ Assert.Same(category, product.Category);
+ Assert.Empty(category7.Products);
+ Assert.Equal(EntityState.Unchanged, context.Entry(category7).State);
  Assert.Equal(EntityState.Unchanged, context.Entry(category).State);
  Assert.Equal(EntityState.Unchanged, context.Entry(product).State);
  }