Skip to content

CDRIVER-4394 Add explicit encryption support and tests for range indexes #1152

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

Merged
merged 73 commits into from
Jan 9, 2023

Conversation

galon1
Copy link
Contributor

@galon1 galon1 commented Nov 28, 2022

Summary

Adds support for explicit encryption with the Range Index.

  • Add mongoc_client_encryption_encrypt_range_opts and pass to libmongocrypt.
  • Add mongoc_client_encryption_encrypt_expression.
  • Add prose tests.
  • Add ASSERT_BSONVALUE_EQ.
  • Bump test dependency of libmongocrypt to 1.7.0-alpha1.

Background & Motivation

See mongodb/specifications@4de4e3c for a description of the API change and prose tests.

Range Index requires MongoDB server 6.2+.

The API is marked experimental due to this comment.

@galon1 galon1 requested a review from kevinAlbs November 28, 2022 15:38
Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

Looking good.

@galon1 galon1 requested a review from kevinAlbs November 28, 2022 22:21
Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

Tests for int LGTM

@galon1 galon1 changed the title CDRIVER-4394 Add explicit encryption support for range indexes CDRIVER-4394 Add explicit encryption support and tests for range indexes Nov 29, 2022
Let libmongocrypt do the error checking
Matches public API docs
Destroying is not necessary for primitive types like int/double.
But prevents a leak if a heap allocated types (e.g. string) is passed.
Produces a consistent error if range_opts are unset for both explicit helpers
It is not required by callers
There is no need to yet. Once there is a stable version of libmongocrypt 1.7.0, will bump the dependency.
@kevinAlbs kevinAlbs marked this pull request as ready for review December 19, 2022 15:13
@kevinAlbs kevinAlbs requested review from kkloberdanz and vector-of-bool and removed request for kevinAlbs December 19, 2022 15:13
}
}
]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest that we have newline characters at the end of files.

static ree_fixture *
range_explicit_encryption_setup (const char *typeStr)
{
ree_fixture *reef = (ree_fixture *) bson_malloc0 (sizeof (ree_fixture));
Copy link
Contributor

Choose a reason for hiding this comment

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

In C, casting from void * is not necessary, so I suggest that we don't cast on malloc. Reasoning can be found here: https://stackoverflow.com/a/605858

test_range_explicit_encryption_case3 (void *ctx)
{
// Case 3: can find encrypted range and return the minimum
const char *typeStr = (const char *) ctx;
Copy link
Contributor

Choose a reason for hiding this comment

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

Casting from void * is not necessary, so I suggest we remove the cast.

test_range_explicit_encryption_case4 (void *ctx)
{
// Case 4: can find encrypted range with an open range query
const char *typeStr = (const char *) ctx;
Copy link
Contributor

Choose a reason for hiding this comment

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

Casting from void * is not necessary, so I suggest we remove the cast.

Copy link
Contributor

@vector-of-bool vector-of-bool left a comment

Choose a reason for hiding this comment

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

Sorry for the late review. LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants