CSHARP-4149: Add FLE 2 behavior for CreateCollection() and Collection.Drop(). #790

DmitryLukyanov · 2022-05-05T15:16:55Z

No description provided.

….Drop().

DmitryLukyanov · 2022-05-05T15:18:50Z

src/MongoDB.Driver.Core/Core/Operations/CreateIndexesOperation.cs

@@ -19,7 +19,9 @@
 using System.Threading;
 using System.Threading.Tasks;
 using MongoDB.Bson;
+using MongoDB.Bson.Serialization.Serializers;


no real changes here, I just removed original CreateIndexesOperation and renamed CreateIndexesUsingCommandOperation to CreateIndexesOperation. This change was missed in one of the previous tickets and it a bit affected tests I wrote in this ticket

src/MongoDB.Driver/Encryption/AutoEncryptionOptions.cs

DmitryLukyanov · 2022-05-05T15:20:34Z

src/MongoDB.Driver/MongoDatabaseImpl.cs

+                });
+        }
+
+        private async Task<IWriteOperation<BsonDocument>> CreateDropCollectionOperationAsync(string name, DropCollectionOptions options, IClientSessionHandle session, CancellationToken cancellationToken)


it's unfortunate that creating collection step requires calling a server, so now we need to have 2 versions of this method: sync and async

DmitryLukyanov · 2022-05-05T15:22:15Z

src/MongoDB.Driver/MongoDatabaseImpl.cs

