-
Notifications
You must be signed in to change notification settings - Fork 201
OGM-1588: Upgrade MongoDB driver dependency to mongodb-driver-legacy 4.11.3 #1127
Conversation
Thanks for your pull request! This pull request does not follow the contribution rules. Could you have a look? ❌ All commit messages should start with a JIRA issue key matching pattern › This message was automatically generated. |
I see the CI job has been disabled as it requires some maintenance: Sorry, someone will have to look at that next week. |
…4.11.3 * Use mongodb-driver-legacy since mongo-java-driver uber jar is not published as of 4.0. * Replace find modifiers, which were removed in 4.0, with calls to `FindIterable` methods * Remove support for $maxScan, which is no longer supported by MongoDB * Remove support for $explain. The only way to explain with the 4.x driver is by calling `FindIterable#explain`. We could do that, and then use `SingleTupleIterator`, but is it worth it? Is explain only supported by OGM as a native query? * Handle breaking changes in WriteConcern class * Handle breaking changes related to MongoCredential
I've started to have a look at the project. It's been a while since I've touched it. In the meanwhile...
Building the main branch should work, but you need to use JDK 8 and run with the included
if you only want to test mongodb:
It will run the tests using embedded databases and the configured maven repositories (Central and JBoss). You can set the maven profile |
I've added a GitHub workflow, if you rebase to the latest changes, you should be able to test the PR. |
The README mentions which properties need to set to test with a remote mongodb: https://github.com/hibernate/hibernate-ogm/pulls?tab=readme-ov-file#mongodb |
I've just noticed that there are a couple of mistakes in the README about testing with an external MongoDB, I'm making some minor changes and updating the workflow to also test remote databases |
@jyemin I've updated the GitHub workflow and it's now testing using MongoDb 3.4. I've tried with 7 but some tests fail. Now you should be able to test the PR if you rebase to the latest changes. |
3.6.23 not 3.4: hibernate-ogm/.github/workflows/build.yml Line 89 in e836145
|
In the 4.0 Java driver, the default mode changed from STRICT to RELAXED for Document#toJson. Explicitly setting the mode to STRICT in tests so that JSON comparisons continue to work.
I'm able to test locally now, using Java 8. Thanks for your help on that. All tests are passing locally against MongoDB 3.6 standalone, but some are failing against MongoDB 7.0.
I think all these tests fail on the main branch as well, as well as quite a few more. |
Regarding this failure:
Can you enable Squash and Merge option on this repo? |
Done! |
Sorry, how can this be unrelated to the PR? Main branch with MongoDB 3.6 seems to work fine. |
If the tests are failing, I think we need to update the tests, or remove them (they might be obsolete). I haven't checked the tests, but it wouldn't surprise me if we are checking some MongoDB behaviour that has changed between 3 and 7. |
Ah, sorry. The test with the embedded MongoDB seems to work fine. What's failing are the integration tests with WildFly and the Infinispan server.
@Sanne Are feature packs still relevant? We used to test by building a JAR and deploying it on WildFly. We also have tests with the HotRod server. |
Yes, I suspect they are all due to changes between MongoDB 3.6 and MongoDB 7.0. For example, as of the 4.2 release MongoDB no longer supports the If we want to get those tests to stop failing, we could add some conditional skip logic in the tests that check the MongoDB version (We do that a lot in the MongoDB Java driver). But it requires a bit of infrastructure in the test harness to check the version, and I'm not sure where that should go in ogm. |
Hey there,
Agreed. Though if you're removing support for older MongoDB versions in this PR, then removing the feature in this PR makes sense.
Assuming you're going to support multiple versions of MongoDB, you're probably going to need multiple If you go down that road, you may be able to use |
I don't think Hibernate OGM is currently using |
By upgrading to the 4.11 driver, the minimum supported version of MongoDB is 3.6, and all tests pass against 3.6. I don't think we need to support anything older than 3.6 at this point. |
Alright then, we'll just need to mention the removed features in the migration guide, explaining the features are no longer supported in MongoDB 🤷 I see the migration guide was previously at https://developer.jboss.org/docs/DOC-52281 , which is now read only because the JBoss.org wiki has been decommissioned... So we should probably consider making the migration guide part of the documentation, like we did on Search for example: https://github.com/hibernate/hibernate-search/tree/main/documentation/src/main/asciidoc . We'll probably need another Jira/PR for that. You could maybe list the changes you're making in a (currently unrendered) asciidoc file in the meantime? |
The integration tests with WildFly are failing: https://github.com/hibernate/hibernate-ogm/actions/runs/10598877464/job/29372507837#step:7:3008 Do you know why? @yrodiere or @Sanne I think the purpose of the integration tests with WildFly is to check that the feature packs are working. Is this still relevant or can we just remove the |
I do see this:
If you can show me how to add an exclusion for that artifact, I can add it and see if that fixes things. |
The bson-record-codec module is not needed for hibernate-ogm, and needs to be excluded at least for tests because the class files in that module are compiled for Java 17 (the minimum LTS release required for record support).
I think I found it. Let me know if it looks right. |
After adding the bson-record-codec exclusion, the test got further but there are still failures. I opened #1130 just to compare the results here with the main branch. |
The integrations succeed on #1130. I'm not sure what is causing the failures on this branch. The errors looks like this:
but I don't see any more information than that. Note though that neo4j tests are also failing with the same error:
|
I will have a look |
@DavideD that seemed to do it. Thanks! Do you know why tests are passing on the main branch without this change? |
No, I couldn't figure it out. At some point I had an error related to a missing |
By the way, this seems only a problem related to the integration tests scaffolding, JUnit tests still work with JDK 8. In any case, all the Hibernate projects require at least JDK 11 nowdays, so I don't think it's a problem to upgrade it. |
Thanks @DavideD. Do you think we're good to (squash and) merge now? |
To clarify, if others are reading this, OGM should still work with JDK 8 after your change. We're building with JDK 11, but producing Java 8 bytecode.
Please see the check failure: https://github.com/hibernate/hibernate-ogm/pull/1127/checks?check_run_id=29474403593 and once that's fixed IMO we can merge -- don't forget about the documentation update (if you changed features) and migration guide though :) |
It also applies a different profile when using JDK 11, so that might also be the reason: Line 892 in e836145
|
Oh and I see you merged main into your branch at some point -- please rebase and force-push to get rid of that merge commit? As a rule we try to avoid merge commits in our repositories... |
Uff... It's actually skipping the integration test with JDK 11.. let me have another look at this... |
@yrodiere, I was ignoring the failing check because I was expecting to use squash and merge option. That will allow us to use a single commit message that includes the issue number in the first line, and it will also get rid of the merge commit.
Opened https://hibernate.atlassian.net/browse/OGM-1592 to track it. |
Ah, I got it. It's the feature pack for MongoDB that's not correct.... I should be able to fix it |
There are two issues:
Also, we couldn't see the errors because I made a mistake when I've created the workflow. I've fixed it with this commit: 058d812 And it's also not necessary to use JDK 11, so you can remove that change, please |
This reverts commit 7976688.
@DavideD do you want to commit DavideD@48bd837 to the main branch and I can merge it in to this branch? Or I can merge it myself from your fork. Whatever you prefer. |
It should really be part of this PR, but you can cherry pick it |
Wildfly 14 cannot create a mongo client with mongo client 4. Error: ``` ^[[0m^[[31m17:52:59,776 ERROR [org.jboss.as.controller.management-operation] (ServerService Thread Pool -- 67) WFLYCTL0013: Operation ("add") failed - address: ([ ("subsystem" => "mongodb"), ("mongo" => "default") ]): java.lang.RuntimeException: Could not get constructor for com.mongodb.MongoClient at org.wildfly.nosql.common.MethodHandleBuilder.declaredConstructor(MethodHandleBuilder.java:105) at org.wildfly.extension.nosql.driver.mongodb.MongoInteraction.<init>(MongoInteraction.java:90) at org.wildfly.extension.nosql.driver.mongodb.MongoClientConnectionsService.<init>(MongoClientConnectionsService.java:62) at org.wildfly.extension.nosql.subsystem.mongodb.MongoDefinition$ProfileAdd.startMongoDriverService(MongoDefinition.java:231) at org.wildfly.extension.nosql.subsystem.mongodb.MongoDefinition$ProfileAdd.performRuntime(MongoDefinition.java:226) at org.jboss.as.controller.AbstractAddStepHandler.performRuntime(AbstractAddStepHandler.java:338) at org.jboss.as.controller.AbstractAddStepHandler$1.execute(AbstractAddStepHandler.java:159) at org.jboss.as.controller.AbstractOperationContext.executeStep(AbstractOperationContext.java:999) at org.jboss.as.controller.AbstractOperationContext.processStages(AbstractOperationContext.java:743) at org.jboss.as.controller.AbstractOperationContext.executeOperation(AbstractOperationContext.java:467) at org.jboss.as.controller.ParallelBootOperationStepHandler$ParallelBootTask.run(ParallelBootOperationStepHandler.java:384) at org.jboss.threads.ContextClassLoaderSavingRunnable.run(ContextClassLoaderSavingRunnable.java:35) at org.jboss.threads.EnhancedQueueExecutor.safeRun(EnhancedQueueExecutor.java:1985) at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.doRunTask(EnhancedQueueExecutor.java:1487) at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1349) at java.lang.Thread.run(Thread.java:748) at org.jboss.threads.JBossThread.run(JBossThread.java:485) Caused by: java.lang.NoSuchMethodException: com.mongodb.MongoClient.<init>(java.util.List, java.util.List, com.mongodb.MongoClientOptions) at java.lang.Class.getConstructor0(Class.java:3082) at java.lang.Class.getDeclaredConstructor(Class.java:2178) at org.wildfly.nosql.common.MethodHandleBuilder.declaredConstructor(MethodHandleBuilder.java:102) ... 16 more ```
Done, but it didn't see to do the trick. |
You didn't apply the fix for the feature pack. Anyway, I've applied the fix to your PR, added the JIRA issue to the commits and rebased it. It seems to work fine now: #1134 I will have a look at the code after the week-end before merging it. Thanks |
Notable changes include:
FindIterable
methodsFindIterable#explain
. We could do that, and then useSingleTupleIterator
, but is it worth it? Is explain only supported by OGM as a native query?Issues: