Skip to content

fix(NODE-7436): EJSON.stringify type signature #870

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.

Sign up for GitHub

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

Open
chdanielmueller wants to merge 4 commits into
base: main
Choose a base branch
from
+222 −11
Open

fix(NODE-7436): EJSON.stringify type signature #870

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d46b85f
Tests to proof NODE-7436
chdanielmueller Feb 10, 2026
6d5b35b
Improve signature for typescript and add missing functionality
chdanielmueller Feb 10, 2026
e2dd8fd
Replace with more complete tests
chdanielmueller Feb 10, 2026
cc24320
Linting
chdanielmueller Feb 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 73 additions & 11 deletions src/extended_json.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,30 @@ export type EJSONParseOptions = EJSONOptionsBase & {
/** @public */
export type EJSONOptions = EJSONSerializeOptions & EJSONParseOptions;

/** @internal */
function isEJSONSerializeOptions(value: unknown): value is EJSONSerializeOptions {
if (value == null || typeof value !== 'object') {
return false;
}

// Check that all properties, if present, are of the correct type
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;
}
Comment on lines +68 to +76
Copy link
Member

@tadjik1 tadjik1 Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Check that there are no invalid properties (only known EJSON serialize options)
const validKeys = ['legacy', 'relaxed', 'ignoreUndefined'];
Copy link
Member

@tadjik1 tadjik1 Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const keys = Object.keys(value);

return keys.every(key => validKeys.includes(key));
}

/** @internal */
type BSONType =
| Binary
Expand Down Expand Up @@ -443,6 +467,7 @@ function parse(text: string, options?: EJSONParseOptions): any {
});
}

