EntityEntry.OriginalValues APIs incorrectly return current values

[Dixin]

REcently I ported some EF code to EF Core, and noticed these behaviors. After changing a tracked entity, get its EntityEntry object, then:

1. EntityEntry.OriginalValues.Clone()
   In EF, it return a clone of original values. In EF Core, it returns a clone of current values.
   Workaround in EF Core:

var originalValues = entry.OriginalValues.Clone(); // Return current values.
originalValues.SetValues(entry.OriginalValues); // Now original values.

2. EntityEntry.OriginalValues.ToObject()
   In EF, it returns an entity with original values. In EF Core, it returns an entity with current values.
   Workaround in EF Core:

var originalValues = entry.OriginalValues.Clone();
originalValues.SetValues(entry.OriginalValues);
Product original = (Product)originalValues.ToObject();

Are these bugs, or am I using it in a wrong way?

[ajcvickers]

Note for triage: confirmed this is a bug.

[tonyeung]

@Dixin thanks for the workarounds. Makes spending two hours trying to figure out if I was going crazy not so bad.

[divega]

@ajcvickers I assigned this to 1.1.1 because I am not sure whether it is a regression. Please fix on the appropriate branch.

[ajcvickers]

@divega Not a regression. OriginalValues did not exist before 1.1. :-)

[ajcvickers]

@divega @rowanmiller Moving this to 1.1.1 as discussed.

[ajcvickers]

Justification: If a developer doesn't catch that the wrong values are returned, then this could result in the wrong data being saved when doing things like resolving concurrency exception.
Risk: Low--only needs a small change to add overrides in an existing derived type.

[Eilon]

This patch bug is approved. Please use the normal code review process w/ a PR and make sure the fix is in the correct branch, then close the bug and mark it as done.

[AndriySvyryd]

✅ Verified

[JasonLeedy]

this does not appear to actually be fixed, exhibiting this behavior in 1.1.2

[AndriySvyryd]

@JasonLeedy Could you share a repro project that shows the issue?

[JasonLeedy]

Yes, take me a little bit to extract it from surrounding code @JasonLeedy<https://github.com/jasonleedy> Could you share a repro project that shows the issue? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#7332 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/Adh5o3MZ5kSG9Qys0iTPmTOLeFWdapYIks5shaEUgaJpZM4LYguq>.

[raygabe]

@AndriySvyryd, I was asked by @JasonLeedy to provide our MVC/EF Core 1.1.2 project (WebAppTestEFC_OriginalValues, attached) to repro the “original same as current value” issue we're having. Please see readme and let us know your thoughts. Essentially, in the SaveChangesAsync() override of the DbContext class, we're expecting the ChangeTracker.Entries(), entry.OriginalValues to match the database prior to committing the update, NOT be the same as entry.CurrentValues or am I missing something?

public override async Task<int> SaveChangesAsync(bool acceptAllChangesOnSuccess, CancellationToken cancellationToken = new CancellationToken())
        {          
                foreach (var entry in ChangeTracker.Entries())
                {
                    if (entry.State == EntityState.Detached || entry.State == EntityState.Unchanged)
                        continue;
                                        
                    var current = entry.CurrentValues["Version"];

                    // original the SAME as current value ??? 
                    var original = entry.OriginalValues["Version"];
                    var original2 = entry.Property("Version").OriginalValue;
...

WebAppTestEFC_OriginalValues.zip

[AndriySvyryd]

@raygabe Thanks for the great repro! The behavior you are seeing is by design.

_context instance is not preserved between requests, you can see this if you examine _context.ChangeTracker.Entries() before calling _context.Update(material). So there's no place to keep the original values. This is good though as keeping and relying on local state could lead to bugs.

If you need the original values you need to get them from the database again:

var entry = _context.Update(material);
entry.OriginalValues.SetValues(await entry.GetDatabaseValuesAsync());

After doing this the rest of your code should behave as expected.

[raygabe]

@AndriySvyryd @JasonLeedy Thanks for the really quick response! Understood. I suspected something like this was possible, but given issue # 7332 affecting OriginalValues.Clone() & .ToObject(), I had to wonder.

It's understandable that _context instance is not preserved between requests, but is this to say "by design" only in EFC 1.1.2 b/c we have evidence of ChangeTracker.Entries() / entry.Properties / property.OriginalValue working as desired in this example: https://www.meziantou.net/2017/08/14/entity-framework-core-history-audit-table, which our EFC 1.1.2 repro example replicates as presented by @meziantou.

While your suggested round-trip "workaround" is nice (re-setting OriginalValues) and does in fact work for us, we had hoped to avoid a 2nd db query (although only 1 record). Our purpose for OriginalValues is to maintain an audit trail to be FDA Title 21 CFR Part 11 compliant.

Again, great appreciation for the EF Core team's tremendous effort and outstanding support. Continued success! :-)

[raygabe]

@AndriySvyryd @JasonLeedy It appears using an untracked copy of the entities made prior to the _context.Update() call and passing it in the override SaveChangesAsync() call does work to "preserve" original values in the MVC app controller's http POST to Edit method:

var utMaterials = _context.Materials.AsNoTracking();
_context.Update(material);
await _context.SaveChangesAsync(@"WebApp\RayG", utMaterials);

AND avoids the call inside SaveChangesAsync() to "re-load" OriginalValues:

entry.OriginalValues.SetValues(await entry.GetDatabaseValuesAsync());

Although Not sure if this what you meant by problems w/"relying on local state", but appears to be safe and reliable in this read-only scenario and w/out the overhead of tracking on the copy! :-)

NOW, just need to consider concurrency in the disconnected model...

[raygabe]

@AndriySvyryd @JasonLeedy Should have known, but just realized AsNoTracking() IS a database query. Actually had known this, but in my haste for resolution was thinking this was a simple copy method.

var utMaterials = _context.Materials.AsNoTracking());

Obviously, nothing gained by this so will proceed w/making a database query to get "original values" for audit trail.

entry.OriginalValues.SetValues(await entry.GetDatabaseValuesAsync());

Thanks again. Consider this matter closed. :-)

[bakintunde]

@AndriySvyryd Thanks for your clarification above. But what's the whole point of having the entry.OriginalValues property when I can't trust it to have original values and have to refresh it from the database?

If it's by design as you say, the docs should state this clearly and they don't. :(. Would've saved us hours on my team. We're trying to implement audit using EF Core

Also, entry.OriginalValues behaves inconsistently. It returns correct values for the entity at the root of the object graph ONLY but currentvalues for other connected entities/entries.

At a minimum, the docs should state the correct behavior.

My scenario.

• Fetch entity from db and use .include to include related entities
• Make changes to all entities
• OriginalValues behaves consistently returning pre-change values only for entity, not related ones.