(RC2) Persist Id values into owned collection JSON documents

src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs

@@ -2788,23 +2788,24 @@ protected virtual void ValidateJsonEntityKey(
         {
             if (primaryKeyProperty.GetJsonPropertyName() != null)
             {
-                // issue #28594
+                // Issue #28594
                 throw new InvalidOperationException(
                     RelationalStrings.JsonEntityWithExplicitlyConfiguredJsonPropertyNameOnKey(
                         primaryKeyProperty.Name, jsonEntityType.DisplayName()));
             }
-        }
 
-        if (!ownership.IsUnique)
-        {
-            // for collection entities, make sure that ordinal key is not explicitly defined
-            var ordinalKeyProperty = primaryKeyProperties.Last();
-            if (!ordinalKeyProperty.IsOrdinalKeyProperty())
+            if (!ownership.IsUnique)
             {
-                // issue #28594
-                throw new InvalidOperationException(
-                    RelationalStrings.JsonEntityWithExplicitlyConfiguredOrdinalKey(
-                        jsonEntityType.DisplayName()));
+                // For collection entities, no key properties other than the generated ones are allowed because they
+                // will not be persisted.
+                if (!primaryKeyProperty.IsOrdinalKeyProperty()
+                    && !primaryKeyProperty.IsForeignKey())
+                {
+                    // issue #28594
+                    throw new InvalidOperationException(
+                        RelationalStrings.JsonEntityWithExplicitlyConfiguredKey(
+                            jsonEntityType.DisplayName(), primaryKeyProperty.Name));
+                }
             }
         }
 

src/EFCore.Relational/Metadata/Conventions/Infrastructure/RelationalConventionSetBuilder.cs

@@ -79,9 +79,15 @@ public override ConventionSet CreateConventionSet()
 
         conventionSet.Replace<ValueGenerationConvention>(
             new RelationalValueGenerationConvention(Dependencies, RelationalDependencies));
+
+        conventionSet.Replace<KeyDiscoveryConvention>(
+            new RelationalKeyDiscoveryConvention(Dependencies, RelationalDependencies));
+
         conventionSet.Replace<QueryFilterRewritingConvention>(
             new RelationalQueryFilterRewritingConvention(Dependencies, RelationalDependencies));
-        conventionSet.Replace<RuntimeModelConvention>(new RelationalRuntimeModelConvention(Dependencies, RelationalDependencies));
+
+        conventionSet.Replace<RuntimeModelConvention>(
+            new RelationalRuntimeModelConvention(Dependencies, RelationalDependencies));
 
         return conventionSet;
     }

src/EFCore.Relational/Metadata/Conventions/RelationalKeyDiscoveryConvention.cs

