Skip to content

Commit

Permalink
Don't leave unknown FK values when principal is known but has not-set…
Browse files Browse the repository at this point in the history 
…, non-generated, value (#23875)

Fixes #23730
  • Loading branch information
@ajcvickers
ajcvickers committed Feb 17, 2021
1 parent cfd206a commit 2c4876d
Show file tree
Hide file tree
Showing 2 changed files with 123 additions and 16 deletions.
27 changes: 13 additions & 14 deletions src/EFCore/ChangeTracking/Internal/KeyPropagator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,14 @@ public virtual InternalEntityEntry PropagateValue(InternalEntityEntry entry, IPr
{
Check.DebugAssert(property.IsForeignKey(), $"property {property} is not part of an FK");

var principalEntry = TryPropagateValue(entry, property);
var generationProperty = property.FindGenerationProperty();
var principalEntry = TryPropagateValue(entry, property, generationProperty);

if (principalEntry == null
&& property.IsKey()
&& !property.IsForeignKeyToSelf())
{
var valueGenerator = TryGetValueGenerator(property);

var valueGenerator = TryGetValueGenerator(generationProperty);
if (valueGenerator != null)
{
var value = valueGenerator.Next(new EntityEntry(entry));
Expand Down Expand Up @@ -93,12 +94,13 @@ public virtual async Task<InternalEntityEntry> PropagateValueAsync(
{
Check.DebugAssert(property.IsForeignKey(), $"property {property} is not part of an FK");

var principalEntry = TryPropagateValue(entry, property);
var generationProperty = property.FindGenerationProperty();
var principalEntry = TryPropagateValue(entry, property, generationProperty);

if (principalEntry == null
&& property.IsKey())
{
var valueGenerator = TryGetValueGenerator(property);

var valueGenerator = TryGetValueGenerator(generationProperty);
if (valueGenerator != null)
{
var value = await valueGenerator.NextAsync(new EntityEntry(entry), cancellationToken)
Expand All @@ -120,7 +122,7 @@ public virtual async Task<InternalEntityEntry> PropagateValueAsync(
return principalEntry;
}

private static InternalEntityEntry TryPropagateValue(InternalEntityEntry entry, IProperty property)
private static InternalEntityEntry TryPropagateValue(InternalEntityEntry entry, IProperty property, IProperty generationProperty)
{
var entityType = entry.EntityType;
var stateManager = entry.StateManager;
Expand Down Expand Up @@ -158,7 +160,8 @@ private static InternalEntityEntry TryPropagateValue(InternalEntityEntry entry,
if (principalProperty != property)
{
var principalValue = principalEntry[principalProperty];
if (!principalProperty.ClrType.IsDefaultValue(principalValue))
if (generationProperty == null
|| !principalProperty.ClrType.IsDefaultValue(principalValue))
{
if (principalEntry.HasTemporaryValue(principalProperty))
{
Expand All @@ -182,13 +185,9 @@ private static InternalEntityEntry TryPropagateValue(InternalEntityEntry entry,
return null;
}

private ValueGenerator TryGetValueGenerator(IProperty property)
{
var generationProperty = property.GetGenerationProperty();

return generationProperty != null
private ValueGenerator TryGetValueGenerator(IProperty generationProperty)
=> generationProperty != null
? _valueGeneratorSelector.Select(generationProperty, generationProperty.DeclaringEntityType)
: null;
}
}
}
112 changes: 110 additions & 2 deletions test/EFCore.Tests/ChangeTracking/Internal/OwnedFixupTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.ComponentModel.DataAnnotations.Schema;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.EntityFrameworkCore.Diagnostics;
Expand Down Expand Up @@ -384,7 +385,17 @@ public void Add_principal_with_dependent_principal_nav(EntityState entityState,
var subDependentEntry = context.Entry(subDependent);
Assert.Equal(principal.Id, subDependentEntry.Property("ParentId").CurrentValue);
Assert.Equal(useTrackGraph == null ? EntityState.Added : entityState, subDependentEntry.State);
Assert.Equal(nameof(ChildPN.SubChild), subDependentEntry.Metadata.DefiningNavigationName);
Assert.Equal(
typeof(Parent).ShortDisplayName()
+ "."
+ nameof(Parent.Child1)
+ "#"
+ nameof(Child)
+ "."
+ nameof(Child.SubChild)
+ "#"
+ nameof(Child.SubChild),
subDependentEntry.Metadata.DisplayName());
});
}

Expand Down Expand Up @@ -2441,7 +2452,6 @@ public void Parent_swapped_bidirectional(EntityState entityState)
Assert.Equal(principal2.Id, dependent2Entry.Property("ParentId").CurrentValue);
Assert.Equal(entityState == EntityState.Added ? EntityState.Added : EntityState.Modified, dependent2Entry.State);
Assert.Equal(nameof(Parent.Child1), dependent2Entry.Metadata.DefiningNavigationName);

Assert.Same(subDependent1, dependent1.SubChild);
Assert.Same(dependent1, subDependent1.Parent);
var subDependentEntry1 = dependent1Entry.Reference(p => p.SubChild).TargetEntry;
Expand Down Expand Up @@ -2845,6 +2855,14 @@ public void Parent_and_identity_changed_bidirectional(EntityState entityState)
principal2.Child1 = dependent;
principal1.Child2 = null;

if (entityState != EntityState.Added)
{
Assert.Equal(
CoreStrings.KeyReadOnly("ParentId", "Parent.Child2#Child"),
Assert.Throws<InvalidOperationException>(() => context.ChangeTracker.DetectChanges()).Message);
return;
}

context.ChangeTracker.DetectChanges();

Assert.True(context.ChangeTracker.HasChanges());
Expand Down Expand Up @@ -3043,6 +3061,14 @@ public void Parent_and_identity_changed_bidirectional_collection(EntityState ent
principal2.ChildCollection1 = principal1.ChildCollection2;
principal1.ChildCollection2 = null;

if (entityState != EntityState.Added)
{
Assert.Equal(
CoreStrings.KeyReadOnly("ParentId", "Parent.ChildCollection2#Child"),
Assert.Throws<InvalidOperationException>(() => context.ChangeTracker.DetectChanges()).Message);
return;
}

context.ChangeTracker.DetectChanges();

Assert.True(context.ChangeTracker.HasChanges());
Expand Down Expand Up @@ -3212,6 +3238,14 @@ public void Parent_and_identity_swapped_bidirectional(EntityState entityState)
principal2.Child1 = dependent1;
principal1.Child2 = dependent2;

if (entityState != EntityState.Added)
{
Assert.Equal(
CoreStrings.KeyReadOnly("ParentId", "Parent.Child2#Child"),
Assert.Throws<InvalidOperationException>(() => context.ChangeTracker.DetectChanges()).Message);
return;
}

context.ChangeTracker.DetectChanges();

Assert.True(context.ChangeTracker.HasChanges());
Expand Down Expand Up @@ -3500,6 +3534,14 @@ public void Parent_and_identity_swapped_bidirectional_collection(EntityState ent
.FindEntry(subDependent2);
newSubDependentEntry2.Property<int>("Id").CurrentValue = subDependentEntry2.Property<int>("Id").CurrentValue;

if (entityState != EntityState.Added)
{
Assert.Equal(
CoreStrings.KeyReadOnly("ParentId", "Parent.ChildCollection2#Child"),
Assert.Throws<InvalidOperationException>(() => context.ChangeTracker.DetectChanges()).Message);
return;
}

context.ChangeTracker.DetectChanges();

Assert.True(context.ChangeTracker.HasChanges());
Expand Down Expand Up @@ -4211,6 +4253,72 @@ EntityState GetEntryState<TEntity>(EquatableEntitiesContext context, string role
}
}

[ConditionalTheory]
[InlineData(false)]
[InlineData(true)]
public async Task SaveChanges_when_owner_has_PK_with_default_values(bool async)
{
using (var context = new OneRowContext(async))
{
var blog = new Blog { Type = new OwnedType { Value = "A" } };

_ = async
? await context.AddAsync(blog)
: context.Add(blog);

Assert.Equal(EntityState.Added, context.Entry(blog).State);
Assert.Equal(EntityState.Added, context.Entry(blog.Type).State);
Assert.Equal(0, blog.Id);
Assert.Equal(0, context.Entry(blog.Type).Property<int>("BlogId").CurrentValue);

_ = async
? await context.SaveChangesAsync()
: context.SaveChanges();

Assert.Equal(EntityState.Unchanged, context.Entry(blog).State);
Assert.Equal(EntityState.Unchanged, context.Entry(blog.Type).State);
Assert.Equal(0, blog.Id);
Assert.Equal(0, context.Entry(blog.Type).Property<int>("BlogId").CurrentValue);
}

using (var context = new OneRowContext(async))
{
// Trying to do the same thing again will throw since only one row can have ID zero.

context.Add(new Blog { Type = new OwnedType { Value = "A" } });
Assert.Throws<ArgumentException>(() => context.SaveChanges());
}
}

private class OneRowContext : DbContext
{
private readonly bool _async;

public OneRowContext(bool async)
{
_async = async;
}

protected internal override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
=> optionsBuilder.UseInMemoryDatabase(nameof(OneRowContext) + _async);

public DbSet<Blog> Blogs { get; set; }
}

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

public OwnedType Type { get; set; }
}

[Owned]
public class OwnedType
{
public string Value { get; set; }
}

private class User
{
public Guid UserId { get; set; }
Expand Down

0 comments on commit 2c4876d

Please sign in to comment.