Address review comments

src/EFCore.Relational/Update/ColumnModification.cs

@@ -55,110 +55,72 @@ public ColumnModification(in ColumnModificationParameters columnModificationPara
         UseParameter = _generateParameterName != null;
     }
 
-    /// <summary>
-    ///     The <see cref="IUpdateEntry" /> that represents the entity that is being modified.
-    /// </summary>
+    /// <inheritdoc />
     public virtual IUpdateEntry? Entry { get; }
 
-    /// <summary>
-    ///     The property that maps to the column.
-    /// </summary>
+    /// <inheritdoc />
     public virtual IProperty? Property { get; }
 
-    /// <summary>
-    ///     The relational type mapping for the column.
-    /// </summary>
+    /// <inheritdoc />
     public virtual RelationalTypeMapping? TypeMapping { get; }
 
-    /// <summary>
-    ///     A value indicating whether the column could contain a null value.
-    /// </summary>
+    /// <inheritdoc />
     public virtual bool? IsNullable { get; }
 
-    /// <summary>
-    ///     Indicates whether a value must be read from the database for the column.
-    /// </summary>
+    /// <inheritdoc />
     public virtual bool IsRead { get; }
 
-    /// <summary>
-    ///     Indicates whether a value must be written to the database for the column.
-    /// </summary>
+    /// <inheritdoc />
     public virtual bool IsWrite { get; }
 
-    /// <summary>
-    ///     Indicates whether the column is used in the <c>WHERE</c> clause when updating.
-    /// </summary>
+    /// <inheritdoc />
     public virtual bool IsCondition { get; }
 
-    /// <summary>
-    ///     Indicates whether the column is part of a primary or alternate key.
-    /// </summary>
+    /// <inheritdoc />
     public virtual bool IsKey { get; }
 
-    /// <summary>
-    ///     Indicates whether the original value of the property must be passed as a parameter to the SQL.
-    /// </summary>
+    /// <inheritdoc />
     public virtual bool UseOriginalValueParameter
         => UseParameter && UseOriginalValue;
 
-    /// <summary>
-    ///     Indicates whether the current value of the property must be passed as a parameter to the SQL.
-    /// </summary>
+    /// <inheritdoc />
     public virtual bool UseCurrentValueParameter
         => UseParameter && UseCurrentValue;
 
-    /// <summary>
-    ///     Indicates whether the original value of the property should be used.
-    /// </summary>
+    /// <inheritdoc />
     public virtual bool UseOriginalValue
         => IsCondition;
 
-    /// <summary>
-    ///     Indicates whether the current value of the property should be used.
-    /// </summary>
+    /// <inheritdoc />
     public virtual bool UseCurrentValue
         => IsWrite;
 
-    /// <summary>
-    ///     Indicates whether the value of the property must be passed as a parameter to the SQL as opposed to being inlined.
-    /// </summary>
+    /// <inheritdoc />
     public virtual bool UseParameter { get; }
 
-    /// <summary>
-    ///     The parameter name to use for the current value parameter (<see cref="UseCurrentValueParameter" />), if needed.
-    /// </summary>
+    /// <inheritdoc />
     public virtual string? ParameterName
         => _parameterName ??= UseCurrentValueParameter ? _generateParameterName!() : null;
 
-    /// <summary>
-    ///     The parameter name to use for the original value parameter (<see cref="UseOriginalValueParameter" />), if needed.
-    /// </summary>
+    /// <inheritdoc />
     public virtual string? OriginalParameterName
         => _originalParameterName ??= UseOriginalValueParameter ? _generateParameterName!() : null;
 
-    /// <summary>
-    ///     The name of the column.
-    /// </summary>
+    /// <inheritdoc />
     public virtual string ColumnName { get; }
 
-    /// <summary>
-    ///     The database type of the column.
-    /// </summary>
+    /// <inheritdoc />
     public virtual string? ColumnType { get; }
 
-    /// <summary>
-    ///     The original value of the property mapped to this column.
-    /// </summary>
+    /// <inheritdoc />
     public virtual object? OriginalValue
         => Entry == null
             ? _originalValue
             : Entry.SharedIdentityEntry == null
                 ? Entry.GetOriginalValue(Property!)
                 : Entry.SharedIdentityEntry.GetOriginalValue(Property!);
 
-    /// <summary>
-    ///     Gets or sets the current value of the property mapped to this column.
-    /// </summary>
+    /// <inheritdoc />
     public virtual object? Value
     {
         get => Entry == null
@@ -186,10 +148,7 @@ public virtual object? Value
         }
     }
 
-    /// <summary>
-    ///     Adds a modification affecting the same database value.
-    /// </summary>
-    /// <param name="modification">The modification for the shared column.</param>
+    /// <inheritdoc />
     public virtual void AddSharedColumnModification(IColumnModification modification)
     {
         Check.DebugAssert(Entry is not null, "Entry is not null");
@@ -258,4 +217,8 @@ public virtual void AddSharedColumnModification(IColumnModification modification
 
         _sharedColumnModifications.Add(modification);
     }
+
+    /// <inheritdoc />
+    public void ResetParameterNames()
+        => _parameterName = _originalParameterName = null;
 }

src/EFCore.Relational/Update/IColumnModification.cs

@@ -122,4 +122,9 @@ public interface IColumnModification
     /// </summary>
     /// <param name="modification">The modification for the shared column.</param>
     public void AddSharedColumnModification(IColumnModification modification);
+
+    /// <summary>
+    ///     Resets parameter names, so they can be regenerated if the command needs to be re-added to a new batch.
+    /// </summary>
+    public void ResetParameterNames();
 }

src/EFCore.Relational/Update/ReaderModificationCommandBatch.cs

