feat(NODE-4874): support EJSON parse for BigInt from $numberLong

[W-A-James]

Description

Adding support to parse bigints from EJSON

Is there new documentation needed for these changes?

No

What is the motivation for this change?

Continue BigInt support improvement.

Double check the following

• Ran npm run lint script
• Self-review completed using the steps outlined here
• PR title follows the correct format: type(NODE-xxxx)[!]: description
  • Example: feat(NODE-1234)!: rewriting everything in coffeescript
• Changes are covered by tests
• New TODOs have a related JIRA ticket

[W-A-James]

Did this to clean up how default options are handled. It is not handled cleanly in a number of places in this file

[nbbeeken]

Starting with code change comments, looking at the tests now

[nbbeeken]

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 fail

And 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.

[nbbeeken]

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 subclass

[addaleax]

There should be no leading zeros permitted

They are currently permitted when deserializing to Long, as far as I can tell. I would recommend to keep validation consistent between deserializing to Long and deserializing to bigint (and changing validation for deserializing to Long would probably be breaking, but this might be exactly the right time for that if it’s something you want to do).

[nbbeeken]

If we're all in agreement (@dariakp @baileympearson @durran @W-A-James) I think we can do the validation for both bigint and Long 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 recursive fromString 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 :)

[W-A-James]

I agree that at the very least, we should add the string length check to inputs of fromExtendedJSON in both the bigInt and Long 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:

{"$numberLong": <64-bit signed integer as a string>}

[W-A-James]

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

[nbbeeken]

We can add a test that checks BigInt usage is all behind gated logic. Take a look at the example where we test the library with and without a crypto global here: https://github.com/mongodb/js-bson/blob/main/test/node/byte_utils.test.ts#L603

We can load the BSON library with BigInt set to undefined, that doesn't cover using trailing n syntax but it's something

[W-A-James]

Would it be appropriate to do this in another PR as just removing the BigInt field from the globalThis object isn't enough to be sure that our BigInt work doesn't break anything in environments that don't support BigInts. We'd have to go through the ES BigInt spec and make sure that we're not using any of the associated functionality and that seems outside the scope of this PR.

[nbbeeken]

sure, let's file a follow up