Navigational property not deleting on update in EF Core

Question

I have an entity in my database with the following properties

 public class Employee : 
    {
        public long Id { get; set; }
        public string EmployeeNumber { get; set; }
    
        [ForeignKey("EmployeesId")]
        public List<PrisonsLkpt> Prisons { get; set; }  

        // .....     
    }

It has other properties but they are not that important. So when I add a new entity with 4 prisons, it works perfectly with insert. But on an update, if I remove 2 prisons from the list, it does not remove the 2 prisons from the collection. However if I add one more prison it will add to the collection. The following is the code for the create/Update of the entity.

public async Task<BaseOutputDto> CreateOrEditEmployee(CreateUpdateEmployeeInputDto input)
{
    BaseOutputDto response = new BaseOutputDto();
    CreateEditChecks(input, true);
    var prisons = await _prisonsLkptRepo.GetAllListAsync(x => input.PrisonIds.Contains(x.Id));
    try
    {
        Employee employee = _objectMapper.Map<Employee>(input);
        employee.Prisons = prisons;
 
        if (input.Id != 0)
        {
            await _employeeRepo.UpdateAsync(employee);
        }
        else
        {
            await _employeeRepo.InsertAsync(employee);
        }     

        response.IsSuccessful = true;
        return response;
    }
    catch (Exception ex)
    {
        response.IsSuccessful = false;
        throw new UserFriendlyException(ex.InnerException == null ? ex.Message : ex.InnerException.Message);
    }
}

Answer 1

It will depend on how exactly you are trying to remove the related entities. For example to remove specific prisons from an employee:

int[] prisonIdsToRemove = new { 1, 2 };

var employee = context.Employees
    .Include(x => x.Prisons)
    .Single(x => x.Id == employeeId);

var prisonsToRemove = employee.Prisons
    .Where(x => prisonIdsToRemove.Contains(x.Id));

foreach (var prison in prisonsToRemove)
    employee.Prisons.Remove(prison);

context.SaveChanges();

Basically eager load the prisons /w Include then identify the instances on the employee that you want to remove and remove them from the employee's collection. With EF it is using instance tracking to determine whether records should be added, removed, associated, or de-associated. You need to avoid "shortcuts" like setting the employee.Prisons = new List<Prison>(); or such to clear the prisons to re-associate the new set of prisons. This wipes the tracking collection reference that EF would be using to know what existing prisons the employee had. Also make sure you haven't disabled tracking on the DbContext or read the employee /w AsNoTracking().

Edit: When it comes to EF, I do not recommend "Upsert" operations, do explicit inserts or explicit updates depending on if your action intends to be applied to a new employee or an existing employee. The code and dealing with tracking instances will be different in each scenario. For instance in an insert scenario we can optimize to treat all prison associations as new adds, and we can map a new instance of the employee In an update, we need to potentially add or remove prison associations to an existing instance.

You can have an Upsert action, but determine whether you want to insert or update and hand off to the appropriate action.

public async Task<BaseOutputDto> CreateOrEditEmployee(CreateUpdateEmployeeInputDto input)
{
    try
    {
        CreateEditChecks(input, true); // What if this fails? throw?
 
        if (input.EmployeeId == 0)
            await createEmployee(input);
        else
            await updateEmployee(input);

        return BaseResponseDto(true);
    }
    catch (Exception ex)
    {
        _logger.Log(UserFriendlyException(ex.InnerException == null ? ex.Message : ex.InnerException.Message));
        return new BaseResponseDto(false);
    |
}

private async Task createEmployee(CreateUpdateEmployeeInputDto input)
{
    Employee employee = _objectMapper.Map<Employee>(input);
    var prisons = await _context.Prisons
        .Where(x => input.PrisonIds.Contains(x.Id))
        .ToListAsync();
    // Note: For collection references in entities you should remove access to setters. No employee.Prisons = ***
    foreach(var prison in prisons)
        employee.Prisons.Add(prison);

    _context.Employees.Add(employee);
    await _context.SaveChanges();
}

private async Task updateEmployee(CreateUpdateEmployeeInputDto input)
{
    Employee employee = await _context.Employees
        .Include(x => x.Prisons)
        .Single();
  
   // TODO: Copy over any values from input that can be updated on the employee into the loaded employee.    


    var existingPrisonIds = employee.Prisons.Select(x => x.Id).ToList();
    var updatedPrisonIds = input.PrisonIds;
    var prisonIdsToAdd = updatedPrisonIds.Except(existingPrisonIds).ToList();
    var prisonIdsToRemove = existingPrisonIds.Except(updatedPrisonIds).ToList();

    if (prisonIdsToRemove.Any())
    {
        var prisonsToRemove = employee.Prisons
            .Where(x => prisonIdsToRemove.Contains(x.Id));
        foreach(var prison in prisonsToRemove)
            employee.Prisons.Remove(prison);
    }
    if (prisonIdsToAdd.Any())
    {
        var prisonsToAdd = await _context.Prisons
            .Where(x => prisonIdsToAdd.Contains(x.Id))
            .ToListAsync();
        foreach(var prison in prisonsToAdd)
            employee.Prisons.Add(prison);
    }
    await _context.SaveChangesAsync()

}

Here we check if we are doing an insert or an update. Inserts are simple, basically the same as the code you had. Map the Employee, load the applicable prisons, associate them, insert, and save. The key change here is that with collection references you should always hide away access to the setter. Setting a collection on a new entity isn't a problem, but leaving an accessible setter for an existing entity to be updated would lead to many problems. Just initialize the collection property in the entity and use Add/Remove/Clear on the collection.

Update is a different beast. While it is possible to update entities by mapping to a new instance and calling Update on the DbContext, it is inadvisable and error prone. Firstly it will overwrite all existing data on record which means you need to absolutely trust that the passed in data is accurate and complete. If some fields are missing, you will get errors or overwrites erasing existing data. If someone tampers with the form POST data before calling the service, you will get errors or worse, invalid data state being written. For update scenarios you should fetch the entity and its associated prisons, copy any values from the provided DTO into the employee that you allow to be changed, then compare the collections to determine what prisons need to be added or removed. A cart-blanche replacement of the existing collection won't work as EF will expect to be inserting all entries as new, and hit existing associated records.

I highly recommend doing away with things like generic repository patterns that return materialized entities. These will either add a lot of complexity or a lot of limitations. For instance in this case when updating an employee I want to eager load the prisons. In a generic repository scenario I either always eager load, or have to introduce methods to provide employees with vs without prisons, or the complexity of expressions to determine what, if anything I need to eager load. EF already provides a repository in the form of the DbSet<TEntity>. Where I do recommend repository patterns is to make testing easier or enforce base rules like tenancy/filtering as an alternative to global filters. In these cases the repository returns IQueryable<TEnity> which allows consumers to use everything EF has to offer in terms of eager loading, projection, pagination, etc.