Javadoc: explain that distinctUntilChanged requires non-mutating data to work as expected #6311
Conversation
…e in flow(Observable) Add few tests for scenarios related to mutable data type flow in distinctUntilChanged() methods for Observable
…e in flow(Flowable) Add few tests for scenarios related to mutable data type flow in distinctUntilChanged() methods for Flowable
Codecov Report
@@ Coverage Diff @@
## 2.x #6311 +/- ##
===========================================
- Coverage 98.23% 98.2% -0.03%
+ Complexity 6284 6283 -1
===========================================
Files 672 672
Lines 44992 44992
Branches 6223 6223
===========================================
- Hits 44196 44183 -13
- Misses 252 267 +15
+ Partials 544 542 -2
Continue to review full report at Codecov.
|
| @@ -8740,6 +8740,13 @@ public final Completable concatMapCompletableDelayError(Function<? super T, ? ex | |||
| * <p> | |||
| * Note that the operator always retains the latest item from upstream regardless of the comparison result | |||
| * and uses it in the next comparison with the next upstream item. | |||
| * <p> | |||
| * Note that if element {@code T} type in the flow is mutable, the comparison of the previous and current | |||
| @@ -141,6 +141,84 @@ public void testDistinctUntilChangedOfSourceWithExceptionsFromKeySelector() { | |||
| inOrder.verify(w, never()).onComplete(); | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
There is no reason to test this unwanted behavior.
There was a problem hiding this comment.
That's what I thought initially. Will remove all tests.
| * {@code CharSequence}s or {@code List}s where the objects will actually have the same | ||
| * references when they are modified and {@code distinctUntilChanged} will evaluate subsequent items as same. | ||
| * To avoid such situation, it is recommended that mutable data is converted to an immutable one, | ||
| * for example using `map(CharSequence::toString)` or `map(Collections::unmodifiableList)`. |
There was a problem hiding this comment.
Collections::unmodifiableList isn't the right example to use since its a view to the list it is referencing, so if a mutable list is passed then any changes would still reflect the List it returns. Should probably be replaced with ArrayList::new
This documentation is linked to by Collections::unmodifiableList:
Note that changes to the backing collection might still be possible, and if they occur, they are visible through the unmodifiable view. Thus, an unmodifiable view collection is not necessarily immutable. However, if the backing collection of an unmodifiable view is effectively immutable, or if the only reference to the backing collection is through an unmodifiable view, the view can be considered effectively immutable.
There was a problem hiding this comment.
The best would be ImmutableList::of but we can't depend on external collections libraries.
Perhaps we can have both by list -> Collections.unmodifiableList(new ArrayList<>(list)). This will make a copy of the list but also prevent it from being modified later in the pipeline. Unfortunately, we can't do much about the element mutability, which also affects equals.
There was a problem hiding this comment.
As discussed, I think we should change map(Collections::unmodifiableList) to map(list -> Collections.unmodifiableList(new ArrayList<>(list)). @akarnokd Let me know if we should go forward with that and I'll create a PR.
There was a problem hiding this comment.
Yes, please modify the text as my suggestion.
distinctUntilChanged()methods inFlowableandObservableclass explaining about unexpected results to expect when using mutable data sources like Mutable CharSequence or Lists.Resolves: #6290