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
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
c0f5350
initial blah
aditi-khare-mongoDB Apr 18, 2024
272e4f1
another tempp commit
aditi-khare-mongoDB Apr 18, 2024
4483ac4
finished validateStringCharacters
aditi-khare-mongoDB Apr 18, 2024
a87dade
feat(NODE-5648): add Long.fromStringStrict method
aditi-khare-mongoDB Apr 18, 2024
76801e8
EOD progress
aditi-khare-mongoDB Apr 18, 2024
a001590
ready for review
aditi-khare-mongoDB Apr 19, 2024
c4818d9
make error message for overflows more accurate
aditi-khare-mongoDB Apr 19, 2024
59f3911
Merge branch 'main' into NODE-5648/long-validateString
aditi-khare-mongoDB Apr 19, 2024
4c7d5b5
add whitespace test case
aditi-khare-mongoDB Apr 19, 2024
8939fe9
fix test naming
aditi-khare-mongoDB Apr 19, 2024
667168e
requested changes
aditi-khare-mongoDB Apr 22, 2024
88fb767
lint fix
aditi-khare-mongoDB Apr 22, 2024
e616e88
requested changes team review
aditi-khare-mongoDB Apr 24, 2024
70c6162
add argument parsing check within fromStringStrict
aditi-khare-mongoDB Apr 24, 2024
1d261c5
removed extraneous functionality from _fromString and moved it to fro…
aditi-khare-mongoDB Apr 24, 2024
60b2272
remove unneeded param from _fromString
aditi-khare-mongoDB Apr 24, 2024
9f308aa
requested changes: alter support for infinity/nan cases and add expli…
aditi-khare-mongoDB Apr 25, 2024
63beeea
optimize removeLeadingZerosAndExplicitPlus function and fix name typo
aditi-khare-mongoDB Apr 25, 2024
d212f7e
requested changes - check str length in removeLeadingZerosAndExplicit…
aditi-khare-mongoDB Apr 29, 2024
c98b513
remove fromString fix
aditi-khare-mongoDB Apr 29, 2024
152db8f
lint fix
aditi-khare-mongoDB Apr 29, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions src/int_32.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { BSON_INT32_MAX, BSON_INT32_MIN } from './constants';
import { BSONError } from './error';
import type { EJSONOptions } from './extended_json';
import { type InspectFn, defaultInspect } from './parser/utils';
import { removeLeadingZeros } from './utils/string_utils';

