Repository navigation
Conversation
…umberLong "NaN"
EJSON.stringify accepted a Date whose time value is not finite and wrote it as a $numberLong
holding the string "NaN", which is not valid Extended JSON and which this library's own parser
refuses:
EJSON.stringify({ d: new Date(NaN) }) // {"d":{"$date":{"$numberLong":"NaN"}}}
EJSON.parse('{"d":{"$date":{"$numberLong":"NaN"}}}')
// BSONError: $numberLong string "NaN" is in an invalid format
so the two halves of the codec disagreed about what can be represented. The legacy modes were
worse: they reached toISOString and threw a RangeError out of the implementation.
serializeValue reaches the numeric branch for any date outside 1970-9999, and NaN is outside it,
so the string came from getTime().toString(). Reject a non-finite time value before the mode is
consulted, which covers the default, relaxed and legacy paths in one place.
Two cases in test/node/extended_json.test.ts check that all five option combinations reject an
invalid Date and that valid dates still serialize exactly as the neighbouring tests expect.
Contributor
|
@PerryLink , thanks for the detailed PR! This is being tracked in NODE-7892 and the team will prioritize it in our next triage session. |
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
EJSON.stringifyaccepts aDatewhose time value is not finite and writes it as a$numberLongthat holds the string"NaN". That is not valid Extended JSON, and this library's own parser refuses it, so the two halves of the codec disagree about what can be represented:The legacy modes are worse: they reach
getISOString, andtoISOString()throws aRangeErrorout of the implementation rather than aBSONError.Why
serializeValuedecides between an ISO string and a numeric timestamp from whether the time value falls in 1970-9999:NaNis not in that range, so an invalid Date takes the numeric branch and the string comes fromNaN.toString(). The range check is doing two jobs: choosing a representation, and implicitly deciding whether the date is representable at all.The change
An invalid Date is rejected before the mode is consulted, so the default,
relaxed,relaxed: false,legacyandlegacy, relaxed: falsepaths all report the same thing:BSONErroris what the parser already throws for the document this used to produce, and what the rest of the repository throws for values it cannot represent.Tests
Two cases in
test/node/extended_json.test.ts, next to the existing date cases, check that all five option combinations reject an invalid Date, and that valid dates still serialize to exactly the strings the neighbouring tests expect:Without the change the first one fails:
Full node suite with the change: 4601 passing, 36 pending, 1 failing. The one failure is
Code … when it contains multiline code, which also fails on the unmodified checkout: it compares a fixture containing\r\nagainst an expected string with\n, so it is a line-ending artifact of my Windows checkout rather than something this change touches. I verified that by stashing the source change and running the case on its own.Out of scope
BSON.serialize({ d: new Date(NaN) })does not throw; it writes a date field with a NaN payload that reads back asnew Date(0). That is a separate asymmetry in the BSON path, so it is left alone here.There is no NODE ticket for this. If you would like one, point me at it and I will rename the commit to match.