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

feat(NODE-5648): add Long.fromStringStrict() #675

Merged
merged 21 commits into from
Apr 30, 2024

Conversation

aditi-khare-mongoDB
Copy link
Contributor

@aditi-khare-mongoDB aditi-khare-mongoDB commented Apr 18, 2024

Description

Add Long.fromStringStrict which throws an error if the input string has invalid characters.

What is changing?

See above.

Is there new documentation needed for these changes?

Yes, we should request docs changes, and there is an API docs addition.

What is the motivation for this change?

User submitted bug: NODE-5638.

Release Highlight

Add Long.fromStringStrict method

The Long.fromStringStrict method is almost identical to the Long.fromString method, except it throws a BSONError if any of the following are true:

  • input string has invalid characters, for the given radix
  • the string contains whitespace
  • the value the input parameters represent is too large or too small to be a 64-bit Long

Unlike Long.fromString, this method does not coerce the inputs '+/-Infinity' and 'NaN' to Long.ZERO, in any case.

Examples:

Long.fromStringStrict('1234xxx5'); // throws BSONError
Long.fromString('1234xxx5'); // coerces input and returns new Long(123400)

// when writing in radix 10, 'n' and 'a' are both invalid characters
Long.fromStringStrict('NaN'); // throws BSONError
Long.fromString('NaN'); // coerces input and returns Long.ZERO

Note

Long.fromStringStrict's functionality will be present in Long.fromString in the V7 BSON release.

Double check the following

  • Ran npm run check: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

@aditi-khare-mongoDB aditi-khare-mongoDB force-pushed the NODE-5648/long-validateString branch from 68d736d to c4818d9 Compare April 19, 2024 20:47
@aditi-khare-mongoDB aditi-khare-mongoDB marked this pull request as ready for review April 19, 2024 20:47
@baileympearson baileympearson self-assigned this Apr 22, 2024
@baileympearson baileympearson added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Apr 22, 2024
test/node/long.test.ts Outdated Show resolved Hide resolved
src/long.ts Outdated Show resolved Hide resolved
src/long.ts Outdated Show resolved Hide resolved
src/long.ts Outdated Show resolved Hide resolved
src/long.ts Outdated Show resolved Hide resolved
src/long.ts Outdated Show resolved Hide resolved
src/long.ts Outdated Show resolved Hide resolved
@nbbeeken nbbeeken self-requested a review April 22, 2024 21:22
baileympearson
baileympearson previously approved these changes Apr 23, 2024
@baileympearson baileympearson added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Apr 23, 2024
src/long.ts Outdated Show resolved Hide resolved
src/long.ts Show resolved Hide resolved
src/long.ts Outdated Show resolved Hide resolved
src/long.ts Outdated Show resolved Hide resolved
src/long.ts Show resolved Hide resolved
src/long.ts Outdated Show resolved Hide resolved
test/node/long.test.ts Show resolved Hide resolved
test/node/long.test.ts Outdated Show resolved Hide resolved
test/node/long.test.ts Show resolved Hide resolved
src/int_32.ts Outdated Show resolved Hide resolved
src/long.ts Outdated Show resolved Hide resolved
test/node/utils/string_utils.test.ts Show resolved Hide resolved
src/utils/string_utils.ts Outdated Show resolved Hide resolved
src/utils/string_utils.ts Outdated Show resolved Hide resolved
nbbeeken
nbbeeken previously approved these changes Apr 25, 2024
src/long.ts Outdated
Comment on lines 409 to 415
if (str === 'NaN' && radix < 24) {
// radix does not support n, so coerce to zero
return Long.ZERO;
} else if ((str === 'Infinity' || str === '+Infinity' || str === '-Infinity') && radix < 35) {
// radix does not support y, so coerce to zero
return Long.ZERO;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we checking the radix here? This changes the behavior of Long.fromString - something we want to avoid in this PR. The logic was previously:

if (str === 'NaN' || str === 'Infinity' || str === '+Infinity' || str === '-Infinity')
      return Long.ZERO;

Copy link
Contributor Author

@aditi-khare-mongoDB aditi-khare-mongoDB Apr 29, 2024

Choose a reason for hiding this comment

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

It does change the behavior of Long.fromString, but its another bug fix, (see updated release notes).

We previously did not allow valid inputs to radices, because we interpreted them as 'not a number,' but in radix >= 24, 'nan' is a valid number (since 'n' and 'a' are in the character set for numbers).

Example:
Long.fromString('NaN', 27); // new Long(17060)

The original Long.fromString would return Long.ZERO on '+/- Infinity' and 'NaN' cases, and then check for radix later. This behavior is still preserved, other than in the case that the user is not actually inputting 'not a number' or an infinite value, but a valid non-infinite Long written in a high radix.

src/utils/string_utils.ts Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants