Expose `Observable[Unit]` instead of `Observable[Void]` #1282

stIncMale · 2023-12-15T20:09:09Z

This PR is a proof that even a single ape typing behind a keyboard can produce a PR that compiles. Vigilance from reviewers is especially needed here.

JAVA-4303

stIncMale · 2023-12-15T20:13:27Z

driver-scala/src/main/scala/org/mongodb/scala/Observable.scala

+  @deprecated(
+    "Is no longer needed because of the `ToSingleObservableUnit` implicit class. Scheduled for removal in a major release",
+    "5.0"
+  )
  def completeWithUnit(): SingleObservable[Unit] = UnitObservable(this)


Note that I propose to remove this method in the future.

When do you propose to remove it? Prior to 5.0, from the annotation, I gather?

5.0 is the version from which the method is deprecated. Once it's deprecated, we'll be able to remove it in the next major release, which is 6.0.

If we were to release a 4.x version with the deprecation, then we would have been able to delete the method in 5.0. But I am not sure we can do that because we can only deprecate completeWithUnit when there is an alternative (ToSingleObservableUnit), and adding that alternative breaks compatibility, which is why the deprecation has to happen in 5.0, and the deletion can only happen after that.

So should the "5.0" become "6.0" in the annotation?

I don't think so, because "5.0" is the value of the since attribute, not a version in which we promise to remove the method.

Well that makes a lot of sense :)

stIncMale · 2023-12-15T21:24:48Z

driver-scala/src/main/scala/org/mongodb/scala/ObservableImplicits.scala

