A bug fix for the Types.LONGLONG number range #1376

Dinwy · 2016-03-29T17:21:04Z

According to MDN(https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number#Integer_range_for_Number),
Integer range for Number is -9007199254740992 to 9007199254740992.

Bugs In the original code,

There is no range for the negative values.
Can't print out 9007199254740993 value. Because of the gt sign at line 103 : Number(numberString) > IEEE_754_BINARY_64_PRECISION.

According to MDN(https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number#Integer_range_for_Number), Integer range for Number is -9007199254740992 to 9007199254740992. But In the original code, 1. There is no range for the negative value. 2. Can't print out 9007199254740993 value. Because of the gt sign at line 103 : Number(numberString) > IEEE_754_BINARY_64_PRECISION.

dougwilson · 2016-03-29T17:24:54Z

Thank you very much! In order to accept this pull request, you need to add tests. Since it is a bug fix, there needs to be at least one test that fails without your change and passes with your change. Please ping me when you add the test to the pull request and I'll take another look :)!

dougwilson · 2016-03-29T17:26:34Z

lib/protocol/packets/RowDataPacket.js

@@ -1,7 +1,6 @@
 var Types                        = require('../constants/types');
 var Charsets                     = require('../constants/charsets');
 var Field                        = require('./Field');


Please re-align these statements now that the longest has been removed.

According to MDN(https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number#Integer_range_for_Number), Integer range for Number is -9007199254740992 to 9007199254740992. But In the original code, 1. There is no range for the negative value. 2. Can't print out 9007199254740993 value. Because of the gt sign at line 103 : Number(numberString) > IEEE_754_BINARY_64_PRECISION.

According to MDN(https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number#Integer_range_for_Number), Integer range for Number is -9007199254740992 to 9007199254740992. But In the original code, 1. There is no range for the negative value. 2. Can't print out 9007199254740993 value. Because of the gt sign at line 103 : Number(numberString) > IEEE_754_BINARY_64_PRECISION. Fixed, And re-aligned.

Dinwy · 2016-03-29T19:06:47Z

@dougwilson re-aligned the statements and bug fixed. Please take a look after being tested.

dougwilson · 2016-03-29T19:07:33Z

Thanks, @Dinwy! I still need you to add tests for this. In order to accept this pull request, you need to add tests. Since it is a bug fix, there needs to be at least one test that fails without your change and passes with your change. Please ping me when you add the test to the pull request and I'll take another look :)!

Dinwy · 2016-03-29T20:01:50Z

@dougwilson
on the issue #1371 ,

Before

There were two problems,

A. Incorrect integer value comes out

Before, the integer value that exceed Integer limits comes out incorrect.
Already being adressed on #1371 ,
-185395119379775634 automatically changed to -185395119379775650.

Also,

[{"id":1,"big":"9223372036854775807"},{"id":2,"big":-9223372036854776000},
{"id":3,"big":"1111111111111111111"},{"id":4,"big":-1111111111111111200}]

As you can see, Positive Integers that exceed Integer limits are parsed to string but Negative does not and that values are incorrect.

B. Can't print out +- 9007199254740993 value.

Because of the gt sign at line 103 : Number(numberString) > IEEE_754_BINARY_64_PRECISION,
+-9007199254740993 changed to +-9007199254740992(max/min value of integer).

{"id":4,"big":9007199254740992},{"id":5,"big":-9007199254740992}]

Fixed

A. correct integer value comes out
-185395119379775634 comes out correctly.
{"id":5,"big":"-185395119379775634"}]

also the others.

[{"id":1,"big":"9223372036854775807"},{"id":2,"big":"-9223372036854775807"},
{"id":3,"big":"1111111111111111111"},{"id":4,"big":"-1111111111111111111"}]

B. Possible to print out +- 9007199254740993 value.

{"id":4,"big":"9007199254740993"},{"id":5,"big":"-9007199254740993"}]

dougwilson · 2016-03-29T20:07:26Z

Hi @Dinwy, I understand that, but you need to write tests for that in our tests/ folder, please. I can do it if you can't, but cannot make promises on the timeline. In order to accept this pull request, you need to add tests. Since it is a bug fix, there needs to be at least one test that fails without your change and passes with your change. Please ping me when you add the test to the pull request and I'll take another look :)!

Dinwy · 2016-03-29T20:14:27Z

@dougwilson Oh, I'm sorry. I will add a test soon.

# Conflicts: # lib/protocol/packets/RowDataPacket.js

mysqljs#1371 Fix bigint out of range problem

Dinwy · 2016-03-30T23:08:49Z

@dougwilson Could you please check this?

ronkorving · 2016-04-05T05:57:07Z

Thanks for the contribution. Does this solve a known bug or did you find it yourself? I wasn't aware of it.

Dinwy · 2016-04-05T22:09:18Z

@ronkorving This will solve a known bug #1371.

dougwilson · 2016-04-05T22:18:30Z

test/common.js

@@ -149,7 +149,8 @@ function mergeTestConfig(config) {
    host     : process.env.MYSQL_HOST,
    port     : process.env.MYSQL_PORT,
    user     : process.env.MYSQL_USER,
-    password : process.env.MYSQL_PASSWORD
+    password : process.env.MYSQL_PASSWORD,
+    supportBigNumbers: true


Can you move this to just be set in the one test that needs it, instead of set in every single test? common.getTestConnection accepts an optional first argument to provide test-specific configuration to merge with the common config here.

I changed my code and I'm sorry for the additional trouble caused.

Fix mysqljs#1371 bigint error

ronkorving · 2016-04-06T03:39:16Z

test/common.js

@@ -150,7 +150,7 @@ function mergeTestConfig(config) {
    port     : process.env.MYSQL_PORT,
    user     : process.env.MYSQL_USER,
    password : process.env.MYSQL_PASSWORD
-  }, config);
+    }, config);


Needless whitespace change. This file should probably remain untouched in this PR.

How could I miss this. Thanks.

Fix mysqljs#1371 bigint error

fixes #1376

dougwilson reviewed Mar 29, 2016
View reviewed changes

dougwilson added bug patch needs tests labels Mar 29, 2016

dougwilson self-assigned this Mar 29, 2016

Dinwy added 3 commits March 30, 2016 02:32

Update RowDataPacket.js

c012136

Dinwy added 5 commits March 30, 2016 07:25

add tests

21d8bad

Merge branch 'master' of https://github.com/Dinwy/node-mysql

0345aeb

# Conflicts: # lib/protocol/packets/RowDataPacket.js

Delete test0.js

b55d927

Fix bigint out of range problem

b8ece31

Fix bigint out of range problem

7588c28

Dinwy force-pushed the master branch from 8d635cc to 7588c28 Compare March 30, 2016 00:12

Dinwy added 3 commits March 31, 2016 07:36

Fixed bigint out of range problem

fc77df8

Fixed bigint out of range problem

3032796

Merge pull request #1 from Dinwy/iss1371

bd1ce80

mysqljs#1371 Fix bigint out of range problem

dougwilson reviewed Apr 5, 2016
View reviewed changes

Dinwy added 2 commits April 6, 2016 11:16

Fix mysqljs#1371 bigint error

4273828

Merge pull request #2 from Dinwy/iss1371

d31b186

Fix mysqljs#1371 bigint error

ronkorving reviewed Apr 6, 2016
View reviewed changes

Dinwy added 2 commits April 6, 2016 12:55

Fix mysqljs#1371 bigint error

fb1d14d

Merge pull request #3 from Dinwy/iss1371

ff37d5d

Fix mysqljs#1371 bigint error

dougwilson force-pushed the master branch 2 times, most recently from 48ff604 to e9dcd8a Compare May 7, 2016 02:46

dougwilson removed the needs tests label May 8, 2016

dougwilson closed this in 8c4df89 May 8, 2016

dougwilson pushed a commit that referenced this pull request May 8, 2016

Fix type cast for BIGINT columns when number is negative

d36ca19

fixes #1376

dougwilson pushed a commit that referenced this pull request May 8, 2016

Fix type cast for BIGINT columns when number is negative

5847ec8

fixes #1376

dougwilson pushed a commit that referenced this pull request May 8, 2016

Fix type cast for BIGINT columns when number is negative

be37e88

fixes #1376

dveeden pushed a commit to dveeden/mysql that referenced this pull request Jan 31, 2023

update changelog for Version 1.7 (mysqljs#1376)

5cee457

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A bug fix for the Types.LONGLONG number range #1376

A bug fix for the Types.LONGLONG number range #1376

Dinwy commented Mar 29, 2016

dougwilson commented Mar 29, 2016

dougwilson Mar 29, 2016

Dinwy commented Mar 29, 2016

dougwilson commented Mar 29, 2016

Dinwy commented Mar 29, 2016

dougwilson commented Mar 29, 2016

Dinwy commented Mar 29, 2016

Dinwy commented Mar 30, 2016

ronkorving commented Apr 5, 2016

Dinwy commented Apr 5, 2016

dougwilson Apr 5, 2016

Dinwy Apr 6, 2016

ronkorving Apr 6, 2016

Dinwy Apr 6, 2016

A bug fix for the Types.LONGLONG number range #1376

A bug fix for the Types.LONGLONG number range #1376

Conversation

Dinwy commented Mar 29, 2016

dougwilson commented Mar 29, 2016

dougwilson Mar 29, 2016

Choose a reason for hiding this comment

Dinwy commented Mar 29, 2016

dougwilson commented Mar 29, 2016

Dinwy commented Mar 29, 2016

Before

Fixed

dougwilson commented Mar 29, 2016

Dinwy commented Mar 29, 2016

Dinwy commented Mar 30, 2016

ronkorving commented Apr 5, 2016

Dinwy commented Apr 5, 2016

dougwilson Apr 5, 2016

Choose a reason for hiding this comment

Dinwy Apr 6, 2016

Choose a reason for hiding this comment

ronkorving Apr 6, 2016

Choose a reason for hiding this comment

Dinwy Apr 6, 2016

Choose a reason for hiding this comment