Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect Entries Values in DbUpdateException when an error occurs in the SQL MERGE statement during insert #29379

Closed
thumer opened this issue Oct 17, 2022 · 4 comments · Fixed by #29571
Assignees
Labels
area-save-changes closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Milestone

Comments

@thumer
Copy link
Contributor

thumer commented Oct 17, 2022

Entities and DbContext
public class MyDbContext : DbContext
{
  protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
  {
      base.OnConfiguring(optionsBuilder);
      optionsBuilder
          .UseSqlServer("Server=localhost;Database=EFCore_UniqueKeyExceptionHandlingIssue_Sample;Integrated Security=true;TrustServerCertificate=True");
  }

  protected override void OnModelCreating(ModelBuilder modelBuilder)
  {
      base.OnModelCreating(modelBuilder);

      modelBuilder.Entity<BookStore>();
      modelBuilder.Entity<Book>(b =>
      {
          b.HasIndex(book => book.Number).IsUnique();
      });
  }
  public DbSet<BookStore> BookStores { get; set; }
  public DbSet<Book> Books { get; set; }
}

public sealed class BookStore
{
  public int Id { get; set; }
  public string? Name { get; set; }

  public override string ToString()
   => $"{nameof(BookStore)}: {Name}";
}

public sealed class Book
{
  public int Id { get; set; }
  public int Number { get; set; }
  public string? Title { get; set; }
  public int? BookStoreId { get; set; }
  public BookStore? BookStore { get; set; }

  public override string ToString()
      => $"{nameof(Book)}: #{Number} - {Title}";
}
Setup
await using (MyDbContext dbContext = new())
{
  await dbContext.Database.EnsureDeletedAsync();
  await dbContext.Database.EnsureCreatedAsync();
}

await using (MyDbContext dbContext = new())
{
  Book book = new () { Number = 3, Title = "Book3" };
  dbContext.Add(book);

  await dbContext.SaveChangesAsync();
}

await using (MyDbContext dbContext = new())
{
  BookStore store = new() { Name = "MainBookStore" };
  dbContext.Add(store);

  dbContext.AddRange(new List<Book>()
  {
          new Book() { Number = 1, Title = "Book1" },
          new Book() { Number = 2, Title = "Book2" },
          new Book() { Number = 3, Title = "Book3" },
          new Book() { Number = 4, Title = "Book4" }
  });

  try
  {
      await dbContext.SaveChangesAsync();
  }
  catch (DbUpdateException ex) when (ex.InnerException is SqlException sqlException && sqlException.Number == 2601)
  {
      Console.WriteLine($"SqlException: {sqlException.Message}");
      Console.WriteLine($"Duplicates: {string.Join(", ", ex.Entries.Select(e => e.Entity.ToString()).ToArray())}");
  }
}
Generated SQL
exec sp_executesql N'SET NOCOUNT ON;
INSERT INTO [BookStores] ([Name])
OUTPUT INSERTED.[Id]
VALUES (@p0);
MERGE [Books] USING (
VALUES (@p1, @p2, @p3, 0),
(@p4, @p5, @p6, 1),
(@p7, @p8, @p9, 2),
(@p10, @p11, @p12, 3)) AS i ([BookStoreId], [Number], [Title], _Position) ON 1=0
WHEN NOT MATCHED THEN
INSERT ([BookStoreId], [Number], [Title])
VALUES (i.[BookStoreId], i.[Number], i.[Title])
OUTPUT INSERTED.[Id], i._Position;
',N'@p0 nvarchar(4000),@p1 int,@p2 int,@p3 nvarchar(4000),@p4 int,@p5 int,@p6 nvarchar(4000),@p7 int,@p8 int,@p9 nvarchar(4000),@p10 int,@p11 int,@p12 nvarchar(4000)',@p0=N'MainBookStore',@p1=NULL,@p2=1,@p3=N'Book1',@p4=NULL,@p5=2,@p6=N'Book2',@p7=NULL,@p8=3,@p9=N'Book3',@p10=NULL,@p11=4,@p12=N'Book4'

If you are catching a DbUpdateException, because of a SqlException (UniqueKey violation), the DbUpdateException.Entries will be deliver only the first entity, if the insert statement was a SQL MERGE Statement.

Output:

SqlException: Cannot insert duplicate key row in object 'dbo.Books' with unique index 'IX_Books_Number'. The duplicate key value is (3).
Duplicates: Book: #1 - Book1

If you only insert 3 books and the Books table is configured with a trigger (-> NO MERGE STATEMENT) - it will work:

Correct Output:

SqlException: Cannot insert duplicate key row in object 'dbo.Books' with unique index 'IX_Books_Number'. The duplicate key value is (3).
Duplicates: Book: #3 - Book3

In our production environment we also had the case that even the wrong entity was delivered. In this case - a BookStore entity instead of the Book entity. However, I could not reproduce this scenario in this example.

Include provider and version information

EF Core version: 7.0 RC1
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET 6.0 / .NET 7.0
Operating system: Windows 11
IDE: Visual Studio 2022 17.3.6

@ajcvickers
Copy link
Contributor

@roji or @AndriySvyryd Can you take a look at this? Looks like DbUpdateException.Entries is returning only a single entry even when the database command includes many entries.

@ajcvickers
Copy link
Contributor

@roji Ping for potentially fixing this in a patch.

@roji
Copy link
Member

roji commented Nov 15, 2022

Unfortunately, it's impossible to make DbUpdateException.Entries contain the right entry for SQL Server bulk insert scenarios.

As a reminder, EF Core 6.0 generated the following SQL (also generated by 7.0 for trigger scenarios):

DECLARE @inserted1 TABLE ([Id] int, [_Position] [int]);
MERGE [Books] USING (
VALUES (@p1, @p2, @p3, 0),
(@p4, @p5, @p6, 1),
(@p7, @p8, @p9, 2),
(@p10, @p11, @p12, 3)) AS i ([BookStoreId], [Number], [Title], _Position) ON 1=0
WHEN NOT MATCHED THEN
INSERT ([BookStoreId], [Number], [Title])
VALUES (i.[BookStoreId], i.[Number], i.[Title])
OUTPUT INSERTED.[Id], i._Position
INTO @inserted1;

SELECT [i].[Id] FROM @inserted1 i
ORDER BY [i].[_Position];

The rows coming back from the databases are ordered by position, so EF naturally knows which row corresponds to which entry.

EF Core 7.0 stopped using the temporary table, and so cannot apply ordering to the MERGE results; instead, we return the position as an output column:

MERGE [Books] USING (
VALUES (@p1, @p2, @p3, 0),
(@p4, @p5, @p6, 1),
(@p7, @p8, @p9, 2),
(@p10, @p11, @p12, 3)) AS i ([BookStoreId], [Number], [Title], _Position) ON 1=0
WHEN NOT MATCHED THEN
INSERT ([BookStoreId], [Number], [Title])
VALUES (i.[BookStoreId], i.[Number], i.[Title])
OUTPUT INSERTED.[Id], i._Position;

But if there's an exception, the row containing the position is never read (DbDataReader.Read raises the exception), so we cannot know which entry triggered it.

We should still correct the behavior to return an empty all Entries rather than an incorrect one. We should also add a breaking change note. Note that non-bulk operations aren't affected by this - only bulk insert.

@AndriySvyryd
Copy link
Member

We should still correct the behavior to return an empty Entries rather than an incorrect one

It should contain all entities in the batch, including the correct one

roji added a commit to roji/efcore that referenced this issue Nov 16, 2022
roji added a commit to roji/efcore that referenced this issue Nov 16, 2022
@ajcvickers ajcvickers modified the milestones: 7.0.x, 8.0.0 Nov 16, 2022
roji added a commit to roji/efcore that referenced this issue Nov 17, 2022
@ghost ghost closed this as completed in #29571 Nov 17, 2022
ghost pushed a commit that referenced this issue Nov 17, 2022
@roji roji added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Nov 17, 2022
@ajcvickers ajcvickers modified the milestones: 8.0.0, 8.0.0-preview1 Jan 29, 2023
@ajcvickers ajcvickers modified the milestones: 8.0.0-preview1, 8.0.0 Nov 14, 2023
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-save-changes closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants