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(Async) with IncludeGraphOperationBuilder IgnoreOnMergeUpdate not being honored #797

Open
tsanton opened this issue Apr 25, 2024 · 8 comments
Assignees

Comments

@tsanton
Copy link

tsanton commented Apr 25, 2024

1. Description

I'm having some issues with the IncludeGraphOperationBuilder.
I have a parent with three children, where I want to upsert the parent, upsert child1, insert child2 and conditionally insert child3.

In short my merge config looks as such:

options.IncludeGraphOperationBuilder = bulk =>
    {
        switch (bulk)
        {
            case BulkOperation<ZEntity> z:
                z.InsertIfNotExists = true;
                z.IgnoreOnMergeUpdateExpression = c => new { c.Tid, c.Fid, c.Id, c.K };
                z.MergeMatchedAndOneNotConditionExpression = c => new { c.e, c.f };
                break;
            case BulkOperation<UEntity> u:
                u.InsertIfNotExists = true;
                u.IgnoreOnMergeUpdateExpression = c => new {c.Id, c.UAT, c.UAS, c.UT };
                u.MergeMatchedAndOneNotConditionExpression = c => new { c.R, c.RB };
                break;
            case BulkOperation<UAEntity> ua:
                ua.IgnoreOnMergeUpdate = true;
                ua.InsertIfNotExists = true;
                break;
            case BulkOperation<USEntity> us:
                us.IgnoreOnMergeUpdate = true;
                us.InsertIfNotExists = true;
                us.ColumnPrimaryKeyExpression = c => new { c.Tid, c.Id, c.S };
                break;
            default: throw new NotImplementedException()
        }
    };

Here is what I want to achieve:

  • z (parent): partial update or full insert
  • u (child): partial update or full insert
  • ua (child of u): full insert (primary key will always be unique), no update.
  • us (child of u): full insert when PK-expression not matched, no update.

As of now the IgnoreOnMergeUpdate is not being honored. I still see the UPDATE statements generated for UA and US. I know I can achieve the desired outcome by just IgnoreOnMergeUpdateExpression all properties in the entity, but I am somewhat hoping for an easier fix by simply ignoring the update (making it a flat conditional insert) for the sake of performance.

Haven't tested the other way around, but I would hope InsertIfNotExists = false would remove the insert part in cases where you only want updates but not inserts in a somewhat complex setup.

Further technical details

  • EF version: [EF Core v8.0.2]
  • EF Plus version: [EF Plus Core v8.102.2.3]
  • Database Server version: [PG 16.1]
  • Database Provider version (NuGet): [Npssql 8.0.2]
@JonathanMagnan JonathanMagnan self-assigned this Apr 25, 2024
@JonathanMagnan
Copy link
Member

Hello @tsanton ,

Thank you for reporting. My developer will look into it tomorrow morning.

Best Regards,

Jon

@JonathanMagnan
Copy link
Member

Hello @tsanton ,

Thank you again for reporting.

We currently have the right behavior for SQL Server but indeed as you reported, it looks like we have a bug with PostgreSQL.

We will look more and try to fix it.

Best Regards,

Jon

@tsanton
Copy link
Author

tsanton commented Apr 26, 2024

Many thanks @JonathanMagnan! Hope to hear back from you here when the issue is closed or a resolution timeline exist :)

Have a great weekend when that time comes; speak soon!

/T

@JonathanMagnan
Copy link
Member

Hello @tsanton ,

A new version has been deployed today.

Could you try it and confirm us that the issue has been fixed correctly?

Best Regards,

Jon

@tsanton
Copy link
Author

tsanton commented Apr 30, 2024

Hi @JonathanMagnan!

Just tested it and I can confirm that the {opt => opt.IgnoreOnMergeUpdate = true; opt.InsertIfNotExists = true} generates only an insert statement. This is according to expectations and solves my "performance" issue. Many thanks!

Just for giggles I tried the inverse: {opt => opt.IgnoreOnMergeUpdate = false; opt.InsertIfNotExists = false}. In this case I expect only the update statement to be generated. From what I'm seeing this is not the case. It seems to be that opt.InsertIfNotExists = false is not being honored because I'm stilling seeing both an update and an insert for my entity.

My issue is solved, but I don't think that the config is behaving exactly as expected.

I still would like to say many thanks for the expedience you show in regard to replying, following up, solving issues!

/T

@JonathanMagnan
Copy link
Member

Hello @tsanton ,

My bad, I forgot to tell you that the option InsertIfNotExists only works with BulkInsert

So, from what I understand, you would like an option IgnoreOnMergeInsert that behaves like the IgnoreOnMergeUpdate but for the INSERT statement.

I will ask my developer to look at it. Unfortunately, it will not be available before the end of May as we are currently pausing release for 3 weeks during my vacation.

Best Regards,

Jon

@tsanton
Copy link
Author

tsanton commented May 1, 2024

@JonathanMagnan I see, copy that!

As of right now this isn't a blocker/issue as UpdateNotInsert is a not a part of my current use pattern.
On the other hand, I can imagine scenarios where one would like only to update and not to insert (in the context of a "complex" merge with multiple child entities). If I am free to chose I would vote "do it".

I do think this leans towards the "nice to have" side, rather than "must have". In my opinion it would enhance the package simply because it turns Merge into a powerhouse, capable of any constellation of UpdateInsert(Delete?) for any and all entities. In combination with transactions it certainly lets me execute complex operations with high fidelity.

What I would suggest, in combination with this implementation, is to enhance the Merge documentation with examples where a complex Insert/Update case much like my scenario above with multiple child entities is well described with config in code and behaviour output as SQL. The functionality of your Merge is second to none, but I do think it's difficult for people without "hardcore" SQL-backgrounds to fully grasp how powerful, functional and flexible the method is.

I'll leave you guys be for the next three weeks then and get back to bothering you come June :)

Enjoy your vacation!

/T

@JonathanMagnan
Copy link
Member

I do think this leans towards the "nice to have" side, rather than "must have"

I 100% agree with you. That's something I will make sure it happens. I do not believe it will be really hard on our side but surely ask us a few changes.

Enjoy your vacation!

Thank ;) I will sure do!!!

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