Feature: MergeManyChangeSets (New Operator for Cache ChangeSets)

[dwcullop]

Description

This PR addresses #457 by adding a set of observable Cache ChangeSet-Aware operators name MergeManyChangeSets. These operators work just like MergeMany except allows the result to be ChangeSets which are then merged together, as if they came from a single source cache. It correctly handles both Remove and Update events from the source observable by removing any items added via the changeset associated with the item being removed/updated. It also handles when the same key is emitted from multiple child changesets by allowing the consumer to provide an IComparer instance to determine priority.

It was developed by cloning the code for DynamicCombiner, but since it only needs to handle the "Or" case, it could then be optimized for the various operations (Add/Remove/Update).

Optional Parameters

Using the two overloads, the consumer can provide an IEqualityComparer, an IComparer, or both (or neither).

IEqualityComparer

The IEqualityComparer instance that is used to determine equality of items in the result changeset. An Update change will not be fired for an item if it is equal to the previously observed value for the same key.

IComparer

When provided, the IComparer instance will be used to determine priority when multiple upstream changesets produces values with the same key. If a child changeset emits a value with the same key as previously emitted by another child changeset, the downstream will observe an update if and only if the new value has a higher priority according to the comparer (otherwise, it won't observe anything). When a value is removed from a child changeset, if it is the value that has most recently been emitted downstream, an Update event will be emitted replacing the value with the NEXT highest priority item from all of the available values for that key. If there are no available values, a Remove event will be observed downstream instead.

The IComparer is entirely optional. If not supplied, then the first value to be observed will have priority. If that value is later removed, it will be replaced by the first available value. If the child changesets do not produce overlapping keys, there's no reason to supply this value at all.

Unit Tests

There is a suite of unit tests that are based around the contrived example of having different markets having different prices for the same items and the need to aggregate the prices from all the markets using different criteria (lowest price, highest price, etc).

Note: These overloads only work on Cache ChangeSets. Supporting List ChangeSets will require a different implementation and should be a different PR.

[codecov]

Codecov Report

Merging #736 (ece8a39) into main (0292f19) will increase coverage by 0.45%.
The diff coverage is 94.30%.

@@            Coverage Diff             @@
##             main     #736      +/-   ##
==========================================
+ Coverage   71.77%   72.22%   +0.45%     
==========================================
  Files         216      217       +1     
  Lines       10900    11057     +157     
==========================================
+ Hits         7823     7986     +163     
+ Misses       3077     3071       -6     

Files	Coverage Δ
src/DynamicData/Cache/Internal/MergeMany.cs	97.36% <100.00%> (+3.61%)	⬆️
src/DynamicData/Cache/ObservableCacheEx.cs	51.54% <100.00%> (+0.88%)	⬆️
...micData/Cache/Internal/MergeManyCacheChangeSets.cs	92.68% <92.68%> (ø)

... and 2 files with indirect coverage changes

[dwcullop]

I ran into an issue implementing this with the overloads suggested by @RolandPheasant in #457. The overload resolution works just as he said it would, but therein is the problem. There is at least one place in this codebase that uses MergeMany with ChangeSets but does not expect the ChangeSet-aware behavior. Specifically, the unit tests for TransformMany started to fail because it uses MergeMany with ChangeSets internally.

Initially, I was just going to update TransformMany so that it opts out of the new overload (by specifying the 3 type parameters explicitly because the new one uses 4). However, I started to wonder how much other existing code is out there, using this amazing library, that would also be broken after updating to a version that adds this new overload.

Instead, I removed the overloads that don't take an IEqualityComparer or IComparer, so that the new changeset aware behavior is opt-in. This isn't ideal either because it makes it harder to find and use this new behavior.

@RolandPheasant I'm not sure how you want to handle this. I'd really like to have the overload that doesn't require any extra parameters, so that it works just like regular MergeMany, but that could break people who have already worked around the old behavior.

Maybe we can document the addition and rev the version so that they don't just pick up a version that will break them by mistake? Or maybe we can use a slightly different name, like MergeManyChangeSets? I understand your resistance to adding more operators though.

[RolandPheasant]

Yikes, Ill get back to you tomorrow, but MergeManyChangeSets may be the way forward.

If that's the solution, it will be worth writing some anaylsers to advise when MergeMany is used instead of MergeManyChangeSets

[dwcullop]

If that's the solution, it will be worth writing some anaylsers to advise when MergeMany is used instead of MergeManyChangeSets

That's a great idea but not something I'm familiar with doing. It seems very complicated.

I should have realized there was going to be a larger problem when I had to add explicit type parameters to the internal call to MergeMany to avoid an infinite loop. 😜

As far as a new name, MergeChangeSets or ChangeSetMerge would also work, but I think the name MergeManyChangeSets most accurately describes the concept (it is kind of verbose though). Please let me know how you want to proceed, and I'll make the appropriate changes.

It does make sense to define a new operator here because what is actually required is a family of operators. This PR covers the Cache ChangeSet -> Cache ChangeSet case. But there are 3 others that should be considered at some point:

1. Cache ChangeSet ▶️ List ChangeSet
2. List ChangeSet ▶️ Cache ChangeSet
3. List ChangeSet ▶️ List ChangeSet

[RolandPheasant]

Lets commit to using MergeManyChangeSets, and we can worry about the other permutations which you mention above as a distinct task.

[RolandPheasant]

@dwcullop The good news is I am soon about to start a role where the organisation already uses Dynamic Data. This will mean I am closer to the code base and it will sure lead to me breathing some fresh life into this project, and I'll certainly be keen to fill any gaps within the operator base. This PR falls into that category, so thanks for the effort.

[dwcullop]

@dwcullop The good news is I am soon about to start a role where the organisation already uses Dynamic Data. This will mean I am closer to the code base and it will sure lead to me breathing some fresh life into this project, and I'll certainly be keen to fill any gaps within the operator base. This PR falls into that category, so thanks for the effort.

You're quite welcome. I'm happy to contribute to this project. It has saved me a lot of time and energy, so it feels right to give back. Congrats on the new role!

[dwcullop]

I've updated everything to use the name MergeManyChangeSets. There are 4 overloads in total. There are 2 of each (With and without keys in the selector function) for each of these:

1. Optional IEqualityComparer / IComparer (defaulting to null)
2. Required IComparer

That should allow consumers to use either, both, or neither of the comparers.

I also renamed the internal implementation file to specify "Cache" so that the "List" version can be made with minimal confusion.

I also realized that I had forgotten an important use-case (important for my project anyway): The Observable resulting from MergeManyChangeSets needs to complete if (and only if) the source ChangeSet AND all of the child ChangeSets complete (At that point, there can't be any more changes, so there is no reason to keep the Observable open).

When adding support for this, I was surprised to learn that MergeMany doesn't already work this way and cleans up the children streams immediately when the source stream completes. To work around this, I created a lightweight counter that keeps track of the number of child subscriptions and exposes an Observable that gets Concated to the source which completes when all the children have been cleaned up. This effectively blocks the resulting observable from completing while either the source stream or any of the child streams are active.

Originally, I was going to put it in MergeManyChangeSets but it really seems like it belongs one layer higher, so I put it in the MergeMany operator instead (I really don't think I'm the only one who assumes MergeMany keeps the child streams open).

I don't think it will break any existing users, but if you prefer, we can hide the new behavior behind a parameter (or go back to the original plan of putting it in MergeManyChangeSets). It just really seems like it belongs in MergeMany to me. All of the unit tests pass (including the new ones I added to verify the behavior).

Let me know what you think. We could also put the MergeMany change in a different PR to isolate it.

[dwcullop]

I think there are still some gaps with regard to expected behavior. This PR covers the case for transforming a changeset of Ts into multiple changesets of Us and merging them together. But what if the consumer just multiple changesets of Ts and want to have them merged as a single steam with the extra functionality (such a comparer to resolve conflicting keys in the merged stream)?

It would be nice to have something like:

IObservable<IChangeSet<T, TKey>> MergeChangeSets<T, TKey>(this IEnumerable<IObservable<IChangeSet<T, TKey>>> sources, ...);

Or:

IObservable<IChangeSet<T, TKey>> MergeChangeSets<T, TKey>(this IObservable<IChangeSet<T, TKey>> first, params IObservable<IChangeSet<T, TKey>>>[] others, ...);

I guess that is more like Merge than MergeMany, but the implementation is the same. I happen to use Merge with IChangeSet, but it only works because I specifically don't have any overlapping keys.

This PR relies on the source stream having a key that is used to index the source changesets. The above wouldn't have that, so I guess that it is more like the Source List to Source Cache case (except the list is static), so maybe we should wait for that. I guess I'll work on that next because we can get the above operators for free once we have something that does Source List to Source Cache changeset merges.

[RolandPheasant]

This is ace, thanks.

[RolandPheasant]

Regarding the additional MergeChangeSets overloads, it makes sense that these are required. I suggest this can be done as a distinct task. Otherwise this PR would become overblown.

[RolandPheasant]

I also realized that I had forgotten an important use-case (important for my project anyway): The Observable resulting from MergeManyChangeSets needs to complete if (and only if) the source ChangeSet AND all of the child ChangeSets complete (At that point, there can't be any more changes, so there is no reason to keep the Observable open).

I think you're right on this one. I cannot remember for the life of me why this is not the default case.

[github-actions]

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