Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update MongoDB driver to the 4.7.1 release #290

Closed
wants to merge 2 commits into from
Closed

Conversation

jyemin
Copy link
Contributor

@jyemin jyemin commented Aug 5, 2022

Fixes #289

@jyemin
Copy link
Contributor Author

jyemin commented Aug 8, 2022

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
Copy link
Contributor

tsegismont commented Aug 8, 2022 via email

@jyemin
Copy link
Contributor Author

jyemin commented Aug 8, 2022

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
Copy link
Contributor

@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:

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

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
Copy link
Contributor Author

jyemin commented Aug 8, 2022

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
Copy link
Contributor Author

jyemin commented Aug 8, 2022

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

@tsegismont
Copy link
Contributor

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
Copy link
Contributor

cc @jponge @vietj @pmlopes

@jyemin
Copy link
Contributor Author

jyemin commented Aug 8, 2022

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
Copy link
Contributor Author

jyemin commented Aug 18, 2022

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

@agafox
Copy link

agafox commented Sep 8, 2022

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

@vietj vietj added this to the 4.3.4 milestone Sep 9, 2022
@tsegismont
Copy link
Contributor

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.

@tsegismont tsegismont closed this Sep 29, 2022
@tsegismont tsegismont removed this from the 4.3.4 milestone Sep 29, 2022
@tsegismont tsegismont reopened this Sep 29, 2022
@tsegismont tsegismont closed this Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Update to 4.7 MongoDB driver
4 participants