v1: auto_encryption_options (CXX-3237, CXX-3238)

[eramongodb]

Resolves CXX-3237 and CXX-3238 for the v1::auto_encryption_options component.

The "key_vault_client" and "key_vault_pool" fields cannot be converted to or from v1 due to depending on object identity. Even though we could in theory permit using the internal v1::client or v1::pool objects of a refactored v_noabi::client and v_noabi::pool respectively, this will likely be error-prone and non-obvious given the internal v1 objects of refactored v_noabi components are an implementation detail (that is, v_noabi::client does not publically inherit v1::client or expose direct access to the data member in its public API). Therefore, these fields are documented as ignored during the conversion process, similar to how the "ordered" field for v_noabi::options::insert is ignored due to having no v1::insert_one_options equivalent.

[kevinAlbs]

Rather than ignore, could an exception be thrown if these fields are set? The ignored options might go quietly unnoticed. That may result in a later runtime error when trying to decrypt. The key_vault_client and key_vault_pool enable users to use a different MongoDB cluster to store/load data keys.

[eramongodb]

I am concerned that throwing an exception would impact the usability of these conversion functions. With the current proposal:

// Explicitly:
auto opts_v1 = v_noabi::to_v1(opts_v_noabi); // Unset.
opts_v1.key_vault_client(&client_v1);        // (Re)set.
use_v1(opts_v1);

// Inline:
use_v1(v_noabi::to_v1(opts_v_noabi).key_vault_client(&client));

Although this extra step is unfortunate, I don't think it is as problematic as the proposed option of throwing an exception, which does not change the need for the additional step of (re)setting the key vault client/pool:

try {
  opts_v_noabi.key_vault_client(nullptr);      // A new required initial step (note: "null" != "unset").
  auto opts_v1 = v_noabi::to_v1(opts_v_noabi); // Suppose we permit "null" == "unset" for this conversion.
  opts_v1.key_vault_client(&client_v1);        // This step is still necessary.
  use_v1(opts_v1);
} catch (...) {
  // When `.key_vault_client(nullptr)` is forgotten: recover why and how?
}

I would prefer to avoid throwing exceptions from options/result class if possible, deferring validation and error handling to the client/pool/database/collection/etc. that ultimately uses them. However, you make a good point about the potential silently-incorrect behavior.

Proposing an alternative solution: prevent implicit (or explicit) conversion directly to/from v1 and instead require the use of the from_v1()/to_v1() conversion functions (similar to what is currently done for v1 -> v_noabi for v_noabi::document::value), providing overloads to explicitly set the new key vault client/pool as an argument to the conversion operation instead of requiring a followup (re)set step:

// error: no (implicit or explicit) direct conversions.
use_v1(v1::auto_encryption_options{opts_v_noabi});
use_v_noabi(opts_v1);

use_v1(v_noabi::to_v1(opts_v_noabi));             // None (all unset).
use_v1(v_noabi::to_v1(opts_v_noabi, &client_v1)); // With new key vault client.
use_v1(v_noabi::to_v1(opts_v_noabi, &pool_v1));   // With new key vault pool.

use_v_noabi(v_noabi::from_v1(opts_v1));                  // None (all unset).
use_v_noabi(v_noabi::from_v1(opts_v1, &client_v_noabi)); // With new key vault client.
use_v_noabi(v_noabi::from_v1(opts_v1, &pool_v_noabi));   // With new key vault pool.

This should at the very least prevent implicit v1 -> v_noabi conversion which may lose the key vault client/pool field: such a conversion must always be explicit using from_v1().

[kevinAlbs]

Proposed alternative SGTM. I expect that will at least better inform users that a client/pool must be separately set.