From 53a3f3561c560667bf1e8b470ec76f75fbe8869a Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Tue, 23 Aug 2022 18:55:00 +0200 Subject: [PATCH] Fix base parameter index calculation for sprocs Fixes #28803 --- .../AffectedCountModificationCommandBatch.cs | 54 ++++++++----------- .../Update/StoredProcedureUpdateTestBase.cs | 23 ++++++++ .../StoredProcedureUpdateSqlServerTest.cs | 14 +++++ 3 files changed, 59 insertions(+), 32 deletions(-) diff --git a/src/EFCore.Relational/Update/AffectedCountModificationCommandBatch.cs b/src/EFCore.Relational/Update/AffectedCountModificationCommandBatch.cs index 5edd4fac4fe..9faca89b6a9 100644 --- a/src/EFCore.Relational/Update/AffectedCountModificationCommandBatch.cs +++ b/src/EFCore.Relational/Update/AffectedCountModificationCommandBatch.cs @@ -82,20 +82,25 @@ protected override void Consume(RelationalDataReader reader) reader.DbDataReader.Close(); var parameterCounter = 0; + IReadOnlyModificationCommand command; - for (commandIndex = 0; commandIndex < CommandResultSet.Count; commandIndex++) + for (commandIndex = 0; + commandIndex < CommandResultSet.Count; + commandIndex++, parameterCounter += command.StoreStoredProcedure!.Parameters.Count) { + command = ModificationCommands[commandIndex]; + if (!CommandResultSet[commandIndex].HasFlag(ResultSetMapping.HasOutputParameters)) { continue; } - var command = ModificationCommands[commandIndex]; - + // Note: we assume that the return value is the parameter at position 0, and skip it here for the purpose of calculating + // the right baseParameterIndex to pass to PropagateOutputParameters below. var rowsAffectedDbParameter = command.RowsAffectedColumn is IStoreStoredProcedureParameter rowsAffectedParameter ? reader.DbCommand.Parameters[parameterCounter + rowsAffectedParameter.Position] : command.StoreStoredProcedure!.ReturnValue is not null - ? reader.DbCommand.Parameters[parameterCounter] // TODO: Assumption that the return value is the 1st parameter. + ? reader.DbCommand.Parameters[parameterCounter++] : null; if (rowsAffectedDbParameter is not null) @@ -111,24 +116,14 @@ protected override void Consume(RelationalDataReader reader) else { throw new InvalidOperationException( - RelationalStrings.StoredProcedureRowsAffectedNotPopulated(command.StoreStoredProcedure!.SchemaQualifiedName)); + RelationalStrings.StoredProcedureRowsAffectedNotPopulated( + command.StoreStoredProcedure!.SchemaQualifiedName)); } } if (command.RequiresResultPropagation) { - // TODO: this assumes that the return value is the parameter at position 0. - // I think that this by-position logic may be getting too complicated... The alternative would be to have the column modification - // reference its DbParameter directly; we already "mutate" column modification for generating parameter names, so maybe this is - // ok... - if (command.StoreStoredProcedure!.ReturnValue is not null) - { - parameterCounter++; - } - command.PropagateOutputParameters(reader.DbCommand.Parameters, parameterCounter); - - parameterCounter += command.StoreStoredProcedure!.Parameters.Count; } } } @@ -202,20 +197,25 @@ protected override async Task ConsumeAsync( await reader.DbDataReader.CloseAsync().ConfigureAwait(false); var parameterCounter = 0; + IReadOnlyModificationCommand command; - for (commandIndex = 0; commandIndex < CommandResultSet.Count; commandIndex++) + for (commandIndex = 0; + commandIndex < CommandResultSet.Count; + commandIndex++, parameterCounter += command.StoreStoredProcedure!.Parameters.Count) { + command = ModificationCommands[commandIndex]; + if (!CommandResultSet[commandIndex].HasFlag(ResultSetMapping.HasOutputParameters)) { continue; } - var command = ModificationCommands[commandIndex]; - + // Note: we assume that the return value is the parameter at position 0, and skip it here for the purpose of calculating + // the right baseParameterIndex to pass to PropagateOutputParameters below. var rowsAffectedDbParameter = command.RowsAffectedColumn is IStoreStoredProcedureParameter rowsAffectedParameter ? reader.DbCommand.Parameters[parameterCounter + rowsAffectedParameter.Position] : command.StoreStoredProcedure!.ReturnValue is not null - ? reader.DbCommand.Parameters[parameterCounter] // TODO: Assumption that the return value is the 1st parameter. + ? reader.DbCommand.Parameters[parameterCounter++] : null; if (rowsAffectedDbParameter is not null) @@ -232,24 +232,14 @@ await ThrowAggregateUpdateConcurrencyExceptionAsync( else { throw new InvalidOperationException( - RelationalStrings.StoredProcedureRowsAffectedNotPopulated(command.StoreStoredProcedure!.SchemaQualifiedName)); + RelationalStrings.StoredProcedureRowsAffectedNotPopulated( + command.StoreStoredProcedure!.SchemaQualifiedName)); } } if (command.RequiresResultPropagation) { - // TODO: this assumes that the return value is the parameter at position 0. - // I think that this by-position logic may be getting too complicated... The alternative would be to have the column modification - // reference its DbParameter directly; we already "mutate" column modification for generating parameter names, so maybe this is - // ok... - if (command.StoreStoredProcedure!.ReturnValue is not null) - { - parameterCounter++; - } - command.PropagateOutputParameters(reader.DbCommand.Parameters, parameterCounter); - - parameterCounter += command.StoreStoredProcedure!.Parameters.Count; } } } diff --git a/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateTestBase.cs b/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateTestBase.cs index 4b931b81883..3878b662591 100644 --- a/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateTestBase.cs @@ -173,6 +173,29 @@ public virtual async Task Delete(bool async) } } + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual async Task Delete_and_insert(bool async) + { + await using var context = CreateContext(); + + var entity1 = new Entity { Name = "Entity1" }; + context.WithOutputParameter.Add(entity1); + await context.SaveChangesAsync(); + + ClearLog(); + + context.WithOutputParameter.Remove(entity1); + context.WithOutputParameter.Add(new Entity { Name = "Entity2" }); + await SaveChanges(context, async); + + using (Fixture.TestSqlLoggerFactory.SuspendRecordingEvents()) + { + Assert.Equal(0, await context.WithOutputParameter.CountAsync(b => b.Name == "Entity1")); + Assert.Equal(1, await context.WithOutputParameter.CountAsync(b => b.Name == "Entity2")); + } + } + [ConditionalTheory] [MemberData(nameof(IsAsyncData))] public virtual async Task Rows_affected_parameter(bool async) diff --git a/test/EFCore.SqlServer.FunctionalTests/Update/StoredProcedureUpdateSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Update/StoredProcedureUpdateSqlServerTest.cs index c05553eaf72..c962b77bd2b 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Update/StoredProcedureUpdateSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Update/StoredProcedureUpdateSqlServerTest.cs @@ -126,6 +126,20 @@ public override async Task Delete(bool async) EXEC [WithOutputParameter_Delete] @p0;"); } + public override async Task Delete_and_insert(bool async) + { + await base.Delete_and_insert(async); + + AssertSql( + @"@p0='1' +@p1='Entity2' (Size = 4000) +@p2='2' (Direction = Output) + +SET NOCOUNT ON; +EXEC [WithOutputParameter_Delete] @p0; +EXEC [WithOutputParameter_Insert] @p1, @p2 OUTPUT;"); + } + public override async Task Rows_affected_parameter(bool async) { await base.Rows_affected_parameter(async);