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

fix:resolve data precision loss in JSON messages #1507

Merged
merged 4 commits into from Dec 4, 2023
Merged

Conversation

ni00
Copy link
Contributor

@ni00 ni00 commented Nov 30, 2023

PR Checklist

If you have any questions, you can refer to the Contributing Guide

What is the current behavior?

If the MQTT response contains a large number (e.g. 8 bytes like 47348660069859329), using JSON format will cause the accept to overflow without any indication.The functions of subscribing messages, sending messages, and input box formatting in the web end, desktop end, and CLI end all have the issue of JSON precision loss.

image1

Issue Number

#1505 #1410 #740 #1454

What is the new behavior?

display normally
image2

Does this PR introduce a breaking change?

  • Yes
  • No

Specific Instructions

Are there any specific instructions or things that should be known prior to review?

Other information

@ysfscream ysfscream added enhancement New feature or request fix Fix bug or issues CLI MQTTX CLI web MQTTX Web desktop MQTTX Desktop labels Nov 30, 2023
@ysfscream ysfscream added this to the v1.9.7 milestone Nov 30, 2023
@Red-Asuka
Copy link
Member

The path of the .nvmrc file configured in action is incorrect, can you fix it by the way?

Error: The specified node version file at: /home/runner/work/MQTTX/MQTTX/.nvmrc does not exist

@Red-Asuka
Copy link
Member

By the way, how does json-bigint perform compared to the native method? Do you have any relevant tests to share?

@ni00
Copy link
Contributor Author

ni00 commented Dec 1, 2023

By the way, how does json-bigint perform compared to the native method? Do you have any relevant tests to share?

I ran some code tests, and found that in the case of parsing and stringifying 10,000 times, the JSON-BigInt parser took roughly three times longer than the native JSON parser. I think this time overhead is completely acceptable.

Test Code:

const JSONbigNative = require("json-bigint")({ useNativeBigInt: true });

// Test String
const str = `
{"code":10000,"value":{"phoneBTMac":"11:1A:2B:3C:4D:AA","pinCode":134123,"serializable":172444261856920371423423424,"userId":16888884830636646423242342}}
`;

// Native
console.time("Native");
for (let i = 0; i < 10000; i++) {
  JSON.stringify(JSON.parse(str));
}
console.timeEnd("Native");

// Bigint
console.time("Bigint");
for (let i = 0; i < 10000; i++) {
  JSONbigNative.stringify(JSONbigNative.parse(str));
}
console.timeEnd("Bigint");

Output:
Native: 22.733ms
Bigint: 76.5ms

@ni00
Copy link
Contributor Author

ni00 commented Dec 1, 2023

The path of the .nvmrc file configured in action is incorrect, can you fix it by the way?

Error: The specified node version file at: /home/runner/work/MQTTX/MQTTX/.nvmrc does not exist

Okay, I have fixed the error in the CI. The path to the .nvmrc file was correct; it was just the order of the commands that was wrong. I adjusted the order according to the build-cli.yaml, and now the deploy-web can run successfully. But I'm also a little puzzled about why the CI would fail due to the command order. @ysfscream @Red-Asuka

@ni00
Copy link
Contributor Author

ni00 commented Dec 1, 2023

In my recent test, using json-bigint and native bigint will cause the JSON string to fail to parse floating-point numbers (because it will treat all numbers as bigint), which will lead to serious errors, so I will modify the code. Please do not merge this PR for the time being.

eg:
SyntaxError: Cannot convert 7.2444261856920371423423424 to a BigInt

@ni00 ni00 marked this pull request as draft December 1, 2023 15:49
@ni00
Copy link
Contributor Author

ni00 commented Dec 2, 2023

I fixed the previous issue and optimized the code. JavaScript has a built-in bigint type for handling long integers, but it lacks a built-in type for long floating-point numbers, so I introduced a fallback with BigNumber.

I used the native JSON library to handle MQTTX configuration files (such as user parameters, meta parameters, etc.), protobuf schema content, and script content. This is because MQTTX configuration files do not contain long integers or long floating-point numbers. Additionally, the protobuf library's validation only supports the native JavaScript number type and does not support extended bigint and bignumber types. Furthermore, the scripts use JavaScript operations and the sandbox does not have a built-in bignumber, which may result in loss of precision. Therefore, the native JSON library is sufficient for these purposes.

When the JSON payload does not contain floating-point values internally, I used JSONBigInt, which is almost identical to native JSON, except that the number type becomes bigint, while the object structure remains unchanged. Conversely, I used JSONBigNumber when the JSON file contains floating-point numbers. After parsing the JSON, long integers and floating-point numbers will be represented uniformly as BigNumber objects. When stringified, floating-point numbers will remain unchanged, while long integers will be converted to scientific notation floating-point numbers (which is supported by JSON). Both JSONBigInt and JSONBigNumber only affect the payload and do not alter the specific content.

image

@ni00 ni00 marked this pull request as ready for review December 2, 2023 13:33
@ni00 ni00 requested a review from Red-Asuka December 2, 2023 13:33
@ysfscream
Copy link
Member

@ni00 Sure. Thank you very much for your effort and the solution to the JSON precision issue in MQTTX. Using JavaScript's bigint and BigNumber to handle long integers and floating-point numbers is a great approach. Thank you again for your contribution! I will merge the PR and test it locally.

@@ -0,0 +1,24 @@
const JSONBigNumber = require('json-bigint')
const JSONBigInt = require('json-bigint')({ useNativeBigInt: true })
Copy link
Member

Choose a reason for hiding this comment

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

Can it be changed to ES6 import?

Copy link
Member

Choose a reason for hiding this comment

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

It's just a matter of code style. I will merge the PR first.

@Red-Asuka Red-Asuka merged commit 3e82b95 into emqx:main Dec 4, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI MQTTX CLI desktop MQTTX Desktop enhancement New feature or request fix Fix bug or issues web MQTTX Web
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants