-
Notifications
You must be signed in to change notification settings - Fork 1.3k
CSHARP-4255: Automatically create Queryable Encryption keys. #961
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to return anything?
yeah, I also think it can be simply void
method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
|
||
if (storedEncryptedFields.TryGetValue("fields", out var fields) && fields is BsonArray fieldsArray) | ||
{ | ||
foreach (var field in fieldsArray) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, I like first one, second one probably is a bit harder to read, done
} | ||
} | ||
|
||
yield break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/// <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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like ModifyEndryptedFields
is spelled wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true
f2a5bd9
to
6013fc2
Compare
No description provided.