Integrate mongodb-crypt module #1487

vbabanin · 2024-08-27T05:05:48Z

Description

This PR relocates the mongocrypt Java bindings from the libmongocrypt repository to the mongo-java-driver repository to streamline maintenance and the release process.

Key changes to the Gradle build script, originally copied from the libmongocrypt repository, include:

Removal of Maven publication configuration; now managed by the publish.gradle script.
Removal of signing configurations; managed by the the publish.gradle script.
Removal of explicit Slf4j dependency, as it is inherited from the root build script located here.

Review Process

To aid the review process, I have taken several steps:

Downloaded the mongocrypt version 1.11 from Maven Central and unzipped the source, Javadoc, runtime jar, and pom into the git folder.
Run a Maven local publication specifically for the mongocrypt module using the command gradle mongocrypt:clean mongocrypt:publishToMavenLocal -x signMavenJavaPublication to produce a publication. Subsequently, I unzipped and applied a git diff over the ones from version 1.11. The diff can be viewed here.

Notable differences include many Javadoc variations due to the mongocrypt module inheriting configurations from javadoc.gradle. Key areas to focus on are the pom and manifest files, along with some source files modified by the Spotless plugin, which corrected redundant imports.

I have built the jar files using JDK 11.0.24 (Homebrew), which is the same version used to build 1.11 currently on Maven Central.

Note for Reviewers:

I would appreciate it if reviewers could be particularly vigilant when reviewing the build configurations and produced artifacts in this PR. If you have any doubts or cannot be certain that the changes do not impact previously guaranteed functionalities, please do not hesitate to raise your concerns.

NOTE: Publishing snapshot to Sonatype has not been tested yet.

JAVA-5582
JAVA-5583

… project. JAVA-5582

JAVA-5582

NathanQingyangXu · 2024-08-27T12:14:37Z

mongocrypt/.gitignore

+
+# local settings
+**/gradle.properties
+local.properties


This is super minor, just for fun of tech discussion. In .gitignore, "filrname" means the same as "**/filename" but the latter takes effect only after git v1.8.2

The only reason to use "**" is for such example: a/**/b

No need to change if it has been a pattern elsewhere already. Feel free to ignore, without saying.

Thanks for the insightful comment! We currently lack consistency in the root .gitignore. We could consider addressing this if it becomes a source of confusion.

As for now, I've removed the .gitignore from this module since we don’t need an additional one; there’s already a .gitignore in the root folder of our repo.

JAVA-5582

vbabanin · 2024-08-28T05:44:29Z

mongocrypt/build.gradle.kts

