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.

Sign up for GitHub

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

Jump to bottom

Fix check for whether crypt shared lib is available #1004

Merged
merged 2 commits into from
Sep 29, 2022
Merged

Fix check for whether crypt shared lib is available #1004

merged 2 commits into from
Sep 29, 2022

Conversation

jyemin
Copy link
Contributor

@jyemin jyemin commented Sep 28, 2022

JAVA-4739

  • Extract method for duplicated logic from sync and reactive CommandMarker
  • Fix the bug and and unit tests for the extracted method

@jyemin
Extract method for duplicated logic checking for whether mongocryptd …
d878810 
…is disabled

JAVA-4739
@jyemin
Fix check for whether crypt shared lib is available
1e48303 
Also add unit tests for isMongocryptdSpawningDisabled helper method

JAVA-4739
@jyemin jyemin requested a review from rozza September 28, 2022 17:58
@jyemin jyemin self-assigned this Sep 28, 2022
rozza
rozza approved these changes Sep 29, 2022
View reviewed changes
Copy link
Member

@rozza rozza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Good work on DRY'ing up the code also and adding a regression test. 👍

driver-core/src/main/com/mongodb/internal/capi/MongoCryptHelper.java
final AutoEncryptionSettings settings) {
boolean cryptSharedLibIsAvailable = cryptSharedLibVersion != null && !cryptSharedLibVersion.isEmpty();
boolean cryptSharedLibRequired = (boolean) settings.getExtraOptions().getOrDefault("cryptSharedLibRequired", false);
return settings.isBypassAutoEncryption() || settings.isBypassQueryAnalysis() || cryptSharedLibRequired || cryptSharedLibIsAvailable;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that this is all in one place but wow what a return statement!!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wish it were simpler...

@jyemin jyemin merged commit aa5ad82 into mongodb:master Sep 29, 2022
@jyemin jyemin deleted the j4739 branch September 29, 2022 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rozza rozza rozza approved these changes

Assignees

@jyemin jyemin

Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@jyemin @rozza