+   *
+   * @param pub A `Publisher` representing a finite stream.
+   */
+  implicit class ToSingleObservableUnit(pub: => Publisher[Void]) extends SingleObservable[Unit] {


This class is why we are able to simply replace Observable[Void] with Observable[Unit] in the codebase.

implicits are magic and have been changed in Scala 3 in favour of extension methods (probably more akin to Kotlin extension methods).

But for now we are sticking with implicits, right?

stIncMale · 2023-12-15T21:25:18Z

driver-scala/src/main/scala/org/mongodb/scala/ObservableImplicits.scala

+   *
+   * @param pub A `Publisher` representing a finite stream.
+   */
+  implicit class ToGridFSUploadPublisherUnit(pub: => GridFSUploadPublisher[Void]) extends GridFSUploadPublisher[Unit] {


This class is why we are able to simply replace GridFSUploadPublisher[Void] with GridFSUploadPublisher[Unit] in the codebase.

This doesn't need to be a global implicit so can be added here instead: gridfs/package.scala

stIncMale · 2023-12-15T21:59:12Z

driver-scala/src/main/scala/org/mongodb/scala/ObservableImplicits.scala

@@ -116,17 +118,41 @@ trait ObservableImplicits {
    override def subscribe(observer: Observer[_ >: GridFSFile]): Unit = Mono.from(publisher).subscribe(observer)
  }

-  implicit class ToSingleObservableVoid(pub: => Publisher[Void]) extends SingleObservable[Void] {


ToSingleObservableVoid is part of public API (https://mongodb.github.io/mongo-java-driver/4.11/apidocs/mongo-scala-driver/org/mongodb/scala/ObservableImplicits$ToSingleObservableVoid.html), but my understanding is that

There was never a reason for any user to use it explicitly (though I found a single explicit import here https://github.com/hmrc/event-hub/blob/main/app/uk/gov/hmrc/eventhub/service/TransactionHandlerImpl.scala#L19).

The implicit conversion was convenient for the driver code, and is completely unneeded for any user of the Scala driver API, because it does not expose Publisher[Void]. I wonder if ToSingleObservableVoid is part of public API simply because there is no way to remove it from there, even though users don't need it.

For the aforementioned reasons, it seems to me that removing ToSingleObservableVoid and introducing ToSingleObservableUnit is fine (they can't coexist).

Yeap its an implicit class, so using it directly is strange. However, having it in scope makes it to be usable in customers code. Either way this is a breaking change but fixes a bug so makes sense.

JAVA-4303

rozza

One minor change - code move, one test suggestion then LGTM

rozza · 2024-01-03T16:05:44Z

driver-scala/src/main/scala/org/mongodb/scala/ObservableImplicits.scala

@@ -116,17 +118,41 @@ trait ObservableImplicits {
    override def subscribe(observer: Observer[_ >: GridFSFile]): Unit = Mono.from(publisher).subscribe(observer)
  }

-  implicit class ToSingleObservableVoid(pub: => Publisher[Void]) extends SingleObservable[Void] {


Yeap its an implicit class, so using it directly is strange. However, having it in scope makes it to be usable in customers code. Either way this is a breaking change but fixes a bug so makes sense.

rozza · 2024-01-03T16:10:51Z

driver-scala/src/main/scala/org/mongodb/scala/ObservableImplicits.scala

+   *
+   * @param pub A `Publisher` representing a finite stream.
+   */
+  implicit class ToGridFSUploadPublisherUnit(pub: => GridFSUploadPublisher[Void]) extends GridFSUploadPublisher[Unit] {


This doesn't need to be a global implicit so can be added here instead: gridfs/package.scala

rozza · 2024-01-03T16:11:48Z

driver-scala/src/main/scala/org/mongodb/scala/ObservableImplicits.scala

+   *
+   * @param pub A `Publisher` representing a finite stream.
+   */
+  implicit class ToSingleObservableUnit(pub: => Publisher[Void]) extends SingleObservable[Unit] {


implicits are magic and have been changed in Scala 3 in favour of extension methods (probably more akin to Kotlin extension methods).

rozza · 2024-01-03T16:25:13Z

driver-scala/src/test/scala/org/mongodb/scala/internal/UnitObservableSpec.scala

+
+import org.mongodb.scala.{ BaseSpec, Observer, Subscription }
+
+class UnitObservableSpec extends BaseSpec {


Lets add a chainability case to ensure this actually fixes the reported issue.

it should "work with for comprehensions" in { val h = for { _ <- UnitObservable(TestObservable(1 to 2)) _ <- UnitObservable(TestObservable(20 to 30)) } yield List(1, 2, 3) val results = ArrayBuffer[Int]() var completed = false h.subscribe((s: List[Int]) => results.addAll(s), (t: Throwable) => t, () => completed = true) results should equal(List(1, 2, 3)) completed should equal(true) }

Thank you, done.

… test proposed by Ross JAVA-4303

…d 2.12 JAVA-4303

jyemin

Added a comment to the thread re deprecations

rozza

LGTM

stIncMale requested a review from rozza December 15, 2023 20:09

stIncMale self-assigned this Dec 15, 2023

stIncMale force-pushed the JAVA-4303 branch from a02cca2 to 342b331 Compare December 15, 2023 20:10

stIncMale commented Dec 15, 2023

View reviewed changes

Expose Observable[Unit] instead of Observable[Void]

74f4fda

JAVA-4303

stIncMale force-pushed the JAVA-4303 branch from 342b331 to 74f4fda Compare December 15, 2023 22:03

rozza requested changes Jan 3, 2024

View reviewed changes

Move ToGridFSUploadPublisherUnit such that it's not global; add the…

2fba894

… test proposed by Ross JAVA-4303

stIncMale requested a review from rozza January 4, 2024 18:17

Update the test code such that it hopefully compiles in Scala 2.11 an…

f82d75b

…d 2.12 JAVA-4303

stIncMale requested a review from jyemin January 4, 2024 22:45

jyemin reviewed Jan 4, 2024

View reviewed changes

jyemin self-requested a review January 4, 2024 23:06

jyemin approved these changes Jan 4, 2024

View reviewed changes

rozza approved these changes Jan 5, 2024

View reviewed changes

stIncMale merged commit e78a2dc into mongodb:master Jan 5, 2024
60 checks passed

stIncMale deleted the JAVA-4303 branch January 5, 2024 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose `Observable[Unit]` instead of `Observable[Void]` #1282

Expose `Observable[Unit]` instead of `Observable[Void]` #1282

stIncMale commented Dec 15, 2023 •

edited

Loading

stIncMale Dec 15, 2023

jyemin Jan 4, 2024

stIncMale Jan 4, 2024

stIncMale Jan 4, 2024 •

edited

Loading

jyemin Jan 4, 2024

stIncMale Jan 4, 2024

jyemin Jan 4, 2024

stIncMale Dec 15, 2023

rozza Jan 3, 2024

stIncMale Jan 4, 2024

rozza Jan 5, 2024

stIncMale Dec 15, 2023

rozza Jan 3, 2024

stIncMale Jan 4, 2024

stIncMale Dec 15, 2023

rozza Jan 3, 2024

rozza left a comment •

edited

Loading

rozza Jan 3, 2024

rozza Jan 3, 2024

rozza Jan 3, 2024

rozza Jan 3, 2024

stIncMale Jan 4, 2024

jyemin left a comment

rozza left a comment


		import org.mongodb.scala.{ BaseSpec, Observer, Subscription }

		class UnitObservableSpec extends BaseSpec {

Expose Observable[Unit] instead of Observable[Void] #1282

Expose Observable[Unit] instead of Observable[Void] #1282

Conversation

stIncMale commented Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stIncMale Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rozza left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyemin left a comment

Choose a reason for hiding this comment

rozza left a comment

Choose a reason for hiding this comment

Expose `Observable[Unit]` instead of `Observable[Void]` #1282

Expose `Observable[Unit]` instead of `Observable[Void]` #1282

stIncMale commented Dec 15, 2023 •

edited

Loading

stIncMale Jan 4, 2024 •

edited

Loading

rozza left a comment •

edited

Loading