android10 / frodo

Android Library for Logging RxJava Observables and Subscribers.
1.48k stars 101 forks source link

@RxLogSubscriber crashes with NullPointerException #18

Closed spirosoik closed 8 years ago

spirosoik commented 8 years ago

Fixed #14. The @RxLogSubscriber crashes with NullPointerException when the param values are empty eg. Observable.just(null)

spirosoik commented 8 years ago

@android10 please review. Any thoughts ideas for a better approach than this?

android10 commented 8 years ago

@spirosoik I will review it by the end of the week so we can generate another version asap :)

spirosoik commented 8 years ago

ok dude @android10

android10 commented 8 years ago

There are a few comments. We are more error prone with this PR but I still think that sending a null value in an observable it does not make too much sense. In my production code I would wrap my value in an Optional.class

android10 commented 8 years ago

By the way I added more samples to reproduce the problem, and by using Observable.just(null) works well:

D  Frodo => [@Observable#doNothing -> onSubscribe()]
D  Frodo => [@Observable#doNothing -> onCompleted()]
D  Frodo => [@Observable#doNothing -> onTerminate()]
D  Frodo => [@Observable#doNothing -> @Emitted -> 0 elements :: @Time -> 0 ms]
D  Frodo => [@Observable#doNothing ->  :: @ObserveOn -> main]
D  Frodo => [@Observable#doNothing -> onUnsubscribe()]                        

D  Frodo => [@Observable#doSomething -> onSubscribe()]
D  Frodo => [@Observable#doSomething -> onNext() -> null]
D  Frodo => [@Observable#doSomething -> onCompleted()]
D  Frodo => [@Observable#doSomething -> onTerminate()]
D  Frodo => [@Observable#doSomething -> @Emitted -> 1 element :: @Time -> 0 ms]
D  Frodo => [@Observable#doSomething -> @SubscribeOn -> RxNewThreadScheduler-5 :: @ObserveOn -> main]
D  Frodo => [@Observable#doSomething -> onUnsubscribe()]                        

D  Frodo => [@Observable#sendNull -> onSubscribe()]
D  Frodo => [@Observable#sendNull -> onNext() -> null]
D  Frodo => [@Observable#sendNull -> onCompleted()]
D  Frodo => [@Observable#sendNull -> onTerminate()]
D  Frodo => [@Observable#sendNull -> @Emitted -> 1 element :: @Time -> 0 ms]
D  Frodo => [@Observable#sendNull -> @SubscribeOn -> RxNewThreadScheduler-6 :: @ObserveOn -> main]
D  Frodo => [@Observable#sendNull -> onUnsubscribe()]

Here is the commit: https://github.com/android10/frodo/commit/e281739cea7577d8143858b371627252535f4808

spirosoik commented 8 years ago

@android10 Sometimes the crowd does not use Java8 so to be completely defensive we must be sure that if we got a null value the library will work without a problem.

Also, If you try my test case without the fixes you'll see that the error occurred.

I'll send an update @android10 for every of you comment that's why we have the code reviews :)

android10 commented 8 years ago

@spirosoik your argument makes sense. Let's be defensive here. Looking forward for the changes so I merge it :)

spirosoik commented 8 years ago

By the way I added more samples to reproduce the problem, and by using Observable.just(null) works well:

The error occurred. Check that it's going to onError because of the exception of the Null pointer.

spirosoik commented 8 years ago

@android10 review dude!

android10 commented 8 years ago

@spirosoik nice! Great job! I'm gonna merge this although I might refactor a little bit the usage of nulls in favor of optionals :)

spirosoik commented 8 years ago

:+1: thanks dude.