@@ -824,7 +908,7 @@ private MessageEncoderSettings GetMessageEncoderSettings()
 #pragma warning disable 618
            if (BsonDefaults.GuidRepresentationMode == GuidRepresentationMode.V2)
            {
-                messageEncoderSettings.Add(MessageEncoderSettingsName.GuidRepresentation, _settings.GuidRepresentation);
+                messageEncoderSettings.Add(MessageEncoderSettingsName.GuidRepresentation, withEncryption ? GuidRepresentation.Unspecified : _settings.GuidRepresentation);


@rstam can you please take a look at this change? In 2 words, since create/drop collection with FLE happens without libmongocrypt (ie LibMongoCryptController) call, but server still expects subType=4 in the provided FLE configuration (encryptedFields), we have to configure correct guidRepresentation manually. In this case we rely on what was provided in the schema

I don't think you need a withEncryption parameter at all, you can just do:

messageEncoderSettings.Add(MessageEncoderSettingsName.GuidRepresentation, GuidRepresentation.Unspecified);

whether encryption is configured or not.

And it is correct to only do this when GuidRepresentationMode is V2.

discussed offline

BorisDog

Mostly LGTM, some minor comments.

src/MongoDB.Driver.Core/Core/Encryption/EncryptedCollectionHelper.cs

src/MongoDB.Driver.Core/Core/Operations/CreateCollectionOperation.cs

src/MongoDB.Driver.Core/Core/Operations/CompositeWriteOperation.cs

tests/MongoDB.Driver.Tests/AutoEncryptionOptionsTests.cs

...MongoDB.Driver.Tests/Specifications/client-side-encryption/ClientSideEncryptionTestRunner.cs

BorisDog

LGTM, two minor comments for you considerations.

BorisDog · 2022-05-09T22:24:46Z

src/MongoDB.Driver/MongoDatabaseImpl.cs

+            {
+                if (autoEncryptionOptions != null)
+                {
+                    var listCollectionOptions = new ListCollectionsOptions() { Filter = $"{{ name : '{collectionNamespace.CollectionName}' }}" };


minor: I'd combine the two conditions into single statement.

BorisDog · 2022-05-09T22:27:06Z

tests/MongoDB.Driver.Core.Tests/Core/Operations/CompositeWriteOperationTests.cs

+            var subject = new CompositeWriteOperation<BsonDocument>((healthyOperation1.Object, IsMainOperation: false), (faultyOperation2.Object, IsMainOperation: false), (healthyOperation3.Object, IsMainOperation: true));
+
+            var resultedException = async
+                ? Record.Exception(() => subject.ExecuteAsync(Mock.Of<IWriteBinding>(), CancellationToken.None).GetAwaiter().GetResult())


Not sure that it's crucial to change all the old tests. But lots of new new tests are definitely using the usual TPL oriented approach.

rstam · 2022-05-11T17:05:47Z

src/MongoDB.Driver/MongoDatabaseImpl.cs

-#pragma warning restore
+            var collectionNamespace = new CollectionNamespace(_databaseNamespace, name);
+
+            _ = EncryptedCollectionHelper.TryGetEffectiveEncryptedFields(collectionNamespace, options.EncryptedFields, _client.Settings?.AutoEncryptionOptions?.EncryptedFieldsMap, out var effectiveEncryptedFields);


How can you ignore the return value? What if TryGetEffectiveEncryptedFields returns false?

This "looks" like a bug. Consider a new helper method instead:

var effectiveEncryptedFields = EncryptedCollectionHelper.GetEffectiveEncryptedFields(collectionNamespace, options.EncryptedFields, _client.Settings?.AutoEncryptionOptions?.EncryptedFieldsMap);

The new helper method could be:

public static BsonDocument GetEffectiveEncryptedFields(CollectionNamespace collectionNamespace, BsonDocument encryptedFields, IReadOnlyDictionary<string, BsonDocument> encryptedFieldsMap) { if (TryGetEffectiveEncryptedFields(collectionNamespace, encryptedFields, encryptedFieldsMap, out var effectiveEncryptedFields)) { return effectiveEncryptedFields; } else { return null; } }

I realize there's a bit of redundancy in this proposed helper method but it's very clear and doesn't rely on any side effects of the TryGetEffectiveEncryptedFields method.

rstam · 2022-05-11T17:32:57Z

tests/MongoDB.Driver.Core.Tests/Core/Operations/CompositeWriteOperationTests.cs

+            var subject = new CompositeWriteOperation<BsonDocument>((healthyOperation1.Object, IsMainOperation: false), (faultyOperation2.Object, IsMainOperation: false), (healthyOperation3.Object, IsMainOperation: true));
+
+            var resultedException = async
+                ? Record.Exception(() => subject.ExecuteAsync(Mock.Of<IWriteBinding>(), CancellationToken.None).GetAwaiter().GetResult())


I think the only reason OLD tests don't return Task is that when they were written xUnit did not yet support that.

And there was probably some period of time between when xUnit started supporting async tests and when we became aware of it and started using that.

I think it's fine to use async tests going forward. No need to change old tests. Perhaps if we work on on old tests for some reason we can change them little by little.

rstam · 2022-05-11T17:42:03Z

src/MongoDB.Driver/MongoDatabaseImpl.cs

@@ -291,28 +292,48 @@ public override Task CreateCollectionAsync(IClientSessionHandle session, string

        public override void DropCollection(string name, CancellationToken cancellationToken)
        {
-            UsingImplicitSession(session => DropCollection(session, name, cancellationToken), cancellationToken);
+            UsingImplicitSession(session => DropCollection(session, name, options: null, cancellationToken), cancellationToken);


I would avoid duplicating even small amounts of logic. Simply provide the default value for the missing parameter and delegate to the next method:

DropCollection(name, options: null, cancellationToken);

rstam · 2022-05-11T17:42:47Z

src/MongoDB.Driver/MongoDatabaseImpl.cs

            ExecuteWriteOperation(session, operation, cancellationToken);
        }

        public override Task DropCollectionAsync(string name, CancellationToken cancellationToken)
        {
-            return UsingImplicitSessionAsync(session => DropCollectionAsync(session, name, cancellationToken), cancellationToken);
+            return UsingImplicitSessionAsync(session => DropCollectionAsync(session, name, options: null, cancellationToken), cancellationToken);


Same here, avoid duplicating even the smallest amount of logic:

return DropCollectionAsync(name, options: null, cancellationToken);

rstam · 2022-05-11T17:44:58Z

src/MongoDB.Driver/MongoDatabaseImpl.cs

@@ -824,7 +908,7 @@ private MessageEncoderSettings GetMessageEncoderSettings()
 #pragma warning disable 618
            if (BsonDefaults.GuidRepresentationMode == GuidRepresentationMode.V2)
            {
-                messageEncoderSettings.Add(MessageEncoderSettingsName.GuidRepresentation, _settings.GuidRepresentation);
+                messageEncoderSettings.Add(MessageEncoderSettingsName.GuidRepresentation, withEncryption ? GuidRepresentation.Unspecified : _settings.GuidRepresentation);


I don't think you need a withEncryption parameter at all, you can just do:

messageEncoderSettings.Add(MessageEncoderSettingsName.GuidRepresentation, GuidRepresentation.Unspecified);

whether encryption is configured or not.

rstam · 2022-05-11T17:47:00Z

src/MongoDB.Driver.Core/Core/Encryption/EncryptedCollectionHelper.cs

+            }
+
+            return true;
+        }


I found the logic of the above method somewhat confusing and it took me a long time to convince myself that it could be correct. I think the following refactoring expresses the intent more clearly:

public static bool TryGetEffectiveEncryptedFields(CollectionNamespace collectionNamespace, BsonDocument encryptedFields, IReadOnlyDictionary<string, BsonDocument> encryptedFieldsMap, out BsonDocument effectiveEncryptedFields) { if (encryptedFields != null) { effectiveEncryptedFields = encryptedFields; return true; } if (encryptedFieldsMap != null) { return encryptedFieldsMap.TryGetValue(collectionNamespace.ToString(), out effectiveEncryptedFields); } effectiveEncryptedFields = null; return false; }

What do you think?

rstam · 2022-05-11T17:49:42Z

src/MongoDB.Driver.Core/Core/Operations/CreateCollectionOperation.cs

+                    (CreateInnerCollectionOperation(EncryptedCollectionHelper.GetAdditionalCollectionName(encryptedFields, collectionNamespace, HelperCollectionForEncryption.Esc)), IsMainOperation: false),
+                    (CreateInnerCollectionOperation(EncryptedCollectionHelper.GetAdditionalCollectionName(encryptedFields, collectionNamespace, HelperCollectionForEncryption.Ecc)), IsMainOperation: false),
+                    (CreateInnerCollectionOperation(EncryptedCollectionHelper.GetAdditionalCollectionName(encryptedFields, collectionNamespace, HelperCollectionForEncryption.Ecos)), IsMainOperation: false),
+                    (mainOperation, true),


Suggestion:

(mainOperation, IsMainOperation: true),

rstam · 2022-05-11T17:51:30Z

src/MongoDB.Driver/MongoDatabaseImpl.cs

@@ -824,7 +908,7 @@ private MessageEncoderSettings GetMessageEncoderSettings()
 #pragma warning disable 618
            if (BsonDefaults.GuidRepresentationMode == GuidRepresentationMode.V2)
            {
-                messageEncoderSettings.Add(MessageEncoderSettingsName.GuidRepresentation, _settings.GuidRepresentation);
+                messageEncoderSettings.Add(MessageEncoderSettingsName.GuidRepresentation, withEncryption ? GuidRepresentation.Unspecified : _settings.GuidRepresentation);


And it is correct to only do this when GuidRepresentationMode is V2.

rstam

LGTM

DmitryLukyanov added 5 commits May 2, 2022 04:11

CSHARP-4149: Add FLE 2 behavior for CreateCollection() and Collection…

2c2f089

….Drop().

CSHARP-4149: Refactoring.

86630a7

CSHARP-4149: Update xml comment.

3461943

CSHARP-4149: Add tests.

0b760f7

CSHARP-4149: Add tests.

f2ccdaf

DmitryLukyanov commented May 5, 2022

View reviewed changes

DmitryLukyanov requested a review from JamesKovacs May 5, 2022 15:24

JamesKovacs requested review from BorisDog and removed request for JamesKovacs May 5, 2022 19:04

BorisDog reviewed May 9, 2022

View reviewed changes

CSHARP-4149: Code review.

215fc1d

DmitryLukyanov requested a review from BorisDog May 9, 2022 19:46

CSHARP-4149: Code review.

ef9cc1f

BorisDog approved these changes May 9, 2022

View reviewed changes

rstam requested changes May 11, 2022

View reviewed changes

CSHARP-4149: Code review.

5992841

DmitryLukyanov requested a review from rstam May 11, 2022 21:31

DmitryLukyanov added 2 commits May 12, 2022 03:20

CSHARP-4149: Code review.

c719ffc

CSHARP-4149: Make operation.EncryptedFields internal.

3c271c3

rstam approved these changes May 12, 2022

View reviewed changes

Merge branch 'master' into csharp4149

3431deb

DmitryLukyanov merged commit c7c1510 into mongodb:master May 12, 2022

CSHARP-4149: Add FLE 2 behavior for CreateCollection() and Collection.Drop(). #790

CSHARP-4149: Add FLE 2 behavior for CreateCollection() and Collection.Drop(). #790

Uh oh!

Conversation

DmitryLukyanov commented May 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DmitryLukyanov May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DmitryLukyanov May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BorisDog left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BorisDog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rstam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DmitryLukyanov May 5, 2022 •

edited

Loading

DmitryLukyanov May 5, 2022 •

edited

Loading