EntityFramework pretends to have updated a row but it doesn't

Question

I wrote an InsertOrUpdate() method which uses my EntityFramework dbcontext and I want the method to be able to receive an object and then either insert a new row or update an existing one if it exists already. Inserting works fine but unfortunately, EF won't let me do that for the update. It actually pretends it has worked but then, after reading the row, I see it didn't change.

public async Task<bool> InsertOrUpdateUserByExternalId(UserModel user, CancellationToken cancellationToken = default)
{
    if(user.User_ExternalID is null)
        return false;

    var userInDb = (await _db.Users
            .AsNoTracking()
            .FirstOrDefaultAsync(
                u => u.User_ExternalID == user.User_ExternalID,
                cancellationToken))
        .Adapt<UserModel>();

    if (userInDb is not null)
    {
        user.User_ExternalID = null; // Without this I get a "duplicate index" error
        userInDb = user;
        _db.Users.Update(userInDb.Adapt<User>());
    }
    else
    {
        await _db.Users.AddAsync(user.Adapt<User>(), cancellationToken);
    }

    var changes = _db.SaveChanges(); // This returns 1 which seems like it was successful

    return changes == 1;
}

In this version I don't convert the row from the database to a model but instead work with the entity and convert the user from the parameter to an entity. Unfortunately, I get the same outcome.

public async Task<bool> InsertOrUpdateUserByExternalId(UserModel user, CancellationToken cancellationToken = default)
{
    if (user.User_ExternalID is null)
        return false;

    var userInDb = (await _db.Users
            .AsNoTracking()
            .FirstOrDefaultAsync(
                u => u.User_ExternalID == user.User_ExternalID,
                cancellationToken));

    if (userInDb is not null)
    {
        user.User_ExternalID = null;
        userInDb = user.Adapt<User>();
        _db.Users.Update(userInDb);
    }
    else
    {
        await _db.Users.AddAsync(user.Adapt<User>(), cancellationToken);
    }

    var changes = _db.SaveChanges();

    return changes == 1;
}

Answer 1

In a nutshell, "you're doing it wrong". Don't map entities/view models back and forth when updating data if you do not need to.

public async Task<bool> InsertOrUpdateUserByExternalId(UserModel user, CancellationToken cancellationToken = default)
{
    try
    {
        if (user.User_ExternalID is null)
            return false;

        var userInDb = (await _db.Users
            .SingleOrDefaultAsync(
                u => u.User_ExternalID == user.User_ExternalID,
                cancellationToken));

        if (userInDb is not null)
        {
            userInDb.Name = user.Name;
            //  Repeat for values you allow to be changed, or use something like Automapper to map from VM into the userInDb.. (Do not map/create a new User)
        }
        else
        {
            await _db.Users.AddAsync(user.Adapt<User>(), cancellationToken);
        }

        _db.SaveChanges();
        return true;
    }
    catch (Exception ex)
    {
        // Consider logging the exception...
        return false;
    }
}

Note that your return changes == 1; won't be accurate if you update an existing row without actually changing any values. If you happen to hit edit, not change anything then call this update method the change tracker will ignore creating any UPDATE SQL statement entirely if nothing actually changed. The advantage of the change tracker over Update is the change tracker UPDATE statement only gets generated if needed, and only for columns that change. Update results in an UPDATE statement for all columns. This can be a problem if there are values injected into the view model or otherwise missing/empty when the view model converts to an entity which can end up wiping data or causing constraint violations. Maybe not today, but as the application evolves it is easy to introduce things that break persistence.

EF is designed to optimize updates through change tracking. It will handle the fact that values may, or may not be actually changed. Update is used for working with detached entities and using detached entities is something that should be avoided unless there is a really, really good reason for it. AsNoTracking() is an optimization you should consider for Read operations, especially when loading large blocks of data. (if not using projection) but certainly not "Update" scenarios where individual records are loaded. If you are finding that the DbContext change tracker is getting bloated down or retrieving stale entities then the issue is most likely that the DbContext is "alive" too long. DbContext instances should be short-lived such as per-web-request / operation, or even shorter.