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.
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
feat(NODE-4874): support EJSON parse for BigInt from $numberLong #552
feat(NODE-4874): support EJSON parse for BigInt from $numberLong #552
Changes from 16 commits
75a6207
7cd7f6c
df97552
07904f9
d0dd23d
5120ca5
d67757d
4f15e42
2f5db05
822dff8
5666255
79f0c81
260b667
6a5364a
7eca3ea
f6aed3d
b0e67aa
4361871
92ae357
fff3d32
a86a942
d9f539f
0acf248
cdf0334
027b5df
a02236f
31f8bf7
f22ce03
8b0a741
e0049b3
47cdb65
d502ddb
f80b707
c90b783
43b39c1
b99a141
a0b5cf0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Unlike in BSON and the 'number' case for EJSON, there isn't restrictions on value size or length.
Luckily BigInt casting function will throw an error for bad syntax unlike Number (yay!) (can we add a test?
{$numberLong: "blah!"}
)But we have added support here for syntax that should not work:
{ $numberLong: "0xA" }
should fail{ $numberLong: "0o7" }
should fail{ $numberLong: "0b1" }
should failAnd we've unintentionally allowed strings of any length to be provided here, I'm not sure what would be the best approach here, we can either figure out some way to assert some maximum length. Or we can construct a Long.fromString and use long.toBigInt here, asserting that the fromString logic would prevent an extremely long string from being converted.
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.
From discussion: We want to assert a maximum length on the string, this isn't the same as what Long does but our newer API can be better about preventing malformed input. There should be no leading zeros permitted but a plus or minus sign needs to be supported. We can iterate the string or use regexp to assert this.
Consider making a new BSONError subclassThere 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.
They are currently permitted when deserializing to
Long
, as far as I can tell. I would recommend to keep validation consistent between deserializing toLong
and deserializing tobigint
(and changing validation for deserializing toLong
would probably be breaking, but this might be exactly the right time for that if it’s something you want to do).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.
If we're all in agreement (@dariakp @baileympearson @durran @W-A-James) I think we can do the validation for both
bigint
andLong
cases, I was aiming to only impact the bigint case here since it would be opt-in, but useBigInt64 and increasing the validation are divergent concerns so we shouldn't overload the option's behavior.We certainly don't want to add support for the hex, oct, and bin formats just because useBigInt64 is turned on, so that assertion I think we're all on board with.
Long.fromString
currently allows a number of things that probably should be errors like"-----------123"
- the number of minus signs corresponds to the number of recursivefromString
calls (bigint will throw on this input)"00000000123"
- leading zeros are ignored"12.23"
- becomes 12'8'.repeat(25)
- too large, can be determined by string length (gets truncated)"9223372036854775808"
- too large int64.max + 1 (gets truncated)We can decide to apply all, some, or none of the above validations, I think the most valuable and least pedantic is applying a string length check, but they're all great if we want to improve things :)
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 that at the very least, we should add the string length check to inputs of
fromExtendedJSON
in both thebigInt
andLong
cases to keep with the canonical EJSON spec and to prevent hanging on strings that would be too long to be a valid 64-bit integer and may be the result of malicious user input.Canonical EJSON representation of a BSON.Long:
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.
Updating with results of Slack discussion;
Going to change bigint case to truncate rather than throw an error on encountering a bigint outside of int64 range to be consistent with behaviour of
Long.toString