Skip to content

Javadoc: explain that distinctUntilChanged requires non-mutating data to work as expected #6311

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 16, 2018
Merged

Javadoc: explain that distinctUntilChanged requires non-mutating data to work as expected #6311

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d67beee
Javadoc : Explain distinctUntilChanged requires non-mutating data typ…
punitda Nov 16, 2018
377a3c3
Javadoc : Explain distinctUntilChanged requires non-mutating data typ…
punitda Nov 16, 2018
42b56e4
Remove tests and fix typo in javadoc
punitda Nov 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions src/main/java/io/reactivex/Flowable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8740,6 +8740,13 @@ public final <K> Flowable<T> distinct(Function<? super T, K> keySelector,
* <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 type {@code T} in the flow is mutable, the comparison of the previous and current
* item may yield unexpected results if the items are mutated externally. Common cases are mutable
* {@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)`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please modify the text as my suggestion.

* <dl>
* <dt><b>Backpressure:</b></dt>
* <dd>The operator doesn't interfere with backpressure which is determined by the source {@code Publisher}'s
Expand Down Expand Up @@ -8776,6 +8783,13 @@ public final Flowable<T> distinctUntilChanged() {
* <p>
* Note that the operator always retains the latest key from upstream regardless of the comparison result
* and uses it in the next comparison with the next key derived from the next upstream item.
* <p>
* Note that if element type {@code T} in the flow is mutable, the comparison of the previous and current
* item may yield unexpected results if the items are mutated externally. Common cases are mutable
* {@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)`.
* <dl>
* <dt><b>Backpressure:</b></dt>
* <dd>The operator doesn't interfere with backpressure which is determined by the source {@code Publisher}'s
Expand Down Expand Up @@ -8808,6 +8822,13 @@ public final <K> Flowable<T> distinctUntilChanged(Function<? super T, K> keySele
* <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 type {@code T} in the flow is mutable, the comparison of the previous and current
* item may yield unexpected results if the items are mutated externally. Common cases are mutable
* {@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)`.
* <dl>
* <dt><b>Backpressure:</b></dt>
* <dd>The operator doesn't interfere with backpressure which is determined by the source {@code Publisher}'s
Expand Down
21 changes: 21 additions & 0 deletions src/main/java/io/reactivex/Observable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7824,6 +7824,13 @@ public final <K> Observable<T> distinct(Function<? super T, K> keySelector, Call
* <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 type {@code T} in the flow is mutable, the comparison of the previous and current
* item may yield unexpected results if the items are mutated externally. Common cases are mutable
* {@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)`.
* <dl>
* <dt><b>Scheduler:</b></dt>
* <dd>{@code distinctUntilChanged} does not operate by default on a particular {@link Scheduler}.</dd>
Expand Down Expand Up @@ -7856,6 +7863,13 @@ public final Observable<T> distinctUntilChanged() {
* <p>
* Note that the operator always retains the latest key from upstream regardless of the comparison result
* and uses it in the next comparison with the next key derived from the next upstream item.
* <p>
* Note that if element type {@code T} in the flow is mutable, the comparison of the previous and current
* item may yield unexpected results if the items are mutated externally. Common cases are mutable
* {@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)`.
* <dl>
* <dt><b>Scheduler:</b></dt>
* <dd>{@code distinctUntilChanged} does not operate by default on a particular {@link Scheduler}.</dd>
Expand Down Expand Up @@ -7884,6 +7898,13 @@ public final <K> Observable<T> distinctUntilChanged(Function<? super T, K> keySe
* <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 type {@code T} in the flow is mutable, the comparison of the previous and current
* item may yield unexpected results if the items are mutated externally. Common cases are mutable
* {@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)`.
* <dl>
* <dt><b>Scheduler:</b></dt>
* <dd>{@code distinctUntilChanged} does not operate by default on a particular {@link Scheduler}.</dd>
Expand Down