Skip to content

Commit

Permalink
Make DbUpdateException.Entries contain all entries when we can't popu…
Browse files Browse the repository at this point in the history 
…late it (#29571)

Closes #29379
  • Loading branch information
@roji
roji authored Nov 17, 2022
1 parent 089a035 commit 8fdc756
Show file tree
Hide file tree
Showing 5 changed files with 209 additions and 59 deletions.
135 changes: 82 additions & 53 deletions src/EFCore.Relational/Update/AffectedCountModificationCommandBatch.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -270,43 +270,58 @@ await ThrowAggregateUpdateConcurrencyExceptionAsync(
/// <returns>The ordinal of the next result set that must be consumed.</returns>
protected virtual int ConsumeResultSet(int startCommandIndex, RelationalDataReader reader)
{
var commandIndex = startCommandIndex;
var rowsAffected = 0;
do
IReadOnlyModificationCommand? command = null;

try
{
if (!reader.Read())

var commandIndex = startCommandIndex;
var rowsAffected = 0;
do
{
var expectedRowsAffected = rowsAffected + 1;
while (++commandIndex < ResultSetMappings.Count
&& ResultSetMappings[commandIndex - 1].HasFlag(ResultSetMapping.NotLastInResultSet))
if (!reader.Read())
{
expectedRowsAffected++;
var expectedRowsAffected = rowsAffected + 1;
while (++commandIndex < ResultSetMappings.Count
&& ResultSetMappings[commandIndex - 1].HasFlag(ResultSetMapping.NotLastInResultSet))
{
expectedRowsAffected++;
}

ThrowAggregateUpdateConcurrencyException(reader, commandIndex, expectedRowsAffected, rowsAffected);
}
else
{
var resultSetMapping = ResultSetMappings[commandIndex];

ThrowAggregateUpdateConcurrencyException(reader, commandIndex, expectedRowsAffected, rowsAffected);
}
else
{
var resultSetMapping = ResultSetMappings[commandIndex];
command = ModificationCommands[
resultSetMapping.HasFlag(ResultSetMapping.IsPositionalResultMappingEnabled)
? startCommandIndex + reader.DbDataReader.GetInt32(reader.DbDataReader.FieldCount - 1)
: commandIndex];

var command = ModificationCommands[
resultSetMapping.HasFlag(ResultSetMapping.IsPositionalResultMappingEnabled)
? startCommandIndex + reader.DbDataReader.GetInt32(reader.DbDataReader.FieldCount - 1)
: commandIndex];
Check.DebugAssert(
!resultSetMapping.HasFlag(ResultSetMapping.ResultSetWithRowsAffectedOnly),
"!resultSetMapping.HasFlag(ResultSetMapping.ResultSetWithRowsAffectedOnly)");

command.PropagateResults(reader);

Check.DebugAssert(
!resultSetMapping.HasFlag(ResultSetMapping.ResultSetWithRowsAffectedOnly),
"!resultSetMapping.HasFlag(ResultSetMapping.ResultSetWithRowsAffectedOnly)");
command = null;
}

command.PropagateResults(reader);
rowsAffected++;
}
while (++commandIndex < ResultSetMappings.Count
&& ResultSetMappings[commandIndex - 1].HasFlag(ResultSetMapping.NotLastInResultSet));

rowsAffected++;
return commandIndex - 1;
}
catch (Exception ex) when (ex is not DbUpdateException and not OperationCanceledException)
{
throw new DbUpdateException(
RelationalStrings.UpdateStoreException,
ex,
command?.Entries ?? ModificationCommands.SelectMany(c => c.Entries).ToList());
}
while (++commandIndex < ResultSetMappings.Count
&& ResultSetMappings[commandIndex - 1].HasFlag(ResultSetMapping.NotLastInResultSet));

return commandIndex - 1;
}

/// <summary>
Expand All @@ -326,44 +341,58 @@ protected virtual async Task<int> ConsumeResultSetAsync(
RelationalDataReader reader,
CancellationToken cancellationToken)
{
var commandIndex = startCommandIndex;
var rowsAffected = 0;
do
IReadOnlyModificationCommand? command = null;

try
{
if (!await reader.ReadAsync(cancellationToken).ConfigureAwait(false))
var commandIndex = startCommandIndex;
var rowsAffected = 0;
do
{
var expectedRowsAffected = rowsAffected + 1;
while (++commandIndex < ResultSetMappings.Count
&& ResultSetMappings[commandIndex - 1].HasFlag(ResultSetMapping.NotLastInResultSet))
if (!await reader.ReadAsync(cancellationToken).ConfigureAwait(false))
{
expectedRowsAffected++;
var expectedRowsAffected = rowsAffected + 1;
while (++commandIndex < ResultSetMappings.Count
&& ResultSetMappings[commandIndex - 1].HasFlag(ResultSetMapping.NotLastInResultSet))
{
expectedRowsAffected++;
}

await ThrowAggregateUpdateConcurrencyExceptionAsync(
reader, commandIndex, expectedRowsAffected, rowsAffected, cancellationToken).ConfigureAwait(false);
}
else
{
var resultSetMapping = ResultSetMappings[commandIndex];

await ThrowAggregateUpdateConcurrencyExceptionAsync(
reader, commandIndex, expectedRowsAffected, rowsAffected, cancellationToken).ConfigureAwait(false);
}
else
{
var resultSetMapping = ResultSetMappings[commandIndex];
command = ModificationCommands[
resultSetMapping.HasFlag(ResultSetMapping.IsPositionalResultMappingEnabled)
? startCommandIndex + reader.DbDataReader.GetInt32(reader.DbDataReader.FieldCount - 1)
: commandIndex];

var command = ModificationCommands[
resultSetMapping.HasFlag(ResultSetMapping.IsPositionalResultMappingEnabled)
? startCommandIndex + reader.DbDataReader.GetInt32(reader.DbDataReader.FieldCount - 1)
: commandIndex];
Check.DebugAssert(
!resultSetMapping.HasFlag(ResultSetMapping.ResultSetWithRowsAffectedOnly),
"!resultSetMapping.HasFlag(ResultSetMapping.ResultSetWithRowsAffectedOnly)");

command.PropagateResults(reader);

Check.DebugAssert(
!resultSetMapping.HasFlag(ResultSetMapping.ResultSetWithRowsAffectedOnly),
"!resultSetMapping.HasFlag(ResultSetMapping.ResultSetWithRowsAffectedOnly)");
command = null;
}

command.PropagateResults(reader);
rowsAffected++;
}
while (++commandIndex < ResultSetMappings.Count
&& ResultSetMappings[commandIndex - 1].HasFlag(ResultSetMapping.NotLastInResultSet));

rowsAffected++;
return commandIndex - 1;
}
catch (Exception ex) when (ex is not DbUpdateException and not OperationCanceledException)
{
throw new DbUpdateException(
RelationalStrings.UpdateStoreException,
ex,
command?.Entries ?? ModificationCommands.SelectMany(c => c.Entries).ToList());
}
while (++commandIndex < ResultSetMappings.Count
&& ResultSetMappings[commandIndex - 1].HasFlag(ResultSetMapping.NotLastInResultSet));

return commandIndex - 1;
}

/// <summary>
Expand Down
35 changes: 35 additions & 0 deletions test/EFCore.Relational.Specification.Tests/Update/NonSharedModelUpdatesTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,41 @@ private class Book
public Author? Author { get; set; }
}
[ConditionalTheory] // Issue #29379
[MemberData(nameof(IsAsyncData))]
public virtual async Task DbUpdateException_Entries_is_correct_with_multiple_inserts(bool async)
{
var contextFactory = await InitializeAsync<DbContext>(onModelCreating: mb => mb.Entity<Blog>().HasIndex(b => b.Name).IsUnique());
await ExecuteWithStrategyInTransactionAsync(
contextFactory,
async context =>
{
context.Add(new Blog { Name = "Blog2" });
await context.SaveChangesAsync();
},
async context =>
{
context.Add(new Blog { Name = "Blog1" });
context.Add(new Blog { Name = "Blog2" });
context.Add(new Blog { Name = "Blog3" });
var exception = async
? await Assert.ThrowsAsync<DbUpdateException>(() => context.SaveChangesAsync())
: Assert.Throws<DbUpdateException>(() => context.SaveChanges());
var entry = Assert.Single(exception.Entries);
Assert.Equal("Blog2", ((Blog)entry.Entity).Name);
});
}
public class Blog
{
public int Id { get; set; }
public string? Name { get; set; }
}
protected virtual void ExecuteWithStrategyInTransaction(
ContextFactory<DbContext> contextFactory,
Action<DbContext> testOperation,
Expand Down
7 changes: 1 addition & 6 deletions test/EFCore.SqlServer.FunctionalTests/SqlServerValueGenerationScenariosTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1234,12 +1234,7 @@ public void Insert_with_explicit_non_default_keys_by_default()
// inner exception for details.
// SqlException : Cannot insert explicit value for identity column in table
// 'Blog' when IDENTITY_INSERT is set to OFF.
context.Database.CreateExecutionStrategy().Execute(
context, c =>
{
var updateException = Assert.Throws<DbUpdateException>(() => c.SaveChanges());
Assert.Single(updateException.Entries);
});
context.Database.CreateExecutionStrategy().Execute(context, c => Assert.Throws<DbUpdateException>(() => c.SaveChanges()));
}

[ConditionalFact]
Expand Down
61 changes: 61 additions & 0 deletions test/EFCore.SqlServer.FunctionalTests/Update/NonSharedModelUpdatesSqlServerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,67 @@ public class DailyDigest
public User User { get; set; }
}

