Skip to content
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

fix(NODE-3821): nullish check before using toBSON override function #477

Merged
merged 8 commits into from Jan 4, 2022
Merged

fix(NODE-3821): nullish check before using toBSON override function #477

merged 8 commits into from Jan 4, 2022

Conversation

cplepage
Copy link
Contributor

Description

In the serializer, when it comes to check for an override value (toBSON), changing the value check to compare with null and undefined instead of relying on the truthiness of value.

What is the motivation for this change?

Cannot extend BigInt prototype with.toBSON() since 0n === false

trying to make this work

BigInt.prototype.toBSON = function() {
    return Long.fromBits(Number(this.valueOf() & BigInt(0xffffffff)),
        Number(this.valueOf() >> BigInt(32)))
}

// src/parser/serializer.ts:769
// Is there an override value
if(value && value.toBSON){
    // this won't go through if value === 0n
    ...
}

...

// then trows error here since value hasn't been overidden
// src/parser/serializer.ts:779
else if (typeof value === 'bigint') {
    throw new BSONTypeError('Unsupported type BigInt, please use Decimal128');
}

@nbbeeken nbbeeken changed the title Value check with null and undefined instead of truthiness fix(NODE-3821): nullish check before using toBSON override function Dec 17, 2021
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

Hi @cplepage, thanks so much for contributing here! I think this is something we would certainly want to fix. I've gone ahead and created a JIRA ticket to track the work and I have some suggestions of improvements for ya. If you get the chance adding a test of your usecase to: test/node/to_bson_test.js would be a great help.

@dariakp @durran @baileympearson, Just wanna get y'all's attention here I'm suggesting we drop the throw case with toBSON not being of type 'function'. While very unlikely it seems wrong that our BSON library would not allow the document: { toBSON: 1 } to be serialized. That being said we could easily keep it, and revisit that at a later time.

src/parser/serializer.ts Outdated Show resolved Hide resolved
src/parser/serializer.ts Outdated Show resolved Hide resolved
cplepage and others added 3 commits December 19, 2021 23:15
Co-authored-by: Neal Beeken <neal.beeken@mongodb.com>
Co-authored-by: Neal Beeken <neal.beeken@mongodb.com>
@cplepage
Copy link
Contributor Author

cplepage commented Dec 20, 2021

@nbbeeken good idea on the optional chaining, looks clean!
I added a test on the current particular case. Let me know your thoughts

@nbbeeken
Copy link
Contributor

Hey @cplepage do you mind if I push up some changes of my own here? due to our support for older node versions the testing arrangement can be kinda strange, I can make sure we still get bigint coverage on later versions of node 🙂

@cplepage
Copy link
Contributor Author

@nbbeeken please go ahead! Tried a few things for the tests, but CI never passed

Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

@cplepage Let me know if you see anything I missed, I'll ask the team for a final look now 🙂 thanks again!

@nbbeeken nbbeeken requested a review from durran December 20, 2021 19:32
@cplepage
Copy link
Contributor Author

@nbbeeken Looks good! Thanks to you for the quick replies!!

@nbbeeken nbbeeken merged commit 1d898b6 into mongodb:master Jan 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants