Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BulkMerge/Update does not use primary key by default #536

Open
Ta1sty opened this issue Jul 12, 2023 · 5 comments
Open

BulkMerge/Update does not use primary key by default #536

Ta1sty opened this issue Jul 12, 2023 · 5 comments
Assignees

Comments

@Ta1sty
Copy link

Ta1sty commented Jul 12, 2023

Description

I have a table (Proceedings) where i want to update a value and for that i do a bulk merge/or bulkUpdate for that matter.
However when I execute the bulkMerge below it produces a query that uses the alternate key instead of defaulting to the primary key.

Here is the relevant config

builder.Entity<ProceedingEntity>(entity =>
{
  entity.HasKey(x => x.ProceedingId);
  entity.HasIndex(x => new { x.TenantID, x.TenantLocationID, x.ProceedingId }).IsUnique();
}

The executed statement:

await context.Proceedings.BulkMergeAsync(
   updated.Values.Select(x => new ProceedingEntity { ProceedingId = x.ProceedingId, CourtId = x.NewCourtId }),
   opt => opt.ColumnInputExpression = x => new { x.ProceedingId, x.CourtId },
   token);

The produced query

MERGE INTO [Proceedings]  AS DestinationTable
USING
(
SELECT TOP 100 PERCENT * FROM (SELECT @0_0 AS [ProceedingId], @0_1 AS [CourtId], @0_2 AS [TenantID], @0_3 AS [TenantLocationID], @0_4 AS ZZZ_Index) AS StagingTable ORDER BY ZZZ_Index
) AS StagingTable
ON DestinationTable.[ProceedingId] = StagingTable.[ProceedingId] AND DestinationTable.[TenantID] = StagingTable.[TenantID] AND DestinationTable.[TenantLocationID] = StagingTable.[TenantLocationID]
WHEN MATCHED   THEN
UPDATE
SET     [CourtId] = StagingTable.[CourtId]
WHEN NOT MATCHED THEN
INSERT ( [ProceedingId], [CourtId] )
VALUES ( [ProceedingId], [CourtId] )

If i configure the query with

 opt.ColumnPrimaryKeyExpression = x => new { x.ProceedingId };

The produces query works fine. What I dont get is why does it default to using the alternate key, even though I is not included in the ColumnInputExpression. If I do a BulkMerge it atleast fails because it tries to insert NULL into another column and does fail.

HOWEVER:
If BulkUpdate is used, then it never matches and fails silently, resulting in failing updates all over our codebase without producing an error.

I would like to know if this actually intended that the ColumnPrimaryKeyExpression defaults to anything else other than the configured PrimaryKey.

The same thing also happens if I use a unique index instead of an alternate key.

EF: 7.0.8
Z.EntityFramework.Extensions 7.22.3
Provider: SqlServer

@Ta1sty Ta1sty changed the title BulkMerge uses alternate instead of primary key BulkMerge uses alternate instead of primary key by default Jul 12, 2023
@Ta1sty Ta1sty changed the title BulkMerge uses alternate instead of primary key by default BulkMerge does not use primary key by default Jul 12, 2023
@Ta1sty Ta1sty changed the title BulkMerge does not use primary key by default BulkMerge/Update does not use primary key by default Jul 12, 2023
@JonathanMagnan JonathanMagnan self-assigned this Jul 12, 2023
@JonathanMagnan
Copy link
Member

JonathanMagnan commented Jul 12, 2023

Hello @Ta1sty ,

Our library should correctly use the Key and not the Index.

Here is an online example with your scenario: https://dotnetfiddle.net/vPWHbR

I believe we missed a piece of the puzzle that could explain this behavior.

Is it possible for you to reproduce the issue by using the Fiddle we provided or by creating a runnable project? It doesn’t need to be your project, just a new solution with the minimum code to reproduce the issue. You can send it in private here: info@zzzprojects.com

Best Regards,

Jon

@Ta1sty
Copy link
Author

Ta1sty commented Jul 12, 2023

I have reviewed your example and there are missing some things.
I have adjusted the fiddle to reproduce the error:
https://dotnetfiddle.net/6NWR1N

What was missing was another entity that refernces the alternate key I believe

@Ta1sty
Copy link
Author

Ta1sty commented Jul 12, 2023

@JonathanMagnan seems like .NetFiddle isnt working for me. Here is code to reproduce the issue:


public class Program
{
    public static void Main()
    {
        var courtId1 = Guid.NewGuid();
        var courtId2 = Guid.NewGuid();
        var proceedingId1 = Guid.NewGuid();
        var proceedingId2 = Guid.NewGuid();
        var proceedingId3 = Guid.NewGuid();
        // Create BD 
        using (var context = new EntityContext())
        {
            context.Database.EnsureCreated();
        }

        // SETUP
        using (var context = new EntityContext())
        {
            context.Add(new CourtEntity { CourtId = courtId1, Name = "1", });
            context.Add(new CourtEntity { CourtId = courtId2, Name = "2", });
            context.Add(new ProceedingEntity { ProceedingId = proceedingId1, TenantID = Guid.NewGuid(), TenantLocationID = Guid.NewGuid(), CourtId = null });
            context.Add(new ProceedingEntity { ProceedingId = proceedingId2, TenantID = Guid.NewGuid(), TenantLocationID = Guid.NewGuid(), CourtId = courtId2 });
            context.Add(new ProceedingEntity { ProceedingId = proceedingId3, TenantID = Guid.NewGuid(), TenantLocationID = Guid.NewGuid(), CourtId = null });
            context.SaveChanges();
        }

        // TEST  
        using (var context = new EntityContext())
        {
            List<ProceedingEntity> list = new List<ProceedingEntity>();
            list.Add(new ProceedingEntity { ProceedingId = proceedingId1, CourtId = courtId1 });
            // WORKS
            context.BulkMerge(list, options =>
            {
                options.ColumnPrimaryKeyExpression = x => new { x.ProceedingId }; // Explicitly use configured Primary key
                options.ColumnInputExpression = x => new
                {
                    x.ProceedingId, x.CourtId
                };
                options.UseLogDump = true;
                options.Log = (s) => Console.WriteLine(s);
            });

            // DOESNT WORK (does not throw!!)
            context.BulkUpdate(list, options =>
            {
                // the value for ColumnPrimaryKeyExpression defaults to x => new {x.ProceedingId, x.TenantID, x.TenantLocationID} instead of x => new {x.ProceedingId}
                options.ColumnInputExpression = x => new
                {
                    x.ProceedingId, x.CourtId
                };
                options.UseLogDump = true;
                options.Log = (s) => Console.WriteLine(s);
            });

            // DOESNT WORK (throws error)
            context.BulkMerge(list, options =>
            {
                // the value for ColumnPrimaryKeyExpression defaults to x => new {x.ProceedingId, x.TenantID, x.TenantLocationID} instead of x => new {x.ProceedingId}
                options.ColumnInputExpression = x => new
                {
                    x.ProceedingId, x.CourtId
                };
                options.UseLogDump = true;
                options.Log = (s) => Console.WriteLine(s);
            });
        }
    }

    public class EntityContext : DbContext
    {
        public EntityContext()
        {
        }

        public DbSet<ProceedingEntity> ProceedingEntities { get; set; }

        public DbSet<CourtEntity> CourtEntities { get; set; }

        public DbSet<SettingsEntity> SettingEntities { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            optionsBuilder.UseSqlServer(new SqlConnection(FiddleHelper.GetConnectionStringSqlServer()));
            base.OnConfiguring(optionsBuilder);
        }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            var proceeding = modelBuilder.Entity<ProceedingEntity>();
            proceeding.HasKey(x => x.ProceedingId);
            proceeding.HasIndex(x => new
            {
                x.TenantID, x.TenantLocationID, x.ProceedingId
            }).IsUnique();
            proceeding.HasOne(x => x.Court).WithMany(x => x.Proceedings).HasForeignKey(x => x.CourtId).OnDelete(DeleteBehavior.Restrict);
            modelBuilder.Entity<SettingsEntity>(settings =>
            {
                settings.HasKey(x => x.SettingId);
                settings.HasOne(x => x.Proceeding).WithMany().HasForeignKey(x => new
                {
                    TID = x.TenantId, TLID = x.TenantLocationId, PID = x.ProceedingId
                }).HasPrincipalKey(x => new
                {
                    TID = x.TenantID, TLID = x.TenantLocationID, PID = x.ProceedingId
                }).OnDelete(DeleteBehavior.Restrict);
            });
            var court = modelBuilder.Entity<CourtEntity>();
            court.HasKey(x => x.CourtId);
            base.OnModelCreating(modelBuilder);
        }
    }

    public class ProceedingEntity
    {
        public Guid ProceedingId { get; set; }

        public Guid TenantID { get; set; }

        public Guid TenantLocationID { get; set; }

        public Guid? CourtId { get; set; }

        public CourtEntity Court { get; set; }
    }

    public class CourtEntity
    {
        public Guid CourtId { get; set; }

        public List<ProceedingEntity> Proceedings { get; set; }

        public string Name { get; set; }
    }

    public class SettingsEntity
    {
        public Guid SettingId { get; set; }

        public Guid TenantId { get; set; }

        public Guid? TenantLocationId { get; set; }

        public Guid? ProceedingId { get; set; }

        public ProceedingEntity Proceeding { get; set; }
    }
}

@JonathanMagnan
Copy link
Member

Thank you @Ta1sty for the additional information.

That will surely help us to better understand the issue.

Best Regards,

Jon

@JonathanMagnan
Copy link
Member

Hello @Ta1sty ,

Just to give you an update:

This is definitely a mistake on our part and something we didn't expect. However, fixing it is currently impossible as too many major changes are required.

We first need to release our new IncludeGraph version (a full rewrite with performance enhancement), which we currently target in mid-September, and then we should be able to work on this issue.

So this is definitely something that we will try to fix in a few weeks.

Best Regards,

Jon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants