Sproc fixes

* Don't attempt to propagate rows affected result columns Fixes dotnet#28803
roji · Aug 31, 2022 · 8ea1d60 · 8ea1d60
1 parent 5525fce
commit 8ea1d60
Show file tree

Hide file tree

Showing 6 changed files with 79 additions and 10 deletions.
diff --git a/src/EFCore.Relational/Update/AffectedCountModificationCommandBatch.cs b/src/EFCore.Relational/Update/AffectedCountModificationCommandBatch.cs
@@ -51,10 +51,7 @@ protected override void Consume(RelationalDataReader reader)
 
                 if (resultSetMapping.HasFlag(ResultSetMapping.HasResultRow))
                 {
-                    if (onResultSet == false)
-                    {
-                        Check.DebugFail("Missing a result set");
-                    }
+                    Check.DebugAssert(onResultSet != false, "Missing a result set");
 
                     var lastHandledCommandIndex = command.RequiresResultPropagation
                         ? ConsumeResultSetWithPropagation(commandIndex, reader)
@@ -75,7 +72,9 @@ protected override void Consume(RelationalDataReader reader)
                 }
             }
 
-            Debug.Assert(onResultSet != true, "Unexpected result set found at end");
+            // TODO: This should be a warning, not an assertion; it gets tripped when a user executes a sproc which returns something
+            // (e.g. rows affected) but forgets to configure that in metadata
+            // Check.DebugAssert(onResultSet != true, "Unexpected result set found at end");
 
             if (hasOutputParameters)
             {
@@ -166,10 +165,7 @@ protected override async Task ConsumeAsync(
 
                 if (resultSetMapping.HasFlag(ResultSetMapping.HasResultRow))
                 {
-                    if (onResultSet == false)
-                    {
-                        Check.DebugFail("Missing a result set");
-                    }
+                    Check.DebugAssert(onResultSet != false, "Missing a result set");
 
                     var lastHandledCommandIndex = command.RequiresResultPropagation
                         ? await ConsumeResultSetWithPropagationAsync(commandIndex, reader, cancellationToken).ConfigureAwait(false)
@@ -190,7 +186,9 @@ protected override async Task ConsumeAsync(
                 }
             }
 
-            Debug.Assert(onResultSet != true, "Unexpected result set found at end");
+            // TODO: This should be a warning, not an assertion; it gets tripped when a user executes a sproc which returns something
+            // (e.g. rows affected) but forgets to configure that in metadata
+            // Check.DebugAssert(onResultSet != true, "Unexpected result set found at end");
 
             if (hasOutputParameters)
             {

diff --git a/src/EFCore.Relational/Update/ModificationCommand.cs b/src/EFCore.Relational/Update/ModificationCommand.cs
@@ -866,6 +866,11 @@ public virtual void PropagateResults(RelationalDataReader relationalReader)
                     break;
 
                 case IStoreStoredProcedureResultColumn resultColumn:
+                    if (ReferenceEquals(RowsAffectedColumn, resultColumn))
+                    {
+                        continue;
+                    }
+
                     // For stored procedure result sets, we need to get the column ordering from metadata.
                     readerIndex = resultColumn.Position;
 #if DEBUG

diff --git a/...Specification.Tests/TestModels/StoredProcedureUpdateModel/StoredProcedureUpdateContext.cs b/...Specification.Tests/TestModels/StoredProcedureUpdateModel/StoredProcedureUpdateContext.cs
@@ -22,6 +22,9 @@ public DbSet<EntityWithAdditionalProperty> WithTwoResultColumns
     public DbSet<EntityWithAdditionalProperty> WithOutputParameterAndResultColumn
         => Set<EntityWithAdditionalProperty>(nameof(WithOutputParameterAndResultColumn));
 
+    public DbSet<EntityWithAdditionalProperty> WithOutputParameterAndRowsAffectedResultColumn
+        => Set<EntityWithAdditionalProperty>(nameof(WithOutputParameterAndRowsAffectedResultColumn));
+
     public DbSet<EntityWithTwoAdditionalProperties> WithOutputParameterAndResultColumnAndResultValue
         => Set<EntityWithTwoAdditionalProperties>(nameof(WithOutputParameterAndResultColumnAndResultValue));
 

diff --git a/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateFixtureBase.cs b/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateFixtureBase.cs
@@ -61,6 +61,21 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
                         .HasResultColumn(w => w.AdditionalProperty));
             });
 
+        modelBuilder.SharedTypeEntity<EntityWithAdditionalProperty>(
+            nameof(StoredProcedureUpdateContext.WithOutputParameterAndRowsAffectedResultColumn),
+            b =>
+            {
+                b.Property(w => w.AdditionalProperty).HasComputedColumnSql("8");
+
+                b.UpdateUsingStoredProcedure(
+                    nameof(StoredProcedureUpdateContext.WithOutputParameterAndRowsAffectedResultColumn) + "_Update",
+                    spb => spb
+                        .HasParameter(w => w.Id)
+                        .HasParameter(w => w.Name)
+                        .HasParameter(w => w.AdditionalProperty, pb => pb.IsOutput())
+                        .HasRowsAffectedResultColumn());
+            });
+
         modelBuilder.SharedTypeEntity<EntityWithAdditionalProperty>(nameof(StoredProcedureUpdateContext.WithTwoOutputParameters))
             .UpdateUsingStoredProcedure(
                 nameof(StoredProcedureUpdateContext.WithTwoOutputParameters) + "_Update", spb => spb

diff --git a/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateTestBase.cs b/test/EFCore.Relational.Specification.Tests/Update/StoredProcedureUpdateTestBase.cs
@@ -152,6 +152,31 @@ public virtual async Task Update_partial(bool async)
         }
     }
 
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public virtual async Task Update_with_output_parameter_and_rows_affected_result_column(bool async)
+    {
+        await using var context = CreateContext();
+
+        var entity = new EntityWithAdditionalProperty { Name = "Foo" };
+        context.WithOutputParameterAndRowsAffectedResultColumn.Add(entity);
+        await context.SaveChangesAsync();
+
+        entity.Name = "Updated";
+
+        ClearLog();
+
+        await SaveChanges(context, async);
+
+        using (Fixture.TestSqlLoggerFactory.SuspendRecordingEvents())
+        {
+            var actual = await context.WithOutputParameterAndRowsAffectedResultColumn.SingleAsync(w => w.Id == entity.Id);
+
+            Assert.Equal("Updated", actual.Name);
+            Assert.Equal(8, actual.AdditionalProperty);
+        }
+    }
+
     [ConditionalTheory]
     [MemberData(nameof(IsAsyncData))]
     public virtual async Task Delete(bool async)

diff --git a/test/EFCore.SqlServer.FunctionalTests/Update/StoredProcedureUpdateSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Update/StoredProcedureUpdateSqlServerTest.cs
@@ -115,6 +115,19 @@ public override async Task Update_partial(bool async)
 EXEC [WithTwoOutputParameters_Update] @p0, @p1, @p2;");
     }
 
+    public override async Task Update_with_output_parameter_and_rows_affected_result_column(bool async)
+    {
+        await base.Update_with_output_parameter_and_rows_affected_result_column(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;");
+    }
+
     public override async Task Delete(bool async)
     {
         await base.Delete(async);
@@ -384,6 +397,7 @@ public override void CleanData()
 TRUNCATE TABLE [WithOriginalAndCurrentValueOnNonConcurrencyToken];
 TRUNCATE TABLE [WithOutputParameter];
 TRUNCATE TABLE [WithOutputParameterAndResultColumn];
+TRUNCATE TABLE [WithOutputParameterAndRowsAffectedResultColumn];
 TRUNCATE TABLE [WithOutputParameterAndResultColumnAndResultValue];
 TRUNCATE TABLE [WithResultColumn];
 TRUNCATE TABLE [WithTwoResultColumns];
@@ -458,10 +472,19 @@ AS BEGIN
 
 GO
 
+CREATE PROCEDURE WithOutputParameterAndRowsAffectedResultColumn_Update(@Id int, @Name varchar(max), @AdditionalProperty int OUT)
+AS BEGIN
+    UPDATE [WithOutputParameterAndRowsAffectedResultColumn] SET [Name] = @Name, @AdditionalProperty = [AdditionalProperty] WHERE [Id] = @Id;
+    SELECT @@ROWCOUNT;
+END;
+
+GO
+
 CREATE PROCEDURE WithTwoOutputParameters_Update(@Id int, @Name varchar(max), @AdditionalProperty int)
 AS UPDATE [WithTwoOutputParameters] SET [Name] = @Name, [AdditionalProperty] = @AdditionalProperty WHERE [Id] = @id;
 
 GO
+
 CREATE PROCEDURE WithRowsAffectedParameter_Update(@Id int, @Name varchar(max), @RowsAffected int OUT)
 AS BEGIN
     UPDATE [WithRowsAffectedParameter] SET [Name] = @Name WHERE [Id] = @Id;