public override async Task DbUpdateException_Entries_is_correct_with_multiple_inserts(bool async)
{
// SQL Server's bulk insert support makes it impossible to populate the entry which caused the exception, since the position
// used to find the entry is returned as an output column, but the row is never received in case of an exception.
// Instead we make sure Entries contains all entries.
var contextFactory = await InitializeAsync<DbContext>(onModelCreating: mb => mb.Entity<Blog>().HasIndex(b => b.Name).IsUnique());

await ExecuteWithStrategyInTransactionAsync(
contextFactory,
async context =>
{
context.Add(new Blog { Name = "Blog2" });
await context.SaveChangesAsync();
},
async context =>
{
context.Add(new Blog { Name = "Blog1" });
context.Add(new Blog { Name = "Blog2" });
context.Add(new Blog { Name = "Blog3" });
var exception = async
? await Assert.ThrowsAsync<DbUpdateException>(() => context.SaveChangesAsync())
: Assert.Throws<DbUpdateException>(() => context.SaveChanges());
Assert.Collection(
exception.Entries.Select(e => (Blog)e.Entity).OrderBy(b => b.Name),
b => Assert.Equal("Blog1", b.Name),
b => Assert.Equal("Blog2", b.Name),
b => Assert.Equal("Blog3", b.Name));
});

AssertSql(
"""
@p0='Blog2' (Size = 450)

SET IMPLICIT_TRANSACTIONS OFF;
SET NOCOUNT ON;
INSERT INTO [Blog] ([Name])
OUTPUT INSERTED.[Id]
VALUES (@p0);
""",
//
"""
@p0='Blog1' (Size = 450)
@p1='Blog2' (Size = 450)
@p2='Blog3' (Size = 450)

SET IMPLICIT_TRANSACTIONS OFF;
SET NOCOUNT ON;
MERGE [Blog] USING (
VALUES (@p0, 0),
(@p1, 1),
(@p2, 2)) AS i ([Name], _Position) ON 1=0
WHEN NOT MATCHED THEN
INSERT ([Name])
VALUES (i.[Name])
OUTPUT INSERTED.[Id], i._Position;
""");
}


private void AssertSql(params string[] expected)
=> TestSqlLoggerFactory.AssertBaseline(expected);

Expand Down
30 changes: 30 additions & 0 deletions test/EFCore.Sqlite.FunctionalTests/Update/NonSharedModelUpdatesSqliteTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,36 @@ DELETE FROM "Author"
""");
}

public override async Task DbUpdateException_Entries_is_correct_with_multiple_inserts(bool async)
{
await base.DbUpdateException_Entries_is_correct_with_multiple_inserts(async);

AssertSql(
"""
@p0='Blog2' (Size = 5)

INSERT INTO "Blog" ("Name")
VALUES (@p0)
RETURNING "Id";
""",
//
"""
@p0='Blog1' (Size = 5)

INSERT INTO "Blog" ("Name")
VALUES (@p0)
RETURNING "Id";
""",
//
"""
@p0='Blog2' (Size = 5)

INSERT INTO "Blog" ("Name")
VALUES (@p0)
RETURNING "Id";
""");
}

private void AssertSql(params string[] expected)
=> TestSqlLoggerFactory.AssertBaseline(expected);

Expand Down

0 comments on commit 8fdc756

Please sign in to comment.