Feature: List MergeManyChangeSets for Cache ChangeSets #744
Conversation
…com/dwcullop/DynamicData into feature/list-cache-merge-changesets
dwcullop
changed the title
|
@dwcullop I've work it out. If you change the following: var sourceListOfCaches = _source
.WhereReasonsAreNot(ListChangeReason.Moved, ListChangeReason.Refresh)
.Transform(obj => new ChangeSetCache<TDestination, TDestinationKey>(_changeSetSelector(obj)))
.Synchronize(locker)
.AsObservableList();to var sourceListOfCaches = _source
.Transform(obj => new ChangeSetCache<TDestination, TDestinationKey>(_changeSetSelector(obj)))
.Synchronize(locker)
.AsObservableList();The problem goes away. The reason is WhereReasonsAreNot removes the index from all changes because the index is unreliable when reasons are removed. I think I used to rely on that operator internally but due to such issues I stopped using it. That said, we should always mitigate against such things in the operators (in case someone is using WhereReasonsAreNot). So for safety the replace method in Transformer should become: case ListChangeReason.Replace:
{
var change = item.Item;
if (change.CurrentIndex == -1 || change.PreviousIndex == -1)
{
// Find the original, with it's corresponding index
var previous = transformed.First(x => x.Source.Equals(change.Previous.Value));
var index = transformed.IndexOf(previous);
if (index == -1)
{
throw new UnspecifiedIndexException($"Cannot find index of {change.Previous.Value}");
}
transformed[index] = _containerFactory(change.Current, previous.Destination, index);
}
else
{
Optional<TDestination> previous = transformed[change.PreviousIndex].Destination;
if (change.CurrentIndex == change.PreviousIndex)
{
transformed[change.CurrentIndex] = _containerFactory(change.Current, previous, change.CurrentIndex);
}
else
{
transformed.RemoveAt(change.PreviousIndex);
transformed.Insert(change.CurrentIndex, _containerFactory(change.Current, Optional<TDestination>.None, change.CurrentIndex));
}
}
break;
} |
Hurray! That resolved the issue! PR has been updated and I'll mark it as ready! |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #744 +/- ##
==========================================
+ Coverage 64.74% 65.85% +1.11%
==========================================
Files 226 228 +2
Lines 11459 11161 -298
Branches 2334 2299 -35
==========================================
- Hits 7419 7350 -69
+ Misses 3083 2877 -206
+ Partials 957 934 -23 ☔ View full report in Codecov by Sentry. |
| } | ||
|
|
||
| [Fact] | ||
| public void MergedObservableWillFailIfSourceFails() |
There was a problem hiding this comment.
Should we also test that errors from child streams propagate out?
There was a problem hiding this comment.
I decidedly did not want errors from child streams to tear down the whole thing. I could be convinced that is the wrong behavior, but my thinking was that an error on the child stream shouldn't disrupt all of the other child streams or the main stream.
| // then | ||
| receivedError.Should().Be(expectedError); | ||
| } | ||
|
|
There was a problem hiding this comment.
Following off of the bug I fixed yesterday, could/should we add a test to cover downstream-teardown (I.E. unsubscription) behavior? Internally, this means cleaning up all the outstanding child stream subscriptions, right?
There was a problem hiding this comment.
We probably should do. I wonder whether we should raise an issue and gradually trawl through the system.
There was a problem hiding this comment.
I wouldn't expect that there's a systemic issue with not handling downstream-teardown, that was just a defect I introduced a few days ago. But it is a behavior worth testing, wherever we have resources that need cleanup.
Perhaps instead what we need is a guidelines document for testing operators.
| // when a source item is removed, all of its sub-items need to be removed | ||
| var removedItems = shared | ||
| .OnItemRemoved(mc => changeTracker.RemoveItems(mc.Cache.KeyValues, observer)) | ||
| .Subscribe(); |
There was a problem hiding this comment.
.Subscribe() calls here should probably all be .SubscribeSafe() as its documentation recommends its usage within operators. This literally just try/catches the .Subscribe() call and feeds anything it catches to onError. This is important for operators, because it prevents things like .OnNext() or .OnCompleted() from throwing, when operators that orchestrate different streams together (like, say .Concat()) are involved.
I was also going to suggest passing observer.OnError to this subscription, but I'm less sold on that, after thinking it through. Since the source here is shared with .Publish() the only errors that can occur on this subscription (that aren't already going to be propagated from the "main" subscription) are from .OnItemRemoved(), which doesn't involve any user code. If there's a bug on our part, that's going to throw out of .Subscribe() or .OnNext() or .OnComplete() or whatever the user was doing that triggered the removal operation. But maybe that's actually preferable, since it's not the user's fault?
There was a problem hiding this comment.
Since it's a shared Observable, I don't think they all need to be SubscribeSafe. This one in particular is only used internally, so if it throws something is really wrong.
Maybe the one below should be SubscribeSafe so that the consumer can detect a bad subscription via their OnError. That one also passes in observer.OnError.
But I do like Roland's suggestion of uniformly addressing these as a single PR.
There was a problem hiding this comment.
Let's get this in, and @JakenVeina we should review places where we can improve usages in Subscription (safe or not) here and throughout the system in a subsequent PR
|
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. |
Description
This PR adds
MergeManyChangeSetsfor List ChangeSets where the child changeset is a Cache ChangeSet. In other words, it allows you to create a Cache ChangeSet from a child cache of items in a Source List.Functionality is identical to that in #743 except the input changeset type is list instead of cache.
Dependencies
What was once a single PR is now three. While the other two are independent, this PR requires them both to work (or else the tests won't pass and/or it might not build).