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-4874): support EJSON parse for BigInt from $numberLong #552

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
75a6207
feat(NODE-4874): WIP
W-A-James Jan 5, 2023
7cd7f6c
test(NODE-4874): Start table test
W-A-James Jan 5, 2023
df97552
fix(NODE-4874): Get canonical mode parsing to work
W-A-James Jan 6, 2023
07904f9
style(NODE-4874): Style
W-A-James Jan 6, 2023
d0dd23d
style(NODE-4874): eslint
W-A-James Jan 6, 2023
5120ca5
fix(NODE-4874): Fix handling of flags
W-A-James Jan 6, 2023
d67757d
test(NODE-4874): Finish tests
W-A-James Jan 6, 2023
4f15e42
style(NODE-4874): fixup condition
W-A-James Jan 6, 2023
2f5db05
fix(NODE-4874): Remove dead code
W-A-James Jan 6, 2023
822dff8
fix(NODE-4874): Add fix and test for double bug
W-A-James Jan 11, 2023
5666255
fix(NODE-4874): Implement requested fixes
W-A-James Jan 11, 2023
79f0c81
test(NODE-4874): Add new table tests
W-A-James Jan 11, 2023
260b667
test(NODE-4874): Add test to check that legacy flag doesn't affect ou…
W-A-James Jan 11, 2023
6a5364a
test(NODE-4874): Add another assertion
W-A-James Jan 11, 2023
7eca3ea
style(NODE-4874): Update comment
W-A-James Jan 11, 2023
f6aed3d
test(NODE-4874): Remove 'only' annotation
W-A-James Jan 11, 2023
b0e67aa
fix(NODE-4874): Update Long.fromExtendedJSON
W-A-James Jan 11, 2023
4361871
test(NODE-4874): Add new tests
W-A-James Jan 11, 2023
92ae357
fix(NODE-4874): Add fixes
W-A-James Jan 12, 2023
fff3d32
test(NODE-4874): Update tests
W-A-James Jan 12, 2023
a86a942
style(NODE-4874): Change variable name
W-A-James Jan 12, 2023
d9f539f
fix(NODE-4874): Fix MAX_INT64_STRING_LENGTH constant
W-A-James Jan 13, 2023
0acf248
test(NODE-4874): Test fixups
W-A-James Jan 17, 2023
cdf0334
fix(NODE-4874): Fix fromExtendedJSON checks and errors
W-A-James Jan 17, 2023
027b5df
fix(NODE-4874): Update fromExtendedJSON
W-A-James Jan 17, 2023
a02236f
test(NODE-4874): New tests
W-A-James Jan 17, 2023
31f8bf7
test(NODE-4874): Add tests for current validation behaviour
W-A-James Jan 18, 2023
f22ce03
fix(NODE-4874): Small style fix
W-A-James Jan 18, 2023
8b0a741
test(NODE-4874): Add comments and fix test value
W-A-James Jan 18, 2023
e0049b3
fix(NODE-4874): Make error messages more verbose
W-A-James Jan 18, 2023
47cdb65
fix(NODE-4874): Fix error messages
W-A-James Jan 18, 2023
d502ddb
Merge branch 'main' into NODE-4874/support_ejson_parse_for_BigInt_fro…
W-A-James Jan 18, 2023
f80b707
fix(NODE-4874): merge conflict fix
W-A-James Jan 18, 2023
c90b783
fix(NODE-4874): Update fromExtendedJSON behaviour to be more consiste…
W-A-James Jan 19, 2023
43b39c1
test(NODE-4874): move EJSON.parse tests back under the correct descri…
W-A-James Jan 19, 2023
b99a141
test(NODE-4874): Unnest EJSON.stringify fron EJSON.parse
W-A-James Jan 19, 2023
a0b5cf0
test(NODE-4874): Remove todo
W-A-James Jan 19, 2023
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
5 changes: 5 additions & 0 deletions src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ export const BSON_INT64_MAX = Math.pow(2, 63) - 1;
/** @internal */
export const BSON_INT64_MIN = -Math.pow(2, 63);

/** @internal */
export const BSON_INT64_MAX_N = 2n ** 63n -1n;
/** @internal */
export const BSON_INT64_MIN_N = 2n ** 63n -1n;

/**
* Any integer up to 2^53 can be precisely represented by a double.
* @internal
Expand Down
6 changes: 3 additions & 3 deletions src/extended_json.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ function deserializeValue(value: any, options: EJSONOptions = {}) {
const in64BitRange = value <= BSON_INT64_MAX && value >= BSON_INT64_MIN;
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved

if (options.relaxed || options.legacy) {
return Number.isInteger(value) && options.useBigInt64 ? BigInt(value) : value;
return value;
}

if (Number.isInteger(value) && !Object.is(value, -0)) {
Expand Down Expand Up @@ -202,8 +202,8 @@ function serializeValue(value: any, options: EJSONSerializeOptions): any {

throw new BSONError(
'Converting circular structure to EJSON:\n' +
` ${leadingPart}${alreadySeen}${circularPart}${current}\n` +
` ${leadingSpace}\\${dashes}/`
` ${leadingPart}${alreadySeen}${circularPart}${current}\n` +
` ${leadingSpace}\\${dashes}/`
);
}
options.seenObjects[options.seenObjects.length - 1].obj = value;
Expand Down
28 changes: 26 additions & 2 deletions src/long.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { BSONError } from './error';
import type { EJSONOptions } from './extended_json';
import type { Timestamp } from './timestamp';
import { BSON_INT64_MAX, BSON_INT64_MIN } from './constants';

interface LongWASMHelpers {
/** Gets the high bits of the last operation performed */
Expand Down Expand Up @@ -75,6 +76,18 @@ const INT_CACHE: { [key: number]: Long } = {};
/** A cache of the Long representations of small unsigned integer values. */
const UINT_CACHE: { [key: number]: Long } = {};

/**
* Validate an int64 string.
*
* Fails on strings longer than 22 characters
* Fails on non-decimal strings */
function validateInt64String(input: string): boolean {
if (input.length > 22) {
return false;
}
return /^(\+|\-)?[1-9][0-9]+$/.test(input);
}

