Inner join, updates to left key

[andersstorhaug]

From InnerJoin.cs, current head of main (follow link for more context):

DynamicData/src/DynamicData/Cache/Internal/InnerJoin.cs

Line 106 in b38c0d0

joinedCache.Remove((leftKey, change.Key));

It appears that for an inner join, if a right value updates its left key, then InnerJoin will fail to remove it from the results if it no longer joins.

That's because on update of right values, results are removed only based on the current value of the left key, not the previous. To fix this, on update, It would seem that both current/previous left keys need to be taken into account.

I haven't had the time to look into this further / confirm, but I wanted to raise an issue here before I lose sight of it.

[RolandPheasant]

Thanks for pointing this out. I will take a look soon (on vacation right now).

[glennawatson]

If you have time and want to help @RolandPheasant out, if you get time might be worth just providing a unit test to help out.

[andersstorhaug]

Will do, I believe that both the reproduction and the fix are fairly simple, so I may be able to get a PR out here within a couple of days.

[RolandPheasant]

@andersstorhaug Is this still and issue?

[jbooth88]

I came here to add a feature request for this topic, and found this thread.

As of version 8.3.27 I think this is still happening. I encountered my own issue where an inner join had strange side effects when a key on the right side is modified at runtime.

https://github.com/jbooth88/DynamicData-Filtering-Join

Above is a link to a sample project I posted in Slack for review, it illustrates the issue. Unfortunately I am not comfortable in making unit tests, so I cannot help you there. :(

[JakenVeina]

I can confirm that this is still an issue, with a very simple reproduction.

using var parentItems = new SourceCache<ParentItem, int>(static parent => parent.Id);
using var childItems = new SourceCache<ChildItem, int>(static child => child.Id);

using var subscription = ObservableCacheEx.InnerJoin(
    left:               parentItems.Connect(),
    right:              childItems.Connect()
        .AutoRefresh(static child => child.ParentId),
    rightKeySelector:   static child => child.ParentId,
    resultSelector:     static (parent, child) => new ParentChildPair()
    {
        Parent  = parent,
        Child   = child
    })
    .ValidateSynchronization()
    .ValidateChangeSets(static pair => (pair.Parent.Id, pair.Child.Id))
    .RecordCacheItems(out var results);

parentItems.AddOrUpdate(new[]
{
    new ParentItem() { Id = 1, Name = "Parent #1" },
    new ParentItem() { Id = 2, Name = "Parent #2" },
    new ParentItem() { Id = 3, Name = "Parent #3" }
});

childItems.AddOrUpdate(new[]
{
    new ChildItem() { Id = 1, ParentId = 1, Name = "Child #1" },
    new ChildItem() { Id = 2, ParentId = 1, Name = "Child #2" },
    new ChildItem() { Id = 3, ParentId = 2, Name = "Child #3" }
});

childItems.Lookup(2).Value.ParentId = 2;

Here, we setup "Child #2" to belong to "Parent #1", and then switch it to "Parent #2". We can see that the end result is that the "Parent #1"/"Child #2" remains in the result after we change the ParentId for "Child #2", and send a corresponding Refresh change into the operator. As @andersstorhaug identified in the code, the operator isn't even attempting to re-evaluate the join upon a Refresh, it just naively forwards it downstream.

I dug further to see if this re-joining works properly if we update a foreign key by replacing the whole object at the source, instead of just updating it inline, and that scenario's even WORSE. It adds a new pairing for "Parent #2/Child#2", but leaves in the original pairing of "Parent #1"/Child #2" so now "Child #2" is duplicated downstream.

To fix both of these issues, we're probably going to have to add specific logic to track the foreign key values, so we can detect when they change, across both Update and Refresh changes. Or we can just bite the bullet and re-write and re-optimize the operator, as we've done with others.

@RolandPheasant @dwcullop Any objections to a re-write, if there's no straightforward fix?

Edit: Nevermind, it was pretty trivial.

[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.