CSHARP-4255: Fix bug and some tests. #993
Conversation
| /// <param name="createCollectionOptions">The create collection options.</param> | ||
| /// <param name="kmsProvider">The kms provider.</param> | ||
| /// <param name="dataKeyOptions">The datakey options.</param> | ||
| /// <param name="cancellationToken">The cancellation token.</param> | ||
| /// <remarks> | ||
| /// if EncryptionFields contains a keyId with a null value, a data key will be automatically generated and assigned to keyId value. | ||
| /// </remarks> | ||
| public void CreateEncryptedCollection<TCollection>(CollectionNamespace collectionNamespace, CreateCollectionOptions createCollectionOptions, string kmsProvider, DataKeyOptions dataKeyOptions, CancellationToken cancellationToken = default) | ||
| public void CreateEncryptedCollection(IMongoDatabase database, string collectionName, CreateCollectionOptions createCollectionOptions, string kmsProvider, DataKeyOptions dataKeyOptions, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
spec API is described here
| @@ -82,59 +82,59 @@ public ClientEncryption(ClientEncryptionOptions clientEncryptionOptions) | |||
| /// <summary> | |||
| /// Create encrypted collection. | |||
| /// </summary> | |||
| /// <param name="collectionNamespace">The collection namespace.</param> | |||
| /// <param name="database">The database.</param> | |||
| /// <param name="collectionName">The collectionName.</param> | |||
| } | ||
|
|
||
| /// <summary> | ||
| /// Create encrypted collection. | ||
| /// </summary> | ||
| /// <param name="collectionNamespace">The collection namespace.</param> | ||
| /// <param name="database">The database.</param> | ||
| /// <param name="collectionName">The collectionName.</param> |
| void AssertInvalidOperationException(Exception ex, string message) => ex.Should().BeOfType<InvalidOperationException>().Which.Message.Should().Be(message); | ||
| } | ||
|
|
||
| private class EncryptedFieldsComparer : IEqualityComparer<BsonDocument> |
| bDocument["keyId"] = aDocument["keyId"]; | ||
| } | ||
| }); | ||
| public int GetHashCode(BsonDocument obj) => obj.GetHashCode(); |
| if (BsonDocument.TryParse(expectedResult, out var encryptedFields)) | ||
| { | ||
| var createCollectionOptions = new CreateCollectionOptions() { EncryptedFields = encryptedFieldsStr != null ? BsonDocument.Parse(encryptedFieldsStr) : null }; | ||
| subject.CreateEncryptedCollection(database, collectionName: collectionName, createCollectionOptions, kmsProvider: kmsProvider, dataKeyOptions); |
There was a problem hiding this comment.
I think it's better to omit trivial parameter naming like:
collectionName: collectionName, and kmsProvider: kmsProvider. Here and elsewhere.
| public override void GetObjectData(SerializationInfo info, StreamingContext context) | ||
| { | ||
| base.GetObjectData(info, context); | ||
| info.AddValue("_encryptedFields", _encryptedFields); |
There was a problem hiding this comment.
Is nameof applicable here?
| } | ||
| catch (Exception ex) | ||
| { | ||
| throw new MongoEncryptionCreateCollectionException(ex, encryptedFields); |
There was a problem hiding this comment.
Returning operation data, which is unrelated to exception, within exception is a bit unusual, but I am fine with this here.
@JamesKovacs this is follow up for our slack discussion, please double check that you are ok with this as well.
There was a problem hiding this comment.
Discussed with Dima via Slack. While from an API perspective it feels odd to return successfully-created keys in the exception, I don't see another reasonable way to avoid phantom encryption keys. While we could modify CreateCollectionOptions as we generate dataKeys, it violates the principle of least surprise to modify one's input parameters unexpectedly. I think this is the best option given the available trade-offs.
src/MongoDB.Driver/Encryption/MongoEncryptionCreateCollectionException.cs
Show resolved
Hide resolved
| var createCollectionOptions = new CreateCollectionOptions() { EncryptedFields = encryptedFieldsStr != null ? BsonDocument.Parse(encryptedFieldsStr) : null }; | ||
| AssertInvalidOperationException(Record.Exception(() => subject.CreateEncryptedCollection(database, collectionName, createCollectionOptions, kmsProvider, dataKeyOptions)), expectedResult); | ||
|
|
||
| createCollectionOptions = new CreateCollectionOptions() { EncryptedFields = encryptedFieldsStr != null ? BsonDocument.Parse(encryptedFieldsStr) : null }; |
There was a problem hiding this comment.
No need to recreate CreateCollectionOptions everywhere now.
| effectiveEncryptedFields.EncryptedFields.WithComparer(new EncryptedFieldsComparer()).Should().Be(encryptedFields.DeepClone()); | ||
|
|
||
| createCollectionOptions = new CreateCollectionOptions() { EncryptedFields = encryptedFieldsStr != null ? BsonDocument.Parse(encryptedFieldsStr) : null }; | ||
| effectiveEncryptedFields = await subject.CreateEncryptedCollectionAsync(database, collectionName, createCollectionOptions, kmsProvider, dataKeyOptions); |
There was a problem hiding this comment.
createCollectionResult instead of effectiveEncryptedFields?
| var exception = Record.Exception(() => subject.CreateEncryptedCollection(database, collectionName, createCollectionOptions, kmsProvider, dataKeyOptions)); | ||
| AssertResults(exception, createCollectionOptions); | ||
|
|
||
| createCollectionOptions = new CreateCollectionOptions() { EncryptedFields = BsonDocument.Parse(encryptedFieldsStr) }; |
There was a problem hiding this comment.
No need to recreate createCollectionOptions.
|
|
||
| internal override CreateCollectionOptions Clone() | ||
| { | ||
| var clone = new CreateCollectionOptions<TDocument>(); |
There was a problem hiding this comment.
nit: you could use initialization syntax.
var = new CreateCollectionOptions() { x= y ... }
No description provided.