-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
feat(NODE-6507): generate encryption configuration on mongoose connect #15320
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
Merged
vkarpov15
merged 16 commits into
Automattic:csfle
from
mongodb-js:schema-maps-auto-generate
Apr 15, 2025
Merged
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
db8eef7
Add support for encrypted models and discriminators
baileympearson f1c986c
misc test cleanups
baileympearson 682fb11
update setup script
baileympearson cda0e26
misc cleanup
baileympearson edfbdfa
documentation
baileympearson 1a48b98
doc comments
baileympearson 065bc99
use object fromentries
baileympearson a8f37eb
fix lint in markdown file
baileympearson 53d31a6
throw errors when discriminators have duplicate keys
baileympearson 888c0a8
Remove table testing in favor of hard-coded it blocks
baileympearson 5d3b51f
consistent capitalization in docs
baileympearson 2c02d45
add missing configuration
baileympearson ecb3f7c
Add support for encrypted discriminators
baileympearson b7016de
add test for default mongoose connection
baileympearson 14a40ff
add test for default mongoose connection
baileympearson 59af7cf
fix discriminator logic
baileympearson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Potential edge case here: discriminator base schema defines nested path, discriminator child schema defines subdocument with same path but different options.
schema.eachpath()
doesn't account for subdocuments because subdocuments have a distinct schema (it does account for nested paths though because nested paths do not have their own schema)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.
Good catch - I wrote a test for this scenario and you were right.
I've been trying out some different approaches and I can't find a better solution than something like this:
This generates all possible paths in the schema (the above doesn't handle arrays, but encryption on fields in arrays isn't supported so that's out of scope here). Not my first choice, because this feels brittle. I'll keep looking at it but
allPaths
exist somewhere?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.
I don't think recursively checking all paths is necessary here because you just want to find conflicts in the top-level paths. So if discriminator schema has a path
pathname
with an encrypted field, and root schema has a nested path withrootSchema.nested[pathname.split('.')[0]]
, you can already call that a conflict and throw an error. Similarly, if discriminator schema has a nested pathpathname
but in root schema you haverootSchema.paths[pathname]
then you can also throw an error.Uh oh!
There was an error while loading. Please reload this page.
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.
Not necessarily, right? That would throw an error in scenarios where the child schema provides a subdocument with the same root path but doesn't modify the encrypted path. ex:
I'd expect this to be fine, because there isn't a conflicting path for
name.first
.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.
In the case you described,
name: new Schema({ age: Number })
would actually overridename: { first: { type: String } }
, so the discriminator schema would not have aname.first
property at all. Discriminators merge nested paths from root schema, but subdocuments override because subdocuments can have middleware.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.
I tried the approach of determining conflicting base paths, as you suggested but I ran into two complications:
I decided that the two above points complicated the approach. The changes in this PR were much simpler for me to work with.. This approach works by:
Let me know what you think - I can rework it if you would prefer a different approach.
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.
I agree, this is a reasonable approach. This approach is more restrictive, but simpler implementation. If it proves to be too restrictive, we can come up with a more flexible approach.