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(pdk): increase the precision of JSON number encoding from 14 to 16 decimals #12019

Merged
merged 7 commits into from
Jan 9, 2024

Conversation

ms2008
Copy link
Contributor

@ms2008 ms2008 commented Nov 15, 2023

Summary

Currently, when Kong is serializing JSON, it can only encode numbers with a precision of up to 14 decimals. For numbers with a precision exceeding 14 decimals, kong will output them using scientific notation, resulting in a loss of decimal precision. This also causes an issue where KIC cannot use integers exceeding 2^46 bits to configure the priority attribute of route.

Now, we are increasing this limitation to 16 decimals. With this change, we can safely store integers up to 2^53 bits without losing precision.

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

Fix FTI-5515

@pull-request-size pull-request-size bot added size/L and removed size/M labels Nov 15, 2023
@ms2008 ms2008 marked this pull request as ready for review November 15, 2023 14:21
@locao locao requested a review from zhongweiy November 21, 2023 17:42
@tzssangglass
Copy link
Member

as a remember: if some else that use require("cjson.safe").new() or require("cjson").new(), the encode_number_precision(16) would be invalid

releated: #11389

@zhongweiy
Copy link
Contributor

How are we going to solve or mitigate the concern from @tzssangglass ? Is it a bug in cjson package?

@ms2008
Copy link
Contributor Author

ms2008 commented Dec 28, 2023

Is it a bug in cjson package?

I don't think so. cjson.new is actually an instantiated copy of a separate Lua CJSON module. The new module has a separate persistent encoding buffer and default settings.

I think we can wrap the cjson.new method in the global patch and set cjson.encode_number_precision(16) by default for instances initialized through it, and that should solve the problem.

@tzssangglass
Copy link
Member

I think we can wrap the cjson.new method in the global patch and set cjson.encode_number_precision(16) by default for instances initialized through it, and that should solve the problem.

work for me, but it is a break change.

@ms2008 ms2008 force-pushed the pdk/json-encoding-number-precision branch 2 times, most recently from 8babe20 to dad99d0 Compare December 28, 2023 15:10
@Tieske
Copy link
Member

Tieske commented Dec 28, 2023

Didn't read up on the specific case here, but we ran into something similar before, see Tieske/lua-resty-ljsonschema#21 by @vm-001

Just leaving it here as info for whomever wants to double check.

@vm-001 vm-001 self-requested a review December 29, 2023 07:23
@ms2008 ms2008 force-pushed the pdk/json-encoding-number-precision branch from dad99d0 to a8740bf Compare December 30, 2023 06:34
@vm-001
Copy link
Contributor

vm-001 commented Jan 2, 2024

Just leaving it here as info for whomever wants to double check.

The PR Tieske/lua-resty-ljsonschema#21 fixed loss precision by string.format, and does not related to cjson lib.

kong/globalpatches.lua Outdated Show resolved Hide resolved
@ms2008 ms2008 force-pushed the pdk/json-encoding-number-precision branch 2 times, most recently from 97a6f1d to cb4fdc5 Compare January 4, 2024 15:58
@ms2008 ms2008 force-pushed the pdk/json-encoding-number-precision branch 4 times, most recently from 56e7849 to 88347c5 Compare January 5, 2024 09:55
@ms2008 ms2008 requested a review from dndx January 9, 2024 01:18
kong/globalpatches.lua Outdated Show resolved Hide resolved
ms2008 added 6 commits January 9, 2024 12:45
decimals

With this, we can safely store integers up to 2^53, with no loss of
precision. Before the change, cJSON started generating scientific
notation output at a much smaller value than 2^53.
…n of 16"

This reverts commit dad99d098a77f2c0a3693a767f87a3cb954b5e81.
and has settings with the encoding of numbers with a precision of 16
decimals
@ms2008 ms2008 force-pushed the pdk/json-encoding-number-precision branch from 503eb0e to f9de965 Compare January 9, 2024 04:45
@ms2008 ms2008 force-pushed the pdk/json-encoding-number-precision branch from f9de965 to 9deee70 Compare January 9, 2024 04:57
@dndx dndx merged commit 1c72eaf into master Jan 9, 2024
23 checks passed
@dndx dndx deleted the pdk/json-encoding-number-precision branch January 9, 2024 05:49
@@ -15,15 +17,16 @@ return function(options)
local meta = require "kong.meta"


local cjson = require("cjson.safe")
cjson.encode_sparse_array(nil, nil, 2^15)
local cjson_safe = require("cjson.safe")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we replace it with kong.tools.cjson?

Copy link
Member

Choose a reason for hiding this comment

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

I doubt you can, since this is in globalpatches, which gets loaded very early on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can't simply replace it, because currently, our global patch doesn't set cjson.decode_array_with_array_mt(true), whereas kong.tools.cjson does.

cjson.decode_array_with_array_mt(true)

Copy link
Contributor

Choose a reason for hiding this comment

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

Got. The code makes me a bit confusing.

@ms2008 ms2008 added the cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee label Jan 10, 2024
@team-gateway-bot
Copy link
Collaborator

Cherry-pick failed for master, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git remote add upstream https://github.com/kong/kong-ee
git fetch upstream master
git worktree add -d .worktree/cherry-pick-12019-to-master-to-upstream upstream/master
cd .worktree/cherry-pick-12019-to-master-to-upstream
git checkout -b cherry-pick-12019-to-master-to-upstream
ancref=$(git merge-base 2a3a013766d99887f6a0416c2a6abcf0ecae9b27 9deee708395ba714a2813bcdba35638b9bc86bea)
git cherry-pick -x $ancref..9deee708395ba714a2813bcdba35638b9bc86bea

windmgc pushed a commit that referenced this pull request Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee core/db core/pdk plugins/zipkin size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants