PawelGerr / Thinktecture.EntityFrameworkCore

These libraries extend Entity Framework Core by a few features to make it easier to work with EF and for easier integration testing or to get more performance in some special cases.
https://dev.azure.com/pawelgerr/Thinktecture.EntityFrameworkCore
BSD 3-Clause "New" or "Revised" License
61 stars 17 forks source link

Bulkupdate identity issue #28

Closed safigi closed 1 year ago

safigi commented 1 year ago

When we try to bulk update a table the generated temp table inherit the parent table identity column. This is a problem because when we want to update the table , the temp table PK column get identity value then the update will update different set of records. Here is a short example:

original table def in sql server: create table Test(Id int identity(1,1), Name varchar(1000)); var lst = context.Tests().ToList(); //do samething with name context.BulkUpdate(lst,propertiesToUpdate: c => c.Name, propertiesToMatchOn: c => c.Id)

the generated sql will be something like that:

create table #Test_1(Id identity(1,1),Name varchar(1000));

geterated insert:

bulk insert #Test_1 (name)

update t1 Name=t2.name from Test as t1 join #Test_1 as t2 on t1.Id=t2.Id;

I think the problem is in the private string GetColumnsDefinitions(SqlServerTempTableCreatorCacheKey options) function because we need different options for temp table when we create a bulk uperation for update!

pseudo code:

if (IsIdentityColumn(property) && generateTempTableForBulkUpdate) sb.Append(" IDENTITY");

PawelGerr commented 1 year ago

I changed the default value of SqlBulkCopyOptions to KeepIdentity for BulkUpdate and BulkInsertOrUpdate. Please check out the new version 4.3.0.

safigi commented 1 year ago

Thank You Pawel, however I think we would have a better options in temptable options where we can set somethink like that:

DoNotGenerateDefaultValues. then: if (IsIdentityColumn(property) && !options.DoNotGenerateDefaultValues) sb.Append(" IDENTITY");

etc,

I modified the code and I can create a fork and you can update back if you want, I hope did not mess anything

....

PawelGerr commented 1 year ago

Do you see any benefit in introducing the new option DoNotGenerateDefaultValues when KeepIdentity leads to same results?

safigi commented 1 year ago

Hi Pawel, yes I see. With default values we have the same problem. For example if we use like this:

create table #temp_Customer_1 (Id GUID default newsequentialid(), name varchar(1000)) or create table #temp_Customer_1 (Id GUID default newgid(), name varchar(1000)) or create table #temp_Customer_1 (Id GUID default (NEXT VALUE FOR mysequence), name varchar(1000)) we are facing to the same issue. (I haven't tried, just I see in the table creation script)

Anyway best EF library I have seen! Good work!

PawelGerr commented 1 year ago

Yes, it is a known limitation

safigi commented 1 year ago

Ok, I see, anyway I created a PR maybe it is good and fix this limitation on SQL server (I cannot guarantee if it work or not). If you want you can accept, or you can ignore it. Thank you,

Istvan