Fix sproc TPH mapping (#28875)

Fixes #28805

src/EFCore.Relational/Update/ColumnModificationParameters.cs

@@ -198,7 +198,7 @@ public ColumnModificationParameters(
     /// <param name="columnIsCondition">Indicates whether the column is used in the <c>WHERE</c> clause when updating.</param>
     /// <param name="sensitiveLoggingEnabled">Indicates whether potentially sensitive data (e.g. database values) can be logged.</param>
     public ColumnModificationParameters(
-        IUpdateEntry entry,
+        IUpdateEntry? entry,
         IProperty? property,
         IColumnBase column,
         Func<string> generateParameterName,

src/EFCore.Relational/Update/ModificationCommand.cs

@@ -360,7 +360,6 @@ private List<IColumnModification> GenerateColumnModifications()
 
             var nonMainEntry = !_mainEntryAdded || entry != _entries[0];
 
-            IEnumerable<IColumnMappingBase> columnMappings;
             var optionalDependentWithAllNull = false;
 
             if (StoreStoredProcedure is null)
@@ -371,25 +370,24 @@ private List<IColumnModification> GenerateColumnModifications()
                     continue;
                 }
 
-                columnMappings = tableMapping.ColumnMappings;
-
                 optionalDependentWithAllNull =
                     entry.EntityState is EntityState.Modified or EntityState.Added
                     && tableMapping.Table.IsOptional(entry.EntityType)
                     && tableMapping.Table.GetRowInternalForeignKeys(entry.EntityType).Any();
+
+                foreach (var columnMapping in tableMapping.ColumnMappings)
+                {
+                    HandleColumnModification(columnMapping);
+                }
             }
-            else
+            else // Stored procedure mapping case
             {
                 var storedProcedureMapping = GetStoredProcedureMapping(entry.EntityType, EntityState);
                 Check.DebugAssert(storedProcedureMapping is not null, "No sproc mapping but StoredProcedure is not null");
-
-                columnMappings = storedProcedureMapping.ParameterMappings
-                    .Concat((IEnumerable<IColumnMappingBase>)storedProcedureMapping.ResultColumnMappings);
-
-                // Stored procedures may have an additional rows affected parameter, result column or return value, which does not have a
-                // property/column mapping but still needs to have be represented via a column modification.
                 var storedProcedure = storedProcedureMapping.StoredProcedure;
 
+                // Stored procedures may have an additional rows affected result column or return value, which does not have a
+                // property/column mapping but still needs to have be represented via a column modification.
                 IColumnBase? rowsAffectedColumnBase = null;
 
                 if (storedProcedure.FindRowsAffectedParameter() is { } rowsAffectedParameter)
@@ -405,10 +403,12 @@ entry.EntityState is EntityState.Modified or EntityState.Added
                     rowsAffectedColumnBase = rowsAffectedReturnValue;
                 }
 
-                if (rowsAffectedColumnBase is not null)
+                // Add a column modification for rows affected result column/return value.
+                // A rows affected output parameter is added below in the correct position, with the rest of the parameters.
+                if (rowsAffectedColumnBase is IStoreStoredProcedureResultColumn or IStoreStoredProcedureReturnValue)
                 {
                     columnModifications.Add(CreateColumnModification(new ColumnModificationParameters(
-                        entry,
+                        entry: null,
                         property: null,
                         rowsAffectedColumnBase,
                         _generateParameterName!,
@@ -419,9 +419,56 @@ entry.EntityState is EntityState.Modified or EntityState.Added
                         columnIsCondition: false,
                         _sensitiveLoggingEnabled)));
                 }
+
+                // In TPH, the sproc has parameters for all entity types in the hierarchy; we must generate null column modifications
+                // for parameters for unrelated entity types.
+                // Enumerate over the sproc parameters in order, trying to match a corresponding parameter mapping.
+                // Note that we produce the column modifications in the same order as their sproc parameters; this is important and assumed
+                // later in the pipeline.
+                foreach (var parameter in StoreStoredProcedure.Parameters)
+                {
+                    if (parameter.FindParameterMapping(entry.EntityType) is { } parameterMapping)
+                    {
+                        HandleColumnModification(parameterMapping);
+                        continue;
+                    }
+
+                    // The parameter has no corresponding mapping; this is either a sibling property in a TPH hierarchy or a rows affected
+                    // output parameter or return value.
+                    columnModifications.Add(CreateColumnModification(new ColumnModificationParameters(
+                        entry: null,
+                        property: null,
+                        parameter,
+                        _generateParameterName!,
+                        parameter.StoreTypeMapping,
+                        valueIsRead: parameter.Direction.HasFlag(ParameterDirection.Output),
+                        valueIsWrite: parameter.Direction.HasFlag(ParameterDirection.Input),
+                        columnIsKey: false,
+                        columnIsCondition: false,
+                        _sensitiveLoggingEnabled)));
+                }
+
+                // Note that we only add column modifications for mapped result columns, even though the sproc may return additional result
+                // columns (e.g. for siblings in TPH). Our result propagation accesses result columns directly by their position.
+                foreach (var columnMapping in storedProcedureMapping.ResultColumnMappings)
+                {
+                    HandleColumnModification(columnMapping);
+                }
             }
 