@@ -53,7 +53,7 @@ protected ReaderModificationCommandBatch(ModificationCommandBatchFactoryDependen
     protected virtual IUpdateSqlGenerator UpdateSqlGenerator { get; }
 
     /// <summary>
-    ///     Gets or sets the relational command builder for the commands in the batch.
+    ///     Gets the relational command builder for the commands in the batch.
     /// </summary>
     protected virtual IRelationalCommandBuilder RelationalCommandBuilder { get; }
 
@@ -105,11 +105,6 @@ public override bool TryAddCommand(IReadOnlyModificationCommand modificationComm
             throw new InvalidOperationException(RelationalStrings.ModificationCommandBatchAlreadyComplete);
         }
 
-        if (!CanAddCommand(modificationCommand))
-        {
-            return false;
-        }
-
         _sqlBuilderPosition = SqlBuilder.Length;
         _commandResultSetCount = CommandResultSet.Count;
         _pendingParameterNames.Clear();
@@ -120,7 +115,7 @@ public override bool TryAddCommand(IReadOnlyModificationCommand modificationComm
         // Check if the batch is still valid after having added the command (e.g. have we bypassed a maximum CommandText size?)
         // A batch with only one command is always considered valid (otherwise we'd get an endless loop); allow the batch to fail
         // server-side.
-        if (IsBatchValid() || _modificationCommands.Count == 0)
+        if (IsValid() || _modificationCommands.Count == 0)
         {
             _modificationCommands.Add(modificationCommand);
 
@@ -129,6 +124,12 @@ public override bool TryAddCommand(IReadOnlyModificationCommand modificationComm
 
         RollbackLastCommand();
 
+        // The command's column modifications had their parameter names generated, that needs to be rolled back as well.
+        foreach (var columnModification in modificationCommand.ColumnModifications)
+        {
+            columnModification.ResetParameterNames();
+        }
+
         return false;
     }
 
@@ -174,18 +175,11 @@ public override bool RequiresTransaction
     protected virtual void SetRequiresTransaction(bool requiresTransaction)
         => _requiresTransaction = requiresTransaction;
 
-    /// <summary>
-    ///     Checks whether a new command can be added to the batch.
-    /// </summary>
-    /// <param name="modificationCommand">The command to potentially add.</param>
-    /// <returns><see langword="true" /> if the command can be added; <see langword="false" /> otherwise.</returns>
-    protected abstract bool CanAddCommand(IReadOnlyModificationCommand modificationCommand);
-
     /// <summary>
     ///     Checks whether the command text is valid.
     /// </summary>
     /// <returns><see langword="true" /> if the command text is valid; <see langword="false" /> otherwise.</returns>
-    protected abstract bool IsBatchValid();
+    protected abstract bool IsValid();
 
     /// <summary>
     ///     Adds Updates the command text for the command at the given position in the <see cref="ModificationCommands" /> list.

src/EFCore.Relational/Update/SingularModificationCommandBatch.cs

@@ -28,19 +28,11 @@ public SingularModificationCommandBatch(ModificationCommandBatchFactoryDependenc
     }
 
     /// <summary>
-    ///     Only returns <see langword="true" /> if the no command has already been added.
-    /// </summary>
-    /// <param name="modificationCommand">The command to potentially add.</param>
-    /// <returns><see langword="true" /> if no command has already been added.</returns>
-    protected override bool CanAddCommand(IReadOnlyModificationCommand modificationCommand)
-        => ModificationCommands.Count == 0;
-
-    /// <summary>
-    ///     Returns <see langword="true" /> since only a single command is generated so the command text must be valid.
+    ///     Returns <see langword="true" /> only when the batch contains a single command.
     /// </summary>
     /// <returns>
-    ///     <see langword="true" />
+    ///     <see langword="true" />.
     /// </returns>
-    protected override bool IsBatchValid()
-        => true;
+    protected override bool IsValid()
+        => ModificationCommands.Count == 1;
 }

src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommandBatch.cs

@@ -50,15 +50,6 @@ public SqlServerModificationCommandBatch(
     protected new virtual ISqlServerUpdateSqlGenerator UpdateSqlGenerator
         => (ISqlServerUpdateSqlGenerator)base.UpdateSqlGenerator;
 
-    /// <summary>
-    ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
-    ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
-    ///     any release. You should only use it directly in your code with extreme caution and knowing that
-    ///     doing so can result in application failures when updating to a new Entity Framework Core release.
-    /// </summary>
-    protected override bool CanAddCommand(IReadOnlyModificationCommand modificationCommand)
-        => ModificationCommands.Count < _maxBatchSize;
-
     /// <summary>
     ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
     ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
@@ -82,8 +73,9 @@ protected override void RollbackLastCommand()
     ///     any release. You should only use it directly in your code with extreme caution and knowing that
     ///     doing so can result in application failures when updating to a new Entity Framework Core release.
     /// </summary>
-    protected override bool IsBatchValid()
-        => SqlBuilder.Length < MaxScriptLength
+    protected override bool IsValid()
+        => ModificationCommands.Count < _maxBatchSize
+            && SqlBuilder.Length < MaxScriptLength
             // A single implicit parameter for the command text itself
             && ParameterValues.Count + 1 < MaxParameterCount;
 

test/EFCore.Relational.Tests/TestUtilities/TestModificationCommandBatch.cs

@@ -15,6 +15,6 @@ public TestModificationCommandBatch(
         _maxBatchSize = maxBatchSize ?? 1;
     }
 
-    protected override bool CanAddCommand(IReadOnlyModificationCommand modificationCommand)
+    protected override bool IsValid()
         => ModificationCommands.Count < _maxBatchSize;
 }

test/EFCore.Relational.Tests/Update/ReaderModificationCommandBatchTest.cs

@@ -15,52 +15,47 @@ namespace Microsoft.EntityFrameworkCore.Update;
 public class ReaderModificationCommandBatchTest
 {
     [ConditionalFact]
-    public void AddCommand_adds_command_if_possible()
+    public void AddCommand_adds_command_if_batch_is_valid()
     {
-        var command = CreateModificationCommand("T1", null, true, columnModifications: null);
+        var command1 = CreateModificationCommand("T1", null, true, columnModifications: null);
+        var command2 = CreateModificationCommand("T2", null, true, columnModifications: null);
 
         var batch = new ModificationCommandBatchFake();
-        batch.TryAddCommand(command);
-        batch.ShouldAddCommand = true;
-
-        batch.TryAddCommand(command);
+        batch.ShouldBeValid = true;
+        Assert.True(batch.TryAddCommand(command1));
+        Assert.True(batch.TryAddCommand(command2));
         batch.Complete();
 
-        Assert.Equal(2, batch.ModificationCommands.Count);
-        Assert.Same(command, batch.ModificationCommands[0]);
-        Assert.Equal(2, batch.FakeSqlGenerator.AppendUpdateOperationCalls);
-    }
-
-    [ConditionalFact]
-    public void AddCommand_does_not_add_command_if_not_possible()
-    {
-        var command = CreateModificationCommand("T1", null, true, columnModifications: null);
+        Assert.Collection(batch.ModificationCommands,
+            m => Assert.Same(command1, m),
+            m => Assert.Same(command2, m));
 
-        var batch = new ModificationCommandBatchFake();
-        batch.TryAddCommand(command);
-        batch.ShouldAddCommand = false;
+        Assert.Equal(@"UPDATE ""T1"" SET ;
+SELECT provider_specific_rowcount();
 
-        batch.TryAddCommand(command);
-        batch.Complete();
+UPDATE ""T2"" SET ;
+SELECT provider_specific_rowcount();
 
-        Assert.Equal(1, batch.ModificationCommands.Count);
-        Assert.Equal(1, batch.FakeSqlGenerator.AppendUpdateOperationCalls);
+",
+            batch.CommandText,
+            ignoreLineEndingDifferences: true);
     }
 
     [ConditionalFact]
-    public void AddCommand_does_not_add_command_if_resulting_sql_is_invalid()
+    public void AddCommand_does_not_add_command_batch_is_invalid()
     {
-        var command = CreateModificationCommand("T1", null, true, columnModifications: null);
+        var command1 = CreateModificationCommand("T1", null, true, columnModifications: null);
+        var command2 = CreateModificationCommand("T2", null, true, columnModifications: null);
 
         var batch = new ModificationCommandBatchFake();
-        batch.TryAddCommand(command);
-        batch.ShouldAddCommand = true;
-        batch.MaxValidSql = batch.CommandText.Length;
+        Assert.True(batch.TryAddCommand(command1));
+        batch.ShouldBeValid = false;
 
-        batch.TryAddCommand(command);
+        Assert.False(batch.TryAddCommand(command2));
         batch.Complete();
 
-        Assert.Equal(1, batch.ModificationCommands.Count);
+        Assert.Same(command1, Assert.Single(batch.ModificationCommands));
+
         Assert.Equal(@"UPDATE ""T1"" SET ;
 SELECT provider_specific_rowcount();
 
@@ -616,8 +611,7 @@ private class ModificationCommandBatchFake : AffectedCountModificationCommandBat
         public ModificationCommandBatchFake(IUpdateSqlGenerator sqlGenerator = null)
             : base(CreateDependencies(sqlGenerator))
         {
-            ShouldAddCommand = true;
-            MaxValidSql = int.MaxValue;
+            ShouldBeValid = true;
 
             _fakeSqlGenerator = Dependencies.UpdateSqlGenerator as FakeSqlGenerator;
         }
@@ -655,15 +649,10 @@ private static ModificationCommandBatchFactoryDependencies CreateDependencies(
         public string CommandText
             => SqlBuilder.ToString();
 
-        public bool ShouldAddCommand { get; set; }
-
-        protected override bool CanAddCommand(IReadOnlyModificationCommand modificationCommand)
-            => ShouldAddCommand;
-
-        public int MaxValidSql { get; set; }
+        public bool ShouldBeValid { get; set; }
 
-        protected override bool IsBatchValid()
-            => SqlBuilder.Length <= MaxValidSql;
+        protected override bool IsValid()
+            => ShouldBeValid;
 
         public new RawSqlCommand StoreCommand
             => base.StoreCommand;