Serious GraphMerger bug with 8.2.7

[michaelcsikos]

It looked like the GraphMerger problems had been resolved, but since Friday I have been trying to work this one out. I have managed to strip down an example to just two tests, one which shows the expected result using just SaveAsync(), and the failure with SaveAndMergeAsync(). The only difference between the two tests is how the save is performed:

if (merge)
    await instance.SaveAndMergeAsync();
else
    instance = await instance.SaveAsync();

GraphMergerTest.zip

Open the Test Explorer and run WidgetTests. UpdateWithMergeWidgetTest will fail, and UpdateWithoutMergeWidgetTest should pass.

In the Standard Output, you'll see that one of the child items has been incorrectly merged in, there is a duplicate Guid, and one of them has been lost. It is usually the last item which is a duplicate of the second.

This is a show stopper for us currently. I could roll back to 6.2.2, but this would require a fair bit of reworking as the Blazor view model methods changed in 7.0.0. I would really appreciate someone looking into this.

[rockfordlhotka]

Adding a reference to the PR that (theoretically) fixed this issue:

#3481

[rockfordlhotka]

fyi, I'm in Prague this week speaking at a conference and otherwise on holiday, so it won't be until sometime next week when I am likely to get a chance to look into this.

@russblair might have an idea, as he dug into the #3481 issue in depth.

[russblair]

I will try to take a look at this later today or tomorrow.

[russblair]

As far as we can tell, the bug is not in the GraphMerge logic but is in the DataPortal logic. The bug is identified after the FetchAsync in the Fetch method on line 118 of the SimpleDataPortal.cs. The obj.Instance identity manager has the incorrect next identity value. We are unclear as to how this is happening or how to narrow it down further.

[michaelcsikos]

DoesSaveAndMergeAsync end up calling Fetch?

[rockfordlhotka]

It does not call Fetch. It calls Update, gets the result, and merges the result with the original graph.

[russblair]

We think we have a working solution. Check it out at https://github.com/russblair/csla/tree/RB/v7.0.3_IdentityManagerIssue%233581.

[russblair]

I did not raise a PR because we have added Michael's test code to the project and I wanted to make sure that was ok with both of you (Micael and Rocky) first.

[michaelcsikos]

That's fine. I have never written a test like this before and have been developing with CSLA for 16 years. I was pleased to discover it was reproducible in a test and not just in Blazor.

[rockfordlhotka]

More tests are good!!

[rockfordlhotka]

@russblair can you create a PR against main as well so this fix gets into CSLA 8 and future versions?

[russblair]

Yes. I can do that

[rockfordlhotka]

@michaelcsikos and @russblair I have pushed a prerelease of 7.0.3 to nuget, so if you can try that version to ensure it fixed the issue I think we'll be all set.

[michaelcsikos]

The 7.0.3 prerelease looks good.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

[michaelcsikos]

Yesterday, I ran into this problem again in CSLA 8.2.7. The difference is when the child collection is more deeply nested in the object graph.

I have updated my original GraphMergerTest to use

Root
  - Branch
    - Leafs
      - Leaf

See the attached GraphMergerRootBranchLeafTest solution. Open the Test Explorer and run the tests. UpdateWithMergeWidgetTest will fail, and UpdateWithoutMergeWidgetTest should pass.

Hopefully, this is a simple fix.

GraphMergerRootBranchLeafTest.zip

[michaelcsikos]

Fortunately, in our Blazor app we can use a workaround for now: VM.Model = VM.Model.SaveAsync() instead of VM.SaveAsync(). However, the user experience is not as pleasant as the UI resets, which is more noticeable with a tabbed interface.

[michaelcsikos]

@rockfordlhotka, @russblair Did you see I reopened this issue?

[russblair]

Unfortunately, I haven't had a chance to look at this yet. I have added this to my to do list.

[StefanOssendorf]

I'll work on this issue starting today

[rockfordlhotka]

@StefanOssendorf can you apply this to 8.2.x as well please?

[StefanOssendorf]

@StefanOssendorf can you apply this to 8.2.x as well please?

It's up :)