CSHARP-4182: Support for Range Indexes. #988

DmitryLukyanov · 2022-12-12T20:11:23Z

No description provided.

BorisDog · 2023-01-06T18:41:31Z

src/MongoDB.Driver.Core/Core/Misc/Feature.cs

@@ -57,6 +57,7 @@ public class Feature
        private static readonly Feature __createIndexCommitQuorum = new Feature("CreateIndexCommitQuorum", WireVersion.Server44);
        private static readonly Feature __createIndexesCommand = new Feature("CreateIndexesCommand", WireVersion.Server26);
        private static readonly Feature __createIndexesUsingInsertOperations = new Feature("CreateIndexesUsingInsertOperations", WireVersion.Zero, WireVersion.Server42);
+        private static readonly Feature __csfleRangeExplicitAlgorithm = new Feature("CsfleRangeExplicitAlgorithm", WireVersion.Server62);


Maybe cslfeRange or cslfeRangeAlgorithm is sufficient?
I see that simple "range" term is used in most case, also do we expect __csfleRangeImplicitAlgorithm or similar?

Explicit in this context means explicit encryption (ie via ClientEncryption). I agree that it's better to remove this part. Will do it.

Also do we want to call it Preview, to differentiate with future non preview?

At this point I think we will use the same server version guard. Most-likely the only difference will be removing Preview suffix. I would defer any change here to the time when it will be clear what final shape we will end up here with.

src/MongoDB.Driver/Encryption/ClientEncryption.cs

BorisDog · 2023-01-06T18:47:21Z

src/MongoDB.Driver/Encryption/ClientEncryption.cs

+            {
+                // should not be reached
+                var butMessage = encryptedValue == null ? " was null" : $"was {encryptedValue.GetType().Name}";
+                throw new InvalidOperationException($"The encrypted data must be {typeof(TEncryptedValue).Name}, but {butMessage}.");


nit: could also use: "was {encryptedValue?.GetType()?.Name ?? "null"}" instead of butMessage
nit: "The encrypted data type must be"

yeah, good point, will do it

BorisDog · 2023-01-06T18:50:37Z

src/MongoDB.Driver/Encryption/EncryptOptions.cs

+    /// For double and decimal128, min/max/precision must all be set, or all be unset.
+    /// RangeOptions only applies when algorithm is "rangePreview".
+    /// </remarks>
+    public class RangeOptions


src/MongoDB.Driver/Encryption/EncryptOptions.cs

...Driver.Tests/Specifications/client-side-encryption/prose-tests/ClientEncryptionProseTests.cs

BorisDog · 2023-01-06T20:15:13Z

...Driver.Tests/Specifications/client-side-encryption/prose-tests/ClientEncryptionProseTests.cs

+                        {
+                            var insertPayload6 = ExplicitEncrypt(clientEncryption, encryptOptions, value6, async);
+                            var decryptedValue = ExplicitDecrypt(clientEncryption, insertPayload6, async);
+                            decryptedValue.Should().Be(BsonValue.Create(value6));


Curious why do we need BsonValue.Create here?

I will double check, maybe it's already not needed. The initial idea (in other tests) is that decryptedValue is always Bson type, where regularly expected values are just a string or numbers. So when we compare it as is, it fails because of different types. But in this particular case value6 is already bson type.

I think this change is relevant in all other cases.

BorisDog · 2023-01-06T20:22:52Z

...Driver.Tests/Specifications/client-side-encryption/prose-tests/ClientEncryptionProseTests.cs

+                            result.Should().BeOfType<BsonDouble>();
+                        }
+                        break;
+                    case "DoublePrecision":


minor: You could combine DoubleNoPrecision and DoublePrecision cases.

method is removed

...Driver.Tests/Specifications/client-side-encryption/prose-tests/ClientEncryptionProseTests.cs

src/MongoDB.Driver/Encryption/EncryptOptions.cs

BorisDog · 2023-01-09T22:53:44Z

src/MongoDB.Driver.Core/Core/Misc/Feature.cs

@@ -57,6 +57,7 @@ public class Feature
        private static readonly Feature __createIndexCommitQuorum = new Feature("CreateIndexCommitQuorum", WireVersion.Server44);
        private static readonly Feature __createIndexesCommand = new Feature("CreateIndexesCommand", WireVersion.Server26);
        private static readonly Feature __createIndexesUsingInsertOperations = new Feature("CreateIndexesUsingInsertOperations", WireVersion.Zero, WireVersion.Server42);
+        private static readonly Feature __csfleRangeExplicitAlgorithm = new Feature("CsfleRangeExplicitAlgorithm", WireVersion.Server62);


Also do we want to call it Preview, to differentiate with future non preview?

BorisDog

Few minor comments.

BorisDog · 2023-01-18T20:56:05Z

...Driver.Tests/Specifications/client-side-encryption/prose-tests/ClientEncryptionProseTests.cs

+                            var exception = Record.Exception(() =>
+                                ExplicitEncrypt(
+                                    clientEncryption,
+                                    encryptOptions.With(rangeOptions: new RangeOptions(sparsity: 1, min: BsonValue.Create(0), max: BsonValue.Create(200), precision: 2)),


I don't think BsonValue.Create is needed.
You could just pass int argument and avoid boxing.

no, it's needed. Since the input type is Optional<BsonValue>, otherwise, it's not compiled

I see, then let's use (BsonValue)0?

I would prefer BsonValue.Create. If you have a factory method, I would use it. NOTE: no huge logic happens behind the scene, it will be almost the same logic as you suggest

BorisDog · 2023-01-18T21:20:21Z

...Driver.Tests/Specifications/client-side-encryption/prose-tests/ClientEncryptionProseTests.cs

+                "Date" => new BsonDateTime(millisecondsSinceEpoch: value),
+                "Int" => new BsonInt32(value),
+                "Long" => new BsonInt64(value),
+                _ => throw new Exception($"Unsupported rangeSupportedType {rangeSupportedType}.")


nit: $"Unsupported {nameof(rangeSupportedType)} {rangeSupportedType}." or even better
new ArgumentException($"Unsupported nameof(rangeSupportedType) {rangeSupportedType}.", nameof(rangeSupportedType))

I prefer the first one, since we throw in test in majority of cases only Exception, done

BorisDog · 2023-01-18T21:22:19Z

...Driver.Tests/Specifications/client-side-encryption/prose-tests/ClientEncryptionProseTests.cs

+            BsonDocument expression,
+            bool async) =>
+            async
+                ? clientEncryption.EncryptExpressionAsync(expression, encryptOptions).GetAwaiter().GetResult()


I would recommend to shift away from using GetAwaiter().GetResult() anti-pattern.
You can return task here, and make the whole test async.

I'm ok with this approach, but don't really want to change unrelated tests in this file just to make them compiled after this change. So I would use an old approach for this file

Isn't this a new method used only by the new RangeExplicitEncryptionTest test?

Yes, but this test touches Insert that then touches everything else

discussed offline, changed

BorisDog · 2023-01-19T23:06:07Z

...Driver.Tests/Specifications/client-side-encryption/prose-tests/ClientEncryptionProseTests.cs

+                            var exception = Record.Exception(() =>
+                                ExplicitEncrypt(
+                                    clientEncryption,
+                                    encryptOptions.With(rangeOptions: new RangeOptions(sparsity: 1, min: BsonValue.Create(0), max: BsonValue.Create(200), precision: 2)),


I see, then let's use (BsonValue)0?

BorisDog · 2023-01-19T23:09:13Z

...Driver.Tests/Specifications/client-side-encryption/prose-tests/ClientEncryptionProseTests.cs

+                "Date" => new BsonDateTime(millisecondsSinceEpoch: value),
+                "Int" => new BsonInt32(value),
+                "Long" => new BsonInt64(value),
+                _ => throw new Exception($"Unsupported {nameof(rangeSupportedType)} {rangeSupportedType}.")


ArgumentException is used a lot in the driver in such cases already, also it's more suitable exception type her, while Exception should be used only when there is no better specific exception.
Up to you.

it's interesting, I expected that we use it rare than it actually happen. We still use: throw new Exception more often.. but I'm ok to change it

BorisDog · 2023-01-19T23:11:38Z

...Driver.Tests/Specifications/client-side-encryption/prose-tests/ClientEncryptionProseTests.cs

+            BsonDocument expression,
+            bool async) =>
+            async
+                ? clientEncryption.EncryptExpressionAsync(expression, encryptOptions).GetAwaiter().GetResult()


Isn't this a new method used only by the new RangeExplicitEncryptionTest test?

BorisDog

LGTM.

DmitryLukyanov added 4 commits December 2, 2022 20:19

DRIVERS-2286 Add specification tests for Range Index.

48bff20

Fix typo.

7788cf4

Fix typo.

9a87459

DRIVERS-2505 Add explicit encryption API and tests for range indexes.

6702aeb

DmitryLukyanov requested a review from JamesKovacs December 12, 2022 20:11

DmitryLukyanov mentioned this pull request Dec 12, 2022

CSHARP-4182: Add specification tests for Range Index. #980

Closed

Fix tests.

bbdd506

JamesKovacs requested review from BorisDog and removed request for JamesKovacs January 4, 2023 02:34

BorisDog requested changes Jan 6, 2023

View reviewed changes

DmitryLukyanov added 2 commits January 10, 2023 01:47

Code review.

f05b427

Code review.

abfe988

DmitryLukyanov requested a review from BorisDog January 9, 2023 22:26

BorisDog reviewed Jan 9, 2023

View reviewed changes

DmitryLukyanov added 2 commits January 10, 2023 03:37

Code review.

b54ba79

Code review.

8adb93d

DmitryLukyanov requested a review from BorisDog January 10, 2023 19:57

BorisDog requested changes Jan 18, 2023

View reviewed changes

DmitryLukyanov added 3 commits January 20, 2023 01:20

Support Decimal128.

4dc846a

Fixing packaging tests.

16e5726

Code review.

d52dd64

DmitryLukyanov requested a review from BorisDog January 19, 2023 21:43

BorisDog reviewed Jan 19, 2023

View reviewed changes

DmitryLukyanov added 2 commits January 20, 2023 03:45

Make ExplicitEncryptExpression async

a9ae9da

Replace exception on ArgumentException in the test

350af1e

DmitryLukyanov requested a review from BorisDog January 19, 2023 23:54

BorisDog approved these changes Jan 20, 2023

View reviewed changes

DmitryLukyanov added 3 commits January 21, 2023 04:34

Update libmongocrypt version.

44f1dd2

Merge branch 'master' into csharp4182

c88e18b

PackagingTests.

a75cc5d

DmitryLukyanov added 5 commits January 21, 2023 05:04

Fix merge conflict.

6ee08db

Use stable libmongocrypt release.

ec29ae6

Skip some tests.

8d6a6e0

Fix tests.

7d6af27

Revert skipping tests on macos

3e36e9c

DmitryLukyanov merged commit 9ee046b into mongodb:master Jan 23, 2023

dnickless pushed a commit to dnickless/mongo-csharp-driver that referenced this pull request Aug 24, 2023

CSHARP-4182: Support for Range Indexes. (mongodb#988)

5a7f8ac

CSHARP-4182: Support for Range Indexes. #988

CSHARP-4182: Support for Range Indexes. #988

Conversation

DmitryLukyanov commented Dec 12, 2022

Choose a reason for hiding this comment

DmitryLukyanov Jan 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BorisDog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BorisDog left a comment

Choose a reason for hiding this comment

DmitryLukyanov Jan 9, 2023 •

edited