requested changes team review

Author: aditi-khare-mongoDB

src/int_32.ts

@@ -3,7 +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';
+import { removeLeadingZerosandExplicitPlus } from './utils/string_utils';
 
 /** @public */
 export interface Int32Extended {
@@ -49,7 +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 = removeLeadingZeros(value);
+    const cleanedValue = removeLeadingZerosandExplicitPlus(value);
 
     const coercedValue = Number(value);
 

src/long.ts

@@ -3,7 +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';
+import * as StringUtils from './utils/string_utils';
 
 interface LongWASMHelpers {
   /** Gets the high bits of the last operation performed */
@@ -246,20 +246,6 @@ export class Long extends BSONValue {
     return Long.fromString(value.toString(), unsigned);
   }
 
-  /**
-   * @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;
-  }
-
   /**
    * @internal
    * Returns a Long representation of the given string, written using the specified radix.
@@ -291,19 +277,19 @@ export class Long extends BSONValue {
 
     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.`);
-    }
-
     let p;
     if ((p = str.indexOf('-')) > 0) throw new BSONError('interior hyphen');
     else if (p === 0) {
       return Long._fromString(str.substring(1), validateStringCharacters, unsigned, radix).neg();
     }
 
+    if (str.trim() !== str) {
+      throw new BSONError(`Input: '${str}' contains leading and/or trailing whitespace`);
+    }
+    if (!StringUtils.validateStringCharacters(str, radix)) {
+      throw new BSONError(`Input: '${str}' contains invalid characters for radix: ${radix}`);
+    }
+
     // Do several (8) digits each time through the loop, so as to
     // minimize the calls to the very expensive emulated div.
     const radixToPower = Long.fromNumber(Math.pow(radix, 8));
@@ -336,13 +322,17 @@ export class Long extends BSONValue {
    * @returns The corresponding Long value
    */
   static fromStringStrict(str: string, unsigned?: boolean, radix?: number): Long {
+    if (str === 'NaN' || str === 'Infinity' || str === '+Infinity' || str === '-Infinity')
+      return Long.ZERO;
+
     // remove leading zeros (for later string comparison and to make math faster)
-    const cleanedStr = removeLeadingZeros(str);
-    // doing this check outside of recursive function so cleanedStr value is consistent
+    const cleanedStr = StringUtils.removeLeadingZerosandExplicitPlus(str);
+
+    // check roundtrip result
     const result = Long._fromString(cleanedStr, true, unsigned, radix);
     if (result.toString(radix).toLowerCase() !== cleanedStr.toLowerCase()) {
       throw new BSONError(
-        `Input: ${str} is not representable as ${result.unsigned ? 'an unsigned' : 'a signed'} 64-bit Long with radix: ${radix}`
+        `Input: ${str} is not representable as ${result.unsigned ? 'an unsigned' : 'a signed'} 64-bit Long ${radix != null ? `with radix: ${radix}` : ''}`
       );
     }
     return result;
@@ -356,7 +346,7 @@ export class Long extends BSONValue {
    * @returns The corresponding Long value
    */
   static fromString(str: string, unsigned?: boolean, radix?: number): Long {
-    return Long._fromString(str, false, unsigned, radix);
+    return Long._fromString(str, true, unsigned, radix);
   }
 
   /**

src/utils/string_utils.ts

@@ -1,11 +1,25 @@
 /**
  * @internal
- * Removes leading zeros from textual representation of a number.
+ * Removes leading zeros and explicit plus from textual representation of a number.
  */
-export function removeLeadingZeros(str: string): string {
-  return !/[^0]+/.test(str)
-    ? str.replace(/^0+/, '0') // all zeros case
+export function removeLeadingZerosandExplicitPlus(str: string): string {
+  return !/[^+?0]+/.test(str)
+    ? str.replace(/^\+?0+/, '0') // all zeros case (remove explicit plus if it exists)
     : str[0] === '-'
       ? str.replace(/^-0+/, '-') // negative number with leading zeros
-      : str.replace(/^\+?0+/, '');
+      : str.replace(/^\+?0*/, ''); // remove explicit plus
+}
+
+/**
+ * @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
+ */
+export function 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;
 }

test/node/int_32_tests.js

@@ -108,7 +108,9 @@ describe('Int32', function () {
       ['a string with zero with leading zeros', '000000', 0],
       ['a string with positive leading zeros', '000000867', 867],
       ['a string with explicity positive leading zeros', '+000000867', 867],
-      ['a string with negative leading zeros', '-00007', -7]
+      ['a string with negative leading zeros', '-00007', -7],
+      ['a string with explicit positive zeros', '+000000', 0],
+      ['a string explicit positive no leading zeros', '+32', 32]
     ];
     const errorInputs = [
       ['Int32.max + 1', '2147483648', 'larger than the maximum value for Int32'],

test/node/long.test.ts

@@ -189,8 +189,13 @@ describe('Long', function () {
       ['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],
-      ['unsigned zero', '0', true, 10]
+      ['signed zeros', '+000000', false, 10, '0'],
+      ['unsigned zero', '0', true, 10],
+      ['explicit positive no leading zeros', '+32', true, 10, '32'],
+      ['Infinity', 'Infinity', false, 21, '0'],
+      ['-Infinity', '-Infinity', false, 13, '0'],
+      ['+Infinity', '+Infinity', false, 13, '0'],
+      ['NaN', 'NaN', false, 11, '0']
     ];
 
     const failureInputs: [name: string, input: string, unsigned: boolean, radix: number][] = [
@@ -208,12 +213,17 @@ describe('Long', function () {
       ['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]
+      ['string with whitespace', '      3503a  ', false, 11],
+      ['negative zero unsigned', '-0', true, 9],
+      ['negative zero signed', '-0', false, 13],
+      ['radix 1', '12', false, 1],
+      ['negative radix', '12', false, -4],
+      ['radix over 36', '12', false, 37]
     ];
 
     for (const [testName, str, unsigned, radix, expectedStr] of successInputs) {
       context(`when the input is ${testName}`, () => {
-        it(`should return a Long represenation of the input`, () => {
+        it(`should return a Long representation of the input`, () => {
           expect(Long.fromStringStrict(str, unsigned, radix).toString(radix)).to.equal(
             expectedStr ?? str.toLowerCase()
           );
@@ -228,36 +238,4 @@ describe('Long', function () {
       });
     }
   });
-
-  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);
-        });
-      });
-    }
-  });
 });

test/node/utils/string_utils.test.ts

@@ -0,0 +1,54 @@
+import * as StringUtils from '../../../src/utils/string_utils';
+
+describe('removeLeadingZerosandExplicitPlus()', function () {
+  const inputs: [testName: string, str: string, expectedStr: string][] = [
+    ['a string with zero with leading zeros', '000000', '0'],
+    ['a string with positive leading zeros', '000000867', '867'],
+    ['a string with explicity positive leading zeros', '+000000867', '867'],
+    ['a string with negative leading zeros', '-00007', '-7'],
+    ['a string with explicit positive zeros', '+000000', '0'],
+    ['a string explicit positive no leading zeros', '+32', '32'],
+    ['a string explicit positive no leading zeros and letters', '+ab00', 'ab00']
+  ];
+
+  for (const [testName, str, expectedStr] of inputs) {
+    context(`when the input is ${testName}`, () => {
+      it(`should return a input string`, () => {
+        expect(StringUtils.removeLeadingZerosandExplicitPlus(str)).to.equal(expectedStr);
+      });
+    });
+  }
+});
+
+describe('validateStringCharacters()', function () {
+  const successInputs: [testName: string, str: string, radix: number][] = [
+    ['radix does allows given alphabet letter', 'eEe', 15],
+    ['empty string', '', 2],
+    ['all possible hexadecimal characters', '12efabc689873dADCDEF', 16],
+    ['leading zeros', '0000000004567e', 16],
+    ['explicit positive no leading zeros', '+32', 10]
+  ];
+
+  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(StringUtils.validateStringCharacters(str, radix)).to.equal(str);
+      });
+    });
+  }
+
+  for (const [testName, str, radix] of failureInputs) {
+    context(`when the input is ${testName}`, () => {
+      it(`should return false`, () => {
+        expect(StringUtils.validateStringCharacters(str, radix)).to.equal(false);
+      });
+    });
+  }
+});