fix(NODE-7436): EJSON.stringify type signature#870
fix(NODE-7436): EJSON.stringify type signature#870chdanielmueller wants to merge 4 commits intomongodb:mainfrom
Conversation
|
@chdanielmueller , thanks so much for your contribution! The team is going to prioritize this PR and NODE-7436 at our next triage session. |
tadjik1
left a comment
There was a problem hiding this comment.
hi @chdanielmueller, thanks a lot for the contribution. The bug is real, well-documented, and the test coverage you added is great.
The core issue is that space = 0 discards the user's space value when options are passed in the replacer position.
However, the fix is significantly more complex than it needs to be. The entire bug can be fixed by deleting one line (space = 0). The existing code already treats any non-array object in replacer position as options.
I would suggest minimal fix:
if (replacer != null && typeof replacer === 'object' && !Array.isArray(replacer)) {
options = replacer;
replacer = undefined;
- space = 0
}The overloads, type guard, and restructured function body are unnecessary for this fix and introduce maintenance overhead.
| } | ||
|
|
||
| // Check that there are no invalid properties (only known EJSON serialize options) | ||
| const validKeys = ['legacy', 'relaxed', 'ignoreUndefined']; |
There was a problem hiding this comment.
This creates a maintenance footgun with to compiler warning to catch if EJSONSerializeOptions gains a new property, or if detection silently breaks. In this case the new property would be treated as a replacer and passed into JSON.stringify.
| if ('legacy' in value && typeof value.legacy !== 'boolean') { | ||
| return false; | ||
| } | ||
| if ('relaxed' in value && typeof value.relaxed !== 'boolean') { | ||
| return false; | ||
| } | ||
| if ('ignoreUndefined' in value && typeof value.ignoreUndefined !== 'boolean') { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The strict key check causes silent misbehavior on typos. If something like {relaxd: false} will be passed - this guard returns false and the object will silently be forwarded to JSON.stringify as a replacer - producing wrong output, the existing typeof replacer === 'object' && !Array.isArray(replacer) would correctly treat it as options. This is more forgiving and safer.
| }); | ||
| }); | ||
|
|
||
| context('stringify: Parameter signature combinations', function () { |
There was a problem hiding this comment.
The test coverage here is great, and these tests are valuable independent of which implementation approach is chosen. Please keep them even with a simplified fix.
| if (isEJSONSerializeOptions(replacerOrOptions)) { | ||
| options = replacerOrOptions; | ||
| replacer = undefined; | ||
| space = 0; |
There was a problem hiding this comment.
I think this is the original bug and removing just this line (avoid unconditional discarding the space value( would make everything work as expected.
| * console.log(EJSON.stringify(doc, { relaxed: false }, 2)); | ||
| * ``` | ||
| */ | ||
| function stringify( |
There was a problem hiding this comment.
These overloads add type-level documentation of supported call patterns, but they also add maintenance hurdle. Since the runtime checks are needed regardless, and the existing signature with the union type already communicates that options can go in the second position - this doesn't provide enough value to justify the complexity.
Description
Summary of Changes
This pull request enhances the flexibility and robustness of the
EJSON.stringifyfunction by supporting multiple parameter signature combinations, similar toJSON.stringify.It introduces a new internal type guard to accurately distinguish between options and replacer parameters, and adds comprehensive tests to ensure correct behavior across all supported overloads.
It supports a previously advertised function signature which was not implemented.
EJSON.stringify(object, { relaxed: false }, 2)Notes for Reviewers
The tests should contain every previously possible and also previously advertised but not working combination of inputs.
The changes within the function have to do with the previous typescript signature which has been used but did not work. See
js-bson/test/bench/etc/generate_documents.ts
Line 27 in 0712bb1
Off Topic: Documented within NODE-7437
Release Highlight
Release notes highlight
Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript