efcore: Throw a better exception message when concurrency token is an array

Problem with the current behaviour that it does not work with detached or mapped Entities over an API.

extremly simplified

put(ClientModel model) {
  var entity = getFromEntityDbContext();
  map(model, entity);
  SaveChanges()
}

ClientModel and Entity has a [TimeStamp]. But if we update the TimeStampe (RowVersion) of Entity with the value from the client it is ignored. So the concurrency does not work.

So we are forced to do a manual concurrency check on the RowVersion, instead of letting EF Core to do the Work. This is not the behaviour we expected.

@UncleFirefox This is not a bug, it’s a feature, an API design problem. I doubt that MS will change the API anytime soon. Maybe in EF Core 6? Let’s see. There should be a dedicated API for concurrency conflict checking alone.

Following the discussion I am not 100% what the best solution to this is. However what I think this is an undocumented breaking behavioral change in EF Core 3.0. Previously it was not possible to modify a RowVersion clientside. This would have thrown an DbConcurrentUpdateException. Now no exception is thrown and instead a given clientside rowversion is accepted as a new value (mapped from a disconnected entity).

So implementations that rely on the old behavior are now broken (implementations like this might not be optimal regarding above thread but I am not sure about this).

To me this seems as a big issue (despite the relative low anticipation this thread has), as it is relatively hard to detect. If there are no tests that explicitly check for concurrent updates, developers will assume their original code still work. Concurrent scenarios are especially difficult to test for single developers. So this behavioral change has the potential to stay undetected for users and developers alike. This also does not crash the app, instead there is “silent data loss”, as concurrent updates just silently override each other.

@ThomasBergholdWieser This is a bug IMO. What is the difference or replacing (timestamp) array values with replacing array from developer perspective? It should be handled the same by EF engine. IMO - it has an array and it should compare it to original value regardless how it was set. If it doesn’t work like that, I guarantee you millions of devs bumping into the same pitfall.

I will create a small sample in a unit tests.

But, this is usefull in Application where it is possible that several People are working on the same Database Record. And in a szenario where the Data is serialized. Like an ASP.NET Core Application. Serialized in the <form/> or over Json in an API. So also the [TimeStamp] RowVersion MUST be serialized, because this is the way how EF Core together with SQL Server supports a simple optimistic Concurrency Check.

And if i have to serialize the RowVersion of the given DataRecord, i must be able to set the RowVersion on the entity. So that EF Core can fill the right parameter with the Value of the RowVersion at the for the SQL Update Query.

User A:
var version1 = getRecord('key');
write version in <form/>  (with RowVersion), user A sees the data
----
User B:
var version1 = getRecord('key');
write version in <form/> (with RowVersion) to user B sees the data
user B changes the data
user B creates save, makes an HTTP post to the server
Server gets current version of the Entity from the db
Server fill the data from the post in the in the entity (including RowVersion)
Server saves the data (Version 2 is stored)
----
user A changes the data
user A creates save, makes an HTTP post to the server
Server gets current version (now Version2) of the Entity from the db
Server fill the data from the post in the in the entity (including RowVersion)
EF Core should throw in Concurrency Exception, but does not. 
And this is the Problem. It simply overwrite the changes of User A.

Only with Updating the RowVersion byte by byte. EF Core works as expected. Or if we manually check the RowVersion.

Current EF Core only work in Szenarios where the Entity are kept long, like in WinForms or WPF Applications.

Any Open Questions?

Ok, today I bumped into this exact problem. I will summarise this: If you set the RowVersion property in your Entity class as a concurrency token. Then the following code will not work:

party.RowVersion = form.RowVersion;
party.... = form...;
db.SaveChanges();

Many of us nowadays use form, then map from form to entity, or to entities, with complex mapping rules and validations. But the way EF and EF Core handles concurrency conflict is, if I guess right: it assumes that the form and the entity have to be one, that is, you are supposed to use the same class for form and entity:

var party = form;
db.Update(party);
db.SaveChanges();

Or, you are supposed to create your own entity object with primary key, then pass it to db.Update, without any additional queries:

var party = MakeEntityFromForm(form);
db.Update(party);
db.SaveChanges();

But nowadays the way we do is like this (I will not judge that if this is right or wrong):

var party = db.Party.Find(id);
ValidateAndMap(form, party);
db.SaveChanges();

EF Core will use the RowNumber of the first data state that it tracks. You cannot normally change it.

Solution: this is the shortest way to force EF to check for concurrency conflict using the RowNumber from the form of the user:

var party = db.Party.Find(id);
ValidateAndMap(form, party);
db.Entry(party).OriginalValues[nameof(Party.RowVersion)] = form.RowVersion;
db.SaveChanges();

I wonder why EF Core’s APIs are so old school with unexpected pitfal like this?

@ThomasBergholdWieser You are right, that scenario works and is a valid one. It would fail if one caches such entities though. However, I still think that the way EF behaves is not a correct one, or at least highly counter intuitive.

Given a postgresql database and using xid as the RowVersion property

        [Timestamp]
        [DatabaseGenerated(DatabaseGeneratedOption.Computed)]
        // ReSharper disable once StringLiteralTypo
        [Column("xmin", TypeName = "xid")]
        public uint RowVersion { get; set; }

If the entity is fetched as tracked, we Automap a DTO/model whose RowVersion is outdated to the entity, and then save changes. Concurrency detection does not work.

Same scenario but fetching the entity with AsNoTracking(), concurrency detection does work.

I would expect it to work in both scenarios. In the tracked mode, Automapper does update the RowVersion property, but the update statement that gets executed has an xid predicate whose value is the one that was fetched from the database, not what was mapped.

I don’t see why this behavior would not be the same in both scenarios. I get that a database generated field wouldn’t expect to be changed, and therefore the tracked mode probably ignores that field when generating the SQL, but then the behavior should be consistent for both tracked and untracked entities. I think the tracked mode is broken.

Can we have a documentation for this in either the RowVersion or IsConcurrencyToken documentation?