Don't add discriminators for embedded entities

Allow to update entities without discriminators
Don't try to add the default discriminator if a conflicting property already exists

Fixes #17764
Fixes #17302
Fixes #17814

.editorconfig

@@ -197,8 +197,8 @@ dotnet_naming_rule.interface_naming.symbols = interface_symbol
 dotnet_naming_rule.interface_naming.style = interface_style
 dotnet_naming_rule.interface_naming.severity = suggestion
 
-# Classes, Structs, Enums, Properties, Methods, Events, Namespaces
-dotnet_naming_symbols.class_symbol.applicable_kinds = class, struct, enum, property, method, event, namespace
+# Classes, Structs, Enums, Properties, Methods, Local Functions, Events, Namespaces
+dotnet_naming_symbols.class_symbol.applicable_kinds = class, struct, enum, property, method, local_function, event, namespace
 dotnet_naming_symbols.class_symbol.applicable_accessibilities = *
 
 dotnet_naming_rule.class_naming.symbols = class_symbol

src/EFCore.Cosmos/Metadata/Conventions/CosmosDiscriminatorConvention.cs

@@ -1,8 +1,9 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
-using System.Collections;
+using System.Linq;
 using JetBrains.Annotations;
+using Microsoft.EntityFrameworkCore.Cosmos.Metadata.Internal;
 using Microsoft.EntityFrameworkCore.Metadata.Builders;
 using Microsoft.EntityFrameworkCore.Metadata.Conventions.Infrastructure;
 using Microsoft.EntityFrameworkCore.Utilities;
@@ -12,7 +13,11 @@ namespace Microsoft.EntityFrameworkCore.Metadata.Conventions
     /// <summary>
     ///     A convention that configures the discriminator value for entity types as the entity type name.
     /// </summary>
-    public class CosmosDiscriminatorConvention : DiscriminatorConvention, IEntityTypeAddedConvention
+    public class CosmosDiscriminatorConvention :
+        DiscriminatorConvention,
+        IForeignKeyOwnershipChangedConvention,
+        IForeignKeyRemovedConvention,
+        IEntityTypeAddedConvention
     {
         /// <summary>
         ///     Creates a new instance of <see cref="CosmosDiscriminatorConvention" />.
@@ -36,22 +41,56 @@ public virtual void ProcessEntityTypeAdded(
             Check.NotNull(context, nameof(context));
 
             var entityType = entityTypeBuilder.Metadata;
-            if (entityTypeBuilder.Metadata.BaseType == null
-                && !Any(entityTypeBuilder.Metadata.GetDerivedTypes()))
+            if (entityType.BaseType == null
+                && !entityType.GetDerivedTypes().Any()
+                && entityType.IsDocumentRoot())
             {
                 entityTypeBuilder.HasDiscriminator(typeof(string))
-                    .HasValue(entityType, entityType.ShortName());
+                    ?.HasValue(entityType, entityType.ShortName());
             }
         }
 
-        private static bool Any([NotNull] IEnumerable source)
+        /// <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(
+            IConventionRelationshipBuilder relationshipBuilder,
+            IConventionContext<IConventionRelationshipBuilder> context)
         {
-            foreach (var _ in source)
+            Check.NotNull(relationshipBuilder, nameof(relationshipBuilder));
+            Check.NotNull(context, nameof(context));
+
+            var entityType = relationshipBuilder.Metadata.DeclaringEntityType;
+            if (relationshipBuilder.Metadata.IsOwnership
+                && !entityType.IsDocumentRoot()
+                && entityType.BaseType == null
+                && !entityType.GetDerivedTypes().Any())
             {
-                return true;
+                entityType.Builder.HasNoDeclaredDiscriminator();
             }
+        }
 
-            return false;
+        /// <summary>
+        ///     Called after a foreign key is removed.
+        /// </summary>
+        /// <param name="entityTypeBuilder"> The builder for the entity type. </param>
+        /// <param name="foreignKey"> The removed foreign key. </param>
+        /// <param name="context"> Additional information associated with convention execution. </param>
+        public virtual void ProcessForeignKeyRemoved(
+            IConventionEntityTypeBuilder entityTypeBuilder,
+            IConventionForeignKey foreignKey,
+            IConventionContext<IConventionForeignKey> context)
+        {
+            var entityType = foreignKey.DeclaringEntityType;
+            if (foreignKey.IsOwnership
+                && !entityType.IsDocumentRoot()
+                && entityType.BaseType == null
+                && !entityType.GetDerivedTypes().Any())
+            {
+                entityType.Builder.HasNoDeclaredDiscriminator();
+            }
         }
 
         /// <summary>
@@ -72,11 +111,14 @@ public override void ProcessEntityTypeBaseTypeChanged(
                 return;
             }
 
-            IConventionDiscriminatorBuilder discriminator;
+            IConventionDiscriminatorBuilder discriminator = null;
             var entityType = entityTypeBuilder.Metadata;
             if (newBaseType == null)
             {
-                discriminator = entityTypeBuilder.HasDiscriminator(typeof(string));
+                if (entityType.IsDocumentRoot())
+                {
+                    discriminator = entityTypeBuilder.HasDiscriminator(typeof(string));
+                }
             }
             else
             {

src/EFCore.Cosmos/Metadata/Conventions/Internal/CosmosConventionSetBuilder.cs

@@ -49,6 +49,10 @@ public override ConventionSet CreateConventionSet()
             conventionSet.EntityTypeBaseTypeChangedConventions.Add(storeKeyConvention);
             ReplaceConvention(conventionSet.EntityTypeBaseTypeChangedConventions, (DiscriminatorConvention)discriminatorConvention);
 
+            conventionSet.ForeignKeyRemovedConventions.Add(discriminatorConvention);
+            conventionSet.ForeignKeyRemovedConventions.Add(storeKeyConvention);
+
+            conventionSet.ForeignKeyOwnershipChangedConventions.Add(discriminatorConvention);
             conventionSet.ForeignKeyOwnershipChangedConventions.Add(storeKeyConvention);
 
             conventionSet.EntityTypeAnnotationChangedConventions.Add(storeKeyConvention);

src/EFCore.Cosmos/Metadata/Conventions/StoreKeyConvention.cs

@@ -23,6 +23,7 @@ namespace Microsoft.EntityFrameworkCore.Metadata.Conventions
     public class StoreKeyConvention :
         IEntityTypeAddedConvention,
         IForeignKeyOwnershipChangedConvention,
+        IForeignKeyRemovedConvention,
         IEntityTypeAnnotationChangedConvention,
         IEntityTypeBaseTypeChangedConvention
     {
@@ -113,6 +114,23 @@ public virtual void ProcessForeignKeyOwnershipChanged(
             Process(relationshipBuilder.Metadata.DeclaringEntityType.Builder);
         }
 
+        /// <summary>
+        ///     Called after a foreign key is removed.
+        /// </summary>
+        /// <param name="entityTypeBuilder"> The builder for the entity type. </param>
+        /// <param name="foreignKey"> The removed foreign key. </param>
+        /// <param name="context"> Additional information associated with convention execution. </param>
+        public virtual void ProcessForeignKeyRemoved(
+            IConventionEntityTypeBuilder entityTypeBuilder,
+            IConventionForeignKey foreignKey,
+            IConventionContext<IConventionForeignKey> context)
+        {
+            if (foreignKey.IsOwnership)
+            {
+                Process(foreignKey.DeclaringEntityType.Builder);
+            }
+        }
+
         /// <summary>
         ///     Called after an annotation is changed on an entity type.
         /// </summary>

src/EFCore.Cosmos/Storage/Internal/CosmosDatabaseWrapper.cs

@@ -204,8 +204,12 @@ private bool Save(IUpdateEntry entry)
                     {
                         document = documentSource.CreateDocument(entry);
 
-                        document[entityType.GetDiscriminatorProperty().GetPropertyName()] =
-                            JToken.FromObject(entityType.GetDiscriminatorValue(), CosmosClientWrapper.Serializer);
+                        var propertyName = entityType.GetDiscriminatorProperty()?.GetPropertyName();
+                        if (propertyName != null)
+                        {
+                            document[propertyName] =
+                                JToken.FromObject(entityType.GetDiscriminatorValue(), CosmosClientWrapper.Serializer);
+                        }
                     }
 
                     return _cosmosClient.ReplaceItem(
@@ -255,8 +259,12 @@ private Task<bool> SaveAsync(IUpdateEntry entry, CancellationToken cancellationT
                     {
                         document = documentSource.CreateDocument(entry);
 
-                        document[entityType.GetDiscriminatorProperty().GetPropertyName()] =
-                            JToken.FromObject(entityType.GetDiscriminatorValue(), CosmosClientWrapper.Serializer);
+                        var propertyName = entityType.GetDiscriminatorProperty()?.GetPropertyName();
+                        if (propertyName != null)
+                        {
+                            document[propertyName] =
+                                JToken.FromObject(entityType.GetDiscriminatorValue(), CosmosClientWrapper.Serializer);
+                        }
                     }
 
                     return _cosmosClient.ReplaceItemAsync(

src/EFCore/Infrastructure/ModelSource.cs

@@ -1,9 +1,6 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
-using System;
-using System.Collections.Concurrent;
-using System.Threading;
 using JetBrains.Annotations;
 using Microsoft.EntityFrameworkCore.Metadata;
 using Microsoft.EntityFrameworkCore.Metadata.Conventions.Infrastructure;

src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs

@@ -655,6 +655,41 @@ private bool CanRemoveProperty(
                    && (canOverrideSameSource || (configurationSource != currentConfigurationSource));
         }
 
+        /// <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>
+        public virtual bool CanAddProperty(Type propertyType, string name,ConfigurationSource? typeConfigurationSource)
+        {
+            var conflictingMember = Metadata.FindPropertiesInHierarchy(name).FirstOrDefault();
+            if (conflictingMember != null
+                && conflictingMember.IsShadowProperty()
+                && conflictingMember.ClrType != propertyType
+                && typeConfigurationSource != null
+                && !typeConfigurationSource.Overrides(conflictingMember.GetTypeConfigurationSource()))
+            {
+                return false;
+            }
+
+            if (Metadata.ClrType == null)
+            {
+                return true;
+            }
+
+            var memberInfo = Metadata.ClrType.GetMembersInHierarchy(name).FirstOrDefault();
+            if (memberInfo != null
+                && propertyType != memberInfo.GetMemberType()
+                && (memberInfo as PropertyInfo)?.IsEFIndexerProperty() != true
+                && typeConfigurationSource != null)
+            {
+                return false;
+            }
+
+            return true;
+        }
+
         /// <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
@@ -3415,11 +3450,18 @@ private bool CanSetDiscriminator(
             string name,
             Type discriminatorType,
             bool fromDataAnnotation)
-            => (name == null && discriminatorType == null)
+            => ((name == null && discriminatorType == null)
                || ((name == null || discriminatorProperty?.Name == name)
                    && (discriminatorType == null || discriminatorProperty?.ClrType == discriminatorType))
                || (fromDataAnnotation ? ConfigurationSource.DataAnnotation : ConfigurationSource.Convention)
-               .Overrides(Metadata.GetDiscriminatorPropertyConfigurationSource());
+                    .Overrides(Metadata.GetDiscriminatorPropertyConfigurationSource()))
+            && (discriminatorProperty != null
+                 || Metadata.RootType().Builder.CanAddProperty(
+                        discriminatorType ?? _defaultDiscriminatorType,
+                        name ?? _defaultDiscriminatorName,
+                        typeConfigurationSource: discriminatorType != null
+                            ? fromDataAnnotation ? ConfigurationSource.DataAnnotation : ConfigurationSource.Convention
+                            : (ConfigurationSource?)null));
 
         /// <summary>
         ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to

test/EFCore.Cosmos.FunctionalTests/CustomConvertersCosmosTest.cs

@@ -113,12 +113,6 @@ public override void Can_insert_and_query_struct_to_string_converter_for_pk()
             base.Can_insert_and_query_struct_to_string_converter_for_pk();
         }
 
-        [ConditionalTheory(Skip = "Issue #17814")]
-        public override Task Can_query_custom_type_not_mapped_by_default_equality(bool isAsync)
-        {
-            return base.Can_query_custom_type_not_mapped_by_default_equality(isAsync);
-        }
-
         public class CustomConvertersCosmosFixture : CustomConvertersFixtureBase
         {
             protected override ITestStoreFactory TestStoreFactory => CosmosTestStoreFactory.Instance;

test/EFCore.Cosmos.FunctionalTests/EmbeddedDocumentsTest.cs

@@ -182,7 +182,7 @@ public virtual async Task Can_add_collection_dependent_to_owner()
                     var addressJson = existingAddressEntry.Property<JObject>("__jObject").CurrentValue;
 
                     Assert.Equal("Second", addressJson[nameof(Address.Street)]);
-                    Assert.Equal(4, addressJson.Count);
+                    Assert.Equal(3, addressJson.Count);
                     Assert.Equal(2, addressJson["unmappedId"]);
 
                     addresses = people[2].Addresses.ToList();