+}
+
+dependencies {
+    api(project(path = ":bson", configuration = "default"))


The latest version of BSON is now used, as opposed to the version specified in the libmongocrypt repository. For reference, see the previous dependency listed here: libmongocrypt build.gradle.kts.

JAVA-5582

- Remove redundant git function from build script. - Integrate MongoCrypt benchmarks. JAVA-5582

JAVA-5582

vbabanin · 2024-09-10T05:45:04Z

mongocrypt/build.gradle.kts

+                "-exportcontents" to "com.mongodb.crypt.capi.*;-noimport:=true",
+                "Automatic-Module-Name" to "com.mongodb.crypt.capi",
+                "Import-Package" to "org.slf4j.*;resolution:=optional,org.bson.*",
+                "Bundle-Name" to "MongoCrypt",
+                "Bundle-SymbolicName" to "com.mongodb.crypt.capi",
+                "Private-Package" to ""


Manifest configuration is preserved as it is from libmongocrypt, except for:

"Import-Package" - now uses a wildcard "org.slf4j.*;resolution:=optional,org.bson.*",

"Build-Version" to gitVersion- is now handled by publish.gradle#L67.

"Bundle-Version" to gitVersion - is now handled by publish.gradle#L68.

JAVA-5582

vbabanin · 2024-09-11T19:06:39Z

mongocrypt/build.gradle.kts

+tasks.register<Download>("downloadJava") {
+    src(downloadUrl)
+    dest("${jnaDownloadsDir}/$localBinariesArchiveName")
+    overwrite(true)
+    /* To make sure we don't download archive with binaries if it hasn't been changed in S3 bucket since last download.*/
+    onlyIfModified(true)
+}
+
+tasks.register<Copy>("unzipJava") {
+    /*
+        Clean up the directory first if the task is not UP-TO-DATE.
+        This can happen if the download revision has been changed and the archive is downloaded again.
+     */
+    doFirst {
+        println("Cleaning up $jnaResourcesDir")
+        delete(fileTree(jnaResourcesDir).matching {
+            include(jnaMapping.keys.flatMap {
+                listOf(
+                    "${it}/nocrypto/**/libmongocrypt.so",
+                    "${it}/lib/**/libmongocrypt.dylib",
+                    "${it}/bin/**/mongocrypt.dll"
+                )
+            })
+        })
+    }
+    from(tarTree(resources.gzip("${jnaDownloadsDir}/$localBinariesArchiveName")))
+    include(jnaMapping.keys.flatMap {
+        listOf("${it}/nocrypto/**/libmongocrypt.so", "${it}/lib/**/libmongocrypt.dylib", "${it}/bin/**/mongocrypt.dll" )
+    })
+    eachFile {
+        path = "${jnaMapping[path.substringBefore("/")]}/${name}"
+    }
+    into(jnaResourcesDir)
+    mustRunAfter("downloadJava")
+
+    doLast {
+        println("jna.library.path contents: \n  ${fileTree(jnaResourcesDir).files.joinToString(",\n  ")}")
+    }
+}


Gradle tests whether task inputs or outputs have changed since the last build as part of the incremental build process. If no changes are detected, it considers the task up-to-date and skips execution.

There are two time-consuming tasks that now leverage incremental builds:

downloadJava: Downloads libmongocrypt binaries from S3. Optimized with the onlyIfModified(true) option, this task avoids redundant downloads by fetching binaries only if they are absent from the build output. The localBinariesArchiveName variable in the ./build folder, containing the zip archive file name with the revision number, ensures that binaries are downloaded just once per revision/tag unless a gradle clean command explicitly clears the build folder.

unzipJava: Unzips binaries into the temporary resource folder. Previously, this task was configured with outputs.upToDateWhen { false } to always execute. Now, it triggers only under these conditions:

The input file name changes, detected from the downloadRevision variable, prompting the unzipping of the latest or previously saved binaries.

Any alteration or corruption in the output file structure within jnaResourcesDir. To ensure correct unzipping, it first cleans jnaResourcesDir and then proceeds to unzip the binaries.

With mongocrypt as a project dependency of driver-core, the incremental build process conserves time when running or testing code unrelated to the mongocrypt module in sync/async modules. It avoids unnecessary rebuilding of the mongocrypt jar for the driver-core module. This is also useful during active development as libmongocrypt commits may update frequently.

vbabanin · 2024-09-11T19:14:06Z

driver-benchmarks/src/main/com/mongodb/benchmark/framework/MongoCryptBenchmarkRunner.java

Please note, the benchmark runner is directly copied from mongocrypt with minor adjustments (added MongoCryptBenchmarkResult). It has not been fully refactored to better align with the benchmark framework; we may consider refactoring in a separate scope. Functionally, it executes the tests, which can be verified by running performance tests on this build and checking the trends graph.

JAVA-5582

…to be used by consumers, to retrieve all the elements necessary to run against this library immitating trasative dependency resolution. JAVA-5582

jyemin

Note that static checks are failing:

The class name com.mongodb.crypt.capi.CAPI$cstring doesn't start with an upper case letter

so we'll have to suppress that. This is actually a spotbugs error not checkstyle, which surprises me.

jyemin · 2024-09-17T12:13:00Z

mongocrypt/build.gradle.kts

+
+    // Tests
+    testImplementation("org.junit.jupiter:junit-jupiter")
+    testRuntimeOnly("ch.qos.logback:logback-classic:1.2.13")


Can this use $logbackVersion?

This line can be removed as its set in the main build.gradle

mongocrypt/build.gradle.kts

rozza

LGTM - fixed a couple of nits, I had discovered doing a diff between the jar contents.

Also, fixed the spotbugs issue.

Only thing that could be improved is the javadoc warnings for the CAPI file.

mongocrypt/build.gradle.kts

JAVA-5582

jyemin

LGTM!

vbabanin · 2024-09-19T06:53:02Z

I'm holding off on merging this as I noticed an issue after running the publish snapshot. The driver-core POM in the snapshot no longer marks mongodb-crypt as an optional dependency, despite it being specified as such in the build configuration: implementation project(path: ':mongocrypt', configuration: 'default'), optional. The setting isn’t being applied correctly. Here's the POM for reference.

rozza · 2024-09-19T16:00:38Z

Hoping to have fixed the 'optional' support - will review the build in the morning

jyemin · 2024-09-19T21:53:35Z

@rozza do you expect/desire that the optional mongodb-crypt dependency appears in driver-core, driver-sync, and driver-reactive-streams? Currently it's only in driver-core (even though driver-core doesn't actually use it!).

…actId, ensuring that the 'optional' plugin functions correctly. JAVA-5582

jyemin · 2024-09-19T22:12:32Z

I think the latest changes might have broken the build. I see a lot of

FAILURE: java.lang.NoClassDefFoundError: com/mongodb/crypt/capi/MongoCryptOptions (java.lang.NoClassDefFoundError)

