Only discover complex types when configured through pre-convention mo…

…del configuration or annotation Throw for complex properties of value types Throw for optional complex properties Part of #13947 Fixes #31344
dotnet · Aug 6, 2023 · 8b69f8d · 8b69f8d
1 parent f9d827d
commit 8b69f8d
Show file tree

Hide file tree

Showing 13 changed files with 123 additions and 66 deletions.
diff --git a/src/EFCore.Relational/Metadata/Conventions/SharedTableConvention.cs b/src/EFCore.Relational/Metadata/Conventions/SharedTableConvention.cs
@@ -211,22 +211,22 @@ private static void TryUniquifyTableNames(
     }
 
     private static void TryUniquifyColumnNames(
-        IConventionEntityType entityType,
-        Dictionary<string, IConventionProperty> properties,
+        IConventionTypeBase type,
+        Dictionary<string, IConventionProperty> columns,
         in StoreObjectIdentifier storeObject,
         int maxLength)
     {
-        foreach (var property in entityType.GetDeclaredProperties())
+        foreach (var property in type.GetDeclaredProperties())
         {
             var columnName = property.GetColumnName(storeObject);
             if (columnName == null)
             {
                 continue;
             }
 
-            if (!properties.TryGetValue(columnName, out var otherProperty))
+            if (!columns.TryGetValue(columnName, out var otherProperty))
             {
-                properties[columnName] = property;
+                columns[columnName] = property;
                 continue;
             }
 
@@ -256,10 +256,10 @@ private static void TryUniquifyColumnNames(
                     && !otherProperty.DeclaringType.IsStrictlyDerivedFrom(property.DeclaringType))
                 || (property.DeclaringType as IConventionEntityType)?.FindRowInternalForeignKeys(storeObject).Any() == true)
             {
-                var newColumnName = TryUniquify(property, columnName, properties, storeObject, usePrefix, maxLength);
+                var newColumnName = TryUniquify(property, columnName, columns, storeObject, usePrefix, maxLength);
                 if (newColumnName != null)
                 {
-                    properties[newColumnName] = property;
+                    columns[newColumnName] = property;
                     continue;
                 }
             }
@@ -269,14 +269,19 @@ private static void TryUniquifyColumnNames(
                     && !otherProperty.DeclaringType.IsStrictlyDerivedFrom(property.DeclaringType))
                 || (otherProperty.DeclaringType as IConventionEntityType)?.FindRowInternalForeignKeys(storeObject).Any() == true)
             {
-                var newOtherColumnName = TryUniquify(otherProperty, columnName, properties, storeObject, usePrefix, maxLength);
+                var newOtherColumnName = TryUniquify(otherProperty, columnName, columns, storeObject, usePrefix, maxLength);
                 if (newOtherColumnName != null)
                 {
-                    properties[columnName] = property;
-                    properties[newOtherColumnName] = otherProperty;
+                    columns[columnName] = property;
+                    columns[newOtherColumnName] = otherProperty;
                 }
             }
         }
+
+        foreach (var complexPropery in type.GetDeclaredComplexProperties())
+        {
+            TryUniquifyColumnNames(complexPropery.ComplexType, columns, storeObject, maxLength);
+        }
     }
 
     private static string? TryUniquify(

diff --git a/src/EFCore/Infrastructure/ModelValidator.cs b/src/EFCore/Infrastructure/ModelValidator.cs
@@ -176,6 +176,19 @@ void Validate(IConventionTypeBase typeBase)
                         CoreStrings.ComplexPropertyCollection(typeBase.DisplayName(), complexProperty.Name));
                 }
 