@@ -0,0 +1,158 @@
+// 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 relational-specific convention inheriting from <see cref="KeyDiscoveryConvention"/>.
+/// </summary>
+/// <remarks>
+///     <para>
+///         See <see href="https://aka.ms/efcore-docs-conventions">Model building conventions</see> for more information and examples.
+///     </para>
+/// </remarks>
+public class RelationalKeyDiscoveryConvention : KeyDiscoveryConvention, IEntityTypeAnnotationChangedConvention
+{
+    /// <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>
+    [EntityFrameworkInternal]
+    public const string SynthesizedOrdinalPropertyName = "__synthesizedOrdinal";
+
+    /// <summary>
+    ///     Creates a new instance of <see cref="RelationalKeyDiscoveryConvention" />.
+    /// </summary>
+    /// <param name="dependencies">Parameter object containing dependencies for this convention.</param>
+    /// <param name="relationalDependencies"> Parameter object containing relational dependencies for this convention.</param>
+    public RelationalKeyDiscoveryConvention(
+        ProviderConventionSetBuilderDependencies dependencies,
+        RelationalConventionSetBuilderDependencies relationalDependencies)
+        : base(dependencies)
+    {
+        RelationalDependencies = relationalDependencies;
+    }
+
+    /// <summary>
+    ///     Relational provider-specific dependencies for this service.
+    /// </summary>
+    protected virtual RelationalConventionSetBuilderDependencies RelationalDependencies { get; }
+
+    /// <inheritdoc />
+    protected override List<IConventionProperty>? DiscoverKeyProperties(IConventionEntityType entityType)
+    {
+        var ownership = entityType.FindOwnership();
+        if (ownership?.DeclaringEntityType != entityType)
+        {
+            ownership = null;
+        }
+
+        // Don't discover key properties for owned collection types mapped to JSON so that we can persist properties
+        // called `Id` without attempting to persist key values.
+        if (ownership?.IsUnique == false
+            && entityType.GetContainerColumnName() is not null)
+        {
+            return [];
+        }
+
+        return base.DiscoverKeyProperties(entityType);
+    }
+
+    /// <inheritdoc />
+    protected override void ProcessKeyProperties(
+        IList<IConventionProperty> keyProperties,
+        IConventionEntityType entityType)
+    {
+        var isMappedToJson = entityType.GetContainerColumnName() is not null;
+        var synthesizedProperty = keyProperties.FirstOrDefault(p => p.Name == SynthesizedOrdinalPropertyName);
+        var ownershipForeignKey = entityType.FindOwnership();
+        if (ownershipForeignKey?.IsUnique == false
+            && isMappedToJson)
+        {
+            // This is an owned collection, so it has a composite key consisting of FK properties pointing to the owner PK,
+            // any additional key properties defined by the application, and then the synthesized property.
+            // Add these in the correct order--this is somewhat inefficient, but we are limited because we have to manipulate the
+            // existing collection.
+            var existingKeyProperties = keyProperties.ToList();
+            keyProperties.Clear();
+
+            // Add the FK properties to form the first part of the composite key.
+            foreach (var conventionProperty in ownershipForeignKey.Properties)
+            {
+                keyProperties.Add(conventionProperty);
+            }
+
+            // Generate the synthesized key property if it doesn't exist.
+            if (synthesizedProperty == null)
+            {
+                var builder = entityType.Builder.CreateUniqueProperty(typeof(int), SynthesizedOrdinalPropertyName, required: true);
+                builder = builder?.ValueGenerated(ValueGenerated.OnAdd) ?? builder;
+                synthesizedProperty = builder!.Metadata;
+            }
+
+            // Add non-duplicate, non-ownership, non-synthesized properties.
+            foreach (var keyProperty in existingKeyProperties)
+            {
+                if (keyProperty != synthesizedProperty
+                    && !keyProperties.Contains(keyProperty))
+                {
+                    keyProperties.Add(keyProperty);
+                }
+            }
+
+            // Finally, the synthesized property always goes at the end.
+            keyProperties.Add(synthesizedProperty);
+        }
+        else
+        {
+            // Not an owned collection or not mapped to JSON.
+            if (synthesizedProperty is not null)
+            {
+                // This was an owned collection, but now is not, so remove the synthesized property.
+                keyProperties.Remove(synthesizedProperty);
+            }
+
+            base.ProcessKeyProperties(keyProperties, entityType);
+        }
+    }
+
+    /// <inheritdoc />
+    public override void ProcessPropertyAdded(
+        IConventionPropertyBuilder propertyBuilder,
+        IConventionContext<IConventionPropertyBuilder> context)
+    {
+        if (propertyBuilder.Metadata.Name != SynthesizedOrdinalPropertyName)
+        {
+            base.ProcessPropertyAdded(propertyBuilder, context);
+        }
+    }
+
+    /// <inheritdoc />
+    public virtual void ProcessEntityTypeAnnotationChanged(
+        IConventionEntityTypeBuilder entityTypeBuilder,
+        string name,
+        IConventionAnnotation? annotation,
+        IConventionAnnotation? oldAnnotation,
+        IConventionContext<IConventionAnnotation> context)
+    {
+        if (name == RelationalAnnotationNames.ContainerColumnName)
+        {
+            Configure(this, entityTypeBuilder);
+        }
+
+        static void Configure(RelationalKeyDiscoveryConvention me, IConventionEntityTypeBuilder builder)
+        {
+            me.TryConfigurePrimaryKey(builder);
+
+            foreach (var ownershipFk in builder.Metadata.GetReferencingForeignKeys())
+            {
+                if (ownershipFk.IsOwnership)
+                {
+                    Configure(me, ownershipFk.DeclaringEntityType.Builder);
+                }
+            }
+        }
+    }
+}

src/EFCore.Relational/Metadata/Internal/RelationalPropertyExtensions.cs

@@ -28,8 +28,5 @@ public static class RelationalPropertyExtensions
     ///     doing so can result in application failures when updating to a new Entity Framework Core release.
     /// </summary>
     public static bool IsOrdinalKeyProperty(this IReadOnlyProperty property)