vbabanin · 2024-09-20T07:06:58Z

Thanks, @rozza for your help on this issue!

I found the root cause of the issue with the existing nebula.optional-base plugin for optional dependencies. The mismatch between the mongocrypt submodule name and the artifactId mongodb-crypt used during publishing caused the plugin to skip appending the optional flag as detailed in this plugin code.

To address this, I renamed the module name to mongodb-crypt while keeping the directory named as mongocrypt. All tests (full set) now pass, and the driver-core POM now correctly includes the optional flag for mongodb-crypt. The dependency is only specified in driver-core as it was before the mongocrypt migration.

I've pushed these changes to a separate branch to avoid overriding current changes and I've run the full set of Evergreen tests, and all tests have passed successfully. You can review the results here.

The commit with the fix is here: 6e3e7068f1b0f2648a2fa0610e2c13a3f1dd2e3f

@jyemin @rozza please review it, and if everything looks good, @rozza feel free to cherry-pick it into this PR.

… issue

rozza · 2024-09-20T09:10:05Z

@vbabanin your change looks good, great catch with the naming. I'm thinking renaming the directory to match the archive name also makes sense. Then we don't have to worry about changing the projects name in settings.gradle

@rozza do you expect/desire that the optional mongodb-crypt dependency appears in driver-core, driver-sync, and driver-reactive-streams? Currently it's only in driver-core (even though driver-core doesn't actually use it!).

I was trying to workaround the feature support limitations but seems it failed anyway. driver-core has to take a dependency as some of the options classes shared between sync and reactivestreams use the library. Thankfully, @vbabanin discovered the cause of the plugin failing 👍

evergreen-ci-prod · 2024-09-20T11:20:26Z

There is an existing patch(es) for this commit SHA:

Evergreen patch with PR

Please note that the status that is posted is not in the context of this PR but rather the (latest) existing patch and that may affect some tests that may depend on the particular PR. If your tests do not rely on any PR-specific values (like base or head branch name) then your tests will report the same status. If you would like a patch to run in the context of this PR and abort the other(s), comment 'evergreen retry'.

jyemin

LGTM again!

jyemin · 2024-09-20T13:15:22Z

evergreen retry

evergreen-ci-prod · 2024-09-20T13:15:25Z

There is an existing patch(es) for this commit SHA that will be aborted:

Evergreen patch with PR

The status reported will be corresponding to this PR rather than the previous existing ones. If you would like a patch to run for another PR and to abort this one, comment 'evergreen retry' on the corresponding PR.

vbabanin added 2 commits August 26, 2024 22:02

Integrate mongodb-crypt module into mongo-java-driver as a new Gradle…

ceb18e5

… project. JAVA-5582

Integrate mongocrypt tests.

fb7c733

JAVA-5582

vbabanin changed the title ~~Integrate mongodb-crypt module into mongo-java-driver as a new Gradle…~~ Integrate mongodb-crypt module into mongo-java-driver Aug 27, 2024

vbabanin changed the title ~~Integrate mongodb-crypt module into mongo-java-driver~~ Integrate mongodb-crypt module Aug 27, 2024

NathanQingyangXu reviewed Aug 27, 2024

View reviewed changes

vbabanin added 2 commits August 27, 2024 16:29

Prevent downloading binaries on each local run.

0750e20

JAVA-5582

Merge branch 'master' into JAVA-5582

578a0a8

vbabanin self-assigned this Aug 28, 2024

vbabanin commented Aug 28, 2024

View reviewed changes

Update manifest.

4d967e3

JAVA-5582

vbabanin requested a review from rozza August 30, 2024 21:50

vbabanin added 7 commits August 30, 2024 14:50

Merge branch 'master' into JAVA-5582

ed15a21

Add slf4j to manifest.

b71f621

JAVA-5582

Remove gitignore due to having one in root directory.

1233c1b

JAVA-5582

- Fix checkstyle issues.

cc32a17

- Remove redundant git function from build script. - Integrate MongoCrypt benchmarks. JAVA-5582

Remove redundant benchmark.

b8bad4e

JAVA-5582

Fix checkstyle issues.

ee7d96c

JAVA-5582

Ensure incremental builds for unzip and downloadJava tasks.

1e38914

JAVA-5582

vbabanin commented Sep 10, 2024

View reviewed changes

vbabanin added 2 commits September 11, 2024 11:26

Remove redundant configs.

a99def1

JAVA-5582

Format code.

d390438

JAVA-5582

vbabanin commented Sep 11, 2024

View reviewed changes

Merge branch 'master' into JAVA-5582

fb6f31f

vbabanin commented Sep 11, 2024

View reviewed changes

Move mongocrypt benchmark.

070fba8

JAVA-5582

