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.

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

Mark owned collections composite key properties to use generated values even after the first two usages #21042

Merged
merged 1 commit into from
Jun 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 1 addition & 5 deletions src/EFCore.InMemory/Storage/Internal/InMemoryTable.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
using System.Linq;
using JetBrains.Annotations;
using Microsoft.EntityFrameworkCore.ChangeTracking;
using Microsoft.EntityFrameworkCore.ChangeTracking.Internal;
using Microsoft.EntityFrameworkCore.InMemory.Internal;
using Microsoft.EntityFrameworkCore.InMemory.ValueGeneration.Internal;
using Microsoft.EntityFrameworkCore.Metadata;
Expand All @@ -26,7 +25,6 @@ namespace Microsoft.EntityFrameworkCore.InMemory.Storage.Internal
/// </summary>
public class InMemoryTable<TKey> : IInMemoryTable
{
// WARNING: The in-memory provider is using EF internal code here. This should not be copied by other providers. See #15096
private readonly IPrincipalKeyValueFactory<TKey> _keyValueFactory;
private readonly bool _sensitiveLoggingEnabled;
private readonly Dictionary<TKey, object[]> _rows;
Expand All @@ -45,7 +43,6 @@ public InMemoryTable([NotNull] IEntityType entityType, [CanBeNull] IInMemoryTabl
{
EntityType = entityType;
BaseTable = baseTable;
// WARNING: The in-memory provider is using EF internal code here. This should not be copied by other providers. See #15096
_keyValueFactory = entityType.FindPrimaryKey().GetPrincipalKeyValueFactory<TKey>();
_sensitiveLoggingEnabled = sensitiveLoggingEnabled;
_rows = new Dictionary<TKey, object[]>(_keyValueFactory.EqualityComparer);
Expand Down Expand Up @@ -314,9 +311,8 @@ public virtual void BumpValueGenerators(object[] row)
}
}

// WARNING: The in-memory provider is using EF internal code here. This should not be copied by other providers. See #15096
private TKey CreateKey(IUpdateEntry entry)
=> _keyValueFactory.CreateFromCurrentValues((InternalEntityEntry)entry);
=> _keyValueFactory.CreateFromCurrentValues(entry);

private static object SnapshotValue(IProperty property, ValueComparer comparer, IUpdateEntry entry)
{
Expand Down
9 changes: 4 additions & 5 deletions src/EFCore.Relational/Update/Internal/CommandBatchPreparer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
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;
Expand Down Expand Up @@ -512,7 +511,7 @@ private Dictionary<IKeyValueIndex, List<ModificationCommand>> CreateKeyValuePred

var principalKeyValue = _keyValueIndexFactorySource
.GetKeyValueIndexFactory(foreignKey.PrincipalKey)
.CreatePrincipalKeyValue((InternalEntityEntry)entry, foreignKey);
.CreatePrincipalKeyValue(entry, foreignKey);

if (principalKeyValue != null)
{
Expand Down Expand Up @@ -547,7 +546,7 @@ private Dictionary<IKeyValueIndex, List<ModificationCommand>> CreateKeyValuePred

var dependentKeyValue = _keyValueIndexFactorySource
.GetKeyValueIndexFactory(foreignKey.PrincipalKey)
.CreateDependentKeyValueFromOriginalValues((InternalEntityEntry)entry, foreignKey);
.CreateDependentKeyValueFromOriginalValues(entry, foreignKey);

if (dependentKeyValue != null)
{
Expand Down Expand Up @@ -594,7 +593,7 @@ private void AddForeignKeyEdges(

var dependentKeyValue = _keyValueIndexFactorySource
.GetKeyValueIndexFactory(foreignKey.PrincipalKey)
.CreateDependentKeyValue((InternalEntityEntry)entry, foreignKey);
.CreateDependentKeyValue(entry, foreignKey);
if (dependentKeyValue == null)
{
continue;
Expand Down Expand Up @@ -622,7 +621,7 @@ private void AddForeignKeyEdges(

var principalKeyValue = _keyValueIndexFactorySource
.GetKeyValueIndexFactory(foreignKey.PrincipalKey)
.CreatePrincipalKeyValueFromOriginalValues((InternalEntityEntry)entry, foreignKey);
.CreatePrincipalKeyValueFromOriginalValues(entry, foreignKey);
if (principalKeyValue != null)
{
AddMatchingPredecessorEdge(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using JetBrains.Annotations;
using Microsoft.EntityFrameworkCore.ChangeTracking.Internal;
using Microsoft.EntityFrameworkCore.Metadata;

namespace Microsoft.EntityFrameworkCore.Update.Internal
Expand All @@ -21,30 +20,30 @@ 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] InternalEntityEntry entry, [NotNull] IForeignKey foreignKey);
IKeyValueIndex CreatePrincipalKeyValue([NotNull] IUpdateEntry entry, [NotNull] 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] InternalEntityEntry entry, [NotNull] IForeignKey foreignKey);
IKeyValueIndex CreatePrincipalKeyValueFromOriginalValues([NotNull] IUpdateEntry entry, [NotNull] 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 CreateDependentKeyValue([NotNull] InternalEntityEntry entry, [NotNull] IForeignKey foreignKey);
IKeyValueIndex CreateDependentKeyValue([NotNull] IUpdateEntry entry, [NotNull] 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 CreateDependentKeyValueFromOriginalValues([NotNull] InternalEntityEntry entry, [NotNull] IForeignKey foreignKey);
IKeyValueIndex CreateDependentKeyValueFromOriginalValues([NotNull] IUpdateEntry entry, [NotNull] IForeignKey foreignKey);
}
}
9 changes: 4 additions & 5 deletions src/EFCore.Relational/Update/Internal/KeyValueIndexFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

using JetBrains.Annotations;
using Microsoft.EntityFrameworkCore.ChangeTracking;
using Microsoft.EntityFrameworkCore.ChangeTracking.Internal;
using Microsoft.EntityFrameworkCore.Metadata;

namespace Microsoft.EntityFrameworkCore.Update.Internal
Expand Down Expand Up @@ -33,7 +32,7 @@ public KeyValueIndexFactory([NotNull] IPrincipalKeyValueFactory<TKey> principalK
/// 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>
public virtual IKeyValueIndex CreatePrincipalKeyValue(InternalEntityEntry entry, IForeignKey foreignKey)
public virtual IKeyValueIndex CreatePrincipalKeyValue(IUpdateEntry entry, IForeignKey foreignKey)
=> new KeyValueIndex<TKey>(
foreignKey,
_principalKeyValueFactory.CreateFromCurrentValues(entry),
Expand All @@ -46,7 +45,7 @@ public virtual IKeyValueIndex CreatePrincipalKeyValue(InternalEntityEntry entry,
/// 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>
public virtual IKeyValueIndex CreatePrincipalKeyValueFromOriginalValues(InternalEntityEntry entry, IForeignKey foreignKey)
public virtual IKeyValueIndex CreatePrincipalKeyValueFromOriginalValues(IUpdateEntry entry, IForeignKey foreignKey)
=> new KeyValueIndex<TKey>(
foreignKey,
_principalKeyValueFactory.CreateFromOriginalValues(entry),
Expand All @@ -59,7 +58,7 @@ public virtual IKeyValueIndex CreatePrincipalKeyValueFromOriginalValues(Internal
/// 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>
public virtual IKeyValueIndex CreateDependentKeyValue(InternalEntityEntry entry, IForeignKey foreignKey)
public virtual IKeyValueIndex CreateDependentKeyValue(IUpdateEntry entry, IForeignKey foreignKey)
=> foreignKey.GetDependentKeyValueFactory<TKey>().TryCreateFromCurrentValues(entry, out var keyValue)
? new KeyValueIndex<TKey>(foreignKey, keyValue, _principalKeyValueFactory.EqualityComparer, fromOriginalValues: false)
: null;
Expand All @@ -70,7 +69,7 @@ public virtual IKeyValueIndex CreateDependentKeyValue(InternalEntityEntry entry,
/// 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>
public virtual IKeyValueIndex CreateDependentKeyValueFromOriginalValues(InternalEntityEntry entry, IForeignKey foreignKey)
public virtual IKeyValueIndex CreateDependentKeyValueFromOriginalValues(IUpdateEntry entry, IForeignKey foreignKey)
=> foreignKey.GetDependentKeyValueFactory<TKey>().TryCreateFromOriginalValues(entry, out var keyValue)
? new KeyValueIndex<TKey>(foreignKey, keyValue, _principalKeyValueFactory.EqualityComparer, fromOriginalValues: true)
: null;
Expand Down
3 changes: 2 additions & 1 deletion src/EFCore.Relational/Update/ModificationCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ private IReadOnlyList<ColumnModification> GenerateColumnModifications()
else if ((updating && property.GetAfterSaveBehavior() == PropertySaveBehavior.Save)
|| (!isKey && nonMainEntry))
{
writeValue = columnPropagator?.TryPropagate(property, (InternalEntityEntry)entry)
writeValue = columnPropagator?.TryPropagate(property, entry)
?? entry.IsModified(property);
}
}
Expand Down Expand Up @@ -457,6 +457,7 @@ public bool TryPropagate(IProperty property, IUpdateEntry entry)
|| (entry.EntityState == EntityState.Modified && !entry.IsModified(property))
|| (entry.EntityState == EntityState.Added && Equals(_originalValue, entry.GetCurrentValue(property)))))
{
// Should be `entry.SetStoreGeneratedValue(property, _currentValue);` but see issue #21041
((InternalEntityEntry)entry)[property] = _currentValue;

return false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ public virtual ConventionSet CreateConventionSet()
conventionSet.ForeignKeyOwnershipChangedConventions.Add(new NavigationEagerLoadingConvention(Dependencies));
conventionSet.ForeignKeyOwnershipChangedConventions.Add(keyDiscoveryConvention);
conventionSet.ForeignKeyOwnershipChangedConventions.Add(relationshipDiscoveryConvention);
conventionSet.ForeignKeyOwnershipChangedConventions.Add(valueGeneratorConvention);

conventionSet.ModelInitializedConventions.Add(new DbSetFindingConvention(Dependencies));

Expand Down
17 changes: 16 additions & 1 deletion src/EFCore/Metadata/Conventions/ValueGenerationConvention.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ public class ValueGenerationConvention :
IForeignKeyAddedConvention,
IForeignKeyRemovedConvention,
IForeignKeyPropertiesChangedConvention,
IEntityTypeBaseTypeChangedConvention
IEntityTypeBaseTypeChangedConvention,
IForeignKeyOwnershipChangedConvention
{
/// <summary>
/// Creates a new instance of <see cref="ValueGenerationConvention" />.
Expand Down Expand Up @@ -205,5 +206,19 @@ private static bool CanBeGenerated(IProperty property)
&& propertyType != typeof(byte))
|| propertyType == typeof(Guid);
}

/// <summary>
/// Called after the ownership value for a foreign key is changed.
/// </summary>
/// <param name="relationshipBuilder"> The builder for the foreign key. </param>
/// <param name="context"> Additional information associated with convention execution. </param>
public virtual void ProcessForeignKeyOwnershipChanged(
IConventionForeignKeyBuilder relationshipBuilder, IConventionContext<bool?> context)
{
foreach (var property in relationshipBuilder.Metadata.DeclaringEntityType.GetProperties())
{
property.Builder.ValueGenerated(GetValueGenerated(property));
}
}
}
}
98 changes: 98 additions & 0 deletions test/EFCore.Tests/Metadata/Internal/EntityTypeTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2266,6 +2266,104 @@ protected internal override void OnModelCreating(ModelBuilder modelBuilder)
}
}

[ConditionalFact]
public void Indexes_for_owned_collection_types_are_calculated_correctly()
{
using var context = new SideBySide();
var model = context.Model;

var parent = model.FindEntityType(typeof(Parent1Entity));
var indexes = GetIndexes(parent.GetPropertiesAndNavigations());
Assert.Equal(2, indexes.Count);
// Order: Index, Shadow, Original, StoreGenerated, Relationship
Assert.Equal((0, -1, 0, 0, 0), indexes[nameof(Parent1Entity.Id)]);
Assert.Equal((0, -1, -1, -1, 1), indexes[nameof(Parent1Entity.Children)]);

indexes = GetIndexes(model.FindEntityType(typeof(ChildEntity), nameof(Parent1Entity.Children), parent).GetProperties());
Assert.Equal(3, indexes.Count);
// Order: Index, Shadow, Original, StoreGenerated, Relationship
Assert.Equal((0, 0, 0, 0, 0), indexes[nameof(Parent1Entity) + "Id"]);
Assert.Equal((1, 1, 1, 1, 1), indexes["Id"]);
Assert.Equal((2, -1, 2, -1, -1), indexes[nameof(ChildEntity.Name)]);

parent = model.FindEntityType(typeof(Parent2Entity));
indexes = GetIndexes(parent.GetPropertiesAndNavigations());
Assert.Equal(2, indexes.Count);
// Order: Index, Shadow, Original, StoreGenerated, Relationship
Assert.Equal((0, -1, 0, 0, 0), indexes[nameof(Parent2Entity.Id)]);
Assert.Equal((0, -1, -1, -1, 1), indexes[nameof(Parent2Entity.Children)]);

indexes = GetIndexes(model.FindEntityType(typeof(ChildEntity), nameof(Parent2Entity.Children), parent).GetProperties());
Assert.Equal(3, indexes.Count);
// Order: Index, Shadow, Original, StoreGenerated, Relationship
Assert.Equal((0, 0, 0, 0, 0), indexes[nameof(Parent2Entity) + "Id"]);
Assert.Equal((1, 1, 1, 1, 1), indexes["Id"]);
Assert.Equal((2, -1, 2, -1, -1), indexes[nameof(ChildEntity.Name)]);

parent = model.FindEntityType(typeof(Parent3Entity));
indexes = GetIndexes(parent.GetPropertiesAndNavigations());
Assert.Equal(2, indexes.Count);
// Order: Index, Shadow, Original, StoreGenerated, Relationship
Assert.Equal((0, -1, 0, 0, 0), indexes[nameof(Parent3Entity.Id)]);
Assert.Equal((0, -1, -1, -1, 1), indexes[nameof(Parent3Entity.Children)]);

indexes = GetIndexes(model.FindEntityType(typeof(ChildEntity), nameof(Parent3Entity.Children), parent).GetProperties());
Assert.Equal(3, indexes.Count);
// Order: Index, Shadow, Original, StoreGenerated, Relationship
Assert.Equal((0, 0, 0, 0, 0), indexes[nameof(Parent3Entity) + "Id"]);
Assert.Equal((1, 1, 1, 1, 1), indexes["Id"]);
Assert.Equal((2, -1, 2, -1, -1), indexes[nameof(ChildEntity.Name)]);

Dictionary<string, (int, int, int, int, int)> GetIndexes(IEnumerable<IPropertyBase> properties)
=> properties.ToDictionary(
p => p.Name,
p =>
(p.GetIndex(),
p.GetShadowIndex(),
p.GetOriginalValueIndex(),
p.GetStoreGeneratedIndex(),
p.GetRelationshipIndex()
));
}

private class SideBySide : DbContext
{
protected internal override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
=> optionsBuilder
.UseInternalServiceProvider(InMemoryFixture.DefaultServiceProvider)
.UseInMemoryDatabase(Guid.NewGuid().ToString());

protected internal override void OnModelCreating(ModelBuilder modelBuilder)
{
modelBuilder.Entity<Parent1Entity>().OwnsMany(e => e.Children);
modelBuilder.Entity<Parent2Entity>().OwnsMany(e => e.Children);
modelBuilder.Entity<Parent3Entity>().OwnsMany(e => e.Children);
}
}

private class Parent1Entity
{
public Guid Id { get; set; }
public ICollection<ChildEntity> Children { get; set; }
}

private class Parent2Entity
{
public Guid Id { get; set; }
public ICollection<ChildEntity> Children { get; set; }
}

private class Parent3Entity
{
public Guid Id { get; set; }
public ICollection<ChildEntity> Children { get; set; }
}

private class ChildEntity
{
public string Name { get; set; }
}

[ConditionalFact]
public void Indexes_are_ordered_by_property_count_then_property_names()
{
Expand Down