-        => property.FindContainingPrimaryKey() is { Properties.Count: > 1 }
-            && !property.IsForeignKey()
-            && property.ClrType == typeof(int)
-            && property.GetJsonPropertyName() == null;
+        => property.Name == RelationalKeyDiscoveryConvention.SynthesizedOrdinalPropertyName;
 }

src/EFCore.Relational/Properties/RelationalStrings.resx

@@ -1,17 +1,17 @@
 <?xml version="1.0" encoding="utf-8"?>
 <root>
-  <!-- 
-    Microsoft ResX Schema 
-    
+  <!--
+    Microsoft ResX Schema
+
     Version 2.0
-    
-    The primary goals of this format is to allow a simple XML format 
-    that is mostly human readable. The generation and parsing of the 
-    various data types are done through the TypeConverter classes 
+
+    The primary goals of this format is to allow a simple XML format
+    that is mostly human readable. The generation and parsing of the
+    various data types are done through the TypeConverter classes
     associated with the data types.
-    
+
     Example:
-    
+
     ... ado.net/XML headers & schema ...
     <resheader name="resmimetype">text/microsoft-resx</resheader>
     <resheader name="version">2.0</resheader>
@@ -26,36 +26,36 @@
         <value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
         <comment>This is a comment</comment>
     </data>
-                
-    There are any number of "resheader" rows that contain simple 
+
+    There are any number of "resheader" rows that contain simple
     name/value pairs.
-    
-    Each data row contains a name, and value. The row also contains a 
-    type or mimetype. Type corresponds to a .NET class that support 
-    text/value conversion through the TypeConverter architecture. 
-    Classes that don't support this are serialized and stored with the 
+
+    Each data row contains a name, and value. The row also contains a
+    type or mimetype. Type corresponds to a .NET class that support
+    text/value conversion through the TypeConverter architecture.
+    Classes that don't support this are serialized and stored with the
     mimetype set.
-    
-    The mimetype is used for serialized objects, and tells the 
-    ResXResourceReader how to depersist the object. This is currently not 
+
+    The mimetype is used for serialized objects, and tells the
+    ResXResourceReader how to depersist the object. This is currently not
     extensible. For a given mimetype the value must be set accordingly:
-    
-    Note - application/x-microsoft.net.object.binary.base64 is the format 
-    that the ResXResourceWriter will generate, however the reader can 
+
+    Note - application/x-microsoft.net.object.binary.base64 is the format
+    that the ResXResourceWriter will generate, however the reader can
     read any of the formats listed below.
-    
+
     mimetype: application/x-microsoft.net.object.binary.base64
-    value   : The object must be serialized with 
+    value   : The object must be serialized with
             : System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
             : and then encoded with base64 encoding.
-    
+
     mimetype: application/x-microsoft.net.object.soap.base64
-    value   : The object must be serialized with 
+    value   : The object must be serialized with
             : System.Runtime.Serialization.Formatters.Soap.SoapFormatter
             : and then encoded with base64 encoding.
 
     mimetype: application/x-microsoft.net.object.bytearray.base64
-    value   : The object must be serialized into a byte array 
+    value   : The object must be serialized into a byte array
             : using a System.ComponentModel.TypeConverter
             : and then encoded with base64 encoding.
     -->
@@ -547,8 +547,8 @@
   <data name="JsonEntityWithExplicitlyConfiguredJsonPropertyNameOnKey" xml:space="preserve">
     <value>Key property '{keyProperty}' on JSON-mapped entity '{jsonEntity}' should not have its JSON property name configured explicitly.</value>
   </data>
-  <data name="JsonEntityWithExplicitlyConfiguredOrdinalKey" xml:space="preserve">
-    <value>Entity type '{jsonEntity}' is part of a collection mapped to JSON and has its ordinal key defined explicitly. Only implicitly defined ordinal keys are supported.</value>
+  <data name="JsonEntityWithExplicitlyConfiguredKey" xml:space="preserve">
+    <value>The property '{entityType}.{property}' is configured as part of the primary key. Explicitly configured primary keys are not supported on entities stored in JSON collections, and any key configuration should be removed.</value>
   </data>
   <data name="JsonEntityWithIncorrectNumberOfKeyProperties" xml:space="preserve">
     <value>Entity type '{jsonEntity}' has an incorrect number of primary key properties. Expected number is: {expectedCount}, actual number is: {actualCount}.</value>