Fix base parameter index calculation for sprocs

Fixes dotnet#28803

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;
  }
  }
  }

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)

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);