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

nbbeeken · 2023-01-06T17:25:40Z

Description

What is changing?

Removes the logic for integer-like doubles

What is the motivation for this change?

Fixes an output incompatibility with other languages

Double check the following

Ran npm run lint script
Self-review completed using the steps outlined here
PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
Changes are covered by tests
New TODOs have a related JIRA ticket

nbbeeken · 2023-01-06T18:07:36Z

The hex strings and stringified doubles are generated by nodejs (using buffer.writeDoubleBE)
We're making sure that:

Java's Double.valueOf parses the strings that Javascript creates
Java will generate the same bytes that node will for the same stringified double
- Notable limitation: NaN with payload cannot be encoded into a string

The tests can be run by putting the following into a file and redirecting it into jshell

$ jshell < ./double_test.jsh

Updated: Jan 9th 2pm EST with new tests from double_test.js

JShell tests

import java.nio.ByteBuffer;

void assertEquals(String a, String b) throws Exception {
    if (!a.equals(b)) {
        throw new Exception(a + "does not equal" + b);
    }
}

char[] HEX_ARRAY = "0123456789ABCDEF".toCharArray();
String bytesToHex(byte[] bytes) {
    char[] hexChars = new char[bytes.length * 2];
    for (int j = 0; j < bytes.length; j++) {
        int v = bytes[j] & 0xFF;
        hexChars[j * 2] = HEX_ARRAY[v >>> 4];
        hexChars[j * 2 + 1] = HEX_ARRAY[v & 0x0F];
    }
    return new String(hexChars);
}

assertEquals( bytesToHex( ByteBuffer.allocate(8).putDouble( Double.valueOf("0.0"                     ) ).array() )   ,   "0000000000000000")
assertEquals( bytesToHex( ByteBuffer.allocate(8).putDouble( Double.valueOf("-0.0"                    ) ).array() )   ,   "8000000000000000")
assertEquals( bytesToHex( ByteBuffer.allocate(8).putDouble( Double.valueOf("-0.0"                    ) ).array() )   ,   "8000000000000000")
assertEquals( bytesToHex( ByteBuffer.allocate(8).putDouble( Double.valueOf("3.0"                     ) ).array() )   ,   "4008000000000000")
assertEquals( bytesToHex( ByteBuffer.allocate(8).putDouble( Double.valueOf("-3.0"                    ) ).array() )   ,   "C008000000000000")
assertEquals( bytesToHex( ByteBuffer.allocate(8).putDouble( Double.valueOf("3.4"                     ) ).array() )   ,   "400B333333333333")
assertEquals( bytesToHex( ByteBuffer.allocate(8).putDouble( Double.valueOf("2.220446049250313e-16"   ) ).array() )   ,   "3CB0000000000000")
assertEquals( bytesToHex( ByteBuffer.allocate(8).putDouble( Double.valueOf("123450000000.0"          ) ).array() )   ,   "423CBE3182800000")
assertEquals( bytesToHex( ByteBuffer.allocate(8).putDouble( Double.valueOf("1234.5"                  ) ).array() )   ,   "40934A0000000000")
assertEquals( bytesToHex( ByteBuffer.allocate(8).putDouble( Double.valueOf("-1234.5"                 ) ).array() )   ,   "C0934A0000000000")
assertEquals( bytesToHex( ByteBuffer.allocate(8).putDouble( Double.valueOf("Infinity"                ) ).array() )   ,   "7FF0000000000000")
assertEquals( bytesToHex( ByteBuffer.allocate(8).putDouble( Double.valueOf("-Infinity"               ) ).array() )   ,   "FFF0000000000000")
assertEquals( bytesToHex( ByteBuffer.allocate(8).putDouble( Double.valueOf("NaN"                     ) ).array() )   ,   "7FF8000000000000")
assertEquals( bytesToHex( ByteBuffer.allocate(8).putDouble( Double.valueOf("1.7976931348623157e+308" ) ).array() )   ,   "7FEFFFFFFFFFFFFF")
assertEquals( bytesToHex( ByteBuffer.allocate(8).putDouble( Double.valueOf("5e-324"                  ) ).array() )   ,   "0000000000000001")
assertEquals( bytesToHex( ByteBuffer.allocate(8).putDouble( Double.valueOf("-1.7976931348623157e+308") ).array() )   ,   "FFEFFFFFFFFFFFFF")
assertEquals( bytesToHex( ByteBuffer.allocate(8).putDouble( Double.valueOf("-5e-324"                 ) ).array() )   ,   "8000000000000001")
assertEquals( bytesToHex( ByteBuffer.allocate(8).putDouble( Double.valueOf("2.2250738585072014e-308" ) ).array() )   ,   "0010000000000000")
assertEquals( bytesToHex( ByteBuffer.allocate(8).putDouble( Double.valueOf("2.225073858507201e-308"  ) ).array() )   ,   "000FFFFFFFFFFFFF")
assertEquals( bytesToHex( ByteBuffer.allocate(8).putDouble( Double.valueOf("5e-324"                  ) ).array() )   ,   "0000000000000001")
assertEquals( bytesToHex( ByteBuffer.allocate(8).putDouble( Double.valueOf("1000000000000000128.0"   ) ).array() )   ,   "43ABC16D674EC801")
/exit

nbbeeken · 2023-01-06T18:09:40Z

Evergreen patch

nbbeeken · 2023-01-06T18:40:56Z

Evergreen patch

nbbeeken · 2023-01-09T19:24:52Z

Evergreen Patch

