Entity Framework Core throws a primary key constraint violation error, when loading, adding, and saving entities without modification

Question

I'm currently developing an Azure Function app that uses Entity Framework Core to persist data in Microsoft SQL Server.

Now, the issue is that I cannot load, add, and save data.

In the service code below, I load records, adds them to the context, and save them. No modifications occur. However, I get violation of primary key constraint error.

I have added all relevant code.

After multiple hours, I can't figure out way Entity Framework Core considers the entities as new and tries to insert them.

Any help appreciated!

Error:

Microsoft.EntityFrameworkCore.DbUpdateException: An error occurred while saving the entity changes. See the inner exception for details.

Microsoft.Data.SqlClient.SqlException (0x80131904): Violation of PRIMARY KEY constraint 'PK_NotificationRecords'. Cannot insert duplicate key in object 'dbo.NotificationRecords'. The duplicate key value is (02289e67-e7f1-401e-f603-08dcf4ef7c1b).

Service:

private async Task DispatchNotifications(List<ApplicationsByOwner> applicationsByOwners, CancellationToken cancellationToken)
{
    var activeNotificationRecords = (await notificationRepository.GetActiveNotificationRecords(assetType: AssetType.Application, cancellationToken: cancellationToken)).ToList();
    await notificationRepository.AddNotificationRecordsAsync(activeNotificationRecords, cancellationToken);
    await notificationRepository.SaveChangesAsync(cancellationToken); # ERROR HERE
}

DbContext:

public sealed class AssetOwnershipContext : DbContext
{
    public DbSet<NotificationRecord> NotificationRecords { get; set; }

    public AssetOwnershipContext(DbContextOptions<AssetOwnershipContext> options) : base(options)
    {
        Database.EnsureCreated();
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        // NotificationRecord
        modelBuilder.Entity<NotificationRecord>().Property(p => p.CreatedAt).HasDefaultValueSql("GETUTCDATE()");
        modelBuilder.Entity<NotificationRecord>().Property(p => p.UpdatedAt).HasDefaultValueSql("GETUTCDATE()").ValueGeneratedOnAdd();
        modelBuilder.Entity<NotificationRecord>().Property(d => d.AssetType).HasConversion<string>();

        base.OnModelCreating(modelBuilder);
    }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        base.OnConfiguring(optionsBuilder);
    }
}

Repository:

public async Task<IEnumerable<NotificationRecord>> GetActiveNotificationRecords(Guid? owner = default, AssetType? assetType = default, CancellationToken cancellationToken = default) 
{
   var query = context.NotificationRecords.Where(x => x.IsActive);

   if (owner.HasValue)
   {
       query = query.Where(x => x.OwnerId == owner);
   }

   if (assetType.HasValue)
   {
       query = query.Where(x => x.AssetType == assetType);
   }

   return await query.ToListAsync(cancellationToken);
}

public async Task AddNotificationRecordsAsync(IEnumerable<NotificationRecord> notificationRecords, CancellationToken cancellationToken = default)
{
    await context.NotificationRecords.AddRangeAsync(notificationRecords, cancellationToken);
}

public async Task<bool> SaveChangesAsync(CancellationToken cancellationToken = default)
{
    return await context.SaveChangesAsync(cancellationToken) >= 0;
}

NotificationRecord:

[Index(nameof(OwnerId))]
[Index(nameof(AssetId))]
[Index(nameof(UserPrincipalName))]
[Index(nameof(CreatedAt))]
public class NotificationRecord
{
    [Key]
    [DatabaseGenerated(DatabaseGeneratedOption.Identity)]
    public Guid Id { get; init; }
    [Required] public Guid OwnerId { get; set; }
    [Required] public Guid AssetId { get; set; }
    [Required] public AssetType AssetType { get; set; }
    [MaxLength(256)] public string? DisplayName { get; set; }
    [MaxLength(256)] public string? GivenName { get; set; }
    [MaxLength(256)] public string? Surname { get; set; }
    [Required][MaxLength(256)] public string? UserPrincipalName { get; set; }
    // [Required] public ICollection<Notification> Notifications { get; } = new List<Notification>();
    [Required] public DateTime? CreatedAt { get; set; }
    [Required] public DateTime? UpdatedAt { get; set; }
    [Required] public bool IsActive { get; set; } = true;
}

Answer 1

This minimal example does nothing except confuse EF depending on what your Repository class actually does.

