Details
-
Task
-
Resolution: Fixed
-
Major - P3
-
None
-
None
-
None
-
None
Description
mongocrypt_opts_t was designed with future-proofing the API and was inspired by curl. Options are identified by an enum value and set with a void*. It's documented here.
However, now I think this isn't the appropriate API for libmongocrypt.
Type erasing the value seems annoying in wrappers. mongocrypt_opts_set_opt accepts generic values by taking a void* value. But wrappers often have to marshal out their native value into the appropriate C value. E.g. in Python, wrapping the mongocrypt_opts_set_opt directly would require Python code like:
PyObject* py_mongocrypt_opts_set_opt (PyObject *self, PyObject *args) {
|
PyObject *py_value;
|
int opt;
|
|
|
PyArg_ParseTuple (args, "iO", &type, &py_value);
|
|
|
if (opt == MONGOCRYPT_AWS_REGION) {
|
/* interpret py_value as a string. */
|
} else if (opt == MONGOCRYPT_LOG_FN) {
|
/* interpret py_value is a function. */
|
}
|
/* and so on ... */
|
So, that wrappers will likely either write individual functions for each option type, or have to do that type checking. This seems to goes against libmongocrypt's design goal of being easy to integrate.
Additionally, mongocrypt_opts_set_opt does not return a status currently. It seems preferable to do option validation when the option is set instead of waiting until the mongocrypt_t is initialized.
I propose we do away with mongocrypt_opts_t entirely and add individual option setters.
Instead of:
/* create opts. */
|
mongocrypt_opts_t* opts = mongocrypt_opts_new ();
|
mongocrypt_opts_set_opt (opts, MONGOCRYPT_AWS_ACCESS_KEY_ID, "ex1");
|
mongocrypt_opts_set_opt (opts, MONGOCRYPT_AWS_SECRET_ACCESS_KEY, "ex2");
|
|
|
mongocrypt_t * crypt = mongocrypt_new ();
|
if (!mongocrypt_init (crypt, opts)) {
|
/* get error status. */
|
mongocrypt_status_t* status = mongocrypt_status_new ();
|
mongocrypt_status (crypt, status)
|
}
|
We'll do:
mongocrypt_t * crypt = mongocrypt_new ();
|
mongocrypt_set_kms_provider_aws (crypt, "ex1", "ex2");
|
if (!mongocrypt_init (crypt, opts)) {
|
/* get error status. */
|
mongocrypt_status_t* status = mongocrypt_status_new ();
|
mongocrypt_status (crypt, status)
|
}
|
The downside is that future options add more API functions. But as it is, we'd still need to add to the enum. Wrapping another function later doesn't seem significantly more difficult than wrapping a new enum value.