diff --git a/src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs b/src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs index f107a1caeea..cffd450b5a5 100644 --- a/src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs +++ b/src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs @@ -359,9 +359,9 @@ private static void ValidateSproc(IStoredProcedure sproc, string mappingStrategy var storeGeneratedProperties = storeObjectIdentifier.StoreObjectType switch { StoreObjectType.InsertStoredProcedure - => properties.Where(p => (p.Value.ValueGenerated & ValueGenerated.OnAdd) != 0).ToDictionary(p => p.Key, p => p.Value), + => properties.Where(p => p.Value.ValueGenerated.HasFlag(ValueGenerated.OnAdd)).ToDictionary(p => p.Key, p => p.Value), StoreObjectType.UpdateStoredProcedure - => properties.Where(p => (p.Value.ValueGenerated & ValueGenerated.OnUpdate) != 0).ToDictionary(p => p.Key, p => p.Value), + => properties.Where(p => p.Value.ValueGenerated.HasFlag(ValueGenerated.OnUpdate)).ToDictionary(p => p.Key, p => p.Value), _ => new Dictionary() }; @@ -574,18 +574,27 @@ private static void ValidateSproc(IStoredProcedure sproc, string mappingStrategy } } - var missedConcurrencyToken = originalValueProperties.Values.FirstOrDefault(p => p.IsConcurrencyToken); - if (missedConcurrencyToken != null - && storeObjectIdentifier.StoreObjectType != StoreObjectType.InsertStoredProcedure - && (sproc.IsRowsAffectedReturned - || sproc.FindRowsAffectedParameter() != null - || sproc.FindRowsAffectedResultColumn() != null)) + if (sproc.IsRowsAffectedReturned + || sproc.FindRowsAffectedParameter() != null + || sproc.FindRowsAffectedResultColumn() != null) { - throw new InvalidOperationException( - RelationalStrings.StoredProcedureConcurrencyTokenNotMapped( - entityType.DisplayName(), - storeObjectIdentifier.DisplayName(), - missedConcurrencyToken.Name)); + if (originalValueProperties.Values.FirstOrDefault(p => p.IsConcurrencyToken) is { } missedConcurrencyToken + && storeObjectIdentifier.StoreObjectType != StoreObjectType.InsertStoredProcedure) + { + throw new InvalidOperationException( + RelationalStrings.StoredProcedureConcurrencyTokenNotMapped( + entityType.DisplayName(), + storeObjectIdentifier.DisplayName(), + missedConcurrencyToken.Name)); + } + + if (sproc.ResultColumns.Any(c => c != sproc.FindRowsAffectedResultColumn())) + { + throw new InvalidOperationException( + RelationalStrings.StoredProcedureRowsAffectedWithResultColumns( + entityType.DisplayName(), + storeObjectIdentifier.DisplayName())); + } } } diff --git a/src/EFCore.Relational/Properties/RelationalStrings.Designer.cs b/src/EFCore.Relational/Properties/RelationalStrings.Designer.cs index 36282a2012c..37fdf1976fc 100644 --- a/src/EFCore.Relational/Properties/RelationalStrings.Designer.cs +++ b/src/EFCore.Relational/Properties/RelationalStrings.Designer.cs @@ -1675,6 +1675,14 @@ public static string StoredProcedureRowsAffectedReturnConflictingParameter(objec GetString("StoredProcedureRowsAffectedReturnConflictingParameter", nameof(sproc)), sproc); + /// + /// The entity type '{entityType}' is mapped to the stored procedure '{sproc}' which returns both result columns and a rows affected value. If the stored procedure returns result columns, a rows affected value isn't needed and can be safely removed. + /// + public static string StoredProcedureRowsAffectedWithResultColumns(object? entityType, object? sproc) + => string.Format( + GetString("StoredProcedureRowsAffectedWithResultColumns", nameof(entityType), nameof(sproc)), + entityType, sproc); + /// /// Both entity type '{entityType1}' and '{entityType2}' were configured to use '{sproc}', stored procedure sharing is not supported. Specify different names for the corresponding stored procedures. /// diff --git a/src/EFCore.Relational/Properties/RelationalStrings.resx b/src/EFCore.Relational/Properties/RelationalStrings.resx index 906dd230f13..f49dc141908 100644 --- a/src/EFCore.Relational/Properties/RelationalStrings.resx +++ b/src/EFCore.Relational/Properties/RelationalStrings.resx @@ -1051,6 +1051,9 @@ The stored procedure '{sproc}' cannot be configured to return the rows affected because a rows affected parameter or a rows affected result column for this stored procedure already exists. + + The entity type '{entityType}' is mapped to the stored procedure '{sproc}' which returns both result columns and a rows affected value. If the stored procedure returns result columns, a rows affected value isn't needed and can be safely removed. + Both entity type '{entityType1}' and '{entityType2}' were configured to use '{sproc}', stored procedure sharing is not supported. Specify different names for the corresponding stored procedures. diff --git a/src/EFCore.Relational/Update/AffectedCountModificationCommandBatch.cs b/src/EFCore.Relational/Update/AffectedCountModificationCommandBatch.cs index 1a2c0aec283..d5e6ece0848 100644 --- a/src/EFCore.Relational/Update/AffectedCountModificationCommandBatch.cs +++ b/src/EFCore.Relational/Update/AffectedCountModificationCommandBatch.cs @@ -34,8 +34,8 @@ protected AffectedCountModificationCommandBatch(ModificationCommandBatchFactoryD protected override void Consume(RelationalDataReader reader) { Check.DebugAssert( - CommandResultSet.Count == ModificationCommands.Count, - $"CommandResultSet.Count of {CommandResultSet.Count} != ModificationCommands.Count of {ModificationCommands.Count}"); + ResultSetMappings.Count == ModificationCommands.Count, + $"CommandResultSet.Count of {ResultSetMappings.Count} != ModificationCommands.Count of {ModificationCommands.Count}"); var commandIndex = 0; @@ -44,10 +44,9 @@ protected override void Consume(RelationalDataReader reader) bool? onResultSet = null; var hasOutputParameters = false; - for (; commandIndex < CommandResultSet.Count; commandIndex++) + for (; commandIndex < ResultSetMappings.Count; commandIndex++) { - var resultSetMapping = CommandResultSet[commandIndex]; - var command = ModificationCommands[commandIndex]; + var resultSetMapping = ResultSetMappings[commandIndex]; if (resultSetMapping.HasFlag(ResultSetMapping.HasResultRow)) { @@ -56,9 +55,9 @@ protected override void Consume(RelationalDataReader reader) throw new InvalidOperationException(RelationalStrings.MissingResultSetWhenSaving); } - var lastHandledCommandIndex = command.RequiresResultPropagation - ? ConsumeResultSetWithPropagation(commandIndex, reader) - : ConsumeResultSetWithoutPropagation(commandIndex, reader); + var lastHandledCommandIndex = resultSetMapping.HasFlag(ResultSetMapping.ResultSetWithRowsAffectedOnly) + ? ConsumeResultSetWithRowsAffectedOnly(commandIndex, reader) + : ConsumeResultSet(commandIndex, reader); Check.DebugAssert(resultSetMapping.HasFlag(ResultSetMapping.LastInResultSet) ? lastHandledCommandIndex == commandIndex @@ -88,12 +87,12 @@ protected override void Consume(RelationalDataReader reader) IReadOnlyModificationCommand command; for (commandIndex = 0; - commandIndex < CommandResultSet.Count; + commandIndex < ResultSetMappings.Count; commandIndex++, parameterCounter += command.StoreStoredProcedure!.Parameters.Count) { command = ModificationCommands[commandIndex]; - if (!CommandResultSet[commandIndex].HasFlag(ResultSetMapping.HasOutputParameters)) + if (!ResultSetMappings[commandIndex].HasFlag(ResultSetMapping.HasOutputParameters)) { continue; } @@ -124,10 +123,7 @@ protected override void Consume(RelationalDataReader reader) } } - if (command.RequiresResultPropagation) - { - command.PropagateOutputParameters(reader.DbCommand.Parameters, parameterCounter); - } + command.PropagateOutputParameters(reader.DbCommand.Parameters, parameterCounter); } } } @@ -152,8 +148,8 @@ protected override async Task ConsumeAsync( CancellationToken cancellationToken = default) { Check.DebugAssert( - CommandResultSet.Count == ModificationCommands.Count, - $"CommandResultSet.Count of {CommandResultSet.Count} != ModificationCommands.Count of {ModificationCommands.Count}"); + ResultSetMappings.Count == ModificationCommands.Count, + $"CommandResultSet.Count of {ResultSetMappings.Count} != ModificationCommands.Count of {ModificationCommands.Count}"); var commandIndex = 0; @@ -162,10 +158,9 @@ protected override async Task ConsumeAsync( bool? onResultSet = null; var hasOutputParameters = false; - for (; commandIndex < CommandResultSet.Count; commandIndex++) + for (; commandIndex < ResultSetMappings.Count; commandIndex++) { - var resultSetMapping = CommandResultSet[commandIndex]; - var command = ModificationCommands[commandIndex]; + var resultSetMapping = ResultSetMappings[commandIndex]; if (resultSetMapping.HasFlag(ResultSetMapping.HasResultRow)) { @@ -174,9 +169,9 @@ protected override async Task ConsumeAsync( throw new InvalidOperationException(RelationalStrings.MissingResultSetWhenSaving); } - var lastHandledCommandIndex = command.RequiresResultPropagation - ? await ConsumeResultSetWithPropagationAsync(commandIndex, reader, cancellationToken).ConfigureAwait(false) - : await ConsumeResultSetWithoutPropagationAsync(commandIndex, reader, cancellationToken).ConfigureAwait(false); + var lastHandledCommandIndex = resultSetMapping.HasFlag(ResultSetMapping.ResultSetWithRowsAffectedOnly) + ? await ConsumeResultSetWithRowsAffectedOnlyAsync(commandIndex, reader, cancellationToken).ConfigureAwait(false) + : await ConsumeResultSetAsync(commandIndex, reader, cancellationToken).ConfigureAwait(false); Check.DebugAssert(resultSetMapping.HasFlag(ResultSetMapping.LastInResultSet) ? lastHandledCommandIndex == commandIndex @@ -206,12 +201,12 @@ protected override async Task ConsumeAsync( IReadOnlyModificationCommand command; for (commandIndex = 0; - commandIndex < CommandResultSet.Count; + commandIndex < ResultSetMappings.Count; commandIndex++, parameterCounter += command.StoreStoredProcedure!.Parameters.Count) { command = ModificationCommands[commandIndex]; - if (!CommandResultSet[commandIndex].HasFlag(ResultSetMapping.HasOutputParameters)) + if (!ResultSetMappings[commandIndex].HasFlag(ResultSetMapping.HasOutputParameters)) { continue; } @@ -243,10 +238,7 @@ await ThrowAggregateUpdateConcurrencyExceptionAsync( } } - if (command.RequiresResultPropagation) - { - command.PropagateOutputParameters(reader.DbCommand.Parameters, parameterCounter); - } + command.PropagateOutputParameters(reader.DbCommand.Parameters, parameterCounter); } } } @@ -266,7 +258,7 @@ await ThrowAggregateUpdateConcurrencyExceptionAsync( /// The ordinal of the first command being consumed. /// The data reader. /// The ordinal of the next result set that must be consumed. - protected virtual int ConsumeResultSetWithPropagation(int startCommandIndex, RelationalDataReader reader) + protected virtual int ConsumeResultSet(int startCommandIndex, RelationalDataReader reader) { var commandIndex = startCommandIndex; var rowsAffected = 0; @@ -275,8 +267,8 @@ protected virtual int ConsumeResultSetWithPropagation(int startCommandIndex, Rel if (!reader.Read()) { var expectedRowsAffected = rowsAffected + 1; - while (++commandIndex < CommandResultSet.Count - && CommandResultSet[commandIndex - 1].HasFlag(ResultSetMapping.NotLastInResultSet)) + while (++commandIndex < ResultSetMappings.Count + && ResultSetMappings[commandIndex - 1].HasFlag(ResultSetMapping.NotLastInResultSet)) { expectedRowsAffected++; } @@ -285,22 +277,24 @@ protected virtual int ConsumeResultSetWithPropagation(int startCommandIndex, Rel } else { - var resultSetMapping = CommandResultSet[commandIndex]; + var resultSetMapping = ResultSetMappings[commandIndex]; var command = ModificationCommands[ resultSetMapping.HasFlag(ResultSetMapping.IsPositionalResultMappingEnabled) ? startCommandIndex + reader.DbDataReader.GetInt32(reader.DbDataReader.FieldCount - 1) : commandIndex]; - Check.DebugAssert(command.RequiresResultPropagation, "RequiresResultPropagation is false"); + Check.DebugAssert( + !resultSetMapping.HasFlag(ResultSetMapping.ResultSetWithRowsAffectedOnly), + "!resultSetMapping.HasFlag(ResultSetMapping.ResultSetWithRowsAffectedOnly)"); command.PropagateResults(reader); } rowsAffected++; } - while (++commandIndex < CommandResultSet.Count - && CommandResultSet[commandIndex - 1].HasFlag(ResultSetMapping.NotLastInResultSet)); + while (++commandIndex < ResultSetMappings.Count + && ResultSetMappings[commandIndex - 1].HasFlag(ResultSetMapping.NotLastInResultSet)); return commandIndex - 1; } @@ -317,7 +311,7 @@ protected virtual int ConsumeResultSetWithPropagation(int startCommandIndex, Rel /// The task contains the ordinal of the next command that must be consumed. /// /// If the is canceled. - protected virtual async Task ConsumeResultSetWithPropagationAsync( + protected virtual async Task ConsumeResultSetAsync( int startCommandIndex, RelationalDataReader reader, CancellationToken cancellationToken) @@ -329,8 +323,8 @@ protected virtual async Task ConsumeResultSetWithPropagationAsync( if (!await reader.ReadAsync(cancellationToken).ConfigureAwait(false)) { var expectedRowsAffected = rowsAffected + 1; - while (++commandIndex < CommandResultSet.Count - && CommandResultSet[commandIndex - 1].HasFlag(ResultSetMapping.NotLastInResultSet)) + while (++commandIndex < ResultSetMappings.Count + && ResultSetMappings[commandIndex - 1].HasFlag(ResultSetMapping.NotLastInResultSet)) { expectedRowsAffected++; } @@ -340,22 +334,24 @@ await ThrowAggregateUpdateConcurrencyExceptionAsync( } else { - var resultSetMapping = CommandResultSet[commandIndex]; + var resultSetMapping = ResultSetMappings[commandIndex]; var command = ModificationCommands[ resultSetMapping.HasFlag(ResultSetMapping.IsPositionalResultMappingEnabled) ? startCommandIndex + reader.DbDataReader.GetInt32(reader.DbDataReader.FieldCount - 1) : commandIndex]; - Check.DebugAssert(command.RequiresResultPropagation, "RequiresResultPropagation is false"); + Check.DebugAssert( + !resultSetMapping.HasFlag(ResultSetMapping.ResultSetWithRowsAffectedOnly), + "!resultSetMapping.HasFlag(ResultSetMapping.ResultSetWithRowsAffectedOnly)"); command.PropagateResults(reader); } rowsAffected++; } - while (++commandIndex < CommandResultSet.Count - && CommandResultSet[commandIndex - 1].HasFlag(ResultSetMapping.NotLastInResultSet)); + while (++commandIndex < ResultSetMappings.Count + && ResultSetMappings[commandIndex - 1].HasFlag(ResultSetMapping.NotLastInResultSet)); return commandIndex - 1; } @@ -367,13 +363,15 @@ await ThrowAggregateUpdateConcurrencyExceptionAsync( /// The ordinal of the command being consumed. /// The data reader. /// The ordinal of the next command that must be consumed. - protected virtual int ConsumeResultSetWithoutPropagation(int commandIndex, RelationalDataReader reader) + protected virtual int ConsumeResultSetWithRowsAffectedOnly(int commandIndex, RelationalDataReader reader) { var expectedRowsAffected = 1; - while (++commandIndex < CommandResultSet.Count - && CommandResultSet[commandIndex - 1].HasFlag(ResultSetMapping.NotLastInResultSet)) + while (++commandIndex < ResultSetMappings.Count + && ResultSetMappings[commandIndex - 1].HasFlag(ResultSetMapping.NotLastInResultSet)) { - Check.DebugAssert(!ModificationCommands[commandIndex].RequiresResultPropagation, "RequiresResultPropagation is true"); + Check.DebugAssert( + ResultSetMappings[commandIndex].HasFlag(ResultSetMapping.ResultSetWithRowsAffectedOnly), + "ResultSetMappings[commandIndex].HasFlag(ResultSetMapping.ResultSetWithRowsAffectedOnly)"); expectedRowsAffected++; } @@ -406,16 +404,18 @@ protected virtual int ConsumeResultSetWithoutPropagation(int commandIndex, Relat /// The task contains the ordinal of the next command that must be consumed. /// /// If the is canceled. - protected virtual async Task ConsumeResultSetWithoutPropagationAsync( + protected virtual async Task ConsumeResultSetWithRowsAffectedOnlyAsync( int commandIndex, RelationalDataReader reader, CancellationToken cancellationToken) { var expectedRowsAffected = 1; - while (++commandIndex < CommandResultSet.Count - && CommandResultSet[commandIndex - 1].HasFlag(ResultSetMapping.NotLastInResultSet)) + while (++commandIndex < ResultSetMappings.Count + && ResultSetMappings[commandIndex - 1].HasFlag(ResultSetMapping.NotLastInResultSet)) { - Check.DebugAssert(!ModificationCommands[commandIndex].RequiresResultPropagation, "RequiresResultPropagation is true"); + Check.DebugAssert( + ResultSetMappings[commandIndex].HasFlag(ResultSetMapping.ResultSetWithRowsAffectedOnly), + "ResultSetMappings[commandIndex].HasFlag(ResultSetMapping.ResultSetWithRowsAffectedOnly)"); expectedRowsAffected++; } diff --git a/src/EFCore.Relational/Update/ColumnModification.cs b/src/EFCore.Relational/Update/ColumnModification.cs index 1fb54784a11..4837e314799 100644 --- a/src/EFCore.Relational/Update/ColumnModification.cs +++ b/src/EFCore.Relational/Update/ColumnModification.cs @@ -1,6 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Data; + namespace Microsoft.EntityFrameworkCore.Update; /// @@ -90,7 +92,9 @@ public virtual bool UseOriginalValueParameter /// public virtual bool UseCurrentValueParameter - => (UseParameter && UseCurrentValue) || (IsRead && Column is IStoreStoredProcedureParameter or IStoreStoredProcedureReturnValue); + => (UseParameter && UseCurrentValue) + || (Column is IStoreStoredProcedureParameter { Direction: ParameterDirection.Output or ParameterDirection.InputOutput } + or IStoreStoredProcedureReturnValue); /// public virtual bool UseOriginalValue diff --git a/src/EFCore.Relational/Update/IReadOnlyModificationCommand.cs b/src/EFCore.Relational/Update/IReadOnlyModificationCommand.cs index b5d588cb1b4..deca7617a31 100644 --- a/src/EFCore.Relational/Update/IReadOnlyModificationCommand.cs +++ b/src/EFCore.Relational/Update/IReadOnlyModificationCommand.cs @@ -42,12 +42,6 @@ public interface IReadOnlyModificationCommand /// public IReadOnlyList ColumnModifications { get; } - /// - /// Indicates whether the database will return values for some mapped properties - /// that will then need to be propagated back to the tracked entities. - /// - public bool RequiresResultPropagation { get; } - /// /// The that represent the entities that are mapped to the row to update. /// diff --git a/src/EFCore.Relational/Update/ModificationCommand.cs b/src/EFCore.Relational/Update/ModificationCommand.cs index b9a9a011451..1372330ca08 100644 --- a/src/EFCore.Relational/Update/ModificationCommand.cs +++ b/src/EFCore.Relational/Update/ModificationCommand.cs @@ -34,7 +34,6 @@ public class ModificationCommand : IModificationCommand, INonTrackedModification private readonly IComparer? _comparer; private readonly List _entries = new(); private List? _columnModifications; - private bool _requiresResultPropagation; private bool _mainEntryAdded; private EntityState _entityState; private readonly IDiagnosticsLogger? _logger; @@ -96,21 +95,6 @@ public virtual EntityState EntityState /// public virtual IColumnBase? RowsAffectedColumn { get; private set; } - /// - /// Indicates whether the database will return values for some mapped properties - /// that will then need to be propagated back to the tracked entities. - /// - public virtual bool RequiresResultPropagation - { - get - { - // ReSharper disable once AssignmentIsFullyDiscarded - _ = ColumnModifications; - - return _requiresResultPropagation; - } - } - /// /// The list of needed to perform the insert, update, or delete. /// @@ -436,31 +420,26 @@ entry.EntityState is EntityState.Modified or EntityState.Added // Stored procedures may have an additional rows affected result column or return value, which does not have a // property/column mapping but still needs to have be represented via a column modification. - IColumnBase? rowsAffectedColumnBase = null; - + // Note that for rows affected parameters/result columns, we add column modifications below along with regular parameters/ + // result columns; for return value we do that here. if (storedProcedure.FindRowsAffectedParameter() is { } rowsAffectedParameter) { - rowsAffectedColumnBase = RowsAffectedColumn = rowsAffectedParameter.StoreParameter; + RowsAffectedColumn = rowsAffectedParameter.StoreParameter; } else if (storedProcedure.FindRowsAffectedResultColumn() is { } rowsAffectedResultColumn) { - rowsAffectedColumnBase = RowsAffectedColumn = rowsAffectedResultColumn.StoreResultColumn; + RowsAffectedColumn = rowsAffectedResultColumn.StoreResultColumn; } else if (storedProcedureMapping.StoreStoredProcedure.ReturnValue is { } rowsAffectedReturnValue) { - rowsAffectedColumnBase = rowsAffectedReturnValue; - } + RowsAffectedColumn = rowsAffectedReturnValue; - // Add a column modification for rows affected result column/return value. - // A rows affected output parameter is added below in the correct position, with the rest of the parameters. - if (rowsAffectedColumnBase is IStoreStoredProcedureResultColumn or IStoreStoredProcedureReturnValue) - { columnModifications.Add(CreateColumnModification(new ColumnModificationParameters( entry: null, property: null, - rowsAffectedColumnBase, + rowsAffectedReturnValue, _generateParameterName!, - rowsAffectedColumnBase.StoreTypeMapping, + rowsAffectedReturnValue.StoreTypeMapping, valueIsRead: true, valueIsWrite: false, columnIsKey: false, @@ -482,25 +461,41 @@ entry.EntityState is EntityState.Modified or EntityState.Added } // The parameter has no corresponding mapping; this is either a sibling property in a TPH hierarchy or a rows affected - // output parameter or return value. + // output parameter. Note that we set IsRead to false since we don't propagate the output parameter. columnModifications.Add(CreateColumnModification(new ColumnModificationParameters( entry: null, property: null, parameter, _generateParameterName!, parameter.StoreTypeMapping, - valueIsRead: parameter.Direction.HasFlag(ParameterDirection.Output), + valueIsRead: false, valueIsWrite: parameter.Direction.HasFlag(ParameterDirection.Input), columnIsKey: false, columnIsCondition: false, _sensitiveLoggingEnabled))); } - // Note that we only add column modifications for mapped result columns, even though the sproc may return additional result - // columns (e.g. for siblings in TPH). Our result propagation accesses result columns directly by their position. - foreach (var columnMapping in storedProcedureMapping.ResultColumnMappings) + foreach (var resultColumn in StoreStoredProcedure.ResultColumns) { - HandleColumnModification(columnMapping); + if (resultColumn.FindColumnMapping(entry.EntityType) is { } resultColumnMapping) + { + HandleColumnModification(resultColumnMapping); + continue; + } + + // The result column has no corresponding mapping; this is either a sibling property in a TPH hierarchy or a rows + // affected result column. Note that we set IsRead to false since we don't propagate the result column. + columnModifications.Add(CreateColumnModification(new ColumnModificationParameters( + entry: null, + property: null, + resultColumn, + _generateParameterName!, + resultColumn.StoreTypeMapping, + valueIsRead: false, + valueIsWrite: false, + columnIsKey: false, + columnIsCondition: false, + _sensitiveLoggingEnabled))); } } @@ -557,11 +552,6 @@ void HandleColumnModification(IColumnMappingBase columnMapping) || writeValue || isCondition) { - if (readValue) - { - _requiresResultPropagation = true; - } - var columnModificationParameters = new ColumnModificationParameters( entry, property, diff --git a/src/EFCore.Relational/Update/ReaderModificationCommandBatch.cs b/src/EFCore.Relational/Update/ReaderModificationCommandBatch.cs index 391c2859a69..6fc8474f3e0 100644 --- a/src/EFCore.Relational/Update/ReaderModificationCommandBatch.cs +++ b/src/EFCore.Relational/Update/ReaderModificationCommandBatch.cs @@ -86,7 +86,7 @@ public override IReadOnlyList ModificationCommands /// /// The s for each command in . /// - protected virtual IList CommandResultSet { get; } = new List(); + protected virtual IList ResultSetMappings { get; } = new List(); /// /// The store command generated from this batch when is called. @@ -107,7 +107,7 @@ public override bool TryAddCommand(IReadOnlyModificationCommand modificationComm } _sqlBuilderPosition = SqlBuilder.Length; - _commandResultSetCount = CommandResultSet.Count; + _commandResultSetCount = ResultSetMappings.Count; _pendingParameters = 0; AddCommand(modificationCommand); @@ -138,9 +138,9 @@ protected virtual void RollbackLastCommand(IReadOnlyModificationCommand modifica SqlBuilder.Length = _sqlBuilderPosition; - while (CommandResultSet.Count > _commandResultSetCount) + while (ResultSetMappings.Count > _commandResultSetCount) { - CommandResultSet.RemoveAt(CommandResultSet.Count - 1); + ResultSetMappings.RemoveAt(ResultSetMappings.Count - 1); } for (var i = 0; i < _pendingParameters; i++) @@ -195,11 +195,11 @@ protected virtual void AddCommand(IReadOnlyModificationCommand modificationComma { bool requiresTransaction; - var commandPosition = CommandResultSet.Count; + var commandPosition = ResultSetMappings.Count; if (modificationCommand.StoreStoredProcedure is not null) { - CommandResultSet.Add( + ResultSetMappings.Add( UpdateSqlGenerator.AppendStoredProcedureCall( SqlBuilder, modificationCommand, commandPosition, out requiresTransaction)); } @@ -208,17 +208,17 @@ protected virtual void AddCommand(IReadOnlyModificationCommand modificationComma switch (modificationCommand.EntityState) { case EntityState.Added: - CommandResultSet.Add( + ResultSetMappings.Add( UpdateSqlGenerator.AppendInsertOperation( SqlBuilder, modificationCommand, commandPosition, out requiresTransaction)); break; case EntityState.Modified: - CommandResultSet.Add( + ResultSetMappings.Add( UpdateSqlGenerator.AppendUpdateOperation( SqlBuilder, modificationCommand, commandPosition, out requiresTransaction)); break; case EntityState.Deleted: - CommandResultSet.Add( + ResultSetMappings.Add( UpdateSqlGenerator.AppendDeleteOperation( SqlBuilder, modificationCommand, commandPosition, out requiresTransaction)); break; diff --git a/src/EFCore.Relational/Update/ResultSetMapping.cs b/src/EFCore.Relational/Update/ResultSetMapping.cs index 499385f1f0b..fef1ac2f4e5 100644 --- a/src/EFCore.Relational/Update/ResultSetMapping.cs +++ b/src/EFCore.Relational/Update/ResultSetMapping.cs @@ -38,15 +38,20 @@ public enum ResultSetMapping /// LastInResultSet = 5, + /// + /// The command maps to a result set which contains only a single rows affected value. + /// + ResultSetWithRowsAffectedOnly = 9, + /// /// When rows with database-generated values are returned in non-deterministic ordering, it is necessary to project out a synthetic /// position value, in order to look up the correct and propagate the values. When this bit is /// enabled, the current result row contains such a position value. /// - IsPositionalResultMappingEnabled = 9, + IsPositionalResultMappingEnabled = 17, /// /// The command has output parameters. /// - HasOutputParameters = 16 + HasOutputParameters = 32 } diff --git a/src/EFCore.Relational/Update/UpdateSqlGenerator.cs b/src/EFCore.Relational/Update/UpdateSqlGenerator.cs index 9761d5a9cb8..ece5f7a2f12 100644 --- a/src/EFCore.Relational/Update/UpdateSqlGenerator.cs +++ b/src/EFCore.Relational/Update/UpdateSqlGenerator.cs @@ -132,11 +132,15 @@ protected virtual ResultSetMapping AppendUpdateReturningOperation( requiresTransaction = false; + var anyReadOperations = readOperations.Count > 0; + AppendUpdateCommand( commandStringBuilder, name, schema, writeOperations, readOperations, conditionOperations, - appendReturningOneClause: readOperations.Count == 0); + appendReturningOneClause: !anyReadOperations); - return ResultSetMapping.LastInResultSet; + return anyReadOperations + ? ResultSetMapping.LastInResultSet + : ResultSetMapping.LastInResultSet | ResultSetMapping.ResultSetWithRowsAffectedOnly; } /// @@ -177,7 +181,7 @@ protected virtual ResultSetMapping AppendDeleteReturningOperation( AppendDeleteCommand( commandStringBuilder, name, schema, Array.Empty(), conditionOperations, appendReturningOneClause: true); - return ResultSetMapping.LastInResultSet; + return ResultSetMapping.LastInResultSet | ResultSetMapping.ResultSetWithRowsAffectedOnly; } /// @@ -357,9 +361,23 @@ public virtual ResultSetMapping AppendStoredProcedureCall( Check.DebugAssert(command.StoreStoredProcedure is not null, "command.StoredProcedure is not null"); var storedProcedure = command.StoreStoredProcedure; - var resultSetMapping = storedProcedure.ResultColumns.Any() - ? ResultSetMapping.LastInResultSet - : ResultSetMapping.NoResults; + + var resultSetMapping = ResultSetMapping.NoResults; + + foreach (var resultColumn in storedProcedure.ResultColumns) + { + resultSetMapping = ResultSetMapping.LastInResultSet; + + if (resultColumn == command.RowsAffectedColumn) + { + resultSetMapping |= ResultSetMapping.ResultSetWithRowsAffectedOnly; + } + else + { + resultSetMapping = ResultSetMapping.LastInResultSet; + break; + } + } Check.DebugAssert(storedProcedure.Parameters.Any() || storedProcedure.ResultColumns.Any(), "Stored procedure call with neither parameters nor result columns"); diff --git a/src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommandBatch.cs b/src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommandBatch.cs index d79876d8a32..27a78ead38f 100644 --- a/src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommandBatch.cs +++ b/src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommandBatch.cs @@ -102,7 +102,7 @@ private void ApplyPendingBulkInsertCommands() return; } - var commandPosition = CommandResultSet.Count; + var commandPosition = ResultSetMappings.Count; var wasCachedCommandTextEmpty = IsCommandTextEmpty; @@ -113,12 +113,12 @@ private void ApplyPendingBulkInsertCommands() for (var i = 0; i < _pendingBulkInsertCommands.Count; i++) { - CommandResultSet.Add(resultSetMapping); + ResultSetMappings.Add(resultSetMapping); } if (resultSetMapping != ResultSetMapping.NoResults) { - CommandResultSet[^1] = ResultSetMapping.LastInResultSet; + ResultSetMappings[^1] = ResultSetMapping.LastInResultSet; } } diff --git a/src/EFCore.SqlServer/Update/Internal/SqlServerUpdateSqlGenerator.cs b/src/EFCore.SqlServer/Update/Internal/SqlServerUpdateSqlGenerator.cs index 61a19ca7da5..584e4e6e47d 100644 --- a/src/EFCore.SqlServer/Update/Internal/SqlServerUpdateSqlGenerator.cs +++ b/src/EFCore.SqlServer/Update/Internal/SqlServerUpdateSqlGenerator.cs @@ -614,9 +614,23 @@ public override ResultSetMapping AppendStoredProcedureCall( Check.DebugAssert(command.StoreStoredProcedure is not null, "command.StoredProcedure is not null"); var storedProcedure = command.StoreStoredProcedure; - var resultSetMapping = storedProcedure.ResultColumns.Any() - ? ResultSetMapping.LastInResultSet - : ResultSetMapping.NoResults; + + var resultSetMapping = ResultSetMapping.NoResults; + + foreach (var resultColumn in storedProcedure.ResultColumns) + { + resultSetMapping = ResultSetMapping.LastInResultSet; + + if (resultColumn == command.RowsAffectedColumn) + { + resultSetMapping |= ResultSetMapping.ResultSetWithRowsAffectedOnly; + } + else + { + resultSetMapping = ResultSetMapping.LastInResultSet; + break; + } + } Check.DebugAssert(storedProcedure.Parameters.Any() || storedProcedure.ResultColumns.Any(), "Stored procedure call with neither parameters nor result columns"); @@ -933,7 +947,7 @@ protected override ResultSetMapping AppendSelectAffectedCountCommand( .AppendLine(SqlGenerationHelper.StatementTerminator) .AppendLine(); - return ResultSetMapping.LastInResultSet; + return ResultSetMapping.LastInResultSet | ResultSetMapping.ResultSetWithRowsAffectedOnly; } /// diff --git a/src/EFCore.Sqlite.Core/Update/Internal/SqliteLegacyUpdateSqlGenerator.cs b/src/EFCore.Sqlite.Core/Update/Internal/SqliteLegacyUpdateSqlGenerator.cs index eecf2cfb4dc..bfa134fb49b 100644 --- a/src/EFCore.Sqlite.Core/Update/Internal/SqliteLegacyUpdateSqlGenerator.cs +++ b/src/EFCore.Sqlite.Core/Update/Internal/SqliteLegacyUpdateSqlGenerator.cs @@ -58,7 +58,7 @@ protected override ResultSetMapping AppendSelectAffectedCountCommand(StringBuild .AppendLine(SqlGenerationHelper.StatementTerminator) .AppendLine(); - return ResultSetMapping.LastInResultSet; + return ResultSetMapping.LastInResultSet | ResultSetMapping.ResultSetWithRowsAffectedOnly; } /// diff --git a/test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/StoredProcedureUpdateContext.cs b/test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/StoredProcedureUpdateContext.cs index 83a22de60d3..03de6ed0761 100644 --- a/test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/StoredProcedureUpdateContext.cs +++ b/test/EFCore.Relational.Specification.Tests/TestModels/StoredProcedureUpdateModel/StoredProcedureUpdateContext.cs @@ -25,9 +25,6 @@ public DbSet WithOutputParameterAndResultColumn public DbSet WithOutputParameterAndRowsAffectedResultColumn => Set(nameof(WithOutputParameterAndRowsAffectedResultColumn)); - public DbSet WithOutputParameterAndResultColumnAndResultValue - => Set(nameof(WithOutputParameterAndResultColumnAndResultValue)); - public DbSet WithTwoOutputParameters => Set(nameof(WithTwoOutputParameters)); diff --git a/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateFixtureBase.cs b/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateFixtureBase.cs index d3e98524fd1..d5a9ec54f3b 100644 --- a/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateFixtureBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateFixtureBase.cs @@ -178,23 +178,6 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con .HasParameter(w => w.Name, pb => pb.IsInputOutput())); }); - modelBuilder.SharedTypeEntity( - nameof(StoredProcedureUpdateContext.WithOutputParameterAndResultColumnAndResultValue), - b => - { - b.Property(w => w.AdditionalProperty1).HasComputedColumnSql("8"); - b.Property(w => w.AdditionalProperty2).HasComputedColumnSql("9"); - - b.UpdateUsingStoredProcedure( - nameof(StoredProcedureUpdateContext.WithOutputParameterAndResultColumnAndResultValue) + "_Update", - spb => spb - .HasParameter(w => w.Id) - .HasParameter(w => w.Name) - .HasParameter(w => w.AdditionalProperty1, pb => pb.IsOutput()) - .HasResultColumn(w => w.AdditionalProperty2) - .HasRowsAffectedReturnValue()); - }); - modelBuilder.Entity(); modelBuilder.Entity( @@ -281,7 +264,6 @@ public virtual void CleanData() context.WithResultColumn.RemoveRange(context.WithResultColumn); context.WithTwoResultColumns.RemoveRange(context.WithTwoResultColumns); context.WithOutputParameterAndResultColumn.RemoveRange(context.WithOutputParameterAndResultColumn); - context.WithOutputParameterAndResultColumnAndResultValue.RemoveRange(context.WithOutputParameterAndResultColumnAndResultValue); context.WithTwoOutputParameters.RemoveRange(context.WithTwoOutputParameters); context.WithRowsAffectedParameter.RemoveRange(context.WithRowsAffectedParameter); context.WithRowsAffectedResultColumn.RemoveRange(context.WithRowsAffectedResultColumn); diff --git a/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateTestBase.cs b/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateTestBase.cs index 8ffbcec4085..3a01d33ac10 100644 --- a/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateTestBase.cs @@ -177,6 +177,32 @@ public virtual async Task Update_with_output_parameter_and_rows_affected_result_ } } + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual async Task Update_with_output_parameter_and_rows_affected_result_column_concurrency_failure(bool async) + { + await using var context1 = CreateContext(); + + var entity1 = new EntityWithAdditionalProperty { Name = "Initial" }; + context1.WithOutputParameterAndRowsAffectedResultColumn.Add(entity1); + await context1.SaveChangesAsync(); + + await using (var context2 = CreateContext()) + { + var entity2 = await context2.WithOutputParameterAndRowsAffectedResultColumn.SingleAsync(w => w.Name == "Initial"); + context2.WithOutputParameterAndRowsAffectedResultColumn.Remove(entity2); + await context2.SaveChangesAsync(); + } + + ClearLog(); + + entity1.Name = "Updated"; + + var exception = await Assert.ThrowsAsync(async () => await SaveChanges(context1, async)); + var entry = exception.Entries.Single(); + Assert.Same(entity1, entry.Entity); + } + [ConditionalTheory] [MemberData(nameof(IsAsyncData))] public virtual async Task Delete(bool async) @@ -503,36 +529,6 @@ public virtual async Task Input_output_parameter_on_non_concurrency_token(bool a } } - [ConditionalTheory] - [MemberData(nameof(IsAsyncData))] - public virtual async Task Update_with_output_parameter_and_result_column_and_return_value(bool async) - { - await using var context = CreateContext(); - - var entity1 = new EntityWithTwoAdditionalProperties { Name = "Foo" }; - context.WithOutputParameterAndResultColumnAndResultValue.Add(entity1); - await SaveChanges(context, async); - - // Clear and attach a new instance to make sure we properly receive and populate the computed properties when updating below - context.ChangeTracker.Clear(); - - var entity2 = new EntityWithTwoAdditionalProperties { Id = entity1.Id }; - context.WithOutputParameterAndResultColumnAndResultValue.Attach(entity2); - entity2.Name = "Updated"; - - ClearLog(); - - await SaveChanges(context, async); - - Assert.Equal(8, entity2.AdditionalProperty1); - Assert.Equal(9, entity2.AdditionalProperty2); - - using (Fixture.TestSqlLoggerFactory.SuspendRecordingEvents()) - { - Assert.Equal("Updated", context.WithOutputParameterAndResultColumnAndResultValue.Single(b => b.Id == entity1.Id).Name); - } - } - [ConditionalTheory] [MemberData(nameof(IsAsyncData))] public virtual async Task Tph(bool async) diff --git a/test/EFCore.Relational.Tests/Infrastructure/RelationalModelValidatorTest.cs b/test/EFCore.Relational.Tests/Infrastructure/RelationalModelValidatorTest.cs index 2d677e3f8a6..8f20fb55884 100644 --- a/test/EFCore.Relational.Tests/Infrastructure/RelationalModelValidatorTest.cs +++ b/test/EFCore.Relational.Tests/Infrastructure/RelationalModelValidatorTest.cs @@ -2918,6 +2918,24 @@ public virtual void Detects_unmapped_concurrency_token() modelBuilder); } + [ConditionalFact] + public virtual void Detects_rows_affected_with_result_columns() + { + var modelBuilder = CreateConventionModelBuilder(); + modelBuilder.Entity() + .UpdateUsingStoredProcedure( + s => s + .HasParameter(a => a.Id) + .HasParameter("FavoritePersonId") + .HasResultColumn(a => a.Name) + .HasRowsAffectedReturnValue()) + .Property(a => a.Name).ValueGeneratedOnUpdate(); + + VerifyError( + RelationalStrings.StoredProcedureRowsAffectedWithResultColumns(nameof(Animal), "Animal_Update"), + modelBuilder); + } + [ConditionalFact] public virtual void Passes_on_valid_UsingDeleteStoredProcedure_in_TPT() { diff --git a/test/EFCore.Relational.Tests/Update/ModificationCommandTest.cs b/test/EFCore.Relational.Tests/Update/ModificationCommandTest.cs index 13047fcda55..d53f05faf66 100644 --- a/test/EFCore.Relational.Tests/Update/ModificationCommandTest.cs +++ b/test/EFCore.Relational.Tests/Update/ModificationCommandTest.cs @@ -375,64 +375,6 @@ public void ModificationCommand_throws_for_unknown_entities(bool sensitive) Assert.Throws(() => command.AddEntry(entry, true)).Message); } - [ConditionalFact] - public void RequiresResultPropagation_false_for_Delete_operation() - { - var entry = CreateEntry( - EntityState.Deleted, generateKeyValues: true, computeNonKeyValue: true); - - var command = CreateModificationCommand(entry, new ParameterNameGenerator().GenerateNext, false, null); - command.AddEntry(entry, true); - - Assert.False(command.RequiresResultPropagation); - } - - [ConditionalFact] - public void RequiresResultPropagation_true_for_Insert_operation_if_store_generated_columns_exist() - { - var entry = CreateEntry( - EntityState.Added, generateKeyValues: true, computeNonKeyValue: true); - - var command = CreateModificationCommand(entry, new ParameterNameGenerator().GenerateNext, false, null); - command.AddEntry(entry, true); - - Assert.True(command.RequiresResultPropagation); - } - - [ConditionalFact] - public void RequiresResultPropagation_false_for_Insert_operation_if_no_store_generated_columns_exist() - { - var entry = CreateEntry(EntityState.Added); - - var command = CreateModificationCommand(entry, new ParameterNameGenerator().GenerateNext, false, null); - command.AddEntry(entry, true); - - Assert.False(command.RequiresResultPropagation); - } - - [ConditionalFact] - public void RequiresResultPropagation_true_for_Update_operation_if_non_key_store_generated_columns_exist() - { - var entry = CreateEntry( - EntityState.Modified, generateKeyValues: true, computeNonKeyValue: true); - - var command = CreateModificationCommand(entry, new ParameterNameGenerator().GenerateNext, false, null); - command.AddEntry(entry, true); - - Assert.True(command.RequiresResultPropagation); - } - - [ConditionalFact] - public void RequiresResultPropagation_false_for_Update_operation_if_no_non_key_store_generated_columns_exist() - { - var entry = CreateEntry(EntityState.Modified, generateKeyValues: true); - - var command = CreateModificationCommand(entry, new ParameterNameGenerator().GenerateNext, false, null); - command.AddEntry(entry, true); - - Assert.False(command.RequiresResultPropagation); - } - private class T1 { public int Id { get; set; } diff --git a/test/EFCore.SqlServer.FunctionalTests/Update/StoredProcedureUpdateSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Update/StoredProcedureUpdateSqlServerTest.cs index 95fe914f8c7..8a5a0352e76 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Update/StoredProcedureUpdateSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Update/StoredProcedureUpdateSqlServerTest.cs @@ -76,20 +76,6 @@ public override async Task Insert_with_output_parameter_and_result_column(bool a EXEC [WithOutputParameterAndResultColumn_Insert] @p0 OUTPUT, @p1;"); } - public override async Task Update_with_output_parameter_and_result_column_and_return_value(bool async) - { - await base.Update_with_output_parameter_and_result_column_and_return_value(async); - - AssertSql( - @"@p0=NULL (Nullable = false) (Direction = Output) (DbType = Int32) -@p1='1' -@p2='Updated' (Size = 4000) -@p3=NULL (Nullable = false) (Direction = Output) (DbType = Int32) - -SET NOCOUNT ON; -EXEC @p0 = [WithOutputParameterAndResultColumnAndResultValue_Update] @p1, @p2, @p3 OUTPUT;"); - } - public override async Task Update(bool async) { await base.Update(async); @@ -124,6 +110,19 @@ public override async Task Update_with_output_parameter_and_rows_affected_result @p1='Updated' (Size = 4000) @p2=NULL (Nullable = false) (Direction = Output) (DbType = Int32) +SET NOCOUNT ON; +EXEC [WithOutputParameterAndRowsAffectedResultColumn_Update] @p0, @p1, @p2 OUTPUT;"); + } + + public override async Task Update_with_output_parameter_and_rows_affected_result_column_concurrency_failure(bool async) + { + await base.Update_with_output_parameter_and_rows_affected_result_column_concurrency_failure(async); + + AssertSql( + @"@p0='1' +@p1='Updated' (Size = 4000) +@p2=NULL (Nullable = false) (Direction = Output) (DbType = Int32) + SET NOCOUNT ON; EXEC [WithOutputParameterAndRowsAffectedResultColumn_Update] @p0, @p1, @p2 OUTPUT;"); } @@ -398,7 +397,6 @@ public override void CleanData() TRUNCATE TABLE [WithOutputParameter]; TRUNCATE TABLE [WithOutputParameterAndResultColumn]; TRUNCATE TABLE [WithOutputParameterAndRowsAffectedResultColumn]; -TRUNCATE TABLE [WithOutputParameterAndResultColumnAndResultValue]; TRUNCATE TABLE [WithResultColumn]; TRUNCATE TABLE [WithTwoResultColumns]; TRUNCATE TABLE [WithRowsAffectedParameter]; @@ -551,15 +549,6 @@ AS BEGIN GO -CREATE PROCEDURE WithOutputParameterAndResultColumnAndResultValue_Update(@Id int, @Name varchar(max), @AdditionalProperty1 int OUT) -AS BEGIN - UPDATE [WithOutputParameterAndResultColumnAndResultValue] SET [Name] = @Name, @AdditionalProperty1 = [AdditionalProperty1] WHERE [Id] = @Id; - SELECT [AdditionalProperty2] FROM [WithOutputParameterAndResultColumnAndResultValue] WHERE [Id] = @Id - RETURN @@ROWCOUNT; -END; - -GO - CREATE PROCEDURE Tph_Insert(@Id int OUT, @Discriminator varchar(max), @Name varchar(max), @Child1Property int, @Child2InputProperty int, @Child2OutputParameterProperty int OUT) AS BEGIN DECLARE @Table table ([Child2OutputParameterProperty] int);