var activeNotificationRecords = (await notificationRepository.GetActiveNotificationRecords(assetType: AssetType.Application, cancellationToken: cancellationToken)).ToList();
await notificationRepository.AddNotificationRecordsAsync(activeNotificationRecords, cancellationToken);
await notificationRepository.SaveChangesAsync(cancellationToken); # ERROR HERE

Based on your repository code is doing calling an "Add" method which attempts to add those items back into the NotificationRecords DbSet on the Context would be telling EF to insert the records into the table when those records already exist.

All you need is:

var activeNotificationRecords = (await notificationRepository.GetActiveNotificationRecords(assetType: AssetType.Application, cancellationToken: cancellationToken)).ToList();
// make your changes to the notification records...

await notificationRepository.SaveChangesAsync(cancellationToken);

You will run into problems if your "changes" involve associating the detached List<ApplicationsByOwner> applicationsByOwners. If you want to update things like navigation properties then your read call to get entities needs to eager load those associations, and any entities you want to associate with those entities needs to be tracked by the DbContext that the operation is using. Avoid passing detached entities around as re-associating them correctly is more complicated and error prone compared to simply fetching new relations by ID.

I strongly recommend removing that Repository pattern layer. EF already provides a repository in the form of the DbSet<NotificationRecord>. Using a thin layer abstraction over-top just serves to confuse and cripple EF operations like this. (At least it isn't a Generic Repository :) An explicit Repository pattern over EF can be justified to simplify unit testing or enforce common, low-level filtering rules (multi-tenancy, soft-delete, etc.) but otherwise keep it simple and leverage what EF already provides. Abstractions like that can hide problems, result in inefficient querying, or end up being highly complex where you're reinventing the wheel to try and maintain some of the flexibility EF already provides out of the box. (projection, pagination, sorting, filtering, eager loading, etc.)

Edit: Unit testing is a very valid justification for introducing a repository. The main issue is when you return IEnumerable<TEntity> you are significantly restricting EF in that the repository always must materialize the query to memory for the consumer. What if you want entities but want to eager load related entities? perform tighter filtering for some consumers? Project the results to a smaller footprint View Model? or simply get a count? Doing these after the fact of materialization is expensive. Adding optional arguments or extra methods to handle these scenarios adds complexity or dozens of very similar methods to sift through. async also isn't a silver bullet, often a synchronous call would be faster given async introduces a small overhead to every query. A good compromise to enable consumers control over the consumption while still offering an abstraction to mock is to have the repository return IQueryable<TEntity>

public IQueryable<NotificationRecord> GetNotificationRecords(bool includeInactive = false) 
{
    IQueryable<NotificationRecord> query = context.NotificationRecords;
    if (!includeInactive)
        query = query.Where(x => x.IsActive);
    return query;
}

Here the query defaults to active-only as a core default rule but we expose the option to include inactive items. The consumer can add optional filtering, sorting, and/or projections such as getting a count without materializing data or needing a separate function on the repository. They also have full control over whether to perform a synchronous or asynchronous materialization.

i.e.

var items = await _repository.GetNotificationRecords()
     .Where(x => x.CreatedAt >= startDate)
     .ProjectTo<NotificationSummaryViewModel>(config)
     .ToListAsync();

// or somewhere else:

var notificationCount = await _repository.GetNotificationRecords()
     .Count(); // builds a count query to DB, not materializing all records.

The only catch to mocking a repository that exposes IQueryable is that while your mock can return something like a stubbed List<TEntity> for synchronous calls, that won't work for asynchronous calls. For this you need the mock to return a collection structure that works with asynchronous enumeration. Packages like MockQueryable help with this. (https://www.nuget.org/packages/MockQueryable.Core/7.0.4-beta) (Or MockQueryable.Moq if you are using Moq for mocking)

Some arguments against a repository exposing IQueryable include that it "leaks" EF-isms. However the justification for the repository is not to abstract solutions from EF, (Honestly a terrible justification for using a repository in most cases) it is to help enforce core, important rules and serve as an abstraction to facilitate unit testing, not hide the fact we are leveraging EF. Having an abstraction to hide EF would be a valid argument for solutions where you actively want to support reading data from a relational DB (EF) but also switch to reading from a NoSQL and/or distributed service where the "repository" pattern serves as a standardization, lowest common denominator common interface to data. This incurs costs as you lose much of the capability that EF can provide, and sadly sometimes this level of abstraction is argued for when a relational DB is the only requirement for sourcing data, either because "one day it might be needed" or "in case we find EF is too slow". (Which forms a self-fulfilling prophecy as introducing this abstraction cripples EF's performance.)