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.

Sign up for GitHub

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-4932): remove .0 suffix from double extended json values #553

Merged
merged 9 commits into from
Jan 10, 2023
+184 −30
Merged

fix(NODE-4932): remove .0 suffix from double extended json values #553

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
578389f
fix(NODE-4932): remove .0 suffix from double extended json values
nbbeeken Jan 6, 2023
318a53b
test byte translation
nbbeeken Jan 6, 2023
114ca90
Add minus zero as a string
nbbeeken Jan 6, 2023
40757a0
fix: ejson type info for -0
nbbeeken Jan 6, 2023
8734edd
use toFixed again
nbbeeken Jan 6, 2023
4c93bf3
test: for toFixed precision
nbbeeken Jan 9, 2023
eaaa214
test: titles and comments
nbbeeken Jan 9, 2023
c3c6346
test: all BSON types test
nbbeeken Jan 9, 2023
9fabfe5
fix: lint
nbbeeken Jan 9, 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
10 changes: 4 additions & 6 deletions src/double.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,12 @@ export class Double {
if (Object.is(Math.sign(this.value), -0)) {
// NOTE: JavaScript has +0 and -0, apparently to model limit calculations. If a user
// explicitly provided `-0` then we need to ensure the sign makes it into the output
return { $numberDouble: '-0.0' };
return { $numberDouble: `-${this.value.toFixed(1)}` };
}

if (Number.isInteger(this.value)) {
return { $numberDouble: `${this.value}.0` };
} else {
return { $numberDouble: `${this.value}` };
}
return {
$numberDouble: Number.isInteger(this.value) ? this.value.toFixed(1) : this.value.toString()
};
}