/* eslint-disable @typescript-eslint/no-explicit-any */
/**
* Converts a BSON document to an Extended JSON string, optionally replacing values if a replacer
* function is specified or optionally including only the specified properties if a replacer array
Expand All @@ -464,28 +489,65 @@ function parse(text: string, options?: EJSONParseOptions): any {
*
* // prints '{"int32":10}'
* console.log(EJSON.stringify(doc));
*
* // prints '{"int32":{"$numberInt":"10"}}' with 2 space indentation
* console.log(EJSON.stringify(doc, { relaxed: false }, 2));
* ```
*/
function stringify(
Copy link
Member

@tadjik1 tadjik1 Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// eslint-disable-next-line @typescript-eslint/no-explicit-any
value: any,
replacer?:
replacer?: (number | string)[] | ((this: any, key: string, value: any) => any) | null,
space?: string | number,
options?: EJSONSerializeOptions
): string;
function stringify(
value: any,
replacer?: (number | string)[] | ((this: any, key: string, value: any) => any) | null,
options?: EJSONSerializeOptions
): string;
function stringify(value: any, options?: EJSONSerializeOptions, space?: string | number): string;
function stringify(
value: any,
replacerOrOptions?:
| (number | string)[]
// eslint-disable-next-line @typescript-eslint/no-explicit-any
| ((this: any, key: string, value: any) => any)
| null
| EJSONSerializeOptions,
space?: string | number,
spaceOrOptions?: string | number | EJSONSerializeOptions,
options?: EJSONSerializeOptions
): string {
if (space != null && typeof space === 'object') {
options = space;
space = 0;
}
if (replacer != null && typeof replacer === 'object' && !Array.isArray(replacer)) {
options = replacer;
let replacer:
| ((this: any, key: string, value: any) => any)
| (number | string)[]
| null
| undefined;
let space: string | number | undefined;
/* eslint-enable @typescript-eslint/no-explicit-any */

// Check if second parameter is options
if (isEJSONSerializeOptions(replacerOrOptions)) {
options = replacerOrOptions;
replacer = undefined;
space = 0;
Copy link
Member

@tadjik1 tadjik1 Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the original bug and removing just this line (avoid unconditional discarding the space value( would make everything work as expected.

space =
typeof spaceOrOptions === 'number' || typeof spaceOrOptions === 'string'
? spaceOrOptions
: undefined;
}
// Check if third parameter is options
else if (isEJSONSerializeOptions(spaceOrOptions)) {
options = spaceOrOptions;
replacer = replacerOrOptions;
space = undefined;
}
// Standard case: replacer, space, options
else {
replacer = replacerOrOptions;
space =
typeof spaceOrOptions === 'number' || typeof spaceOrOptions === 'string'
? spaceOrOptions
: undefined;
}

const serializeOptions = Object.assign({ relaxed: true, legacy: false }, options, {
seenObjects: [{ propertyName: '(root)', obj: null }]
});
Expand Down
149 changes: 149 additions & 0 deletions test/node/extended_json.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -733,4 +733,153 @@ describe('Extended JSON', function () {
expect(() => EJSON.stringify(input)).to.throw(BSONError);
});
});

context('stringify: Parameter signature combinations', function () {
Copy link
Member

@tadjik1 tadjik1 Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const testDoc = {
objectId: ObjectId.createFromHexString('111111111111111111111111'),
int32Number: 300,
name: 'test'
};

it('should work with (value) - only value parameter', function () {
const result = EJSON.stringify(testDoc);
expect(result).to.equal(
'{"objectId":{"$oid":"111111111111111111111111"},"int32Number":300,"name":"test"}'
);
});

it('should work with (value, null, space) - replacer null, space number', function () {
const result = EJSON.stringify(testDoc, null, 2);
expect(result).to.equal(`{
"objectId": {
"$oid": "111111111111111111111111"
},
"int32Number": 300,
"name": "test"
}`);
});

it('should work with (value, null, space, options) - replacer null, space, options', function () {
const result = EJSON.stringify(testDoc, null, 2, { relaxed: false });
expect(result).to.equal(`{
"objectId": {
"$oid": "111111111111111111111111"
},
"int32Number": {
"$numberInt": "300"
},
"name": "test"
}`);
});

it('should work with (value, array, space) - replacer array, space', function () {
const result = EJSON.stringify(testDoc, ['objectId', '$oid', 'name'], 2);
expect(result).to.equal(`{
"objectId": {
"$oid": "111111111111111111111111"
},
"name": "test"
}`);
});

it('should work with (value, array, space, options) - replacer array, space, options', function () {
const result = EJSON.stringify(testDoc, ['objectId', '$oid'], 2, { relaxed: false });
expect(result).to.equal(`{
"objectId": {
"$oid": "111111111111111111111111"
}
}`);
});

it('should work with (value, function, space) - replacer function, space', function () {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const replacer = function (key: string, value: any) {
return key === 'name' ? undefined : value;
};
const result = EJSON.stringify(testDoc, replacer, 2);
expect(result).to.equal(`{
"objectId": {
"$oid": "111111111111111111111111"
},
"int32Number": 300
}`);
});

it('should work with (value, function, space, options) - replacer function, space, options', function () {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const replacer = function (key: string, value: any) {
return key === 'name' ? undefined : value;
};
const result = EJSON.stringify(testDoc, replacer, 2, { relaxed: false });
expect(result).to.equal(`{
"objectId": {
"$oid": "111111111111111111111111"
},
"int32Number": {
"$numberInt": "300"
}
}`);
});

it('should work with (value, null, options) - replacer null, options', function () {
const result = EJSON.stringify(testDoc, null, { relaxed: false });
expect(result).to.equal(
'{"objectId":{"$oid":"111111111111111111111111"},"int32Number":{"$numberInt":"300"},"name":"test"}'
);
});

it('should work with (value, array, options) - replacer array, options', function () {
const result = EJSON.stringify(testDoc, ['objectId', '$oid'], { relaxed: false });
expect(result).to.equal('{"objectId":{"$oid":"111111111111111111111111"}}');
});

it('should work with (value, function, options) - replacer function, options', function () {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const replacer = function (key: string, value: any) {
return key === 'int32Number' ? undefined : value;
};
const result = EJSON.stringify(testDoc, replacer, { relaxed: false });
expect(result).to.equal('{"objectId":{"$oid":"111111111111111111111111"},"name":"test"}');
});

it('should work with (value, options) - value and options only', function () {
const result = EJSON.stringify(testDoc, { relaxed: false });
expect(result).to.equal(
'{"objectId":{"$oid":"111111111111111111111111"},"int32Number":{"$numberInt":"300"},"name":"test"}'
);
});

it('should work with (value, options, space) - value, options, space (second overload)', function () {
const result = EJSON.stringify(testDoc, { relaxed: false }, 2);
expect(result).to.equal(`{
"objectId": {
"$oid": "111111111111111111111111"
},
"int32Number": {
"$numberInt": "300"
},
"name": "test"
}`);
});

it('should work with string space parameter', function () {
const result = EJSON.stringify(testDoc, null, '\t', { relaxed: false });
expect(result).to.equal(`{
\t"objectId": {
\t\t"$oid": "111111111111111111111111"
\t},
\t"int32Number": {
\t\t"$numberInt": "300"
\t},
\t"name": "test"
}`);
});

it('should work with space 0 (no formatting)', function () {
const result = EJSON.stringify(testDoc, null, 0, { relaxed: false });
expect(result).to.equal(
'{"objectId":{"$oid":"111111111111111111111111"},"int32Number":{"$numberInt":"300"},"name":"test"}'
);
});
});
});
Loading