EF Core - adding/updating entity and adding/updating/removing child entities in one request

Question

I am struggling with what seemed to be a couple of basic operations.

Let's say I've got a class named Master:

public class Master
{
    public Master()
    {
        Children = new List<Child>();
    }

    public int Id { get; set; }
    public string SomeProperty { get; set; }

    [ForeignKey("SuperMasterId")]
    public SuperMaster SuperMaster { get; set; }
    public int SuperMasterId { get; set; }

    public ICollection<Child> Children { get; set; }
}

public class Child 
{
    public int Id { get; set; }
    public string SomeDescription { get; set; }
    public decimal Count{ get; set; }

    [ForeignKey("RelatedEntityId")]
    public RelatedEntity RelatedEntity { get; set; }
    public int RelatedEntityId { get; set; }

    [ForeignKey("MasterId")]
    public Master Master { get; set; }
    public int MasterId { get; set; }
}

We have a controller action like this:

public async Task<OutputDto> Update(UpdateDto updateInput)
{
    // First get a real entity by Id from the repository
    // This repository method returns: 
    // Context.Masters
    //    .Include(x => x.SuperMaster)
    //    .Include(x => x.Children)
    //    .ThenInclude(x => x.RelatedEntity)
    //    .FirstOrDefault(x => x.Id == id)
    Master entity = await _masterRepository.Get(input.Id);

    // Update properties
    entity.SomeProperty = "Updated value";
    entity.SuperMaster.Id = updateInput.SuperMaster.Id;

    foreach (var child in input.Children)
    {
        if (entity.Children.All(x => x.Id != child.Id))
        {
            // This input child doesn't exist in entity.Children -- add it
            // Mapper.Map uses AutoMapper to map from the input DTO to entity
            entity.Children.Add(Mapper.Map<Child>(child));
            continue;
        }

        // The input child exists in entity.Children -- update it
        var oldChild = entity.Children.FirstOrDefault(x => x.Id == child.Id);
        if (oldChild == null)
        {
            continue;
        }

        // The mapper will also update child.RelatedEntity.Id
        Mapper.Map(child, oldChild);
    }

    foreach (var child in entity.Children.Where(x => x.Id != 0).ToList())
    {
        if (input.Children.All(x => x.Id != child.Id))
        {
            // The child doesn't exist in input anymore, mark it for deletion
            child.Id = -1;
        }
    }

    entity = await _masterRepository.UpdateAsync(entity);

    // Use AutoMapper to map from entity to DTO
    return MapToEntityDto(entity);
}

Now the repository method (MasterRepository):

public async Task<Master> UpdateAsync(Master entity)
{
    var superMasterId = entity.SuperMaster.Id;

    // Make sure SuperMaster properties are updated in case the superMasterId is changed
    entity.SuperMaster = await Context.SuperMasters
        .FirstOrDefaultAsync(x => x.Id == superMasterId);

    // New and updated children, skip deleted
    foreach (var child in entity.Children.Where(x => x.Id != -1))
    {
        await _childRepo.InsertOrUpdateAsync(child);
    }

    // Handle deleted children
    foreach (var child in entity.Children.Where(x => x.Id == -1))
    {
        await _childRepo.DeleteAsync(child);
        entity.Children.Remove(child);
    }

    return entity;
}

And finally, the relevant code from ChildrenRepository:

public async Task<Child> InsertOrUpdateAsync(Child entity)
{
    if (entity.Id == 0)
    {
        return await InsertAsync(entity, parent);
    }

    var relatedId = entity.RelatedEntity.Id;
    entity.RelatedEntity = await Context.RelatedEntities
        .FirstOrDefaultAsync(x => x.Id == relatedId);

    // We have already updated child properties in the controller method 
    // and it's expected that changed entities are marked as changed in EF change tracker
    return entity;
}

public async Task<Child> InsertAsync(Child entity)
{
    var relatedId = entity.RelatedEntity.Id;
    entity.RelatedEntity = await Context.RelatedEntities
        .FirstOrDefaultAsync(x => x.Id == relatedId);

    entity = Context.Set<Child>().Add(entity).Entity;

    // We need the entity Id, hence the call to SaveChanges
    await Context.SaveChangesAsync();
    return entity;
}

The Context property is actually DbContext and the transaction is started in an action filter. If the action throws an exception, the action filter performs the rollback, and if not, it calls SaveChanges.

The input object being sent looks like this:

{
  "someProperty": "Some property",
  "superMaster": {
     "name": "SuperMaster name",
     "id": 1
  },
  "children": [
  {
    "relatedEntity": {
      "name": "RelatedEntity name",
      "someOtherProp": 20,
      "id": 1
    },
    "count": 20,
    "someDescription": "Something"
  }],
  "id": 10
}

The Masters table currently has one record with Id 10. It has no children.

The exception being thrown is:

Microsoft.EntityFrameworkCore.DbUpdateConcurrencyException: Database operation expected to affect 1 row(s) but actually affected 0 row(s). Data may have been modified or deleted since entities were loaded.

What's going on here? I thought EF was supposed to track changes and that includes knowing that we called SaveChanges in that inner method.

EDIT Removing that call to SaveChanges changes nothing. Also, I couldn't find any INSERT or UPDATE SQL statement generated by EF when watching what happens in SQL Server Profiler.

EDIT2 The INSERT statement is there when SaveChanges is called, but still there is no UPDATE statement for Master entity.

Solution 1

As usual, posting this question to StackOverflow helped me resolve the problem. The code originally didn't look like in the question above, but I was rather fixing the code while writing the question.

Before writing the question, I spent almost a day trying to figure out what the problem was and so I tried different things, such as recreating entity instances and attaching them manually, marking some entities as Unchanged/Modified, using AsNoTracking or even completely disabling automatic change tracking for all entities and marking all of them Added or Modified manually.

Turned out the code which caused this issue was in a private method of that child repository which I omitted as I didn't think it was relevant. It truly wouldn't be relevant though, if I haven't forgot to remove some manual change tracking code from it, which basically fiddled with EF's automatic change tracker and caused it to misbehave.

But, thanks to StackOverflow, the problem was solved. When you talk to someone about the problem, you need to re-analyze it yourself, to be able to explain all the little bits of it in order for that someone who you talk to (in this case, SO community) to understand it. While you re-analyze it, you notice all the little problem-causing bits and then it's easier to diagnose the issue.

So anyway, if someone gets attracted to this question because of the title, via google search or w/e, here are some key points:

• If you are updating entities on multiple levels, always call .Include to include all related navigation properties when getting the existing entity. This will make all of them loaded into the change tracker and you won't need to attach/mark manually. After you're done updating, a call to SaveChanges will save all your changes properly.

• Don't use AutoMapper for the top-level entity when you need to update child entities, especially if you have to implement some additional logic when updating children.

• Never ever update primary keys like I tried to when setting the Id to -1, or like I tried to on this line right here in the controller Update method:

  // The mapper will also update child.RelatedEntity.Id Mapper.Map(child, oldChild);

• If you need to handle deleted items, better detect them and store in a separate list, and then manually call the repository delete method for each of them, where the repository delete method would contain some eventual additional logic regarding the related entities.

• If you need to change the primary key of a related entity, you need to first remove that related entity from the relation and just add a new one with an updated key.

So here is the updated controller action with null and safety checks omitted:

public async Task<OutputDto> Update(InputDto input)
{
    // First get a real entity by Id from the repository
    // This repository method returns: 
    // Context.Masters
    //    .Include(x => x.SuperMaster)
    //    .Include(x => x.Children)
    //    .ThenInclude(x => x.RelatedEntity)
    //    .FirstOrDefault(x => x.Id == id)
    Master entity = await _masterRepository.Get(input.Id);

    // Update the master entity properties manually
    entity.SomeProperty = "Updated value";

    // Prepare a list for any children with modified RelatedEntity
    var changedChildren = new List<Child>();

    foreach (var child in input.Children)
    {
        // Check to see if this is a new child item
        if (entity.Children.All(x => x.Id != child.Id))
        {
            // Map the DTO to child entity and add it to the collection
            entity.Children.Add(Mapper.Map<Child>(child));
            continue;
        }

        // Check to see if this is an existing child item
        var existingChild = entity.Children.FirstOrDefault(x => x.Id == child.Id);
        if (existingChild == null)
        {
            continue;
        }

        // Check to see if the related entity was changed
        if (existingChild.RelatedEntity.Id != child.RelatedEntity.Id)
        {
            // It was changed, add it to changedChildren list
            changedChildren.Add(existingChild);
            continue;
        }

        // It's safe to use AutoMapper to map the child entity and avoid updating properties manually, 
        // provided that it doesn't have child-items of their own
        Mapper.Map(child, existingChild);
    }

    // Find which of the child entities should be deleted
    // entity.IsTransient() is an extension method which returns true if the entity has just been added
    foreach (var child in entity.Children.Where(x => !x.IsTransient()).ToList())
    {
        if (input.Children.Any(x => x.Id == child.Id))
        {
            continue;
        }

        // We don't have this entity in the list sent by the client.
        // That means we should delete it
        await _childRepository.DeleteAsync(child);
        entity.Children.Remove(child);
    }

    // Parse children entities with modified related entities
    foreach (var child in changedChildren)
    {
        var newChild = input.Children.FirstOrDefault(x => x.Id == child.Id);

        // Delete the existing one
        await _childRepository.DeleteAsync(child);
        entity.Children.Remove(child);

        // Add the new one
        // It's OK to change the primary key here, as this one is a DTO, not a tracked entity,
        // and besides, if the keys are autogenerated by the database, we can't have anything but 0 for a new entity
        newChild.Id = 0;
        entity.Djelovi.Add(Mapper.Map<Child>(newChild)); 
    }

    // And finally, call the repository update and return the result mapped to DTO
    entity = await _repository.UpdateAsync(entity);
    return MapToEntityDto(entity);
}

Solution 2

Use this generic sub to mark the child state, easy to use

Notes:

• PromatCon: the entity object
• amList: is the child list that you want to add or modify
• rList: is the child list that you want to remove

updatechild(objCas.ECC_Decision, PromatCon.ECC_Decision.Where(c => c.rid == objCas.rid & !objCas.ECC_Decision.Select(x => x.dcid).Contains(c.dcid)).toList())

public void updatechild<Ety>(ICollection<Ety> amList, ICollection<Ety> rList)
{
        foreach (var obj in amList)
        {
            var x = PromatCon.Entry(obj).GetDatabaseValues();
            if (x == null)
                PromatCon.Entry(obj).State = EntityState.Added;
            else
                PromatCon.Entry(obj).State = EntityState.Modified;
        }
        foreach (var obj in rList.ToList())
            PromatCon.Entry(obj).State = EntityState.Deleted;
}

PromatCon.SaveChanges()