[JAVA-4322] Observable<T> toFuture() implicit returns a future with a null value Created: 30/Sep/21 Updated: 04/May/22 |
|
| Status: | Backlog |
| Project: | Java Driver |
| Component/s: | Scala |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Improvement | Priority: | Unknown |
| Reporter: | Daniel Connelly | Assignee: | Ross Lawley |
| Resolution: | Unresolved | Votes: | 1 |
| Labels: | external-user | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||
| Description |
|
Background While Integrating with the scala mongo driver we discovered https://github.com/mongodb/mongo-java-driver/blob/r4.3.3/driver-scala/src/main/scala/org/mongodb/scala/Observable.scala#L366-L376] the .head method hides a null underneath, as this is not documented nor is it defended against codebases can become vulnerable to null pointer exceptions or invalid responses in an API service, the fallback is to make use of .toFutureOption however as the risk isn't documented teams may only discover this when they hit it. If this is undefended against the code above continues to pass, Until someone tries to access something in the desired object. Solutions 1. ** Document the behaviour and offer more guidance in handling 2. Throw NoSuchElementException instead of null.asInstanceOf 3. Create a custom Exception to indicate what the null pointer means (the query returned nothing.)
|
| Comments |
| Comment by Ross Lawley [ 20/Oct/21 ] |
|
Opening and marking as a future improvement - when the whole API no longer uses Observable<Void>. |
| Comment by Daniel Connelly [ 19/Oct/21 ] |
|
Hi Ross,
I don't fully understand the need for null, when you could throw an exception here which could be transformed by other publishers and bubbled up when not transformed and cause errors in the code instead of null. |
| Comment by Ross Lawley [ 12/Oct/21 ] |
|
Hi daniel.connelly@hotmail.co.uk, Since 4.0, the Scala code now lives in the same repo as the Java driver and reactive streams driver, so that is why we now use the JAVA jira project. Ross |
| Comment by Daniel Connelly [ 12/Oct/21 ] |
|
I would like to update the ticket as well with the correct link to the null occurrence https://github.com/mongodb/mongo-scala-driver/blob/master/driver/src/main/scala/org/mongodb/scala/ObservableImplicits.scala#L347 |
| Comment by Daniel Connelly [ 12/Oct/21 ] |
|
Hi Ross, Thanks for looking at this, just to confirm. as This is now a JAVA ticket, is the JAVA code automatically transformed into scala code? Daniel |
| Comment by Ross Lawley [ 12/Oct/21 ] |
|
Hi daniel.connelly@hotmail.co.uk and sunny.chotai@digital.hmrc.gov.uk, Thanks for your inputs. I think that the issue is the difference in paradigms between reactive stream style observables (Publishers) and Futures. The Observable model has 0-n calls to onNext and then a final state - success or failure. Where as a Future only has success or failure states. The fact that an Observable completes without calling onNext is not an error state and that is what requires the use of null. I think this is more a side effect of the Observable[Void] API ( Ross |
| Comment by Sunny Chotai [ 01/Oct/21 ] |
|
When .toFuture does not complete with a value, Future.Failed may be more appropriate than Future.successful(null). Especially given that .toFutureOption gives us Future.successful(None). ie if you use .toFuture over .toFutureOption then the value not being present is unexpected so this may be better represented using an exception in a Future.Failed. |