2.x: add missing null checks on values returned by user functions #5379
Conversation
Codecov Report
@@ Coverage Diff @@
## 2.x #5379 +/- ##
============================================
- Coverage 96.17% 96.08% -0.09%
+ Complexity 5789 5778 -11
============================================
Files 630 630
Lines 41189 41195 +6
Branches 5726 5726
============================================
- Hits 39613 39582 -31
- Misses 624 639 +15
- Partials 952 974 +22
Continue to review full report at Codecov.
|
| @@ -319,7 +320,7 @@ public boolean test(Notification<Object> t) throws Exception { | |||
| @Override | |||
| public Observable<R> apply(T t) throws Exception { | |||
| return RxJavaPlugins.onAssembly(new SingleToObservable<R>( | |||
| ObjectHelper.requireNonNull(mapper.apply(t), "The mapper returned a null value"))); | |||
| ObjectHelper.requireNonNull(mapper.apply(t), "The mapper returned a null SingleSource"))); | |||
There was a problem hiding this comment.
Isn't it Observable / ObservableSource?
There was a problem hiding this comment.
Just open the collapsed section and see what mapper is: final Function<? super T, ? extends SingleSource<? extends R>> mapper;
| @@ -142,6 +142,7 @@ public void onNext(T t) { | |||
| } catch (Throwable ex) { | |||
| // Exceptions.throwIfFatal(e); TODO add fatal exceptions? | |||
| @@ -264,7 +264,7 @@ public void testFlatMapTransformsOnErrorFuncThrows() { | |||
|
|
|||
| source.flatMap(just(onNext), funcThrow((Throwable) null, onError), just0(onComplete)).subscribe(o); | |||
|
|
|||
| verify(o).onError(any(TestException.class)); | |||
| verify(o).onError(any(CompositeException.class)); | |||
There was a problem hiding this comment.
verify that the exceptions are in right order within the CompositeException?
There was a problem hiding this comment.
Tested separately in the FlowableMapNotificationTest.java additions.
| @@ -1341,6 +1341,7 @@ public void flatMapNotificationOnErrorNull() { | |||
| } | |||
|
|
|||
| @Test(expected = NullPointerException.class) | |||
| @Ignore("No longer crashes with NPE but signals it; tested elsewhere.") | |||
There was a problem hiding this comment.
Are there reasons to keep @Ignored tests in this PR?
There was a problem hiding this comment.
I had two options: I remove it and I get a question on why or ignore it with comments and get a question why...
There was a problem hiding this comment.
Haha :D
But now you have explanation in GitHub history so maybe remove them?
There was a problem hiding this comment.
First it has to get into the 2.x branch for that. Otherwise, there could be a purge of ignored unit test in the future.
There was a problem hiding this comment.
I remove it and I get a question on why or ignore it with comments and get a question why...
I like this way of thinking. :D
This PR adds null checks to places where it was missing.
In addition, the
XMapNotificationoperators now report a composite exception of the originalThrowableand the error thrown by the function that should return the continuation source.Also fixed
TestSubscriber/TestObservernot cancelling the sequence if the fused poll crashed.