From 1735680f8797b6dd28245229c17f4985f278548c Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Mon, 1 Aug 2022 20:52:57 +0200 Subject: [PATCH] Rework logic to support both result columns and output parameters --- .../AffectedCountModificationCommandBatch.cs | 114 ++++++++++-------- .../Update/IReadOnlyModificationCommand.cs | 11 +- .../Update/ModificationCommand.cs | 57 ++++++--- .../Update/UpdateSqlGenerator.cs | 32 ++--- .../StoredProcedureUpdateContext.cs | 1 + .../WithOutputParameterAndResultColumn.cs | 11 ++ .../StoredProcedureUpdateFixtureBase.cs | 32 +++-- .../Update/StoredProcedureUpdateTestBase.cs | 18 +++ .../StoredProcedureUpdateSqlServerTest.cs | 35 ++++-- 9 files changed, 206 insertions(+), 105 deletions(-) create mode 100644 test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/WithOutputParameterAndResultColumn.cs diff --git a/src/EFCore.Relational/Update/AffectedCountModificationCommandBatch.cs b/src/EFCore.Relational/Update/AffectedCountModificationCommandBatch.cs index afbc602ed79..93b6d983247 100644 --- a/src/EFCore.Relational/Update/AffectedCountModificationCommandBatch.cs +++ b/src/EFCore.Relational/Update/AffectedCountModificationCommandBatch.cs @@ -42,7 +42,7 @@ protected override void Consume(RelationalDataReader reader) try { - var onFirstResultSet = true; + bool? onResultSet = null; for (; commandIndex < CommandResultSet.Count; commandIndex++) { @@ -52,46 +52,59 @@ protected override void Consume(RelationalDataReader reader) if (resultSetMapping.HasFlag(ResultSetMapping.HasResultRow)) { - if (onFirstResultSet) + if (onResultSet == false) { - onFirstResultSet = false; - } - else - { - if (!reader.DbDataReader.NextResult()) - { - Check.DebugFail("Missing a result set"); - } + Check.DebugFail("Missing a result set"); } - var newCommandIndex = requiresResultPropagation + var lastHandledCommandIndex = requiresResultPropagation ? ConsumeResultSetWithPropagation(commandIndex, reader) : ConsumeResultSetWithoutPropagation(commandIndex, reader); Check.DebugAssert(resultSetMapping.HasFlag(ResultSetMapping.LastInResultSet) - ? newCommandIndex == commandIndex - : newCommandIndex > commandIndex, "Bad handling of ResultSetMapping and command indexing"); + ? lastHandledCommandIndex == commandIndex + : lastHandledCommandIndex > commandIndex, "Bad handling of ResultSetMapping and command indexing"); + + onResultSet = reader.DbDataReader.NextResult(); + + // Process all output parameters for the rows in the previous result set (they're not populated until NextResult + // is called on SQL Server) + // TODO: Maybe record if there's any sprocs in the batch, and if not, skip this + while (true) + { + if (resultSetMapping.HasFlag(ResultSetMapping.HasOutputParameters)) + { + HandleOutputParameters(); + } + + if (commandIndex == lastHandledCommandIndex) + { + break; + } - commandIndex = newCommandIndex; + resultSetMapping = CommandResultSet[++commandIndex]; + command = ModificationCommands[commandIndex]; + requiresResultPropagation = command.RequiresResultPropagation; + } } else if (resultSetMapping.HasFlag(ResultSetMapping.HasOutputParameters)) { - // Handle output parameters for commands which have no result rows. - // Commands with result rows are handled inside ConsumeResultSet* above + // Handle output parameters for commands which have no result rows. Commands with result rows are handled above. + HandleOutputParameters(); + } + + void HandleOutputParameters() + { if (requiresResultPropagation) { - command.PropagateResults(reader); + command.PropagateOutputParameters(reader); } // TODO: Rows affected via output parameter } } - // No further result sets are expected. Call NextResult to bubble up any exception (e.g. from trailing commands which didn't - // generate a result set). - commandIndex = CommandResultSet.Count - 1; - var unexpectedResultSet = reader.DbDataReader.NextResult(); - Debug.Assert(!unexpectedResultSet, "Unexpected result set found at end"); + Debug.Assert(onResultSet != true, "Unexpected result set found at end"); } catch (Exception ex) when (ex is not DbUpdateException and not OperationCanceledException) { @@ -121,7 +134,7 @@ protected override async Task ConsumeAsync( try { - var onFirstResultSet = true; + bool? onResultSet = null; for (; commandIndex < CommandResultSet.Count; commandIndex++) { @@ -131,46 +144,53 @@ protected override async Task ConsumeAsync( if (resultSetMapping.HasFlag(ResultSetMapping.HasResultRow)) { - if (onFirstResultSet) + if (onResultSet == false) { - onFirstResultSet = false; - } - else - { - if (await reader.DbDataReader.NextResultAsync(cancellationToken).ConfigureAwait(false) == false) - { - Check.DebugFail("Missing a result set"); - } + Check.DebugFail("Missing a result set"); } - var newCommandIndex = requiresResultPropagation + var lastHandledCommandIndex = requiresResultPropagation ? await ConsumeResultSetWithPropagationAsync(commandIndex, reader, cancellationToken).ConfigureAwait(false) : await ConsumeResultSetWithoutPropagationAsync(commandIndex, reader, cancellationToken).ConfigureAwait(false); Check.DebugAssert(resultSetMapping.HasFlag(ResultSetMapping.LastInResultSet) - ? newCommandIndex == commandIndex - : newCommandIndex > commandIndex, "Bad handling of ResultSetMapping and command indexing"); + ? lastHandledCommandIndex == commandIndex + : lastHandledCommandIndex > commandIndex, "Bad handling of ResultSetMapping and command indexing"); - commandIndex = newCommandIndex; + onResultSet = await reader.DbDataReader.NextResultAsync(cancellationToken).ConfigureAwait(false); + + // Process all output parameters for the rows in the previous result set (they're not populated until NextResult + // is called on SQL Server) + // TODO: Maybe record if there's any sprocs in the batch, and if not, skip this + while (true) + { + if (command.StoredProcedure is not null) + { + command.PropagateOutputParameters(reader); + // TODO: Rows affected via output parameter + } + + if (commandIndex == lastHandledCommandIndex) + { + break; + } + + command = ModificationCommands[++commandIndex]; + } } else if (resultSetMapping.HasFlag(ResultSetMapping.HasOutputParameters)) { - // Handle output parameters for commands which have no result rows. - // Commands with result rows are handled inside ConsumeResultSet* above + // Handle output parameters for commands which have no result rows. Commands with result rows are handled above. if (requiresResultPropagation) { - command.PropagateResults(reader); + command.PropagateOutputParameters(reader); } // TODO: Rows affected via output parameter } } - // No further result sets are expected. Call NextResult to bubble up any exception (e.g. from trailing commands which didn't - // generate a result set). - commandIndex = CommandResultSet.Count - 1; - var unexpectedResultSet = await reader.DbDataReader.NextResultAsync(cancellationToken).ConfigureAwait(false); - Debug.Assert(!unexpectedResultSet, "Unexpected result set found at end"); + Debug.Assert(onResultSet != true, "Unexpected result set found at end"); } catch (Exception ex) when (ex is not DbUpdateException and not OperationCanceledException) { @@ -196,7 +216,6 @@ protected virtual int ConsumeResultSetWithPropagation(int startCommandIndex, Rel { if (!reader.Read()) { - // TODO: Handle output parameters here too? var expectedRowsAffected = rowsAffected + 1; while (++commandIndex < CommandResultSet.Count && CommandResultSet[commandIndex - 1].HasFlag(ResultSetMapping.NotLastInResultSet)) @@ -218,8 +237,6 @@ protected virtual int ConsumeResultSetWithPropagation(int startCommandIndex, Rel Check.DebugAssert(command.RequiresResultPropagation, "RequiresResultPropagation is false"); command.PropagateResults(reader); - - // TODO: Rows affected via output parameter } rowsAffected++; @@ -253,7 +270,6 @@ protected virtual async Task ConsumeResultSetWithPropagationAsync( { if (!await reader.ReadAsync(cancellationToken).ConfigureAwait(false)) { - // TODO: Handle output parameters here too? var expectedRowsAffected = rowsAffected + 1; while (++commandIndex < CommandResultSet.Count && CommandResultSet[commandIndex - 1].HasFlag(ResultSetMapping.NotLastInResultSet)) @@ -276,8 +292,6 @@ await ThrowAggregateUpdateConcurrencyExceptionAsync( Check.DebugAssert(command.RequiresResultPropagation, "RequiresResultPropagation is false"); command.PropagateResults(reader); - - // TODO: Rows affected via output parameter } rowsAffected++; diff --git a/src/EFCore.Relational/Update/IReadOnlyModificationCommand.cs b/src/EFCore.Relational/Update/IReadOnlyModificationCommand.cs index 6f650a539a4..09bdcfd16b4 100644 --- a/src/EFCore.Relational/Update/IReadOnlyModificationCommand.cs +++ b/src/EFCore.Relational/Update/IReadOnlyModificationCommand.cs @@ -62,9 +62,16 @@ public interface IReadOnlyModificationCommand public EntityState EntityState { get; } /// - /// Reads values returned from the database in the given and propagates them back to into the - /// appropriate from which the values can be propagated on to tracked entities. + /// Reads result set columns returned from the database in the given and propagates them back + /// to into the appropriate from which the values can be propagated on to tracked entities. /// /// The relational reader containing the values read from the database. public void PropagateResults(RelationalDataReader relationalReader); + + /// + /// Reads output parameters returned from the database in the given and propagates them back to + /// into the appropriate from which the values can be propagated on to tracked entities. + /// + /// The relational reader containing the values read from the database. + public void PropagateOutputParameters(RelationalDataReader relationalReader); } diff --git a/src/EFCore.Relational/Update/ModificationCommand.cs b/src/EFCore.Relational/Update/ModificationCommand.cs index b81b907a970..11eee3f58d5 100644 --- a/src/EFCore.Relational/Update/ModificationCommand.cs +++ b/src/EFCore.Relational/Update/ModificationCommand.cs @@ -485,34 +485,51 @@ public virtual void PropagateResults(RelationalDataReader relationalReader) { var columnModification = ColumnModifications[columnIndex]; - if (!columnModification.IsRead) + // Stored procedure output parameters are only propagated later, since they're populated when moving to the next result set + if (!columnModification.IsRead || columnModification.Column is IStoreStoredProcedureParameter) { continue; } Check.DebugAssert( - columnModification.Property is not null, "No property when propagating results to a readable column modification"); + columnModification.Property is not null, "No property on when propagating results to a readable column modification"); - if (columnModification.Column is IStoreStoredProcedureParameter storedProcedureParameter) - { - Check.DebugAssert(storedProcedureParameter.Direction != ParameterDirection.Input, - "Readable column modification has a stored procedure parameter with direction Input"); - Check.DebugAssert(columnModification.ParameterName is not null, - "Readable column modification has an stored procedure parameter without a name"); - - // TODO: Temporary hack. IColumnModification may need to reference the TypeMappedRelationalParameter in order for us - // to get the non-invariant name. - // Another option is to put the invariant name in the DbParameterCollection (without the prefix); the prefix isn't needed - // there AFAIK (but should check this is OK across databases. - var prefixedName = "@" + columnModification.ParameterName; - var dbParameter = relationalReader.DbCommand.Parameters[prefixedName]; - columnModification.Value = dbParameter.Value; - } - else + columnModification.Value = + columnModification.Property.GetReaderFieldValue(relationalReader, readerIndex++, _detailedErrorsEnabled); + } + } + + /// + public virtual void PropagateOutputParameters(RelationalDataReader relationalReader) + { + // Note that this call sets the value into a sidecar and will only commit to the actual entity + // if SaveChanges is successful. + var columnCount = ColumnModifications.Count; + + for (var columnIndex = 0; columnIndex < columnCount; columnIndex++) + { + var columnModification = ColumnModifications[columnIndex]; + + if (!columnModification.IsRead || columnModification.Column is not IStoreStoredProcedureParameter storedProcedureParameter) { - columnModification.Value = - columnModification.Property.GetReaderFieldValue(relationalReader, readerIndex++, _detailedErrorsEnabled); + continue; } + + Check.DebugAssert( + columnModification.Property is not null, "No property when propagating results to a readable column modification"); + + Check.DebugAssert(storedProcedureParameter.Direction != ParameterDirection.Input, + "Readable column modification has a stored procedure parameter with direction Input"); + Check.DebugAssert(columnModification.ParameterName is not null, + "Readable column modification has an stored procedure parameter without a name"); + + // TODO: Temporary hack. IColumnModification may need to reference the TypeMappedRelationalParameter in order for us + // to get the non-invariant name. + // Another option is to put the invariant name in the DbParameterCollection (without the prefix); the prefix isn't needed + // there AFAIK (but should check this is OK across databases. + var prefixedName = "@" + columnModification.ParameterName; + var dbParameter = relationalReader.DbCommand.Parameters[prefixedName]; + columnModification.Value = dbParameter.Value; } } diff --git a/src/EFCore.Relational/Update/UpdateSqlGenerator.cs b/src/EFCore.Relational/Update/UpdateSqlGenerator.cs index 95a4c8de1c0..750272a2a37 100644 --- a/src/EFCore.Relational/Update/UpdateSqlGenerator.cs +++ b/src/EFCore.Relational/Update/UpdateSqlGenerator.cs @@ -323,8 +323,8 @@ public virtual ResultSetMapping AppendStoredProcedureCall( AppendStoredProcedureCallHeader(commandStringBuilder, storedProcedure); + var resultSetMapping = ResultSetMapping.NoResults; var first = true; - var hasOutputParameters = false; // TODO: Only positional parameter style supported for now; specifying parameter name in metadata is currently problematic because // IColumnBase.Name is non-nullable (see general comment). @@ -332,9 +332,21 @@ public virtual ResultSetMapping AppendStoredProcedureCall( // note in StoredProcedureMapping.AddParameterMapping) foreach (var columnModification in command.ColumnModifications) { - if (columnModification.Column is not IStoreStoredProcedureParameter parameter) + IStoreStoredProcedureParameter parameter; + + switch (columnModification.Column) { - continue; + case IStoreStoredProcedureParameter p: + parameter = p; + break; + + case IStoreStoredProcedureResultColumn: + resultSetMapping |= ResultSetMapping.LastInResultSet; + continue; + + default: + throw new InvalidOperationException( + $"Unexpected column type {columnModification.Column?.GetType().Name} in stored procedure call"); } if (first) @@ -356,7 +368,7 @@ public virtual ResultSetMapping AppendStoredProcedureCall( if (parameter.Direction is ParameterDirection.Output or ParameterDirection.InputOutput) { commandStringBuilder.Append(" OUTPUT"); - hasOutputParameters = true; + resultSetMapping |= ResultSetMapping.HasOutputParameters; } } @@ -365,18 +377,6 @@ public virtual ResultSetMapping AppendStoredProcedureCall( requiresTransaction = true; - var resultSetMapping = ResultSetMapping.NoResults; - - if (storedProcedure.ResultColumns.Any()) - { - resultSetMapping |= ResultSetMapping.LastInResultSet; - } - - if (hasOutputParameters) - { - resultSetMapping |= ResultSetMapping.HasOutputParameters; - } - return resultSetMapping; } diff --git a/test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/StoredProcedureUpdateContext.cs b/test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/StoredProcedureUpdateContext.cs index fbfce89cb5c..19ced2fb500 100644 --- a/test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/StoredProcedureUpdateContext.cs +++ b/test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/StoredProcedureUpdateContext.cs @@ -12,6 +12,7 @@ public StoredProcedureUpdateContext(DbContextOptions options) public DbSet WithOutputParameter { get; set; } public DbSet WithResultColumn { get; set; } + public DbSet WithOutputParameterAndResultColumn { get; set; } public static void Seed(StoredProcedureUpdateContext context) { diff --git a/test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/WithOutputParameterAndResultColumn.cs b/test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/WithOutputParameterAndResultColumn.cs new file mode 100644 index 00000000000..d30462e92e8 --- /dev/null +++ b/test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/WithOutputParameterAndResultColumn.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 WithOutputParameterAndResultColumn +{ + public int Id { get; set; } + public string Name { get; set; } + public int Computed { get; set; } +} diff --git a/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateFixtureBase.cs b/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateFixtureBase.cs index 2d08a125e2c..c2561280e15 100644 --- a/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateFixtureBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateFixtureBase.cs @@ -16,22 +16,34 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con { modelBuilder.Entity() .InsertUsingStoredProcedure( - "WithOutputParameter_Insert", b => b - .HasParameter(b => b.Name) - .HasParameter(b => b.Id, p => p.IsOutput())) + "WithOutputParameter_Insert", spb => spb + .HasParameter(w => w.Name) + .HasParameter(w => w.Id, pb => pb.IsOutput())) .UpdateUsingStoredProcedure( - "WithOutputParameter_Update", b => b - .HasParameter(b => b.Id) - .HasParameter(b => b.Name)) + "WithOutputParameter_Update", spb => spb + .HasParameter(w => w.Id) + .HasParameter(w => w.Name)) .DeleteUsingStoredProcedure( - "WithOutputParameter_Delete", b => b - .HasParameter(b => b.Id)); + "WithOutputParameter_Delete", spb => spb + .HasParameter(w => w.Id)); modelBuilder.Entity() .InsertUsingStoredProcedure( "WithResultColumn_Insert", spb => spb - .HasParameter(b => b.Name) - .HasResultColumn(b => b.Id)); + .HasParameter(w => w.Name) + .HasResultColumn(w => w.Id)); + + modelBuilder.Entity( + b => + { + b.Property(w => w.Computed).HasComputedColumnSql("8"); + + b.InsertUsingStoredProcedure( + "WithOutputParameterAndResultColumn_Insert", spb => spb + .HasParameter(w => w.Id, pb => pb.IsOutput()) + .HasParameter(w => w.Name) + .HasResultColumn(w => w.Computed)); + }); } protected override void Seed(StoredProcedureUpdateContext context) diff --git a/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateTestBase.cs b/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateTestBase.cs index 99a8ac8f3dc..c1fd425a034 100644 --- a/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateTestBase.cs @@ -69,6 +69,24 @@ public virtual async Task Insert_with_result_column(bool async) } } + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual async Task Insert_with_output_parameter_and_result_column(bool async) + { + await using var context = CreateContext(); + + var entity = new WithOutputParameterAndResultColumn { Name = "Foo" }; + context.WithOutputParameterAndResultColumn.Add(entity); + await SaveChanges(context, async); + + Assert.Equal(8, entity.Computed); + + using (Fixture.TestSqlLoggerFactory.SuspendRecordingEvents()) + { + Assert.Equal("Foo", context.WithOutputParameterAndResultColumn.Single(b => b.Id == entity.Id).Name); + } + } + [ConditionalTheory] [MemberData(nameof(IsAsyncData))] public virtual async Task Update(bool async) diff --git a/test/EFCore.SqlServer.FunctionalTests/Update/StoredProcedureUpdateSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Update/StoredProcedureUpdateSqlServerTest.cs index 48e4efa1433..028e02a9074 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Update/StoredProcedureUpdateSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Update/StoredProcedureUpdateSqlServerTest.cs @@ -53,6 +53,18 @@ public override async Task Insert_with_result_column(bool async) EXEC [WithResultColumn_Insert] @p0;"); } + public override async Task Insert_with_output_parameter_and_result_column(bool async) + { + await base.Insert_with_output_parameter_and_result_column(async); + + AssertSql( + @"@p0=NULL (Nullable = false) (Direction = Output) (DbType = Int32) +@p1='Foo' (Size = 4000) + +SET NOCOUNT ON; +EXEC [WithOutputParameterAndResultColumn_Insert] @p0 OUTPUT, @p1;"); + } + public override async Task Update(bool async) { await base.Update(async); @@ -128,26 +140,35 @@ public override TestStore GetOrCreate(string storeName) => SqlServerTestStore.GetOrCreateWithInitScript(storeName, InitScript); private const string InitScript = @" -CREATE OR ALTER PROCEDURE WithOutputParameter_Insert(@Name varchar(max), @Id int OUT) +CREATE PROCEDURE WithOutputParameter_Insert(@Name varchar(max), @Id int OUT) AS BEGIN -INSERT INTO [WithOutputParameter] ([Name]) VALUES (@Name); -SET @Id = SCOPE_IDENTITY(); + INSERT INTO [WithOutputParameter] ([Name]) VALUES (@Name); + SET @Id = SCOPE_IDENTITY(); END; GO -CREATE OR ALTER PROCEDURE WithOutputParameter_Update(@Id int, @Name varchar(max)) +CREATE PROCEDURE WithOutputParameter_Update(@Id int, @Name varchar(max)) AS UPDATE [WithOutputParameter] SET [Name] = @Name WHERE [Id] = @id; GO -CREATE OR ALTER PROCEDURE WithOutputParameter_Delete(@Id int) +CREATE PROCEDURE WithOutputParameter_Delete(@Id int) AS DELETE FROM [WithOutputParameter] WHERE [Id] = @Id; GO -CREATE OR ALTER PROCEDURE WithResultColumn_Insert(@Name varchar(max)) -AS INSERT INTO [WithResultColumn] ([Name]) OUTPUT inserted.[Id] VALUES (@Name);"; +CREATE PROCEDURE WithResultColumn_Insert(@Name varchar(max)) +AS INSERT INTO [WithResultColumn] ([Name]) OUTPUT inserted.[Id] VALUES (@Name); + +GO + +CREATE PROCEDURE WithOutputParameterAndResultColumn_Insert(@Id int OUT, @Name varchar(max)) +AS BEGIN + INSERT INTO [WithOutputParameterAndResultColumn] ([Name]) VALUES (@Name); + SET @Id = SCOPE_IDENTITY(); + SELECT [Computed] FROM [WithOutputParameterAndResultColumn] WHERE [Id] = @Id +END;"; } } }