Warn when saving an optional dependent with all null properties when …

…table splitting

Fixes #24558

Co-authored-by: Umit Kavala <[email protected]>

src/EFCore.Relational/Diagnostics/RelationalEventId.cs

@@ -94,6 +94,7 @@ private enum Id
             BatchSmallerThanMinBatchSize,
             BatchExecutorFailedToRollbackToSavepoint,
             BatchExecutorFailedToReleaseSavepoint,
+            OptionalDependentWithAllNullPropertiesWarning,
         }
 
         private static readonly string _connectionPrefix = DbLoggerCategory.Database.Connection.Name + ".";
@@ -726,7 +727,7 @@ private static EventId MakeValidationId(Id id)
 
         /// <summary>
         ///     <para>
-        ///         A foreign key specifies properties which don't map to the related tables.
+        ///         The entity does not have any property with a non-default value to identify whether the entity exists.
         ///     </para>
         ///     <para>
         ///         This event is in the <see cref="DbLoggerCategory.Model.Validation" /> category.
@@ -790,5 +791,19 @@ private static EventId MakeUpdateId(Id id)
         ///     </para>
         /// </summary>
         public static readonly EventId BatchExecutorFailedToReleaseSavepoint = MakeUpdateId(Id.BatchExecutorFailedToReleaseSavepoint);
+
+        /// <summary>
+        ///     <para>
+        ///         The entity does not have any property with a non-default value to identify whether the entity exists.
+        ///     </para>
+        ///     <para>
+        ///         This event is in the <see cref="DbLoggerCategory.Update" /> category.
+        ///     </para>
+        ///     <para>
+        ///         This event uses the <see cref="UpdateEntryEventData" /> payload when used with a <see cref="DiagnosticSource" />.
+        ///     </para>
+        /// </summary>
+        public static readonly EventId OptionalDependentWithAllNullPropertiesWarning
+            = MakeUpdateId(Id.OptionalDependentWithAllNullPropertiesWarning);
     }
 }

src/EFCore.Relational/Diagnostics/RelationalLoggerExtensions.cs

@@ -2964,5 +2964,74 @@ public static void BatchExecutorFailedToReleaseSavepoint(
                 diagnostics.DispatchEventData(definition, eventData, diagnosticSourceEnabled, simpleLogEnabled);
             }
         }
+
+        /// <summary>
+        ///     Logs the <see cref="RelationalEventId.OptionalDependentWithAllNullPropertiesWarning" /> event.
+        /// </summary>
+        /// <param name="diagnostics"> The diagnostics logger to use. </param>
+        /// <param name="entry"> The entry. </param>
+        public static void OptionalDependentWithAllNullPropertiesWarning(
+            this IDiagnosticsLogger<DbLoggerCategory.Update> diagnostics,
+            IUpdateEntry entry)
+        {
+            var definition = RelationalResources.LogOptionalDependentWithAllNullProperties(diagnostics);
+
+            if (diagnostics.ShouldLog(definition))
+            {
+                definition.Log(diagnostics, entry.EntityType.DisplayName());
+            }
+
+            if (diagnostics.NeedsEventData(definition, out var diagnosticSourceEnabled, out var simpleLogEnabled))
+            {
+                var eventData = new UpdateEntryEventData(
+                    definition,
+                    OptionalDependentWithAllNullPropertiesWarning,
+                    entry);
+
+                diagnostics.DispatchEventData(definition, eventData, diagnosticSourceEnabled, simpleLogEnabled);
+            }
+        }
+
+        private static string OptionalDependentWithAllNullPropertiesWarning(EventDefinitionBase definition, EventData payload)
+        {
+            var d = (EventDefinition<string>)definition;
+            var p = (UpdateEntryEventData)payload;
+            return d.GenerateMessage(p.EntityEntry.EntityType.DisplayName());
+        }
+
+        /// <summary>
+        ///     Logs the <see cref="RelationalEventId.OptionalDependentWithAllNullPropertiesWarning" /> event.
+        /// </summary>
+        /// <param name="diagnostics"> The diagnostics logger to use. </param>
+        /// <param name="entry"> The entry. </param>
+        public static void OptionalDependentWithAllNullPropertiesWarningSensitive(
+            this IDiagnosticsLogger<DbLoggerCategory.Update> diagnostics,
+            IUpdateEntry entry)
+        {
+            var definition = RelationalResources.LogOptionalDependentWithAllNullPropertiesSensitive(diagnostics);
+
+            if (diagnostics.ShouldLog(definition))
+            {
+                definition.Log(diagnostics, entry.EntityType.DisplayName(), entry.BuildCurrentValuesString(entry.EntityType.FindPrimaryKey()!.Properties));
+            }
+
+            if (diagnostics.NeedsEventData(definition, out var diagnosticSourceEnabled, out var simpleLogEnabled))
+            {
+                var eventData = new UpdateEntryEventData(
+                    definition,
+                    OptionalDependentWithAllNullPropertiesWarningSensitive,
+                    entry
+                    );
+
+                diagnostics.DispatchEventData(definition, eventData, diagnosticSourceEnabled, simpleLogEnabled);
+            }
+        }
+
+        private static string OptionalDependentWithAllNullPropertiesWarningSensitive(EventDefinitionBase definition, EventData payload)
+        {
+            var d = (EventDefinition<string, string>)definition;
+            var p = (UpdateEntryEventData)payload;
+            return d.GenerateMessage(p.EntityEntry.EntityType.DisplayName(), p.EntityEntry.BuildCurrentValuesString(p.EntityEntry.EntityType.FindPrimaryKey()!.Properties));
+        }
     }
 }

src/EFCore.Relational/Diagnostics/RelationalLoggingDefinitions.cs

@@ -512,5 +512,23 @@ public abstract class RelationalLoggingDefinitions : LoggingDefinitions
         /// </summary>
         [EntityFrameworkInternal]
         public EventDefinitionBase? LogOptionalDependentWithoutIdentifyingProperty;
+
+        /// <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 EventDefinitionBase? LogOptionalDependentWithAllNullProperties;
+
+        /// <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 EventDefinitionBase? LogOptionalDependentWithAllNullPropertiesSensitive;
     }
 }

src/EFCore.Relational/Properties/RelationalStrings.resx

@@ -559,6 +559,14 @@
     <value>Opening connection to database '{database}' on server '{server}'.</value>
     <comment>Debug RelationalEventId.ConnectionOpening string string</comment>
   </data>
+  <data name="LogOptionalDependentWithAllNullProperties" xml:space="preserve">
+    <value>The entity of type '{entityType}' is an optional dependent using table sharing. The entity does not have any property with a non-default value to identify whether the entity exists. This means that when it is queried no object instance will be created instead of an instance with all properties set to default values. Any nested dependents will also be lost. Either don't save any instance with only default values or mark the incoming navigation as required in the model. Consider using 'DbContextOptionsBuilder.EnableSensitiveDataLogging' to see the key values of the entity.</value>
+    <comment>Warning RelationalEventId.OptionalDependentWithAllNullPropertiesWarning string</comment>
+  </data>
+  <data name="LogOptionalDependentWithAllNullPropertiesSensitive" xml:space="preserve">
+    <value>The entity of type '{entityType}' with primary key values {keyValues} is an optional dependent using table sharing. The entity does not have any property with a non-default value to identify whether the entity exists. This means that when it is queried no object instance will be created instead of an instance with all properties set to default values. Any nested dependents will also be lost. Either don't save any instance with only default values or mark the incoming navigation as required in the model.</value>
+    <comment>Warning RelationalEventId.OptionalDependentWithAllNullPropertiesWarning string string</comment>
+  </data>
   <data name="LogOptionalDependentWithoutIdentifyingProperty" xml:space="preserve">
     <value>The entity type '{entityType}' is an optional dependent using table sharing without any required non shared property that could be used to identify whether the entity exists. If all nullable properties contain a null value in database then an object instance won't be created in the query. Add a required property to create instances with null values for other properties or mark the incoming navigation as required to always create an instance.</value>
     <comment>Warning RelationalEventId.OptionalDependentWithoutIdentifyingPropertyWarning string</comment>

src/EFCore.Relational/Update/Internal/CommandBatchPreparer.cs

@@ -199,13 +199,13 @@ protected virtual IEnumerable<ModificationCommand> CreateModificationCommands(
 
                         command = sharedCommandsMap.GetOrAddValue(
                             entry,
-                            (n, s, c) => new ModificationCommand(n, s, generateParameterName, _sensitiveLoggingEnabled, c));
+                            (n, s, c) => new ModificationCommand(n, s, generateParameterName, _sensitiveLoggingEnabled, c, Dependencies.UpdateLogger));
                         isMainEntry = sharedCommandsMap.IsMainEntry(entry);
                     }
                     else
                     {
                         command = new ModificationCommand(
-                            table.Name, table.Schema, generateParameterName, _sensitiveLoggingEnabled, comparer: null);
+                            table.Name, table.Schema, generateParameterName, _sensitiveLoggingEnabled, comparer: null, Dependencies.UpdateLogger);
                     }
 
                     command.AddEntry(entry, isMainEntry);

src/EFCore.Relational/Update/ModificationCommand.cs

@@ -6,6 +6,7 @@
 using System.Diagnostics;
 using System.Linq;
 using Microsoft.EntityFrameworkCore.Diagnostics;
+using Microsoft.EntityFrameworkCore.Diagnostics.Internal;
 using Microsoft.EntityFrameworkCore.Infrastructure;
 using Microsoft.EntityFrameworkCore.Internal;
 using Microsoft.EntityFrameworkCore.Metadata;
@@ -31,6 +32,7 @@ public class ModificationCommand
         private IReadOnlyList<ColumnModification>? _columnModifications;
         private bool _requiresResultPropagation;
         private bool _mainEntryAdded;
+        private readonly IDiagnosticsLogger<DbLoggerCategory.Update>? _logger;
 
         /// <summary>
         ///     Initializes a new <see cref="ModificationCommand" /> instance.
@@ -40,12 +42,14 @@ public class ModificationCommand
         /// <param name="generateParameterName"> A delegate to generate parameter names. </param>
         /// <param name="sensitiveLoggingEnabled"> Indicates whether or not potentially sensitive data (e.g. database values) can be logged. </param>
         /// <param name="comparer"> A <see cref="IComparer{T}" /> for <see cref="IUpdateEntry" />s. </param>
+        /// <param name="logger">A <see cref="IDiagnosticsLogger{T}" /> for <see cref="DbLoggerCategory.Update" />s.</param>
         public ModificationCommand(
             string name,
             string? schema,
             Func<string> generateParameterName,
             bool sensitiveLoggingEnabled,
-            IComparer<IUpdateEntry>? comparer)
+            IComparer<IUpdateEntry>? comparer,
+            IDiagnosticsLogger<DbLoggerCategory.Update>? logger)
             : this(
                 Check.NotEmpty(name, nameof(name)),
                 schema,
@@ -56,6 +60,7 @@ public ModificationCommand(
 
             _generateParameterName = generateParameterName;
             _comparer = comparer;
+            _logger = logger;
         }
 
         /// <summary>
@@ -306,6 +311,12 @@ private IReadOnlyList<ColumnModification> GenerateColumnModifications()
                     continue;
                 }
 
+                var optionalDependentWithAllNull =
+                    (entry.EntityState == EntityState.Deleted
+                        || entry.EntityState == EntityState.Added)
+                        && tableMapping.Table.IsOptional(entry.EntityType)
+                        && tableMapping.Table.GetRowInternalForeignKeys(entry.EntityType).Any();
+
                 foreach (var columnMapping in tableMapping.ColumnMappings)
                 {
                     var property = columnMapping.Property;
@@ -367,6 +378,25 @@ private IReadOnlyList<ColumnModification> GenerateColumnModifications()
                         }
 
                         columnModifications.Add(columnModification);
+
+                        if (optionalDependentWithAllNull
+                            && columnModification.IsWrite
+                            && (entry.EntityState != EntityState.Added || columnModification.Value is not null))
+                        {
+                            optionalDependentWithAllNull = false;
+                        }
+                    }
+                }
+
+                if (optionalDependentWithAllNull && _logger != null)
+                {
+                    if (_sensitiveLoggingEnabled)
+                    {
+                        _logger.OptionalDependentWithAllNullPropertiesWarningSensitive(entry);
+                    }
+                    else
+                    {
+                        _logger.OptionalDependentWithAllNullPropertiesWarning(entry);
                     }
                 }
             }

src/EFCore/Diagnostics/EntityTypeEventData.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Diagnostics;
 using Microsoft.EntityFrameworkCore.Metadata;
+using Microsoft.EntityFrameworkCore.Update;
 
 namespace Microsoft.EntityFrameworkCore.Diagnostics
 {

src/EFCore/Diagnostics/UpdateEntryEventData.cs

@@ -0,0 +1,36 @@
+// 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.Diagnostics;
+using Microsoft.EntityFrameworkCore.Update;
+
+namespace Microsoft.EntityFrameworkCore.Diagnostics
+{
+    /// <summary>
+    ///     A <see cref="DiagnosticSource" /> event payload class for events that have
+    ///     an entity update entry.
+    /// </summary>
+    public class UpdateEntryEventData : EventData
+    {
+        /// <summary>
+        ///     Constructs the event payload.
+        /// </summary>
+        /// <param name="eventDefinition"> The event definition. </param>
+        /// <param name="messageGenerator"> A delegate that generates a log message for this event. </param>
+        /// <param name="entityEntry"> The entry for the entity instance on which the property value has changed. </param>
+        public UpdateEntryEventData(
+            EventDefinitionBase eventDefinition,
+            Func<EventDefinitionBase, EventData, string> messageGenerator,
+            IUpdateEntry entityEntry)
+            : base(eventDefinition, messageGenerator)
+        {
+            EntityEntry = entityEntry;
+        }
+
+        /// <summary>
+        ///     The entry for the entity instance on which the property value has changed.
+        /// </summary>
+        public virtual IUpdateEntry EntityEntry { get; }
+    }
+}