PHPLIB-913: Database::createEncryptedCollection() helper #1050

jmikola · 2023-03-20T15:39:54Z

https://jira.mongodb.org/browse/PHPLIB-913

tests/SpecTests/ClientSideEncryption/Prose21_AutomaticDataEncryptionKeysTest.php

src/Database.php

tests/SpecTests/ClientSideEncryption/FunctionalTestCase.php

src/Database.php

tests/SpecTests/ClientSideEncryption/Prose21_AutomaticDataEncryptionKeysTest.php

jmikola · 2023-03-31T15:45:05Z

docs/reference/method/MongoDBDatabase-createEncryptedCollection.txt

+   purposes. It is not yet recommended for production deployments as breaking
+   changes may be introduced. See the
+   `Queryable Encryption Preview <https://www.mongodb.com/blog/post/mongodb-releases-queryable-encryption-preview/>`_
+   blog post for more information.


I realized I never added this note to the previous Queryable Encryption APIs we introduced in 1.13. I opened PHPLIB-1095 and #1053 to address that.

jmikola · 2023-03-31T16:13:37Z

src/Database.php

+        $server = select_server($this->manager, $options);
+
+        try {
+            $operation->createDataKeys($clientEncryption, $kmsProvider, $masterKey, $encryptedFields);


I originally implemented two operation classes:

CreateCollectionForQueryableEncryption for createCollection() with an encryptedFields option, which composed several CreateCollection operations and one CreateIndexes operation.

CreateEncryptedCollection for this helper, which composed a CreateCollectionForQueryableEncryption.

In that approach, CreateEncryptedCollection::execute() created data keys before calling CreateCollectionForQueryableEncryption::execute() and contained the logic for CreateEncryptedCollectionException. The downside was updating the options for the inner-most CreateCollection operation when encryptedFields changed.

I also wasn't too happy with the naming for both operations. It made sense for CreateEncryptedCollection to match the helper method, but it was only responsible for creating data keys. CreateCollectionForQueryableEncryption was actually creating an encrypted collection.

I then combined both operations into CreateEncryptedCollection and moved the key generation logic to a separate createDataKeys() method to be called prior to execute(). This made it easier to reconstruct the inner CreateCollection operation.

Using a pass-by-reference parameter to expose modifications to encryptedFields allowed me to avoid dealing with CreateEncryptedCollectionException in the operation class entirely. I briefly explored returning encryptedFields and throwing the exception when needed, and it made both createDataKeys() and the helper's try/catch more complicated.

jmikola · 2023-03-31T16:18:25Z

src/Database.php

+            $result = $operation->execute($server);
+
+            return [$result, $encryptedFields];
+        } catch (Throwable $e) {


I considered limiting this to \Exception, but that would have required silencing a phpcs warning. I think it's unlikely we'd ever get an internal PHP error here, but there's no harm in wrapping it.

This is unlike UnifiedSpecTest.php, which has a specific reason to avoid catching errors.

jmikola · 2023-03-31T16:25:12Z

src/Database.php

+
+            return [$result, $encryptedFields];
+        } catch (Throwable $e) {
+            throw new CreateEncryptedCollectionException($e, $encryptedFields ?? []);


$encryptedFields ?? [] satisfies two psalm warnings:

PossiblyUndefinedVariable: Possibly undefined variable $encryptedFields defined in try block (see https://psalm.dev/018)

PossiblyNullArgument: Argument 2 of CreateEncryptedCollectionException::__construct cannot be null, possibly null value provided (see https://psalm.dev/078)

In practice, I don't think that's possible since it is assigned in the first line of createDataKeys() and we've already verified that the option exists in the operation constructor.

I'm open to any feedback, but this seemed like a harmless, defensive fix.

jmikola · 2023-03-31T16:28:45Z

src/Operation/CreateEncryptedCollection.php

+namespace MongoDB\Operation;
+
+// phpcs:disable SlevomatCodingStandard.Namespaces.UnusedUses.UnusedUse
+use MongoDB\BSON\Binary;


phpcs does not parse @psalm-var annotations, and using @var instead would produce a difference error:

Invalid inline documentation comment format @var array{fields: list<array{keyId: ?Binary}|object{keyId: ?Binary}>}, expected @var type $variable

jmikola · 2023-03-31T16:29:32Z

src/Operation/CreateEncryptedCollection.php

+    private $options;
+
+    /**
+     * @see CreateCollection::__construct() for supported options


I can't tell you how great it felt to write an operation constructor doc block with option documentation copypasta.

jmikola · 2023-03-31T16:34:46Z

src/Operation/CreateEncryptedCollection.php

+                continue;
+            }
+
+            $field = (array) $field;


The earliest version of this logic relied on recursive_copy() to ensure we didn't inadvertently modify an object still referenced by the caller. I eventually realized that array casting here would allow me to avoid that risk and eliminate an extra code path I had for $field being an object.

jmikola · 2023-03-31T16:37:24Z

src/Operation/CreateEncryptedCollection.php

+            }
+        }
+
+        $this->options['encryptedFields'] = $encryptedFields;


Technically, this is only necessary if one or more data keys were created in the preceding foreach; however, the optimization didn't seem necessary (this is hardly a hot code path).

jmikola · 2023-03-31T16:41:09Z

tests/Operation/CreateEncryptedCollectionFunctionalTest.php

+    /** @var ClientEncryption */
+    private $clientEncryption;
+
+    public function setUp(): void


Most of this is lifted from the CSFLE prose tests. Copying this over reminded me of our previous discussion about using traits (vs. piling more onto the base FunctionalTestCase class), but that's something for a future PR.

alcaeus

LGTM - feel free to apply or ignore the suggested patches at your own discretion.

I like the way CreateEncryptedCollection is structured, and that people don't necessarily need to call $db->createEncryptedCollection to create an encrypted collection but can rely on createCollection to do the right thing. 🎉

src/Operation/CreateEncryptedCollection.php

Co-authored-by: Andreas Braun <[email protected]>

jmikola · 2023-04-12T04:00:13Z

Coding standard CI failure is due to PHPLIB-1106 and will be addressed by #1058.

The CI build failures for mongodb-latest are due to PHPLIB-1088 and will be addressed in a subsequent PR.

jmikola requested a review from alcaeus March 21, 2023 04:56

jmikola commented Mar 21, 2023

View reviewed changes

tests/SpecTests/ClientSideEncryption/Prose21_AutomaticDataEncryptionKeysTest.php Show resolved Hide resolved

jmikola force-pushed the phplib-913 branch from 57aeac1 to 6784ad6 Compare March 21, 2023 13:47

alcaeus requested changes Mar 22, 2023

View reviewed changes

jmikola marked this pull request as ready for review March 22, 2023 09:28

jmikola force-pushed the phplib-913 branch from 5defe11 to 3e60d11 Compare March 22, 2023 09:28

jmikola added 13 commits March 31, 2023 23:11

PHPLIB-913: Database::createEncryptedCollection() helper

6d76fba

Docs for Database::createEncryptedCollection()

14d6a26

Revise docs and require $options since encryptedFields is required

89d03f4

Revise docs and exception output

e78f478

Add example for creating new Client with auto encryption

c02d343

Fix exception handling and assertions in invalid keyId test

390299f

Permit coding standard deviations in CSFLE prose tests

5107633

Fix psalm errors in createEncryptedCollection()

0ba8a28

Use more specific type hint in data provider

d244f07

Remove typed property, which requires PHP 7.4+

27dca00

Extract Database methods to CreateEncryptedCollection operation

8d4eeb1

Fix references in function doc blocks

521855d

Update docs for createEncryptedCollection()

e5d736e

jmikola force-pushed the phplib-913 branch from ca616ed to e5d736e Compare March 31, 2023 15:11

Functional tests for CreateEncryptedCollection::createDataKeys()

801784d

jmikola commented Mar 31, 2023

View reviewed changes

jmikola added 4 commits April 1, 2023 00:43

Require replica sets for CreateEncryptedCollectionFunctionalTest

f334816

Re-order CreateEncryptedCollection methods

8dd84f4

Fix class reference in CreateEncryptedCollectionExceptionTest

7c4c13c

Add reference links to CreateCollection operation docs

28b86e1

This was referenced Apr 3, 2023

Update docs for encryptedFields create/drop collection option #1054

Merged

Extract DropEncryptedCollection operation from helper methods #1055

Merged

Test that createDataKey() doesn't modify encryptedFields object option

645fdee

alcaeus approved these changes Apr 11, 2023

View reviewed changes

src/Operation/CreateEncryptedCollection.php Outdated Show resolved Hide resolved

src/Operation/CreateEncryptedCollection.php Outdated Show resolved Hide resolved

jmikola and others added 2 commits April 11, 2023 03:25

Apply suggestions from code review

8f088e2

Co-authored-by: Andreas Braun <[email protected]>

Merge branch 'master' into phplib-913

7c7b0a9

jmikola merged commit b963821 into mongodb:master Apr 12, 2023

jmikola deleted the phplib-913 branch April 12, 2023 04:03

PHPLIB-913: Database::createEncryptedCollection() helper #1050

PHPLIB-913: Database::createEncryptedCollection() helper #1050

Uh oh!

Conversation

jmikola commented Mar 20, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

alcaeus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jmikola commented Apr 12, 2023

Uh oh!

Uh oh!