Repository navigation
Conversation
…rent When an Update changeset entry has Previous matching the predicate and Current not matching, FilterImmutable emits a Remove. Previously this Remove carried the new (non-matching) value as Current, violating the Change<T,K> contract that Remove.Current is the item being removed (the item that just left downstream). Since the new value never reached downstream, only the previous value can satisfy this contract. Consumers that read Current on Remove (e.g. composition with TransformImmutable, side-effect handlers like DisposeMany or OnItemRemoved equivalents) received the wrong reference, silently producing incorrect results or InvalidCastException.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a contract violation in the FilterImmutable cache operator where an Update that transitions from included → excluded was emitting a Remove change whose Current value incorrectly contained the new (excluded) item instead of the previous (included) item that actually left the downstream view.
Changes:
- Adjust
FilterImmutableto emitRemove.Current = change.Previous.ValueforUpdate -> Removetransitions. - Add a regression test validating the
Remove.Currentvalue for this scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/DynamicData/Cache/Internal/FilterImmutable.cs | Fixes Update -> Remove emission to carry the previously-included item in Remove.Current. |
| src/DynamicData.Tests/Cache/FilterImmutableFixture.cs | Adds a regression test asserting the corrected Remove.Current contract behavior. |
JakenVeina
requested changes
Jun 15, 2026
Per @JakenVeina's review on reactivemarbles#1113: swap .AsAggregator() for .ValidateChangeSets(Item.KeySelector).RecordCacheItems(out var results) across all existing FilterImmutable tests. ValidateChangeSets bakes in the changeset contract rules so each test transitively guards against shape violations such as a Remove change carrying a Current value that does not match what was originally added downstream. Kept the new Update_PreviousMatchedCurrentDoesNot test as a focused regression that asserts the specific Remove.Current contract.
JakenVeina
approved these changes
Jun 17, 2026
|
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1112.
Problem
FilterImmutableemitsChangeReason.Removecarrying the wrong value inCurrentwhen an Update changeset entry transitions from matching to not matching the predicate. The emitted Remove carries the new (non-matching)Currentinstead of the previous (matching) value that actually left downstream.This violates the
Change<T,K>contract thatRemove.Currentis the item being removed. Consumers readingCurrenton Remove receive the wrong reference, producing silent corruption (wrong side effects, incorrect projections) orInvalidCastExceptionwhen composed with type-narrowing operators.Fix
When emitting Update->Remove, carry
change.Previous.ValueasCurrent.Previousis dropped (Remove is not an Update).Test
Update_PreviousMatchedCurrentDoesNot_EmitsRemoveCarryingPreviousAsCurrentinFilterImmutableFixturecovers the contract. RED before the fix, GREEN after.Verification
FilterImmutableFixturepass.DynamicData.Tests.Cachepass.DeadlockTortureTest(TransformWithForce_DoesNotDeadlock,AllDangerous_Stacked_DoNotDeadlock) that reproduce on unmodifiedmain. Unrelated to this change.