search

vert-x3 / vertx-mongo-client

Mongo Client for Eclipse Vert.x
http://vertx.io
Apache License 2.0
58 stars 98 forks source link

Update MongoDB driver to the 4.7.1 release #290

Closed jyemin closed 1 year ago

jyemin commented 2 years ago

Fixes #289

jyemin commented 2 years ago

Something is not working right. I ran mvn test before submitting, but that skips most of the tests. What's the best way to debug this locally?

tsegismont commented 2 years ago

mvn verify should do the trick. I'm looking at the failures atm.

Le lun. 8 août 2022 à 13:35, Jeff Yemin @.***> a écrit :

Something is not working right. I ran mvn test before submitting, but that skips most of the tests. What's the best way to debug this locally?

— Reply to this email directly, view it on GitHub https://github.com/vert-x3/vertx-mongo-client/pull/290#issuecomment-1208011336, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALOLNVU6VOWIZM7YXWO43DVYDWHDANCNFSM55WXU4GQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

jyemin commented 2 years ago

It's because I don't have a valid Docker environment:

java.lang.IllegalStateException: Could not find a valid Docker environment. Please see logs and check configuration

    at org.testcontainers.dockerclient.DockerClientProviderStrategy.lambda$getFirstValidStrategy$4(DockerClientProviderStrategy.java:157)
    at java.util.Optional.orElseThrow(Optional.java:290)
    at org.testcontainers.dockerclient.DockerClientProviderStrategy.getFirstValidStrategy(DockerClientProviderStrategy.java:149)
    at org.testcontainers.DockerClientFactory.getOrInitializeStrategy(DockerClientFactory.java:147)
    at org.testcontainers.DockerClientFactory.client(DockerClientFactory.java:188)
    at org.testcontainers.DockerClientFactory$1.getDockerClient(DockerClientFactory.java:102)
    at com.github.dockerjava.api.DockerClientDelegate.authConfig(DockerClientDelegate.java:108)
    at org.testcontainers.containers.GenericContainer.start(GenericContainer.java:325)
    at io.vertx.ext.mongo.MongoTestBase.startMongo(MongoTestBase.java:73)
       ...

Test ignored.
tsegismont commented 2 years ago

@jyemin you can start an external mongo server (e.g docker run --rm --name vertx-mongo -p 27017:27017 mongo) and then force the client to use it with -Dconnection_string=mongodb://localhost:27017.

I found that with the new version of the driver, we can't go past the test setup phase because a codec for org.bson.codecs.DocumentCodec is missing.

Changing this:

https://github.com/vert-x3/vertx-mongo-client/blob/8207b561ad92910d3641795e2844c351dc6410ee/src/main/java/io/vertx/ext/mongo/impl/config/MongoClientOptionsParser.java#L22-L23

to:

  private final static CodecRegistry commonCodecRegistry = CodecRegistries.fromCodecs(new StringCodec(), new IntegerCodec(),
    new BooleanCodec(), new DoubleCodec(), new LongCodec(), new BsonDocumentCodec(), new DocumentCodec());

seems to do the trick. But I don't understand why it was not needed before and why it is now.

Then the test suite passes with the exception of two tests: DistinctTest.testDistinctBatchBadResultClass and DistinctTest.testDistinctBatchWithQueryBadResultClass. They fail because previously a problem with the codec was reported to the subscriber onError callback, whereas now an uncaught exception is thrown immediately on subscription when calling org.reactivestreams.Subscription#request.

I will take care of this second problem but would appreciate if you could shed some light on the first one.

jyemin commented 2 years ago

It looks like in scope of this change we inadvertently added a requirement to be able to decode to a Document. No one else has noticed because most applications require this anyway.

I think it's safe to make the change to the CodecRegistry that you suggested, but let me know if you disagree.

jyemin commented 2 years ago

Also note that as part of that change there is a new dependency on Project Reactor

tsegismont commented 2 years ago

Also note that as part of that change there is a new dependency on Project Reactor

Ok, I need to discuss this with the rest of the team. Thanks for the heads-up.

Do you know why a high-level RS library was required? Has Mutiny Zero been considered as an alternative?

tsegismont commented 2 years ago

cc @jponge @vietj @pmlopes

jyemin commented 2 years ago

We did not consider Mutiny Zero. Thanks for the heads up. I opened JAVA-4704 to track future work to consider it, before we push Project Reactor down into the core of the driver.

jyemin commented 2 years ago

@tsegismont what do you think? Can this be merged?

agafox commented 1 year ago

@tsegismont Can you please update us on this PR? Our team is interested as well and ready to help if anything needed

tsegismont commented 1 year ago

Closing, superseded by #292

@agafox this new version brings a dependency to Project Reactor and we are not sure yet about the consequences across the stack. The work continues in the new PR but it might not make it to 4.3.4. You could patch the Mongo Client temporarily with https://patch-diff.githubusercontent.com/raw/vert-x3/vertx-mongo-client/pull/292.patch until we sort things out.