diff --git a/src/EFCore.Relational/Update/ColumnModificationParameters.cs b/src/EFCore.Relational/Update/ColumnModificationParameters.cs index 5c7f8cae0f1..f2103bbf740 100644 --- a/src/EFCore.Relational/Update/ColumnModificationParameters.cs +++ b/src/EFCore.Relational/Update/ColumnModificationParameters.cs @@ -198,7 +198,7 @@ public ColumnModificationParameters( /// Indicates whether the column is used in the WHERE clause when updating. /// Indicates whether potentially sensitive data (e.g. database values) can be logged. public ColumnModificationParameters( - IUpdateEntry entry, + IUpdateEntry? entry, IProperty? property, IColumnBase column, Func generateParameterName, diff --git a/src/EFCore.Relational/Update/ModificationCommand.cs b/src/EFCore.Relational/Update/ModificationCommand.cs index 69744ad6b91..feb167febd0 100644 --- a/src/EFCore.Relational/Update/ModificationCommand.cs +++ b/src/EFCore.Relational/Update/ModificationCommand.cs @@ -360,7 +360,6 @@ private List GenerateColumnModifications() var nonMainEntry = !_mainEntryAdded || entry != _entries[0]; - IEnumerable columnMappings; var optionalDependentWithAllNull = false; if (StoreStoredProcedure is null) @@ -371,25 +370,24 @@ private List 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)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; diff --git a/src/EFCore.Relational/Update/ReaderModificationCommandBatch.cs b/src/EFCore.Relational/Update/ReaderModificationCommandBatch.cs index ca0466d86d9..391c2859a69 100644 --- a/src/EFCore.Relational/Update/ReaderModificationCommandBatch.cs +++ b/src/EFCore.Relational/Update/ReaderModificationCommandBatch.cs @@ -266,25 +266,10 @@ public override void Complete(bool moreBatchesExpected) /// The modification command for which to add parameters. protected virtual void AddParameters(IReadOnlyModificationCommand modificationCommand) { - IEnumerable 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); } diff --git a/src/EFCore.Relational/Update/UpdateSqlGenerator.cs b/src/EFCore.Relational/Update/UpdateSqlGenerator.cs index 3a8c9cde012..818408c25ef 100644 --- a/src/EFCore.Relational/Update/UpdateSqlGenerator.cs +++ b/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) { diff --git a/src/EFCore.SqlServer/Update/Internal/SqlServerUpdateSqlGenerator.cs b/src/EFCore.SqlServer/Update/Internal/SqlServerUpdateSqlGenerator.cs index 4ae6e84480c..179379c6e68 100644 --- a/src/EFCore.SqlServer/Update/Internal/SqlServerUpdateSqlGenerator.cs +++ b/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) { diff --git a/test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/StoredProcedureUpdateContext.cs b/test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/StoredProcedureUpdateContext.cs index 41bef01f833..b2ebb87e1c0 100644 --- a/test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/StoredProcedureUpdateContext.cs +++ b/test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/StoredProcedureUpdateContext.cs @@ -53,7 +53,7 @@ public DbSet WithInputOutputParameterOnNonConcurrencyToken => Set(nameof(WithInputOutputParameterOnNonConcurrencyToken)); public DbSet TphParent { get; set; } - public DbSet TphChild { get; set; } + public DbSet TphChild { get; set; } public DbSet TptParent { get; set; } public DbSet TptChild { get; set; } public DbSet TptMixedParent { get; set; } diff --git a/test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/TphChild.cs b/test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/TphChild1.cs similarity index 73% rename from test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/TphChild.cs rename to test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/TphChild1.cs index acf252e5ede..1278bb97aca 100644 --- a/test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/TphChild.cs +++ b/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; } } diff --git a/test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/TphChild2.cs b/test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/TphChild2.cs new file mode 100644 index 00000000000..3fdf4059531 --- /dev/null +++ b/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; } +} diff --git a/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateFixtureBase.cs b/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateFixtureBase.cs index feef547609c..9463d4d6d11 100644 --- a/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateFixtureBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateFixtureBase.cs @@ -180,6 +180,15 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con .HasRowsAffectedReturnValue()); }); + modelBuilder.Entity(); + + modelBuilder.Entity( + b => + { + b.Property(w => w.Child2OutputParameterProperty).HasDefaultValue(8); + b.Property(w => w.Child2ResultColumnProperty).HasDefaultValue(9); + }); + modelBuilder.Entity( 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().ToTable("Tph"); - modelBuilder.Entity( b => { diff --git a/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateTestBase.cs b/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateTestBase.cs index 3878b662591..c127eef6ddf 100644 --- a/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateTestBase.cs +++ 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); } } diff --git a/test/EFCore.SqlServer.FunctionalTests/Update/StoredProcedureUpdateSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Update/StoredProcedureUpdateSqlServerTest.cs index c962b77bd2b..642e474132f 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Update/StoredProcedureUpdateSqlServerTest.cs +++ b/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