Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix base parameter index calculation for sprocs #28850

Merged
merged 1 commit into from
Aug 23, 2022
Merged

Fix base parameter index calculation for sprocs #28850

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 22 additions & 32 deletions src/EFCore.Relational/Update/AffectedCountModificationCommandBatch.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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;
}
}
}
Expand Down Expand Up @@ -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)
Expand All @@ -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;
}
}
}
Expand Down
23 changes: 23 additions & 0 deletions test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
14 changes: 14 additions & 0 deletions test/EFCore.SqlServer.FunctionalTests/Update/StoredProcedureUpdateSqlServerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down