/** @public */
export interface LongExtended {
$numberLong: string;
Expand Down Expand Up @@ -1025,11 +1038,22 @@ export class Long {
doc: { $numberLong: string },
options?: EJSONOptions
): number | Long | bigint {
const longResult = Long.fromString(doc.$numberLong);
const defaults = { useBigInt64: false, relaxed: true };
const ejsonOptions = { ...defaults, ...options };

if (!validateInt64String(doc.$numberLong)) {
throw new BSONError('Invalid int64 string');
}
const longResult = Long.fromString(doc.$numberLong);

if (ejsonOptions.useBigInt64) {
return BigInt(doc.$numberLong);
const INT64_MAX = BigInt('0x7fffffffffffffff');
const INT64_MIN = BigInt('0xffffffffffffffff');
const result = BigInt(doc.$numberLong);
if (result > INT64_MAX || result < INT64_MIN) {
throw new BSONError('EJSON numberLong must be in int64 range');
}
return result;
}
if (ejsonOptions.relaxed) {
return longResult.toNumber();
Expand Down
59 changes: 28 additions & 31 deletions test/node/bigint.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ import { expect } from 'chai';
import { BSON_DATA_LONG } from '../../src/constants';
import { BSONDataView } from '../../src/utils/byte_utils';

describe('BSON BigInt support', function () {
describe('BSON.deserialize()', function () {
describe('BSON BigInt support', function() {
describe('BSON.deserialize()', function() {
type DeserialzationOptions = {
useBigInt64: boolean | undefined;
promoteValues: boolean | undefined;
Expand Down Expand Up @@ -60,15 +60,12 @@ describe('BSON BigInt support', function () {

function generateTestDescription(entry: TestTableEntry): string {
const options = entry.options;
const promoteValues = `promoteValues ${
options.promoteValues === undefined ? 'is default' : `is ${options.promoteValues}`
}`;
const promoteLongs = `promoteLongs ${
options.promoteLongs === undefined ? 'is default' : `is ${options.promoteLongs}`
}`;
const useBigInt64 = `useBigInt64 ${
options.useBigInt64 === undefined ? 'is default' : `is ${options.useBigInt64}`
}`;
const promoteValues = `promoteValues ${options.promoteValues === undefined ? 'is default' : `is ${options.promoteValues}`
}`;
const promoteLongs = `promoteLongs ${options.promoteLongs === undefined ? 'is default' : `is ${options.promoteLongs}`
}`;
const useBigInt64 = `useBigInt64 ${options.useBigInt64 === undefined ? 'is default' : `is ${options.useBigInt64}`
}`;
const flagString = `${useBigInt64}, ${promoteValues}, and ${promoteLongs}`;
if (entry.shouldThrow) {
return `throws when ${flagString}`;
Expand Down Expand Up @@ -102,7 +99,7 @@ describe('BSON BigInt support', function () {
}
});

describe('BSON.serialize()', function () {
describe('BSON.serialize()', function() {
// Index for the data type byte of a BSON document with a
// NOTE: These offsets only apply for documents with the shape {a : <n>}
// where n is a BigInt
Expand Down Expand Up @@ -138,13 +135,13 @@ describe('BSON BigInt support', function () {
};
}

it('serializes bigints with the correct BSON type', function () {
it('serializes bigints with the correct BSON type', function() {
const testDoc = { a: 0n };
const serializedDoc = getSerializedDocParts(BSON.serialize(testDoc));
expect(serializedDoc.dataType).to.equal(BSON_DATA_LONG);
});

it('serializes bigints into little-endian byte order', function () {
it('serializes bigints into little-endian byte order', function() {
const testDoc = { a: 0x1234567812345678n };
const serializedDoc = getSerializedDocParts(BSON.serialize(testDoc));
const expectedResult = getSerializedDocParts(
Expand All @@ -158,7 +155,7 @@ describe('BSON BigInt support', function () {
expect(expectedResult.value).to.equal(serializedDoc.value);
});

it('serializes a BigInt that can be safely represented as a Number', function () {
it('serializes a BigInt that can be safely represented as a Number', function() {
const testDoc = { a: 0x23n };
const serializedDoc = getSerializedDocParts(BSON.serialize(testDoc));
const expectedResult = getSerializedDocParts(
Expand All @@ -171,7 +168,7 @@ describe('BSON BigInt support', function () {
expect(serializedDoc).to.deep.equal(expectedResult);
});

it('serializes a BigInt in the valid range [-2^63, 2^63 - 1]', function () {
it('serializes a BigInt in the valid range [-2^63, 2^63 - 1]', function() {
const testDoc = { a: 0xfffffffffffffff1n };
const serializedDoc = getSerializedDocParts(BSON.serialize(testDoc));
const expectedResult = getSerializedDocParts(
Expand All @@ -184,7 +181,7 @@ describe('BSON BigInt support', function () {
expect(serializedDoc).to.deep.equal(expectedResult);
});

it('wraps to negative on a BigInt that is larger than (2^63 -1)', function () {
it('wraps to negative on a BigInt that is larger than (2^63 -1)', function() {
const maxIntPlusOne = { a: 2n ** 63n };
const serializedMaxIntPlusOne = getSerializedDocParts(BSON.serialize(maxIntPlusOne));
const expectedResultForMaxIntPlusOne = getSerializedDocParts(
Expand All @@ -197,7 +194,7 @@ describe('BSON BigInt support', function () {
expect(serializedMaxIntPlusOne).to.deep.equal(expectedResultForMaxIntPlusOne);
});

it('serializes BigInts at the edges of the valid range [-2^63, 2^63 - 1]', function () {
it('serializes BigInts at the edges of the valid range [-2^63, 2^63 - 1]', function() {
const maxPositiveInt64 = { a: 2n ** 63n - 1n };
const serializedMaxPositiveInt64 = getSerializedDocParts(BSON.serialize(maxPositiveInt64));
const expectedSerializationForMaxPositiveInt64 = getSerializedDocParts(
Expand All @@ -221,7 +218,7 @@ describe('BSON BigInt support', function () {
expect(serializedMinPositiveInt64).to.deep.equal(expectedSerializationForMinPositiveInt64);
});

it('truncates a BigInt that is larger than a 64-bit int', function () {
it('truncates a BigInt that is larger than a 64-bit int', function() {
const testDoc = { a: 2n ** 64n + 1n };
const serializedDoc = getSerializedDocParts(BSON.serialize(testDoc));
const expectedSerialization = getSerializedDocParts(
Expand All @@ -234,7 +231,7 @@ describe('BSON BigInt support', function () {
expect(serializedDoc).to.deep.equal(expectedSerialization);
});

it('serializes array of BigInts', function () {
it('serializes array of BigInts', function() {
const testArr = { a: [1n] };
const serializedArr = BSON.serialize(testArr);
const expectedSerialization = bufferFromHexArray([
Expand All @@ -249,7 +246,7 @@ describe('BSON BigInt support', function () {
expect(serializedArr).to.deep.equal(expectedSerialization);
});

it('serializes Map with BigInt values', function () {
it('serializes Map with BigInt values', function() {
const testMap = new Map();
testMap.set('a', 1n);
const serializedMap = getSerializedDocParts(BSON.serialize(testMap));
Expand All @@ -264,7 +261,7 @@ describe('BSON BigInt support', function () {
});
});

describe('EJSON.parse()', function () {
describe('EJSON.parse()', function() {
type ParseOptions = {
useBigInt64: boolean | undefined;
relaxed: boolean | undefined;
Expand Down Expand Up @@ -312,7 +309,7 @@ describe('BSON BigInt support', function () {
};
}

describe('canonical input', function () {
describe('canonical input', function() {
const canonicalInputTestTable = useBigInt64Values.flatMap(useBigInt64 => {
return relaxedValues.flatMap(relaxed => {
return genTestTable(
Expand All @@ -322,8 +319,8 @@ describe('BSON BigInt support', function () {
useBigInt64IsSet
? { a: 23n }
: relaxedIsSet
? { a: 23 }
: { a: BSON.Long.fromNumber(23) }
? { a: 23 }
: { a: BSON.Long.fromNumber(23) }
);
});
});
Expand All @@ -340,18 +337,18 @@ describe('BSON BigInt support', function () {
}
});

describe('relaxed integer input', function () {
describe('relaxed integer input', function() {
const relaxedIntegerInputTestTable = useBigInt64Values.flatMap(useBigInt64 => {
return relaxedValues.flatMap(relaxed => {
return genTestTable(
useBigInt64,
relaxed,
(useBigInt64IsSet: boolean, relaxedIsSet: boolean) =>
useBigInt64IsSet
? { a: 4294967296n }
: relaxedIsSet
relaxedIsSet
? { a: 4294967296 }
: { a: BSON.Long.fromNumber(4294967296) }
: useBigInt64IsSet
? { a: 4294967296n }
: { a: BSON.Long.fromNumber(4294967296) }
);
});
});
Expand All @@ -367,7 +364,7 @@ describe('BSON BigInt support', function () {
}
});

describe('relaxed double input where double is outside of int32 range and useBigInt64 is true', function () {
describe('relaxed double input where double is outside of int32 range and useBigInt64 is true', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, let's file a follow up

const relaxedDoubleInputTestTable = relaxedValues.flatMap(relaxed => {
return genTestTable(true, relaxed, (_, relaxedIsSet: boolean) =>
relaxedIsSet ? { a: 2147483647.9 } : { a: new BSON.Double(2147483647.9) }
Expand Down
51 changes: 45 additions & 6 deletions test/node/long.test.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { expect } from 'chai';
import { Long } from '../register-bson';
import { Long, BSONError } from '../register-bson';

describe('Long', function () {
it('accepts strings in the constructor', function () {
describe('Long', function() {
it('accepts strings in the constructor', function() {
expect(new Long('0').toString()).to.equal('0');
expect(new Long('00').toString()).to.equal('0');
expect(new Long('-1').toString()).to.equal('-1');
Expand All @@ -13,7 +13,7 @@ describe('Long', function () {
expect(new Long('13835058055282163712', true).toString()).to.equal('13835058055282163712');
});

it('accepts BigInts in Long constructor', function () {
it('accepts BigInts in Long constructor', function() {
expect(new Long(0n).toString()).to.equal('0');
expect(new Long(-1n).toString()).to.equal('-1');
expect(new Long(-1n, true).toString()).to.equal('18446744073709551615');
Expand All @@ -23,8 +23,8 @@ describe('Long', function () {
expect(new Long(13835058055282163712n, true).toString()).to.equal('13835058055282163712');
});

describe('static fromExtendedJSON()', function () {
it('is not affected by the legacy flag', function () {
describe('static fromExtendedJSON()', function() {
it('is not affected by the legacy flag', function() {
const ejsonDoc = { $numberLong: '123456789123456789' };
const longRelaxedLegacy = Long.fromExtendedJSON(ejsonDoc, { legacy: true, relaxed: true });
const longRelaxedNonLegacy = Long.fromExtendedJSON(ejsonDoc, {
Expand All @@ -40,5 +40,44 @@ describe('Long', function () {
expect(longRelaxedLegacy).to.deep.equal(longRelaxedNonLegacy);
expect(longCanonicalLegacy).to.deep.equal(longCanonicalNonLegacy);
});

describe('rejects with BSONError', function() {
it('hex strings', function() {
expect(() => {
const ejsonDoc = { $numberLong: '0xffffffff' };
return Long.fromExtendedJSON(ejsonDoc);
}).to.throw(BSONError);
});
it('octal strings', function() {
expect(() => {
const ejsonDoc = { $numberLong: '0o1234567' };
return Long.fromExtendedJSON(ejsonDoc);
}).to.throw(BSONError);
});
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
it('strings longer than 22 characters', function() {
expect(() => {
const ejsonDoc = { $numberLong: '99999999999999999999999' }
return Long.fromExtendedJSON(ejsonDoc);
}).to.throw(BSONError);
});
it('strings with leading zeros', function() {
expect(() => {
const ejsonDoc = { $numberLong: '000123456' }
return Long.fromExtendedJSON(ejsonDoc);
}).to.throw(BSONError);
});
it('non-numeric strings', function() {
expect(() => {
const ejsonDoc = { $numberLong: 'hello world' }
return Long.fromExtendedJSON(ejsonDoc);
}).to.throw(BSONError);
})
it('strings encoding numbers larger than 64 bits wide when useBigInt64 is true', function() {
expect(() => {
const ejsonDoc = { $numberLong: 0xf_ffff_ffff_ffff_ffffn.toString() }
return Long.fromExtendedJSON(ejsonDoc, {useBigInt64: true});
}).to.throw(BSONError);
});
});
});
});
2 changes: 2 additions & 0 deletions test/register-bson.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ chai.use(function (chai) {
try {
throwsAssertion.call(this, ...args);
} catch (assertionError) {
console.log(JSON.stringify(assertionError), 2);
// FIXME(NODE-4874)
if (assertionError.actual?.name === assertionError.expected) {
return;
}
Expand Down