+                if (complexProperty.IsNullable)
+                {
+                    throw new InvalidOperationException(
+                        CoreStrings.ComplexPropertyOptional(typeBase.DisplayName(), complexProperty.Name));
+                }
+
+                if (complexProperty.ComplexType.ClrType.IsValueType)
+                {
+                    throw new InvalidOperationException(
+                        CoreStrings.ValueComplexType(
+                            typeBase.DisplayName(), complexProperty.Name, complexProperty.ComplexType.ClrType.ShortDisplayName()));
+                }
+
                 if (complexProperty.ComplexType.GetMembers().Count() == 0)
                 {
                     throw new InvalidOperationException(

diff --git a/src/EFCore/Metadata/Builders/IConventionModelBuilder.cs b/src/EFCore/Metadata/Builders/IConventionModelBuilder.cs
@@ -189,7 +189,7 @@ public interface IConventionModelBuilder : IConventionAnnotatableBuilder
     /// <returns>
     ///     An <see cref="IConventionModelBuilder" /> to continue configuration if the annotation was set, <see langword="null" /> otherwise.
     /// </returns>
-    IConventionModelBuilder? Complex(
+    IConventionModelBuilder? ComplexType(
         [DynamicallyAccessedMembers(IEntityType.DynamicallyAccessedMemberTypes)] Type type,
         bool fromDataAnnotation = false);
 

diff --git a/src/EFCore/Metadata/Conventions/ComplexTypeAttributeConvention.cs b/src/EFCore/Metadata/Conventions/ComplexTypeAttributeConvention.cs
@@ -33,7 +33,7 @@ protected override void ProcessEntityTypeAdded(
         ComplexTypeAttribute attribute,
         IConventionContext<IConventionEntityTypeBuilder> context)
     {
-        entityTypeBuilder.Metadata.Model.Builder.Complex(entityTypeBuilder.Metadata.ClrType);
+        entityTypeBuilder.Metadata.Model.Builder.ComplexType(entityTypeBuilder.Metadata.ClrType);
 
         if (!entityTypeBuilder.Metadata.IsInModel)
         {

diff --git a/src/EFCore/Metadata/Internal/InternalModelBuilder.cs b/src/EFCore/Metadata/Internal/InternalModelBuilder.cs
@@ -937,7 +937,7 @@ IConventionModel IConventionModelBuilder.Metadata
     ///     doing so can result in application failures when updating to a new Entity Framework Core release.
     /// </summary>
     [DebuggerStepThrough]
-    IConventionModelBuilder? IConventionModelBuilder.Complex(Type type, bool fromDataAnnotation)
+    IConventionModelBuilder? IConventionModelBuilder.ComplexType(Type type, bool fromDataAnnotation)
         => Complex(type, fromDataAnnotation ? ConfigurationSource.DataAnnotation : ConfigurationSource.Convention);
 
     /// <summary>

diff --git a/src/EFCore/Metadata/Internal/InternalTypeBaseBuilder.cs b/src/EFCore/Metadata/Internal/InternalTypeBaseBuilder.cs
@@ -991,8 +991,6 @@ protected virtual void RemovePropertyIfUnused(Property property, ConfigurationSo
                 RemoveMembersInHierarchy(propertyName, configurationSource.Value);
             }
 
-            model.Builder.Complex(complexType!, configurationSource.Value);
-
             complexProperty = typeBase.AddComplexProperty(
                 propertyName, propertyType, memberInfo, complexTypeName, complexType!, collection.Value, configurationSource.Value)!;
 

diff --git a/src/EFCore/Properties/CoreStrings.Designer.cs b/src/EFCore/Properties/CoreStrings.Designer.cs
diff --git a/src/EFCore/Properties/CoreStrings.resx b/src/EFCore/Properties/CoreStrings.resx
@@ -288,6 +288,9 @@
   <data name="ComplexPropertyIndexer" xml:space="preserve">
     <value>Adding the complex property '{type}.{property}' as an indexer property isn't supported.</value>
   </data>
+  <data name="ComplexPropertyOptional" xml:space="preserve">
+    <value>Configuring the complex property '{type}.{property}' as optional is not supported, call 'IsRequired()'.</value>
+  </data>
   <data name="ComplexPropertyShadow" xml:space="preserve">
     <value>Configuring the complex property '{type}.{property}' in shadow state isn't supported.</value>
   </data>
@@ -1560,6 +1563,9 @@
   <data name="ValueCannotBeNull" xml:space="preserve">
     <value>The value for property '{1_entityType}.{0_property}' cannot be set to null because its type is '{propertyType}' which is not a nullable type.</value>
   </data>
+  <data name="ValueComplexType" xml:space="preserve">
+    <value>Adding the complex property '{type}.{property}' isn't supported because it's of a value type '{propertyType}'.</value>
+  </data>
   <data name="VisitIsNotAllowed" xml:space="preserve">
     <value>Calling '{visitMethodName}' is not allowed. Visit the expression manually for the relevant part in the visitor.</value>
   </data>

diff --git a/test/EFCore.Cosmos.FunctionalTests/ConfigPatternsCosmosTest.cs b/test/EFCore.Cosmos.FunctionalTests/ConfigPatternsCosmosTest.cs
@@ -88,7 +88,7 @@ public async Task Should_throw_if_specified_region_is_wrong()
             exception.Message);
     }
 
-    [ConditionalFact(Skip = "Issue Azure/azure-cosmos-dotnet-v3#3990")]
+    [ConditionalFact(Skip = "Issue #runtime/issues/89118")]
     public async Task Should_not_throw_if_specified_connection_mode_is_right()
     {
         var connectionMode = ConnectionMode.Direct;

diff --git a/test/EFCore.Design.Tests/Scaffolding/Internal/CSharpRuntimeModelCodeGeneratorTest.cs b/test/EFCore.Design.Tests/Scaffolding/Internal/CSharpRuntimeModelCodeGeneratorTest.cs
@@ -5322,8 +5322,7 @@ public static RuntimeComplexProperty Create(RuntimeComplexType declaringType)
                         "Microsoft.EntityFrameworkCore.Scaffolding.Internal.CSharpRuntimeModelCodeGeneratorTest+PrincipalBase.Owned#OwnedType.Principal#PrincipalBase",
                         typeof(CSharpRuntimeModelCodeGeneratorTest.PrincipalBase),
                         propertyInfo: typeof(CSharpRuntimeModelCodeGeneratorTest.OwnedType).GetProperty("Principal", BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly),
-                        fieldInfo: typeof(CSharpRuntimeModelCodeGeneratorTest.OwnedType).GetField("<Principal>k__BackingField", BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.DeclaredOnly),
-                        nullable: true);
+                        fieldInfo: typeof(CSharpRuntimeModelCodeGeneratorTest.OwnedType).GetField("<Principal>k__BackingField", BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.DeclaredOnly));
 
                     var complexType = complexProperty.ComplexType;
                     var alternateId = complexType.AddProperty(
@@ -5882,8 +5881,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
                                     .IsRowVersion()
                                     .HasAnnotation("foo", "bar");
                                 eb.Ignore(e => e.Context);
-                                eb.ComplexProperty(o => o.Principal)
-                                    ;
+                                eb.ComplexProperty(o => o.Principal).IsRequired();
                             });
 
                         eb.ToTable("PrincipalBase");

diff --git a/test/EFCore.Tests/ApiConsistencyTest.cs b/test/EFCore.Tests/ApiConsistencyTest.cs
@@ -158,7 +158,7 @@ protected override void Initialize()
             typeof(IConventionAnnotatable).GetMethod(nameof(IConventionAnnotatable.SetAnnotation)),
             typeof(IConventionAnnotatable).GetMethod(nameof(IConventionAnnotatable.SetOrRemoveAnnotation)),
             typeof(IConventionModelBuilder).GetMethod(nameof(IConventionModelBuilder.HasNoEntityType)),
-            typeof(IConventionModelBuilder).GetMethod(nameof(IConventionModelBuilder.Complex)),
+            typeof(IConventionModelBuilder).GetMethod(nameof(IConventionModelBuilder.ComplexType)),
             typeof(IReadOnlyEntityType).GetMethod(nameof(IReadOnlyEntityType.GetConcreteDerivedTypesInclusive)),
             typeof(IMutableEntityType).GetMethod(nameof(IMutableEntityType.AddData)),
             typeof(IReadOnlyNavigationBase).GetMethod("get_DeclaringEntityType"),

diff --git a/test/EFCore.Tests/ModelBuilding/ComplexTypeTestBase.cs b/test/EFCore.Tests/ModelBuilding/ComplexTypeTestBase.cs
@@ -32,7 +32,7 @@ public virtual void Can_set_complex_property_annotation()
             Assert.Equal("bar2", complexProperty["foo2"]);
             Assert.Equal(typeof(Customer).Name, complexProperty.Name);
             Assert.Equal("""
-Customer (Customer)
+Customer (Customer) Required
   ComplexType: ComplexProperties.Customer#Customer
     Properties: 
       AlternateKey (Guid) Required
@@ -1507,7 +1507,7 @@ public virtual void Can_ignore_a_field()
         }
 
         [ConditionalFact]
-        public virtual void Complex_properties_discovered_by_convention()
+        public virtual void Complex_properties_not_discovered_by_convention()
         {
             var modelBuilder = CreateModelBuilder();
 
@@ -1518,10 +1518,10 @@ public virtual void Complex_properties_discovered_by_convention()
                 .Entity<ComplexProperties>()
                 .ComplexProperty(e => e.Customer);
 
-            modelBuilder
-                .Entity<ValueComplexProperties>()
-                .Ignore(e => e.Tuple)
-                .ComplexProperty(e => e.Label);
+            //modelBuilder
+            //    .Entity<ValueComplexProperties>()
+            //    .Ignore(e => e.Tuple)
+            //    .ComplexProperty(e => e.Label);
 
             var model = modelBuilder.FinalizeModel();
 
@@ -1530,26 +1530,26 @@ public virtual void Complex_properties_discovered_by_convention()
             var indexedType = model.FindEntityType(typeof(ComplexProperties))
                 .FindComplexProperty(nameof(ComplexProperties.IndexedClass)).ComplexType;
 
-            var valueType = model.FindEntityType(typeof(ValueComplexProperties));
-            var labelProperty = valueType.FindComplexProperty(nameof(ValueComplexProperties.Label));
-            Assert.False(labelProperty.IsNullable);
-            Assert.Equal(typeof(ProductLabel), labelProperty.ClrType);
-            var labelType = labelProperty.ComplexType;
-            Assert.Equal(typeof(ProductLabel), labelType.ClrType);
-
-            var labelCustomerProperty = labelType.FindComplexProperty(nameof(ProductLabel.Customer));
-            Assert.True(labelCustomerProperty.IsNullable);
-            Assert.Equal(typeof(Customer), labelCustomerProperty.ClrType);
-
-            var oldLabelProperty = valueType.FindComplexProperty(nameof(ValueComplexProperties.OldLabel));
-            Assert.True(oldLabelProperty.IsNullable);
-            Assert.Equal(typeof(ProductLabel?), oldLabelProperty.ClrType);
-            var oldLabelType = oldLabelProperty.ComplexType;
-            Assert.Equal(typeof(ProductLabel), oldLabelType.ClrType);
-
-            var oldLabelCustomerProperty = labelType.FindComplexProperty(nameof(ProductLabel.Customer));
-            Assert.True(oldLabelCustomerProperty.IsNullable);
-            Assert.Equal(typeof(Customer), oldLabelCustomerProperty.ClrType);
+            //var valueType = model.FindEntityType(typeof(ValueComplexProperties));
+            //var labelProperty = valueType.FindComplexProperty(nameof(ValueComplexProperties.Label));
+            //Assert.False(labelProperty.IsNullable);
+            //Assert.Equal(typeof(ProductLabel), labelProperty.ClrType);
+            //var labelType = labelProperty.ComplexType;
+            //Assert.Equal(typeof(ProductLabel), labelType.ClrType);
+
+            //var labelCustomerProperty = labelType.FindComplexProperty(nameof(ProductLabel.Customer));
+            //Assert.True(labelCustomerProperty.IsNullable);
+            //Assert.Equal(typeof(Customer), labelCustomerProperty.ClrType);
+
+            //var oldLabelProperty = valueType.FindComplexProperty(nameof(ValueComplexProperties.OldLabel));
+            //Assert.True(oldLabelProperty.IsNullable);
+            //Assert.Equal(typeof(ProductLabel?), oldLabelProperty.ClrType);
+            //var oldLabelType = oldLabelProperty.ComplexType;
+            //Assert.Equal(typeof(ProductLabel), oldLabelType.ClrType);
+
+            //var oldLabelCustomerProperty = labelType.FindComplexProperty(nameof(ProductLabel.Customer));
+            //Assert.True(oldLabelCustomerProperty.IsNullable);
+            //Assert.Equal(typeof(Customer), oldLabelCustomerProperty.ClrType);
         }
 
         [ConditionalFact]
@@ -1571,7 +1571,22 @@ public virtual void Complex_properties_can_be_configured_by_type()
         }
 
         [ConditionalFact]
-        public virtual void Can_map_tuple()
+        public virtual void Throws_for_optional_complex_property()
+        {
+            var modelBuilder = CreateModelBuilder();
+
+            modelBuilder
+                .Entity<ComplexProperties>()
+                .ComplexProperty(e => e.Customer).IsRequired(false);
+
+            Assert.Equal(
+                CoreStrings.ComplexPropertyOptional(
+                    nameof(ComplexProperties), nameof(ComplexProperties.Customer)),
+                    Assert.Throws<InvalidOperationException>(modelBuilder.FinalizeModel).Message);
+        }
+
+        [ConditionalFact]
+        public virtual void Throws_for_tuple()
         {
             var modelBuilder = CreateModelBuilder();
 
@@ -1581,17 +1596,23 @@ public virtual void Can_map_tuple()
                 .Ignore(e => e.OldLabel)
                 .ComplexProperty(e => e.Tuple);
 
-            var model = modelBuilder.FinalizeModel();
-
-            var valueType = model.FindEntityType(typeof(ValueComplexProperties));
-            var tupleProperty = valueType.FindComplexProperty(nameof(ValueComplexProperties.Tuple));
-            Assert.False(tupleProperty.IsNullable);
-            Assert.Equal(typeof((string, int)), tupleProperty.ClrType);
-            var tupleType = tupleProperty.ComplexType;
-            Assert.Equal(typeof((string, int)), tupleType.ClrType);
-            Assert.Equal("ValueComplexProperties.Tuple#ValueTuple<string, int>", tupleType.DisplayName());
-
-            Assert.Equal(2, tupleType.GetProperties().Count());
+            Assert.Equal(
+                CoreStrings.ValueComplexType(
+                    nameof(ValueComplexProperties), nameof(ValueComplexProperties.Tuple), typeof((string, int)).ShortDisplayName()),
+                    Assert.Throws<InvalidOperationException>(modelBuilder.FinalizeModel).Message);
+
+            // Uncomment when value types are supported.
+            //var model = modelBuilder.FinalizeModel();
+
+            //var valueType = model.FindEntityType(typeof(ValueComplexProperties));
+            //var tupleProperty = valueType.FindComplexProperty(nameof(ValueComplexProperties.Tuple));
+            //Assert.False(tupleProperty.IsNullable);
+            //Assert.Equal(typeof((string, int)), tupleProperty.ClrType);
+            //var tupleType = tupleProperty.ComplexType;
+            //Assert.Equal(typeof((string, int)), tupleType.ClrType);
+            //Assert.Equal("ValueComplexProperties.Tuple#ValueTuple<string, int>", tupleType.DisplayName());
+
+            //Assert.Equal(2, tupleType.GetProperties().Count());
         }
 
         [ConditionalFact]

diff --git a/test/EFCore.Tests/ModelBuilding/TestModel.cs b/test/EFCore.Tests/ModelBuilding/TestModel.cs
@@ -855,19 +855,19 @@ protected interface IReplaceable
     protected class ComplexProperties
     {
         public int Id { get; set; }
-        public Customer? Customer { get; set; }
-        public DoubleProperty? DoubleProperty { get; set; }
-        public IndexedClass? IndexedClass { get; set; }
-        public Quarks? Quarks { get; set; }
+        public Customer Customer { get; set; } = null!;
+        public DoubleProperty DoubleProperty { get; set; } = null!;
+        public IndexedClass IndexedClass { get; set; } = null!;
+        public Quarks Quarks { get; set; } = null!;
 
         [NotMapped]
-        public DynamicProperty? DynamicProperty { get; set; }
+        public DynamicProperty DynamicProperty { get; set; } = null!;
 
         [NotMapped]
-        public EntityWithFields? EntityWithFields { get; set; }
+        public EntityWithFields EntityWithFields { get; set; } = null!;
 
         [NotMapped]
-        public WrappedStringEntity? WrappedStringEntity { get; set; }
+        public WrappedStringEntity WrappedStringEntity { get; set; } = null!;
     }
 
     protected class ValueComplexProperties