Skip to content

Commit

Permalink
Rework logic to support both result columns and output parameters
Browse files Browse the repository at this point in the history
  • Loading branch information
@roji
roji committed Aug 2, 2022
1 parent 8edfbcf commit 1735680
Show file tree
Hide file tree
Showing 9 changed files with 206 additions and 105 deletions.
114 changes: 64 additions & 50 deletions src/EFCore.Relational/Update/AffectedCountModificationCommandBatch.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ protected override void Consume(RelationalDataReader reader)

try
{
var onFirstResultSet = true;
bool? onResultSet = null;

for (; commandIndex < CommandResultSet.Count; commandIndex++)
{
Expand All @@ -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)
{
Expand Down Expand Up @@ -121,7 +134,7 @@ protected override async Task ConsumeAsync(

try
{
var onFirstResultSet = true;
bool? onResultSet = null;

for (; commandIndex < CommandResultSet.Count; commandIndex++)
{
Expand All @@ -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)
{
Expand All @@ -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))
Expand All @@ -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++;
Expand Down Expand Up @@ -253,7 +270,6 @@ protected virtual async Task<int> 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))
Expand All @@ -276,8 +292,6 @@ await ThrowAggregateUpdateConcurrencyExceptionAsync(
Check.DebugAssert(command.RequiresResultPropagation, "RequiresResultPropagation is false");

command.PropagateResults(reader);

// TODO: Rows affected via output parameter
}

rowsAffected++;
Expand Down
11 changes: 9 additions & 2 deletions src/EFCore.Relational/Update/IReadOnlyModificationCommand.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,16 @@ public interface IReadOnlyModificationCommand
public EntityState EntityState { get; }

/// <summary>
/// Reads values returned from the database in the given <paramref name="relationalReader" /> and propagates them back to into the
/// appropriate <see cref="IColumnModification" /> from which the values can be propagated on to tracked entities.
/// Reads result set columns returned from the database in the given <paramref name="relationalReader" /> and propagates them back
/// to into the appropriate <see cref="IColumnModification" /> from which the values can be propagated on to tracked entities.
/// </summary>
/// <param name="relationalReader">The relational reader containing the values read from the database.</param>
public void PropagateResults(RelationalDataReader relationalReader);

/// <summary>
/// Reads output parameters returned from the database in the given <paramref name="relationalReader" /> and propagates them back to
/// into the appropriate <see cref="IColumnModification" /> from which the values can be propagated on to tracked entities.
/// </summary>
/// <param name="relationalReader">The relational reader containing the values read from the database.</param>
public void PropagateOutputParameters(RelationalDataReader relationalReader);
}
57 changes: 37 additions & 20 deletions src/EFCore.Relational/Update/ModificationCommand.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

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

Expand Down
32 changes: 16 additions & 16 deletions src/EFCore.Relational/Update/UpdateSqlGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -323,18 +323,30 @@ 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).
// Note assumption here that the column modifications are ordered in the right way for the sproc, as specified in the user (see
// 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)
Expand All @@ -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;
}
}

Expand All @@ -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;
}

Expand Down
1 change: 1 addition & 0 deletions ...Specification.Tests/TestModels/StoredProcedureUpdateModel/StoredProcedureUpdateContext.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ public StoredProcedureUpdateContext(DbContextOptions options)

public DbSet<WithOutputParameter> WithOutputParameter { get; set; }
public DbSet<WithResultColumn> WithResultColumn { get; set; }
public DbSet<WithOutputParameterAndResultColumn> WithOutputParameterAndResultColumn { get; set; }

public static void Seed(StoredProcedureUpdateContext context)
{
Expand Down
11 changes: 11 additions & 0 deletions ...ication.Tests/TestModels/StoredProcedureUpdateModel/WithOutputParameterAndResultColumn.cs
View file
Original file line number Diff line number Diff line change
@@ -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; }
}
Loading

0 comments on commit 1735680

Please sign in to comment.