CSHARP-4255: Automatically create Queryable Encryption keys. #961
Conversation
| /// <param name="kmsProvider">The kms provider.</param> | ||
| /// <param name="dataKeyOptions">The datakey options.</param> | ||
| /// <param name="cancellationToken">The cancellation token.</param> | ||
| public (IMongoCollection<TCollection> Collection, BsonDocument EncryptedFields) CreateEncryptedCollection<TCollection>(CollectionNamespace collectionNamespace, CreateCollectionOptions createCollectionOptions, string kmsProvider, DataKeyOptions dataKeyOptions, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
I'm going to discuss whether we can add a different return type than this, so might be not the final form
There was a problem hiding this comment.
I've discussed this question with spec author and he's confirmed that we can define public API in the consistent way with previous driver's API methods. So, I consider 2 changes:
- Do not return created collection from this helper method. Instead do the same logic as we do with a regular CreateCollection method ie return nothing and request retrieving collection via GetCollection.
- Instead returning EncryptedFields, we can simply modify input EncryptedFields in the provided options.
I'm not too confident about these changes (as well as about the initial requirement), but I would probably want to avoid returning tuple in public API, thoughts?
There was a problem hiding this comment.
I think Tuples are not recommended in public APIs.
Agreed that there is no need to return the IMongoCollection<TDocument> .
Do we need to return anything?
Also, shouldn't the type parameter be TDocument (not TCollection). But if we don't return the IMongoCollection<TDocument> I don't think this type parameter is needed any more anyway.
There was a problem hiding this comment.
Do we need to return anything?
yeah, I also think it can be simply void method
JamesKovacs
requested review from
BorisDog
and removed request for
JamesKovacs
|
|
||
| if (storedEncryptedFields.TryGetValue("fields", out var fields) && fields is BsonArray fieldsArray) | ||
| { | ||
| foreach (var field in fieldsArray) |
There was a problem hiding this comment.
fieldsArray.OfType<BsonDocument>() ?
or even
fieldsArray
.OfType< BsonDocument>()
.Where(b => b.TryGetElement("keyId", out var keyId) && keyId.Value == BsonNull.Value)
There was a problem hiding this comment.
good point, I like first one, second one probably is a bit harder to read, done
| } | ||
| } | ||
|
|
||
| yield break; |
| /// <param name="kmsProvider">The kms provider.</param> | ||
| /// <param name="dataKeyOptions">The datakey options.</param> | ||
| /// <param name="cancellationToken">The cancellation token.</param> | ||
| public (IMongoCollection<TCollection> Collection, BsonDocument EncryptedFields) CreateEncryptedCollection<TCollection>(CollectionNamespace collectionNamespace, CreateCollectionOptions createCollectionOptions, string kmsProvider, DataKeyOptions dataKeyOptions, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
I think Tuples are not recommended in public APIs.
Agreed that there is no need to return the IMongoCollection<TDocument> .
Do we need to return anything?
Also, shouldn't the type parameter be TDocument (not TCollection). But if we don't return the IMongoCollection<TDocument> I don't think this type parameter is needed any more anyway.
| foreach (var fieldDocument in EncryptedCollectionHelper.IterateEmptyKeyIds(collectionNamespace, effectiveEncryptedFields)) | ||
| { | ||
| var dataKey = CreateDataKey(kmsProvider, dataKeyOptions, cancellationToken); | ||
| EncryptedCollectionHelper.ModifyEndryptedFields(fieldDocument, dataKey); |
There was a problem hiding this comment.
Looks like ModifyEndryptedFields is spelled wrong.
DmitryLukyanov
force-pushed
the
csharp4255
branch
from
f2a5bd9 to
6013fc2
Compare
No description provided.