-            foreach (var columnMapping in columnMappings)
+            if (optionalDependentWithAllNull && _logger != null)
+            {
+                if (_sensitiveLoggingEnabled)
+                {
+                    _logger.OptionalDependentWithAllNullPropertiesWarningSensitive(entry);
+                }
+                else
+                {
+                    _logger.OptionalDependentWithAllNullPropertiesWarning(entry);
+                }
+            }
+
+            void HandleColumnModification(IColumnMappingBase columnMapping)
             {
                 var property = columnMapping.Property;
                 var column = columnMapping.Column;
@@ -488,7 +535,7 @@ entry.EntityState is EntityState.Modified or EntityState.Added
                         {
                             columnPropagator.ColumnModification.AddSharedColumnModification(columnModification);
 
-                            continue;
+                            return;
                         }
 
                         columnPropagator.ColumnModification = columnModification;
@@ -511,18 +558,6 @@ entry.EntityState is EntityState.Modified or EntityState.Added
                     optionalDependentWithAllNull = false;
                 }
             }
-
-            if (optionalDependentWithAllNull && _logger != null)
-            {
-                if (_sensitiveLoggingEnabled)
-                {
-                    _logger.OptionalDependentWithAllNullPropertiesWarningSensitive(entry);
-                }
-                else
-                {
-                    _logger.OptionalDependentWithAllNullPropertiesWarning(entry);
-                }
-            }
         }
 
         return columnModifications;

src/EFCore.Relational/Update/ReaderModificationCommandBatch.cs

