-
Notifications
You must be signed in to change notification settings - Fork 256
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat(NODE-3740): Implement root and top level key utf-8 validation settings for BSON #472
Conversation
…f8s at different levels
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.
Some initial feedback!
Side note: can you run npm install
and push the package-lock changes, looks like its not sync-ed up with main from the last release.
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.
whoops accidentally cleared my review state
I'm going to try and investigate the strange rollup issue in CI tomorrow, but we can manually verify this works as expected in the browser if it comes to it. |
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.
Still working on reviewing the rest of the code, but wanted to capture this small suggestion
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.
Finished reviewing the code, still need to double check the tests
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.
Just a couple of things left to clean up
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.
LGTM!!!
Description
Adds a new validation option in DeserializeOptions that allows all or a specific subset of top-level keys in a document to be excluded from strict utf-8 validation and instead applies the replacement character behavior. Tests are added to make sure that both global and specific-key subsets can have utf8 validation enabled and disabled.
What is the motivation for this change?
Default behavior is to have strict utf-8 validation (an error will be thrown if invalid utf8 is detected). This change gives users the ability to opt into character replacement behavior instead of strict utf-8 validation.
Double check the following
npm run lint
script<type>(NODE-xxxx)<!>: <description>