Index Safety for List Tranform Operator

[dwcullop]

Description

Adds the changes suggested by @RolandPheasant in #744 to avoid throwing an exception when Transform is used in conjunction with other operators such as (WhereReasonsAre*) that remove the Index from the Changes.

Transform will now check for missing indexes and do a lookup to find the correct index values to use for that situation.

Also adds two unit tests to the Fixture for List Transform. Before the other changes, one test would pass and the other would fail. With the changes, they both pass.

[codecov]

Codecov Report

Attention: 421 lines in your changes are missing coverage. Please review.

Comparison is base (53d5f6d) 64.74% compared to head (b8205ed) 65.82%.
Report is 6 commits behind head on main.

Files	Patch %	Lines
src/DynamicData/Cache/ObservableCacheEx.cs	53.48%	19 Missing and 1 partial ⚠️
src/DynamicData/List/ObservableListEx.cs	71.69%	13 Missing and 2 partials ⚠️
src/DynamicData/List/Internal/Transformer.cs	52.00%	9 Missing and 3 partials ⚠️
src/DynamicData/Aggregation/AvgEx.cs	73.68%	10 Missing ⚠️
src/DynamicData/Aggregation/SumEx.cs	74.35%	10 Missing ⚠️
src/DynamicData/ObservableChangeSet.cs	58.33%	10 Missing ⚠️
src/DynamicData/Platforms/net45/ParallelEx.cs	25.00%	7 Missing and 2 partials ⚠️
src/DynamicData/Aggregation/StdDevEx.cs	76.66%	5 Missing and 2 partials ⚠️
src/DynamicData/List/Internal/GroupOnProperty.cs	0.00%	7 Missing ⚠️
src/DynamicData/Aggregation/CountEx.cs	25.00%	6 Missing ⚠️
... and 127 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #762      +/-   ##
==========================================
+ Coverage   64.74%   65.82%   +1.08%     
==========================================
  Files         226      228       +2     
  Lines       11459    11175     -284     
  Branches     2334     2304      -30     
==========================================
- Hits         7419     7356      -63     
+ Misses       3083     2881     -202     
+ Partials      957      938      -19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[JakenVeina]

This feels kinda like a band-aid, to me. If there's operators that aren't preserving indexes that should be, we should fix them. If there's operators that aren't preserving indexes because they can't, then how is .Transform() able to reliably re-acquire that lost info?

I missed whatever conversation spawned this, what's the usage-scenario here?

[RolandPheasant]

This feels kinda like a band-aid, to me. If there's operators that aren't preserving indexes that should be, we should fix them. If there's operators that aren't preserving indexes because they can't, then how is .Transform() able to reliably re-acquire that lost info?

I missed whatever conversation spawned this, what's the usage-scenario here?

Any stateless operator, or any of the operators WhereReasonsAre* operators remove the index, or at least cannot guarantee index integrity. For example

myChangeSetObservable.WhereReasonsAreNot(Remove)

removes the remove events, and additionally does not maintain state. Therefore when a subsequent Add arrives, the index which the Add carries will be incorrect and could cause an index out of range exception. Thus, the WhereReasonsAre*` actually remove the index. It's a necessary evil which unfortunately have to be handled by other operators.

EDIT:

Looking at the current implementation, most of the other reasons handle index = -1. However, refresh also does not and should be fixed. Also, I have a thought of a special case which could be optimized: for the case, when it is only refresh which is removed i.e.

myChangeSetObservable.WhereReasonsAreNot(Refresh)

We could adjust the code to not remove the index. For context, I have used WhereReasonsAreNot(Refresh) and it's overload SuppressRefresh() quite a lot over the years.

@dwcullop would you be able to address this too? Or if you don't have the time we could to it later?

[JakenVeina]

Couldn't any of those filtering operators, in theory, track all the indices and issue Move or Update changes to compensate? Not that that's necessarily better.

Even so, I'm still not quite following how a downstream operator can recreate index information, if it was deemed impossible to determine by the upstream operator.

[dwcullop]

We could adjust the code to not remove the index. For context, I have used WhereReasonsAreNot(Refresh) and it's overload SuppressRefresh() quite a lot over the years.

@dwcullop would you be able to address this too? Or if you don't have the time we could to it later?

I will give it a shot. It doesn't seem like too much work. I'll get to it when I can.

[dwcullop]

I missed whatever conversation spawned this, what's the usage-scenario here?

When I was making the List version of MergeManyChangeSets, I combined the WhenReasonAreNot and the Transform operators which caused Transform to throw during a Replace test case because it assumed the indexes were valid and they weren't (because they had been removed).

If it happened to me, it could happen to anyone, and the library should not throw just by combining valid operators.

The other operators, including Transform, usually handle the case of the indexes being -1, but the Replace change reason was overlooked. This PR just closes that gap by doing a lookup to find the index when the one provided by the Change instance is invalid. I believe this is how all of the operators handle the same situation.

Even so, I'm still not quite following how a downstream operator can recreate index information, if it was deemed impossible to determine by the upstream operator.

I think the issue is reliability of the value. If the operator can't promise that the index will be valid to use later, then it removes it. This signals the downstream operator that it needs to determine the index for itself before performing the operation, which it can do by searching the collection for the value in question. This is not ideal, which is why the indexes are used in the first place, but it works when the indexes are not reliable.

Like I said, this is how most of the operators work, this is just closing a gap in the "Replace" scenario of the Transform operator, which doesn't have the same check as the others.

The important thing to note is that without the fix, the TransformOnReplace test passes, but the TransformOnReplaceWithoutIndex throws an exception (because it tries to use -1 indexes). With the fix, they both pass.

[RolandPheasant]

Alas, using SupressRefresh will not work. See my comment.

[RolandPheasant]

The Codecov things makes it super annoying to actually follow the changes.

[RolandPheasant]

The Codecov things makes it super annoying to actually follow the changes.

@glennawatson is there any way to make it less aggressive?. There's a huge number of false messages.

[RolandPheasant]

@ChrisPulman thanks for the commit. Much appreciated

[dwcullop]

@ChrisPulman thanks for the commit. Much appreciated

Yeah, that was a fast turn around.

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