vbabanin marked this pull request as ready for review September 12, 2024 06:26

vbabanin added 2 commits September 12, 2024 14:53

Delete whole resources dir instead of individual files.

5495133

JAVA-5582

Add runtimeElemens to GraalVM script, as this configuration is meant …

b0438fd

…to be used by consumers, to retrieve all the elements necessary to run against this library immitating trasative dependency resolution. JAVA-5582

vbabanin requested a review from jyemin September 17, 2024 03:42

PR nits

10f3cf5

jyemin requested changes Sep 17, 2024

View reviewed changes

Spotbugs naming convention fix

7b5738b

jyemin requested changes Sep 17, 2024

View reviewed changes

rozza approved these changes Sep 17, 2024

View reviewed changes

jyemin reviewed Sep 17, 2024

View reviewed changes

mongocrypt/build.gradle.kts Outdated Show resolved Hide resolved

Remove logback from dependencies.

204330b

JAVA-5582

jyemin self-requested a review September 19, 2024 02:11

jyemin approved these changes Sep 19, 2024

View reviewed changes

rozza and others added 4 commits September 19, 2024 16:37

Fix driver-core pom optional.

a8cee8d

Merge branch 'master' into JAVA-5582

acf2205

Merge branch 'master' into JAVA-5582

6699721

Add mongocrypt support to driver-sync and reactive streams driver

28c5b83

jyemin closed this Sep 19, 2024

jyemin reopened this Sep 19, 2024

Changed the project name in the configuration to align with the artif…

6e3e706

…actId, ensuring that the 'optional' plugin functions correctly. JAVA-5582

Move mongocrypt to mongodb-crypt to work around optional project name…

a8699de

… issue

jyemin approved these changes Sep 20, 2024

View reviewed changes

evergreen-ci-prod bot mentioned this pull request Sep 20, 2024

Java 5582 rename #1499

Closed

jyemin merged commit 7e3108b into mongodb:master Sep 20, 2024
56 of 59 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate mongodb-crypt module #1487

Integrate mongodb-crypt module #1487

vbabanin commented Aug 27, 2024 •

edited

Loading

NathanQingyangXu Aug 27, 2024 •

edited

Loading

vbabanin Aug 30, 2024

vbabanin Aug 28, 2024

vbabanin Sep 10, 2024 •

edited

Loading

vbabanin Sep 11, 2024 •

edited

Loading

vbabanin Sep 11, 2024 •

edited

Loading

jyemin left a comment

jyemin Sep 17, 2024

rozza Sep 17, 2024

rozza left a comment •

edited

Loading

jyemin left a comment

vbabanin commented Sep 19, 2024

rozza commented Sep 19, 2024

jyemin commented Sep 19, 2024

jyemin commented Sep 19, 2024

vbabanin commented Sep 20, 2024 •

edited

Loading

rozza commented Sep 20, 2024

evergreen-ci-prod bot commented Sep 20, 2024

jyemin left a comment

jyemin commented Sep 20, 2024

evergreen-ci-prod bot commented Sep 20, 2024

Integrate mongodb-crypt module #1487

Integrate mongodb-crypt module #1487

Conversation

vbabanin commented Aug 27, 2024 • edited Loading

Description

Review Process

To aid the review process, I have taken several steps:

Note for Reviewers:

NathanQingyangXu Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

vbabanin Aug 30, 2024

Choose a reason for hiding this comment

vbabanin Aug 28, 2024

Choose a reason for hiding this comment

vbabanin Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

vbabanin Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

vbabanin Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

jyemin left a comment

Choose a reason for hiding this comment

jyemin Sep 17, 2024

Choose a reason for hiding this comment

rozza Sep 17, 2024

Choose a reason for hiding this comment

rozza left a comment • edited Loading

Choose a reason for hiding this comment

jyemin left a comment

Choose a reason for hiding this comment

vbabanin commented Sep 19, 2024

rozza commented Sep 19, 2024

jyemin commented Sep 19, 2024

jyemin commented Sep 19, 2024

vbabanin commented Sep 20, 2024 • edited Loading

rozza commented Sep 20, 2024

evergreen-ci-prod bot commented Sep 20, 2024

jyemin left a comment

Choose a reason for hiding this comment

jyemin commented Sep 20, 2024

evergreen-ci-prod bot commented Sep 20, 2024

vbabanin commented Aug 27, 2024 •

edited

Loading

NathanQingyangXu Aug 27, 2024 •

edited

Loading

vbabanin Sep 10, 2024 •

edited

Loading

vbabanin Sep 11, 2024 •

edited

Loading

vbabanin Sep 11, 2024 •

edited

Loading

rozza left a comment •

edited

Loading

vbabanin commented Sep 20, 2024 •

edited

Loading