-
-
Notifications
You must be signed in to change notification settings - Fork 513
[Encryption] Refactor encrypted fields map generator #2783
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Encryption] Refactor encrypted fields map generator #2783
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Refactors the encrypted fields map generator into a more flexible EncryptedFieldsMapGenerator
, adds a method to produce maps across all metadata, and updates dependent code and tests.
- Renames and splits
EncryptionFieldMap
intoEncryptedFieldsMapGenerator
with two public methods. - Introduces
getEncryptedFieldsMap
to generate maps for all document classes, and adds a recursion guard. - Updates
SchemaManager
andClassMetadata
, removes old tests, and adds new tests for the generator.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
tests/Doctrine/ODM/MongoDB/Tests/Tools/EncryptionFieldMapTest.php | Removed outdated tests for the old generator. |
tests/Doctrine/ODM/MongoDB/Tests/Tools/EncryptedFieldsMapGeneratorTest.php | Added comprehensive tests for the new generator methods. |
lib/Doctrine/ODM/MongoDB/Utility/EncryptedFieldsMapGenerator.php | Renamed class, added getEncryptedFieldsMap and recursion guard. |
lib/Doctrine/ODM/MongoDB/SchemaManager.php | Switched to use EncryptedFieldsMapGenerator methods. |
lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php | Added isDocument() helper used by the generator. |
Comments suppressed due to low confidence (1)
lib/Doctrine/ODM/MongoDB/Utility/EncryptedFieldsMapGenerator.php:87
- The
@todo
indicates that keyId resolution is pending. If this is intentional, consider documenting the placeholder or implement the key extraction to avoid confusion later.
$mapping['encrypt'] ??= []; // @todo get the keyId
lib/Doctrine/ODM/MongoDB/Utility/EncryptedFieldsMapGenerator.php
Outdated
Show resolved
Hide resolved
lib/Doctrine/ODM/MongoDB/Utility/EncryptedFieldsMapGenerator.php
Outdated
Show resolved
Hide resolved
yield from $this->createEncryptedFieldsMapForClass( | ||
$embedMetadata, | ||
$path . $mapping['name'] . '.', | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of the class property $generatorStack
, I would use a method parameter so that the scope is clear and you don't have to unset the class once it's handled.
yield from $this->createEncryptedFieldsMapForClass( | |
$embedMetadata, | |
$path . $mapping['name'] . '.', | |
); | |
yield from $this->createEncryptedFieldsMapForClass( | |
$embedMetadata, | |
$path . $mapping['name'] . '.', | |
$visitedClasses + [$classMetadata->getName() => true], | |
); |
As an improvement: using an int
level counter instead of true
, we can set a limit in the recursion deep (if necessary).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the recursion prevention to a method parameter. Not sure how many recursion levels would be ok, but for the time being I'll prevent any recursion.
e26f275
into
doctrine:feature/queryable-encryption
Refactors the encrypted field map generator and adds a method to generate field maps for all metadata. Note that testing this is difficult, as loading all metadata in tests causes errors due to some documents being invalid on purpose.