/** @internal */
Expand Down
50 changes: 41 additions & 9 deletions test/node/bson_corpus.spec.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,11 @@ describe('BSON Corpus', function () {
describe('valid-extjson', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

it's unclear to me why the changes in this file are necessary. reverting to what's currently in main doesn't make any tests fail. could you explain why we're modifying the test runner?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With toFixed(1) reintroduced we don't need to skip the multi-type test anymore, I put that back, but it is senitive to changes in string format as opposed to value equivalence.

The changes here are meant to raise issue in the future if there's a change to the output format that would break round tripping the string into a numeric value for all doubles. Since the if stmt on main filtered only for the '1.2345678921232E+18' case we did not need the extra handling for the NaN and -0 cases. The second section which tests round tripping through BSON needs to handle the cases where EJSON automatically converts values to int/long/double based on size and fraction.

for (const v of valid) {
it(v.description, function () {
if (v.description === 'All BSON types') {
// TODO(NODE-3987): fix multi-type-deprecated test
this.skip();
}

// read in test case data. if this scenario is for a deprecated
// type, we want to use the "converted" BSON and EJSON, which
// use the upgraded version of the deprecated type. otherwise,
Expand All @@ -181,22 +186,29 @@ describe('BSON Corpus', function () {
// convert inputs to native Javascript objects
const nativeFromCB = bsonToNative(cB);

if (cEJ.includes('1.2345678921232E+18')) {
if (description === 'Double type') {
// The following is special test logic for a "Double type" bson corpus test that uses a different
// string format for the resulting double value
// The test does not have a loss in precision, just different exponential output
// We want to ensure that the stringified value when interpreted as a double is equal
// as opposed to the string being precisely the same
if (description !== 'Double type') {
throw new Error('Unexpected test using 1.2345678921232E+18');
}
const eJSONParsedAsJSON = JSON.parse(cEJ);
const eJSONParsed = EJSON.parse(cEJ, { relaxed: false });
expect(eJSONParsedAsJSON).to.have.nested.property('d.$numberDouble');
expect(eJSONParsed).to.have.nested.property('d._bsontype', 'Double');
const testInputAsFloat = Number.parseFloat(eJSONParsedAsJSON.d.$numberDouble);
const testInputAsNumber = Number(eJSONParsedAsJSON.d.$numberDouble);
const ejsonOutputAsFloat = eJSONParsed.d.valueOf();
expect(ejsonOutputAsFloat).to.equal(testInputAsFloat);
if (eJSONParsedAsJSON.d.$numberDouble === 'NaN') {
expect(ejsonOutputAsFloat).to.be.NaN;
expect(ejsonOutputAsFloat).to.be.NaN;
baileympearson marked this conversation as resolved.
Show resolved Hide resolved
} else {
if (eJSONParsedAsJSON.d.$numberDouble === '-0.0') {
expect(Object.is(ejsonOutputAsFloat, -0)).to.be.true;
}
expect(ejsonOutputAsFloat).to.equal(testInputAsFloat);
expect(ejsonOutputAsFloat).to.equal(testInputAsNumber);
}
} else {
// round tripped EJSON should match the original
expect(nativeToCEJSON(jsonToNative(cEJ))).to.equal(cEJ);
Expand All @@ -220,18 +232,38 @@ describe('BSON Corpus', function () {
expect(nativeToBson(jsonToNative(cEJ))).to.deep.equal(cB);
}

if (cEJ.includes('1.2345678921232E+18')) {
if (description === 'Double type') {
// The round tripped value should be equal in interpreted value, not in exact character match
const eJSONFromBSONAsJSON = JSON.parse(
EJSON.stringify(BSON.deserialize(cB), { relaxed: false })
);
const eJSONParsed = EJSON.parse(cEJ, { relaxed: false });
const stringValueKey = Object.keys(eJSONFromBSONAsJSON.d)[0];
const testInputAsFloat = Number.parseFloat(eJSONFromBSONAsJSON.d[stringValueKey]);
const testInputAsNumber = Number(eJSONFromBSONAsJSON.d[stringValueKey]);

// TODO(NODE-4377): EJSON transforms large doubles into longs
expect(eJSONFromBSONAsJSON).to.have.nested.property('d.$numberLong');
expect(eJSONFromBSONAsJSON).to.have.nested.property(
Number.isFinite(testInputAsFloat) && Number.isInteger(testInputAsFloat)
? testInputAsFloat <= 0x7fffffff &&
testInputAsFloat >= -0x80000000 &&
!Object.is(testInputAsFloat, -0)
? 'd.$numberInt'
: 'd.$numberLong'
: 'd.$numberDouble'
);
expect(eJSONParsed).to.have.nested.property('d._bsontype', 'Double');
const testInputAsFloat = Number.parseFloat(eJSONFromBSONAsJSON.d.$numberLong);
const ejsonOutputAsFloat = eJSONParsed.d.valueOf();
expect(ejsonOutputAsFloat).to.equal(testInputAsFloat);
if (eJSONFromBSONAsJSON.d.$numberDouble === 'NaN') {
expect(ejsonOutputAsFloat).to.be.NaN;
expect(ejsonOutputAsFloat).to.be.NaN;
baileympearson marked this conversation as resolved.
Show resolved Hide resolved
} else {
if (eJSONFromBSONAsJSON.d.$numberDouble === '-0.0') {
expect(Object.is(ejsonOutputAsFloat, -0)).to.be.true;
}
expect(ejsonOutputAsFloat).to.equal(testInputAsFloat);
expect(ejsonOutputAsFloat).to.equal(testInputAsNumber);
}
} else {
// the reverse direction, BSON -> native -> EJSON, should match canonical EJSON.
expect(nativeToCEJSON(nativeFromCB)).to.equal(cEJ);
Expand Down
82 changes: 69 additions & 13 deletions test/node/double_tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,26 +38,82 @@ describe('BSON Double Precision', function () {

describe('.toExtendedJSON()', () => {
const tests = [
{ input: new Double(0), output: { $numberDouble: '0.0' } },
{ input: new Double(-0), output: { $numberDouble: '-0.0' } },
{ input: new Double(3), output: { $numberDouble: '3.0' } },
{ input: new Double(-3), output: { $numberDouble: '-3.0' } },
{ input: new Double(3.4), output: { $numberDouble: '3.4' } },
{ input: new Double(Number.EPSILON), output: { $numberDouble: '2.220446049250313e-16' } },
{ input: new Double(12345e7), output: { $numberDouble: '123450000000.0' } },
{ input: new Double(12345e-1), output: { $numberDouble: '1234.5' } },
{ input: new Double(-12345e-1), output: { $numberDouble: '-1234.5' } },
{ input: new Double(Infinity), output: { $numberDouble: 'Infinity' } },
{ input: new Double(-Infinity), output: { $numberDouble: '-Infinity' } },
{ input: new Double(NaN), output: { $numberDouble: 'NaN' } }
{ input: 0, output: { $numberDouble: '0.0' } },
Copy link
Contributor

Choose a reason for hiding this comment

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

there's a lot of test cases here now, I think it's worthwhile to add descriptions? The context field could be used in the first it block to provide more meaningful test output.

Suggested change
{ input: 0, output: { $numberDouble: '0.0' } },
{ input: 0, output: { $numberDouble: '0.0' }, context: 'positive zero' },

I'd like this change because it's not clear why each test is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added titles

{ input: -0, output: { $numberDouble: '-0.0' } },
{ input: '-0.0', output: { $numberDouble: '-0.0' } },
Copy link
Contributor

Choose a reason for hiding this comment

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

What value does this test provide?

The constructor of Double only supports numbers ( according to Typescript ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not true according to runtime, we do support string inputs, should we make that fix here or in another ticket?

As for the test I was just ensuring the decimal point (.0) doesn't break things here.

{ input: 3, output: { $numberDouble: '3.0' } },
{ input: -3, output: { $numberDouble: '-3.0' } },
{ input: 3.4, output: { $numberDouble: '3.4' } },
{ input: Number.EPSILON, output: { $numberDouble: '2.220446049250313e-16' } },
{ input: 12345e7, output: { $numberDouble: '123450000000.0' } },
{ input: 12345e-1, output: { $numberDouble: '1234.5' } },
{ input: -12345e-1, output: { $numberDouble: '-1234.5' } },
{ input: Infinity, output: { $numberDouble: 'Infinity' } },
{ input: -Infinity, output: { $numberDouble: '-Infinity' } },
{ input: NaN, output: { $numberDouble: 'NaN' } },
{
input: Number.MAX_VALUE,
output: { $numberDouble: '1.7976931348623157e+308' }
},
{ input: Number.MIN_VALUE, output: { $numberDouble: '5e-324' } },
{
input: -Number.MAX_VALUE,
output: { $numberDouble: '-1.7976931348623157e+308' }
},
{ input: -Number.MIN_VALUE, output: { $numberDouble: '-5e-324' } },
// Reference: https://docs.oracle.com/cd/E19957-01/806-3568/ncg_math.html
{
// min positive normal number
input: '2.2250738585072014e-308',
output: { $numberDouble: '2.2250738585072014e-308' }
},
{
// max subnormal number (NOTE: JS does not output same input string, but numeric values are equal)
input: '2.225073858507201e-308',
output: { $numberDouble: '2.225073858507201e-308' }
},
{
// min positive subnormal number (NOTE: JS does not output same input string, but numeric values are equal)
input: '4.9406564584124654e-324',
output: { $numberDouble: '5e-324' }
},
{
// https://262.ecma-international.org/13.0/#sec-number.prototype.tofixed
// Note: calling toString on this integer returns 1000000000000000100, so toFixed is more precise
input: '1000000000000000128',
output: { $numberDouble: '1000000000000000128.0' }
}
Copy link
Contributor

Choose a reason for hiding this comment

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

while this is interesting, I'm not sure this provides any additional value. We already have tests that show that both positive and negative integer numbers serialize to numbers with a decimal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an integer that's beyond the precision that an 8 byte float can hold (hence why toString loses the 28), it confirms that we still output a string that preserves it if that was the input. (Basically, asserts that toFixed is being used for ints).

];

for (const test of tests) {
const input = test.input;
const output = test.output;
const title = `returns ${inspect(output)} when Double is ${input}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

a context block around both tests here would make the output of the tests a lot more understandable

Copy link
Contributor Author

@nbbeeken nbbeeken Jan 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, with what differentiating information? (Is this still relevant with the added titles?)

it(title, () => {
expect(output).to.deep.equal(input.toExtendedJSON({ relaxed: false }));
const inputAsDouble = new Double(input);
expect(inputAsDouble.toExtendedJSON({ relaxed: false })).to.deep.equal(output);
if (!Number.isNaN(inputAsDouble.value)) {
expect(Number(inputAsDouble.toExtendedJSON({ relaxed: false }).$numberDouble)).to.equal(
inputAsDouble.value
);
}
Comment on lines +167 to +171
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused why the if-block and second assertion are necessary. We should already be checking for numeric equality of $numberDouble in the preceding line (expect(..).to.deep.equal(..)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not exactly, the $numberDouble property is a string. This check converts it back to a number and checks that the string conversion did not loose the original value (within JavaScript).

});

it(`input ${typeof input}: ${input} creates the same bytes after stringification`, () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

iiuc, the important aspect of this test is that the value of the double is preserved through ejson stringification. we just happen to test this by asserting that the byte representations are equal. Right?

If so, I propose we adjust the title to reflect what we're actually testing, and add a comment explaining that we determine that we're comparing bytes to assert that no loss of precision occurs.

Suggested change
it(`input ${typeof input}: ${input} creates the same bytes after stringification`, () => {
it(`preserves the precision of the number after stringification`, () => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Precision is related but more so that the string is acting as a correct data transfer format, we want to know that the same bytes can be reconstructed from the generated string, sometimes the string changes "4.9406564584124654e-324" -> "5e-324" but both represent the same ieee754 double bytes.

const ejsonDoubleString = new Double(input).toExtendedJSON().$numberDouble;
const bytesFromInput = (() => {
const b = Buffer.alloc(8);
b.writeDoubleBE(Number(input));
return b.toString('hex');
})();

const bytesFromOutput = (() => {
const b = Buffer.alloc(8);
b.writeDoubleBE(Number(ejsonDoubleString));
return b.toString('hex');
})();

expect(bytesFromOutput).to.equal(bytesFromInput);
});
}
});
Expand Down