baileympearson · 2023-01-09T19:50:04Z

test/node/double_tests.js

-        { 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' } },


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.

added titles

baileympearson · 2023-01-09T19:57:15Z

test/node/double_tests.js

+          // Note: calling toString on this integer returns 1000000000000000100, so toFixed is more precise
+          input: '1000000000000000128',
+          output: { $numberDouble: '1000000000000000128.0' }
+        }
      ];

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


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

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

baileympearson · 2023-01-09T19:57:41Z

test/node/double_tests.js

-        { input: new Double(NaN), output: { $numberDouble: 'NaN' } }
+        { input: 0, output: { $numberDouble: '0.0' } },
+        { input: -0, output: { $numberDouble: '-0.0' } },
+        { input: '-0.0', output: { $numberDouble: '-0.0' } },


What value does this test provide?

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

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.

baileympearson · 2023-01-09T20:10:18Z

test/node/double_tests.js

+        {
+          // 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' }
+        }


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.

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).

baileympearson · 2023-01-09T20:12:46Z

test/node/double_tests.js

+          if (!Number.isNaN(inputAsDouble.value)) {
+            expect(Number(inputAsDouble.toExtendedJSON({ relaxed: false }).$numberDouble)).to.equal(
+              inputAsDouble.value
+            );
+          }


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(..)).

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).

baileympearson · 2023-01-09T20:15:04Z

test/node/double_tests.js

+          }
+        });
+
+        it(`input ${typeof input}: ${input} creates the same bytes after stringification`, () => {


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`, () => {

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.

test/node/bson_corpus.spec.test.js

baileympearson · 2023-01-09T20:33:43Z

test/node/bson_corpus.spec.test.js

@@ -165,6 +165,11 @@ describe('BSON Corpus', function () {
        describe('valid-extjson', function () {


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?

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.

nbbeeken · 2023-01-09T22:00:51Z

Evergreen Patch

baileympearson

from discussion - we'll clean up the tests in a follow up PR but for the purposes of the fix we won't worry about test cleanup

nbbeeken added 2 commits January 6, 2023 12:25

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

578389f

test byte translation

318a53b

nbbeeken marked this pull request as ready for review January 6, 2023 17:51

Add minus zero as a string

114ca90

fix: ejson type info for -0

40757a0

nbbeeken added 2 commits January 6, 2023 16:54

use toFixed again

8734edd

test: for toFixed precision

4c93bf3

baileympearson requested changes Jan 9, 2023

View reviewed changes

nbbeeken added 2 commits January 9, 2023 16:47

test: titles and comments

eaaa214

test: all BSON types test

c3c6346

nbbeeken requested a review from baileympearson January 9, 2023 22:00

fix: lint

9fabfe5

baileympearson approved these changes Jan 10, 2023

View reviewed changes

baileympearson merged commit a298d22 into 4.0 Jan 10, 2023

baileympearson deleted the NODE-4932-rm-suffix-point-zero branch January 10, 2023 19:46

MrRaja23 mentioned this pull request May 24, 2024

[Snyk] Upgrade bson from 4.6.0 to 4.7.2 MrRaja23/beam#57

Open

WontonSam mentioned this pull request Jul 19, 2024

[Snyk] Upgrade bson from 4.6.0 to 4.7.2 WontonSam/cachicloud#236

Closed

WontonSam mentioned this pull request Jul 26, 2024

[Snyk] Upgrade bson from 4.6.0 to 4.7.2 WontonSam/cachicloud#257

Closed

Isuf-Muca mentioned this pull request Aug 10, 2024

[Snyk] Upgrade bson from 4.6.0 to 4.7.2 Isuf-Muca/beam#5

Open

WontonSam mentioned this pull request Sep 11, 2024

[Snyk] Upgrade bson from 4.6.0 to 4.7.2 WontonSam/cachicloud#296

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nbbeeken commented Jan 6, 2023 •

edited

Loading

nbbeeken commented Jan 6, 2023 •

edited

Loading

nbbeeken commented Jan 6, 2023

nbbeeken commented Jan 6, 2023

nbbeeken commented Jan 9, 2023

baileympearson Jan 9, 2023

nbbeeken Jan 9, 2023

baileympearson Jan 9, 2023

nbbeeken Jan 9, 2023 •

edited

Loading

baileympearson Jan 9, 2023

nbbeeken Jan 9, 2023

baileympearson Jan 9, 2023

nbbeeken Jan 9, 2023

baileympearson Jan 9, 2023

nbbeeken Jan 9, 2023

baileympearson Jan 9, 2023

nbbeeken Jan 9, 2023

baileympearson Jan 9, 2023

nbbeeken Jan 9, 2023

nbbeeken commented Jan 9, 2023

baileympearson left a comment

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

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

		@@ -165,6 +165,11 @@ describe('BSON Corpus', function () {
		describe('valid-extjson', function () {

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

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

Conversation

nbbeeken commented Jan 6, 2023 • edited Loading

Description

What is changing?

What is the motivation for this change?

Double check the following

nbbeeken commented Jan 6, 2023 • edited Loading

nbbeeken commented Jan 6, 2023

nbbeeken commented Jan 6, 2023

nbbeeken commented Jan 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbbeeken Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbbeeken commented Jan 9, 2023

baileympearson left a comment

Choose a reason for hiding this comment

nbbeeken commented Jan 6, 2023 •

edited

Loading

nbbeeken commented Jan 6, 2023 •

edited

Loading

nbbeeken Jan 9, 2023 •

edited

Loading