Skip to content

Commit

Permalink
Ensure that, by convention, dependent properties have the same elemen…
Browse files Browse the repository at this point in the history
…t type as principal properties (#32560)

* Ensure that, by convention, dependent properties have the same element type as principal properties

Fixes #32411

The issue here is that when a value converter is applied to a principal property, then that converter is used by the dependent properties unless something else is explicitly configured. However, this meant that when the PK has a converter but the FK does not, then the FK can get configured as a primitive collection, since it doesn't have a converter preventing this.

The fix is to add a convention that sets the element type for dependent properties to match that for principal properties.

* Update based on review
  • Loading branch information
ajcvickers authored Dec 9, 2023
1 parent d1124fa commit fd576b9
Show file tree
Hide file tree
Showing 6 changed files with 301 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ void Validate(IConventionTypeBase typeBase)
var typeMapping = Dependencies.TypeMappingSource.FindMapping((IProperty)property);
if (typeMapping is { ElementTypeMapping: not null })
{
property.SetElementType(property.ClrType.TryGetElementType(typeof(IEnumerable<>)));
property.Builder.SetElementType(property.ClrType.TryGetElementType(typeof(IEnumerable<>)));
}
}

Expand Down
58 changes: 58 additions & 0 deletions src/EFCore/Metadata/Conventions/ElementTypeChangedConvention.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace Microsoft.EntityFrameworkCore.Metadata.Conventions;

/// <summary>
/// A convention that reacts to changes made to element types of primitive collections.
/// </summary>
/// <remarks>
/// See <see href="https://aka.ms/efcore-docs-conventions">Model building conventions</see> for more information and examples.
/// </remarks>
public class ElementTypeChangedConvention : IPropertyElementTypeChangedConvention, IForeignKeyAddedConvention
{
/// <summary>
/// Creates a new instance of <see cref="ElementTypeChangedConvention" />.
/// </summary>
/// <param name="dependencies">Parameter object containing dependencies for this convention.</param>
public ElementTypeChangedConvention(ProviderConventionSetBuilderDependencies dependencies)
{
Dependencies = dependencies;
}

/// <summary>
/// Dependencies for this service.
/// </summary>
protected virtual ProviderConventionSetBuilderDependencies Dependencies { get; }

/// <inheritdoc />
public void ProcessPropertyElementTypeChanged(
IConventionPropertyBuilder propertyBuilder,
IElementType? newElementType,
IElementType? oldElementType,
IConventionContext<IElementType> context)
{
var keyProperty = propertyBuilder.Metadata;
foreach (var key in keyProperty.GetContainingKeys())
{
var index = key.Properties.IndexOf(keyProperty);
foreach (var foreignKey in key.GetReferencingForeignKeys())
{
var foreignKeyProperty = foreignKey.Properties[index];
foreignKeyProperty.Builder.SetElementType(newElementType?.ClrType);
}
}
}

/// <inheritdoc />
public void ProcessForeignKeyAdded(
IConventionForeignKeyBuilder foreignKeyBuilder, IConventionContext<IConventionForeignKeyBuilder> context)
{
var foreignKeyProperties = foreignKeyBuilder.Metadata.Properties;
var principalKeyProperties = foreignKeyBuilder.Metadata.PrincipalKey.Properties;
for (var i = 0; i < foreignKeyProperties.Count; i++)
{
foreignKeyProperties[i].Builder.SetElementType(principalKeyProperties[i].GetElementType()?.ClrType);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ public virtual ConventionSet CreateConventionSet()
conventionSet.Add(new QueryFilterRewritingConvention(Dependencies));
conventionSet.Add(new RuntimeModelConvention(Dependencies));
conventionSet.Add(new ElementMappingConvention(Dependencies));
conventionSet.Add(new ElementTypeChangedConvention(Dependencies));

return conventionSet;
}
Expand Down
11 changes: 11 additions & 0 deletions test/EFCore.Cosmos.FunctionalTests/KeysWithConvertersCosmosTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,10 @@ public override void Can_insert_and_read_back_with_bare_class_key_and_optional_d
public override void Can_insert_and_read_back_with_comparable_class_key_and_optional_dependents_with_shadow_FK()
=> base.Can_insert_and_read_back_with_comparable_class_key_and_optional_dependents_with_shadow_FK();

[ConditionalFact(Skip = "Issue=#26239")]
public override void Can_insert_and_read_back_with_enumerable_class_key_and_optional_dependents()
=> base.Can_insert_and_read_back_with_enumerable_class_key_and_optional_dependents();

public class KeysWithConvertersCosmosFixture : KeysWithConvertersFixtureBase
{
protected override ITestStoreFactory TestStoreFactory
Expand Down Expand Up @@ -537,6 +541,13 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
b.Property(e => e.Id).HasConversion(GenericComparableIntClassKey.Converter);
b.OwnsOne(e => e.Owned);
});

modelBuilder.Ignore<EnumerableClassKeyPrincipal>();
modelBuilder.Ignore<EnumerableClassKeyOptionalDependent>();
modelBuilder.Ignore<EnumerableClassKeyRequiredDependent>();
}

public override DbContextOptionsBuilder AddOptions(DbContextOptionsBuilder builder)
=> base.AddOptions(builder.ConfigureWarnings(w => w.Ignore(CoreEventId.MappedEntityTypeIgnoredWarning)));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,26 @@ public override void Can_query_and_update_owned_entity_with_value_converter()
public override void Can_query_and_update_owned_entity_with_int_bare_class_key()
=> base.Can_query_and_update_owned_entity_with_int_bare_class_key();

[ConditionalFact(Skip = "Issue #26238")]
public override void Can_insert_and_read_back_with_enumerable_class_key_and_optional_dependents()
=> base.Can_insert_and_read_back_with_enumerable_class_key_and_optional_dependents();

public class KeysWithConvertersInMemoryFixture : KeysWithConvertersFixtureBase
{
protected override ITestStoreFactory TestStoreFactory
=> InMemoryTestStoreFactory.Instance;

public override DbContextOptionsBuilder AddOptions(DbContextOptionsBuilder builder)
=> base.AddOptions(builder.ConfigureWarnings(w => w.Ignore(CoreEventId.MappedEntityTypeIgnoredWarning)));

protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext context)
{
base.OnModelCreating(modelBuilder, context);

// Issue #26238
modelBuilder.Ignore<EnumerableClassKeyPrincipal>();
modelBuilder.Ignore<EnumerableClassKeyOptionalDependent>();
modelBuilder.Ignore<EnumerableClassKeyRequiredDependent>();
}
}
}
Loading

0 comments on commit fd576b9

Please sign in to comment.