I have the following method updating an entity. The only biff I had was that when an non-existing ID were provided, I got an harsh exception.

public bool Update(Thing thing)
{
    Context.Things.Update(thing);
    int result = Context.SaveChanges();

    return result == 1;
}

So I added a check to control the exception thrown (plus some nice logging and other facilitation). Eventually, I plan to skip the throwing up entirely.

public bool UpdateWithCheck(Thing thing)
{
    Thing target = Context.Things.SingleOrDefault(a => a.Id == thing.Id);
    if (target == null)
        throw new CustomException($"No thing with ID {thing.Id}.");

    Context.Things.Update(thing);
    int result = Context.SaveChanges();

    return result == 1;
}

No, this doesn't work, because the entity already is being tracked. I have several options to handle that.

1. Change to Context.Where(...).AsNoTracking().
2. Explicitly set the updated fields in target and save it.
3. Horse around with entity states and tampering with the tracker.
4. Removing the present and adding the new one.

I can't decide which is the best practice. Googling gave me the default examples that do not contain the check for pre-existing status in the same operation.

CodePudding user response：

The reason for the exception is because by loading the entity from the Context to check if it exists, you now have a tracked reference. When you go to update the detatched reference, EF will complain that a instance is already tracked.

The simplest work-around would be:

public bool UpdateWithCheck(Thing thing)
{
    bool doesExist = Context.Things.Any(a => a.Id == thing.Id);
    if (!doesExist)
        throw new CustomException($"No thing with ID {thing.Id}.");

    Context.Things.Update(thing);
    int result = Context.SaveChanges();

    return result == 1;
}

However, there are two problems with this approach. Firstly, because we don't know the scope of the DbContext instance or can guarantee the order of methods, it may be possible that at some point that DbContext instance could have loaded and tracked that instance of the thing. This can manifest as seemingly intermittent errors. The proper way to guard against that would be something like:

public bool UpdateWithCheck(Thing thing)
{
    bool doesExist = Context.Things.Any(a => a.Id == thing.Id);
    if (!doesExist)
        throw new CustomException($"No thing with ID {thing.Id}.");

    Thing existing = Context.Things.Local.SingleOrDefault(a => a.Id == thing.Id);
    if (existing != null)
        Context.Entry(existing).State = EntityState.Detached;

    Context.Things.Update(thing);
    int result = Context.SaveChanges();

    return result == 1;
}

This checks the local tracking cache for any loaded instances, and if found, detaches them. The risk here is that any modifications that haven't be persisted in those tracked references will be discarded, and any references floating around that would have assumed were attached, will now be detached.

The second significant issue is with using Update(). When you have detached entities being passed around there is a risk that data you don't intend to be updated could be updated. Update will replace all columns, where typically if a client might only be expected to update a subset of them. EF can be configured to check row versions or timestamps on entities against the database before updating when your database is set up to support them (Such as Snapshot isolation) which can help guard against stale overwrites, but still allow unexpected tampering.

As you've already figured out, the better approach is to avoid passing detached entities around, and instead use dedicated DTOs. This avoids potential confusion about what objects represent view/consumer state vs. data state. By explicitly copying the values across from the DTO to the entity, or configuring a mapper to copy supported values, you also protect your system from unexpected tampering and potential stale overwrites. One consideration with this approach is that you should guard updates to avoid unconditionally overwriting data with potentially stale data by ensuring your Entity and DTO have a RowVersion/Timestamp to compare. Before copying from DTO to the freshly loaded Entity, compare the version, if it matches then nothing has changed in the data row since you fetched and composed your DTO. If it has changed, that means someone else has updated the underlying data row since the DTO was read, so your modifications are against stale data. From there, take an appropriate action such as discard changes, overwrite changes, merge the changes, log the fact, etc.

CodePudding user response：

Just alter properties of target and call SaveChanges() - remove the Update call. I'd say the typical use case these days is for the input thing to not actually be a Thing but to be a ThingViewModel, ThingDto or some other variation on a theme of "an object that carries enough data to identify and update a Thing but isn't actually a DB entity". To that extent, if the notion of updating properties of Thing from ThingViewModel by hand bores you, you can look at a mapper (AutoMapper is probably the most well known but there are many others) to do the copying for you, or even set you up with a new Thing if you decide to turn this method into an Upsert