@@ -266,25 +266,10 @@ public override void Complete(bool moreBatchesExpected)
     /// <param name="modificationCommand">The modification command for which to add parameters.</param>
     protected virtual void AddParameters(IReadOnlyModificationCommand modificationCommand)
     {
-        IEnumerable<IColumnModification> columnModifications;
-
-        if (modificationCommand.StoreStoredProcedure is null)
-        {
-            columnModifications = modificationCommand.ColumnModifications;
-        }
-        else
-        {
-            if (modificationCommand.StoreStoredProcedure.ReturnValue is not null)
-            {
-                AddParameter(modificationCommand.ColumnModifications.First(c => c.Column is IStoreStoredProcedureReturnValue));
-            }
-
-            columnModifications = modificationCommand.ColumnModifications
-                .Where(c => c.Column is IStoreStoredProcedureParameter)
-                .OrderBy(c => ((IStoreStoredProcedureParameter)c.Column!).Position);
-        }
-
-        foreach (var columnModification in columnModifications)
+        Check.DebugAssert(!modificationCommand.ColumnModifications.Any(m => m.Column is IStoreStoredProcedureReturnValue)
+            || modificationCommand.ColumnModifications[0].Column is IStoreStoredProcedureReturnValue,
+            "ResultValue column modification in non-first position");
+        foreach (var columnModification in modificationCommand.ColumnModifications)
         {
             AddParameter(columnModification);
         }

src/EFCore.Relational/Update/UpdateSqlGenerator.cs

@@ -355,13 +355,16 @@ public virtual ResultSetMapping AppendStoredProcedureCall(
 
         // Only positional parameter style supported for now, see #28439
 
-        var orderedParameterModifications = command.ColumnModifications
-            .Where(c => c.Column is IStoreStoredProcedureParameter)
-            .OrderBy(c => ((IStoreStoredProcedureParameter)c.Column!).Position);
-
-        foreach (var columnModification in orderedParameterModifications)
+        // Note: the column modifications are already ordered according to the sproc parameter ordering
+        // (see ModificationCommand.GenerateColumnModifications)
+        for (var i = 0; i < command.ColumnModifications.Count; i++)
         {
-            var parameter = (IStoreStoredProcedureParameter)columnModification.Column!;
+            var columnModification = command.ColumnModifications[i];
+
+            if (columnModification.Column is not IStoreStoredProcedureParameter parameter)
+            {
+                continue;
+            }
 
             if (first)
             {

src/EFCore.SqlServer/Update/Internal/SqlServerUpdateSqlGenerator.cs

@@ -591,13 +591,16 @@ public override ResultSetMapping AppendStoredProcedureCall(
 
             // Only positional parameter style supported for now, see #28439
 
-            var orderedParameterModifications = command.ColumnModifications
-                .Where(c => c.Column is IStoreStoredProcedureParameter)
-                .OrderBy(c => ((IStoreStoredProcedureParameter)c.Column!).Position);
-
-            foreach (var columnModification in orderedParameterModifications)
+            // Note: the column modifications are already ordered according to the sproc parameter ordering
+            // (see ModificationCommand.GenerateColumnModifications)
+            for (var i = 0; i < command.ColumnModifications.Count; i++)
             {
-                var parameter = (IStoreStoredProcedureParameter)columnModification.Column!;
+                var columnModification = command.ColumnModifications[i];
+
+                if (columnModification.Column is not IStoreStoredProcedureParameter parameter)
+                {
+                    continue;
+                }
 
                 if (first)
                 {

test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/StoredProcedureUpdateContext.cs

@@ -53,7 +53,7 @@ public DbSet<Entity> WithInputOutputParameterOnNonConcurrencyToken
         => Set<Entity>(nameof(WithInputOutputParameterOnNonConcurrencyToken));
 
     public DbSet<TphParent> TphParent { get; set; }
-    public DbSet<TphChild> TphChild { get; set; }
+    public DbSet<TphChild1> TphChild { get; set; }
     public DbSet<TptParent> TptParent { get; set; }
     public DbSet<TptChild> TptChild { get; set; }
     public DbSet<TptMixedParent> TptMixedParent { get; set; }

test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/TphChild.cs → test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/TphChild1.cs

@@ -3,7 +3,7 @@
 
 namespace Microsoft.EntityFrameworkCore.TestModels.StoredProcedureUpdateModel;
 
-public class TphChild : TphParent
+public class TphChild1 : TphParent
 {
-    public int ChildProperty { get; set; }
+    public int Child1Property { get; set; }
 }

test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/TphChild2.cs

@@ -0,0 +1,11 @@
+// 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.TestModels.StoredProcedureUpdateModel;
+
+public class TphChild2 : TphParent
+{
+    public int Child2InputProperty { get; set; }
+    public int Child2OutputParameterProperty { get; set; }
+    public int Child2ResultColumnProperty { get; set; }
+}

test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateFixtureBase.cs

@@ -180,6 +180,15 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
                         .HasRowsAffectedReturnValue());
             });
 
+        modelBuilder.Entity<TphChild1>();
+
+        modelBuilder.Entity<TphChild2>(
+            b =>
+            {
+                b.Property(w => w.Child2OutputParameterProperty).HasDefaultValue(8);
+                b.Property(w => w.Child2ResultColumnProperty).HasDefaultValue(9);
+            });
+
         modelBuilder.Entity<TphParent>(
             b =>
             {
@@ -191,11 +200,12 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
                         .HasParameter(w => w.Id, pb => pb.IsOutput())
                         .HasParameter("Discriminator")
                         .HasParameter(w => w.Name)
-                        .HasParameter(nameof(TphChild.ChildProperty)));
+                        .HasParameter(nameof(TphChild1.Child1Property))
+                        .HasParameter(nameof(TphChild2.Child2InputProperty))
+                        .HasParameter(nameof(TphChild2.Child2OutputParameterProperty), o => o.IsOutput())
+                        .HasResultColumn(nameof(TphChild2.Child2ResultColumnProperty)));
             });
 
-        modelBuilder.Entity<TphChild>().ToTable("Tph");
-
         modelBuilder.Entity<TptParent>(
             b =>
             {

test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateTestBase.cs

@@ -514,7 +514,7 @@ public virtual async Task Tph(bool async)
     {
         await using var context = CreateContext();
 
-        var entity1 = new TphChild { Name = "Child", ChildProperty = 8 };
+        var entity1 = new TphChild1 { Name = "Child", Child1Property = 8 };
         context.TphChild.Add(entity1);
         await SaveChanges(context, async);
 
@@ -525,7 +525,7 @@ public virtual async Task Tph(bool async)
             var entity2 = context.TphChild.Single(b => b.Id == entity1.Id);
 
             Assert.Equal("Child", entity2.Name);
-            Assert.Equal(8, entity2.ChildProperty);
+            Assert.Equal(8, entity2.Child1Property);
         }
     }
 

test/EFCore.SqlServer.FunctionalTests/Update/StoredProcedureUpdateSqlServerTest.cs

@@ -291,13 +291,15 @@ public override async Task Tph(bool async)
         await base.Tph(async);
 
         AssertSql(
-            @"@p0='1' (Direction = Output)
-@p1='TphChild' (Nullable = false) (Size = 4000)
+            @"@p0=NULL (Nullable = false) (Direction = Output) (DbType = Int32)
+@p1='TphChild1' (Nullable = false) (Size = 4000)
 @p2='Child' (Size = 4000)
 @p3='8' (Nullable = true)
+@p4=NULL (DbType = Int32)
+@p5=NULL (Direction = Output) (DbType = Int32)
 
 SET NOCOUNT ON;
-EXEC [Tph_Insert] @p0 OUTPUT, @p1, @p2, @p3;");
+EXEC [Tph_Insert] @p0 OUTPUT, @p1, @p2, @p3, @p4, @p5 OUTPUT;");
     }
 
     public override async Task Tpt(bool async)
@@ -535,10 +537,13 @@ AS BEGIN
 
 GO
 
-CREATE PROCEDURE Tph_Insert(@Id int OUT, @Discriminator varchar(max), @Name varchar(max), @ChildProperty int)
+CREATE PROCEDURE Tph_Insert(@Id int OUT, @Discriminator varchar(max), @Name varchar(max), @Child1Property int, @Child2InputProperty int, @Child2OutputParameterProperty int OUT)
 AS BEGIN
-    INSERT INTO [Tph] ([Discriminator], [Name], [ChildProperty]) VALUES (@Discriminator, @Name, @ChildProperty);
+    DECLARE @Table table ([Child2OutputParameterProperty] int);
+    INSERT INTO [Tph] ([Discriminator], [Name], [Child1Property], [Child2InputProperty]) OUTPUT [Inserted].[Child2OutputParameterProperty] INTO @Table VALUES (@Discriminator, @Name, @Child1Property, @Child2InputProperty);
     SET @Id = SCOPE_IDENTITY();
+    SELECT @Child2OutputParameterProperty = [Child2OutputParameterProperty] FROM @Table;
+    SELECT [Child2ResultColumnProperty] FROM [Tph] WHERE [Id] = @Id
 END;
 
 GO