/** @public */
export interface Int32Extended {
Expand Down Expand Up @@ -48,11 +49,7 @@ export class Int32 extends BSONValue {
* @param value - the string we want to represent as an int32.
*/
static fromString(value: string): Int32 {
const cleanedValue = !/[^0]+/.test(value)
? value.replace(/^0+/, '0') // all zeros case
: value[0] === '-'
? value.replace(/^-0+/, '-') // negative number with leading zeros
: value.replace(/^\+?0+/, ''); // positive number with leading zeros
const cleanedValue = removeLeadingZeros(value);
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved

const coercedValue = Number(value);

Expand Down
72 changes: 70 additions & 2 deletions src/long.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { BSONError } from './error';
import type { EJSONOptions } from './extended_json';
import { type InspectFn, defaultInspect } from './parser/utils';
import type { Timestamp } from './timestamp';
import { removeLeadingZeros } from './utils/string_utils';

interface LongWASMHelpers {
/** Gets the high bits of the last operation performed */
Expand Down Expand Up @@ -246,13 +247,37 @@ export class Long extends BSONValue {
}

/**
* @internal
* Returns false for an string that contains invalid characters for its radix, else returns the original string.
* @param str - The textual representation of the Long
* @param radix - The radix in which the text is written (2-36), defaults to 10
*/
private static validateStringCharacters(str: string, radix?: number): false | string {
radix = radix ?? 10;
const validCharacters = '0123456789abcdefghijklmnopqrstuvwxyz'.slice(0, radix);
// regex is case insensitive and checks that each character within the string is one of the validCharacters
const regex = new RegExp(`[^-+${validCharacters}]`, 'i');
return regex.test(str) ? false : str;
}
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved

/**
* @internal
* Returns a Long representation of the given string, written using the specified radix.
* Throws an error if `throwsError` is set to true and any of the following conditions are true:
* - the string contains invalid characters for the given radix
* - the string contains whitespace
* @param str - The textual representation of the Long
* @param validateStringCharacters - Whether or not invalid characters should throw an error
* @param unsigned - Whether unsigned or not, defaults to signed
* @param radix - The radix in which the text is written (2-36), defaults to 10
* @returns The corresponding Long value
*/
static fromString(str: string, unsigned?: boolean, radix?: number): Long {
private static _fromString(
str: string,
validateStringCharacters: boolean,
unsigned?: boolean,
radix?: number
): Long {
if (str.length === 0) throw new BSONError('empty string');
if (str === 'NaN' || str === 'Infinity' || str === '+Infinity' || str === '-Infinity')
return Long.ZERO;
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -263,12 +288,20 @@ export class Long extends BSONValue {
unsigned = !!unsigned;
}
radix = radix || 10;

if (radix < 2 || 36 < radix) throw new BSONError('radix');

if (validateStringCharacters && !Long.validateStringCharacters(str, radix)) {
throw new BSONError(`Input: '${str}' contains invalid characters for radix: ${radix}`);
}
if (validateStringCharacters && str.trim() !== str) {
throw new BSONError(`Input: '${str}' contains leading and/or trailing whitespace.`);
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
}

let p;
if ((p = str.indexOf('-')) > 0) throw new BSONError('interior hyphen');
else if (p === 0) {
return Long.fromString(str.substring(1), unsigned, radix).neg();
return Long._fromString(str.substring(1), validateStringCharacters, unsigned, radix).neg();
}

// Do several (8) digits each time through the loop, so as to
Expand All @@ -291,6 +324,41 @@ export class Long extends BSONValue {
return result;
}

/**
* Returns a Long representation of the given string, written using the specified radix.
* Throws an error if any of the following conditions are true:
* - the string contains invalid characters for the given radix
* - the string contains whitespace
* - the value the string represents is too large or too small to be a Long
* @param str - The textual representation of the Long
* @param unsigned - Whether unsigned or not, defaults to signed
* @param radix - The radix in which the text is written (2-36), defaults to 10
* @returns The corresponding Long value
*/
static fromStringStrict(str: string, unsigned?: boolean, radix?: number): Long {
baileympearson marked this conversation as resolved.
Show resolved Hide resolved
// remove leading zeros (for later string comparison and to make math faster)
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
const cleanedStr = removeLeadingZeros(str);
// doing this check outside of recursive function so cleanedStr value is consistent
baileympearson marked this conversation as resolved.
Show resolved Hide resolved
const result = Long._fromString(cleanedStr, true, unsigned, radix);
if (result.toString(radix).toLowerCase() !== cleanedStr.toLowerCase()) {
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
throw new BSONError(
`Input: ${str} is not representable as ${result.unsigned ? 'an unsigned' : 'a signed'} 64-bit Long with radix: ${radix}`
);
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
}
return result;
}

/**
* Returns a Long representation of the given string, written using the specified radix.
* @param str - The textual representation of the Long
* @param unsigned - Whether unsigned or not, defaults to signed
* @param radix - The radix in which the text is written (2-36), defaults to 10
* @returns The corresponding Long value
*/
static fromString(str: string, unsigned?: boolean, radix?: number): Long {
return Long._fromString(str, false, unsigned, radix);
}

/**
* Creates a Long from its byte representation.
* @param bytes - Byte representation
Expand Down
11 changes: 11 additions & 0 deletions src/utils/string_utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/**
* @internal
* Removes leading zeros from textual representation of a number.
*/
export function removeLeadingZeros(str: string): string {
return !/[^0]+/.test(str)
? str.replace(/^0+/, '0') // all zeros case
: str[0] === '-'
? str.replace(/^-0+/, '-') // negative number with leading zeros
: str.replace(/^\+?0+/, '');
}
97 changes: 97 additions & 0 deletions test/node/long.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,4 +163,101 @@ describe('Long', function () {
});
});
});

describe('static fromStringStrict()', function () {
const successInputs: [
name: string,
input: string,
unsigned: boolean,
radix: number,
expectedStr?: string
][] = [
['basic no alphabet low radix', '1236', true, 8],
['negative basic no alphabet low radix', '-1236', false, 8],
['valid upper and lower case letters in string with radix > 10', 'eEe', true, 15],
['hexadecimal letters', '126073efbcdADEF', true, 16],
['negative hexadecimal letters', '-1267efbcdDEF', false, 16],
['negative leading zeros', '-00000032', false, 15, '-32'],
['leading zeros', '00000032', false, 15, '32'],
['explicit positive leading zeros', '+00000032', false, 15, '32'],
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
['max unsigned binary input', Long.MAX_UNSIGNED_VALUE.toString(2), true, 2],
['max unsigned decimal input', Long.MAX_UNSIGNED_VALUE.toString(10), true, 10],
['max unsigned hex input', Long.MAX_UNSIGNED_VALUE.toString(16), true, 16],
['max signed binary input', Long.MAX_VALUE.toString(2), false, 2],
['max signed decimal input', Long.MAX_VALUE.toString(10), false, 10],
['max signed hex input', Long.MAX_VALUE.toString(16), false, 16],
['min signed binary input', Long.MIN_VALUE.toString(2), false, 2],
['min signed decimal input', Long.MIN_VALUE.toString(10), false, 10],
['min signed hex input', Long.MIN_VALUE.toString(16), false, 16],
['signed zero', '0', false, 10],
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
['unsigned zero', '0', true, 10]
];

const failureInputs: [name: string, input: string, unsigned: boolean, radix: number][] = [
['empty string', '', true, 2],
['invalid numbers in binary string', '234', true, 2],
['non a-z or numeric string', '~~', true, 36],
['alphabet in radix < 10', 'a', true, 9],
['radix does not allow all alphabet letters', 'eee', false, 14],
['over max unsigned binary input', Long.MAX_UNSIGNED_VALUE.toString(2) + '1', true, 2],
['over max unsigned decimal input', Long.MAX_UNSIGNED_VALUE.toString(10) + '1', true, 10],
['over max unsigned hex input', Long.MAX_UNSIGNED_VALUE.toString(16) + '1', true, 16],
['over max signed binary input', Long.MAX_VALUE.toString(2) + '1', false, 2],
['over max signed decimal input', Long.MAX_VALUE.toString(10) + '1', false, 10],
['over max signed hex input', Long.MAX_VALUE.toString(16) + '1', false, 16],
['under min signed binary input', Long.MIN_VALUE.toString(2) + '1', false, 2],
['under min signed decimal input', Long.MIN_VALUE.toString(10) + '1', false, 10],
['under min signed hex input', Long.MIN_VALUE.toString(16) + '1', false, 16],
['string with whitespace', ' 3503a ', false, 11]
];

for (const [testName, str, unsigned, radix, expectedStr] of successInputs) {
context(`when the input is ${testName}`, () => {
it(`should return a Long represenation of the input`, () => {
expect(Long.fromStringStrict(str, unsigned, radix).toString(radix)).to.equal(
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
expectedStr ?? str.toLowerCase()
);
});
});
}
for (const [testName, str, unsigned, radix] of failureInputs) {
context(`when the input is ${testName}`, () => {
it(`should throw BSONError`, () => {
expect(() => Long.fromStringStrict(str, unsigned, radix)).to.throw(BSONError);
});
});
}
});

describe('static validateStringCharacters()', function () {
const successInputs = [
['radix does allows given alphabet letter', 'eEe', 15],
['empty string', '', 2],
['all possible hexadecimal characters', '12efabc689873dADCDEF', 16],
['leading zeros', '0000000004567e', 16]
];

const failureInputs = [
['multiple decimal points', '..', 30],
['non a-z or numeric string', '~~', 36],
['alphabet in radix < 10', 'a', 4],
['radix does not allow all alphabet letters', 'eee', 14]
];

for (const [testName, str, radix] of successInputs) {
context(`when the input is ${testName}`, () => {
it(`should return a input string`, () => {
expect(Long.validateStringCharacters(str, radix)).to.equal(str);
});
});
}

for (const [testName, str, radix] of failureInputs) {
context(`when the input is ${testName}`, () => {
it(`should return false`, () => {
expect(Long.validateStringCharacters(str, radix)).to.equal(false);
});
});
}
});
});
1 change: 1 addition & 0 deletions test/node/release.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ const REQUIRED_FILES = [
'src/utils/byte_utils.ts',
'src/utils/node_byte_utils.ts',
'src/utils/number_utils.ts',
'src/utils/string_utils.ts',
'src/utils/web_byte_utils.ts',
'src/utils/latin.ts',
'src/validate_utf8.ts',
Expand Down