PHPLIB-878: Spec tests for queryable encryption range indexes #1064
Conversation
jmikola
force-pushed
the
phplib-878
branch
3 times, most recently
from
bd4352b to
1cfa754
Compare
| $encryptedFields = Document::fromJSON(file_get_contents(__DIR__ . '/../client-side-encryption/etc/data/range-encryptedFields-' . $type . '.json')); | ||
|
|
||
| $database = $this->encryptedClient->selectDatabase($this->getDatabaseName()); | ||
| $database->dropCollection('explicit_encryption', ['encryptedFields' => $encryptedFields]); |
There was a problem hiding this comment.
Why do you need to set the encryptedFields when dropping the collection?
There was a problem hiding this comment.
This is mentioned in "Test Setup" section of 22. Range Explicit Encryption but it's not explained in detail.
In addition to the main collection, queryable encryption uses additional metadata collections. The driver's createCollection() helper makes those when an encryptedFields option is provided. If an encryptedFields option is passed to the drop helper, it will additionally drop those metadata collections. The notable difference is that encryptedFields is actually an option on the create command, but not drop. The option here is entirely used client-side to tell PHPLIB to clean up additional collections. Drop Collection Helper in the CSFLE spec has more detail on that.
|
Note: test failures on mongodb-latest are due to PHPLIB-1071 and PHPLIB-1115. |
jmikola
force-pushed
the
phplib-878
branch
2 times, most recently
from
62dec14 to
3469567
Compare
Most operations that use ASSERT_SAME_DOCUMENT and ASSERT_SAME_DOCUMENTS should technically use ASSERT_MATCHES_DOCUMENT and ASSERT_DOCUMENTS_MATCH, respectively; however, this is the minimum change needed to satisfy new CSFLE spec tests. Related to dc1bb17
Decimal tests are skipped until bundled libmongocrypt can link a Decimal128 library. Long tests are skipped pending a workaround for preserving Int64 types in spec tests.
tests/SpecTests/ClientSideEncryption/Prose22_RangeExplicitEncryptionTest.php
Show resolved
Hide resolved
tests/SpecTests/ClientSideEncryption/Prose22_RangeExplicitEncryptionTest.php
Show resolved
Hide resolved
|
|
||
| public function tearDown(): void | ||
| { | ||
| $this->collection = null; |
There was a problem hiding this comment.
We don't free the Collection instance in other test cases. Did you hit an issue?
There was a problem hiding this comment.
Since encryptedClient is created with disableClientPersistence, I originally did this to ensure that libmongoc's mongoc_client_t would be destroyed at the end of the test. This has been done before, but usually for session objects to ensure they are returned to the internal pool.
I just tested this locally and can confirm that without this, the Manager objects and mongoc_client_t instances accumulate and are not freed until the end of the test suite (not just the test case). I opened PHPLIB-1154 to track this.
In the meantime, I've added a comment to explain this and removed the unsetting of key1Id, which wasn't necessary.
| @@ -723,7 +723,7 @@ private function getResultAssertionTypeForCollection() | |||
| return ResultExpectation::ASSERT_NOTHING; | |||
| } | |||
|
|
|||
| return ResultExpectation::ASSERT_SAME_DOCUMENTS; | |||
| return ResultExpectation::ASSERT_DOCUMENTS_MATCH; | |||
There was a problem hiding this comment.
If I understand correctly, you're making the assertion less strict, what's the reason?
There was a problem hiding this comment.
This is explained in the commit message for 42bce4e. To quote:
Allow extra fields in aggregate results
Most operations that use ASSERT_SAME_DOCUMENT and ASSERT_SAME_DOCUMENTS should technically use ASSERT_MATCHES_DOCUMENT and ASSERT_DOCUMENTS_MATCH, respectively; however, this is the minimum change needed to satisfy new CSFLE spec tests. Related to dc1bb17.
assertSameDocuments() does not permit extra fields in the actual value, while assertDocumentsMatch() does (see: DocumentsMatchConstraint).
The CSFLE legacy test runner inherits rules from the Transactions legacy test runner, which in turn inherits document matching rules from the CRUD v1 runner -- which is where extra fields are permitted, among other things.
Per the commit message above, more operations should be using the same logic here but I wanted to be conservative and avoid making extra changes to the legacy test runner (looking forward to the day we can just delete this code after porting everything to the unified format). No objections if you'd like to create a ticket to look into this at some point.
|
There is one unrelated task failure. I opened PHPLIb-1155 to track that. |
https://jira.mongodb.org/browse/PHPLIB-878
https://jira.mongodb.org/browse/PHPLIB-1110
Depends on mongodb/mongo-php-driver#1409