feat(core): add support for cjson_encode_number_precision

[sabertobihwy]

This commit introduces a new configuration option called cjson_encode_number_precision. It specifies the precision used when encoding floating point numbers to JSON. The default value is 14, and the maximum value is 16. The commit also includes changes to various modules that make use of this configuration option.

Summary

Checklist

• The Pull Request has tests
• There's an entry in the CHANGELOG
• There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Full changelog

• [Implement ...]

Issue reference

Fix FTI-5279

[CLAassistant]

All committers have signed the CLA.

[sabertobihwy]

There are some points to discuss:

• kong/pdk/vault.lua also has a local cjson = require(“cjson.safe”).new(), but it doesn’t use the cjson.encode function, so I didn’t handle it.
• If a module calls require("cjson.safe").new(), then the cjson.encode_sparse_array(nil, nil, 2^15) in globalpatches.lua will become ineffective. Is this the expected behavior?

[flrgh]

There are some points to discuss:

• kong/pdk/vault.lua also has a local cjson = require(“cjson.safe”).new(), but it doesn’t use the cjson.encode function, so I didn’t handle it.

• If a module calls require("cjson.safe").new(), then the cjson.encode_sparse_array(nil, nil, 2^15) in globalpatches.lua will become ineffective. Is this the expected behavior?

I have an idea. What about wrapping everything up in a kong/tools/json.lua module?

local _M = {}

local cjson = require("cjson")
local cjson_safe = require("cjson.safe")

local conf

_M.encode = cjson.encode
_M.decode = cjson.decode

_M.safe = {
  encode = cjson_safe.encode,
  decode = cjson_safe.decode,
}

function _M.configure(kong_conf)
  conf = kong_conf
  -- set encode_number_precision and other properties accordingly
end

function _M.new()
  local inst = cjson.new()
  if conf then
    -- set properties from conf accordingly
  end
  return inst
end

return _M

There's a chicken/egg condition with .new() and .configure(). It's mildly annoying to solve, but it can be done:

local instances = setmetatable({}, { __mode = "k" })

instances[cjson] = true
instances[cjson_safe] = true

-- repeat this for _M.safe.new, of course
function _M.new()
  local inst = cjson.new()
  if conf then
    -- set properties from conf accordingly
  end

  -- stash it in our registry
  instances[inst] = true

  return inst
end

function _M.configure(kong_conf)
  conf = kong_conf
  
  for inst in pairs(instances) do
    -- set encode_number_precision and other properties accordingly
  end
end

It's not 100% air-tight, but it should cover most cases. We could even expose this in the PDK at _G.kong.json or something of that nature. Existing code that uses cjson will just need s/cjson/kong.tools.json/.

[sabertobihwy]

It's not 100% air-tight, but it should cover most cases. We could even expose this in the PDK at _G.kong.json or something of that nature. Existing code that uses cjson will just need s/cjson/kong.tools.json/.

Reasonable.

I think it need to open another PR to handle this.

[StarlightIbuki]

I feel it is more direct and intuitive to specify the default value instead of relying on the libraries' default behavior.

[ADD-SP]

Is there any context for this PR? Which issues is this PR going to fix?

It pains me to add this configuration option like this, we don't want to add configuration options if we don't have to, because it would make configuring Kong more complicated.

[ms2008]

If a module calls require("cjson.safe").new(), then the cjson.encode_sparse_array(nil, nil, 2^15) in globalpatches.lua will become ineffective. Is this the expected behavior?

I think this issue should also be addressed in this PR to ensure that cjson behavior is consistent so as not to annoy users.

[sabertobihwy]

Is there any context for this PR? Which issues is this PR going to fix?

background: https://konghq.atlassian.net/browse/FTI-5265?focusedCommentId=108202

Simply put, the default precision (14) cannot meet the needs of some users.

It pains me to add this configuration option like this, we don't want to add configuration options if we don't have to, because it would make configuring Kong more complicated.

I also don’t like adding configurations. Another way is to increase the default precision from 14 to 16, but this is a breaking change, so I didn’t do it initially.

[sabertobihwy]

I think this issue should also be addressed in this PR to ensure that cjson behavior is consistent so as not to annoy users.

I think it would be better to complete this with another PR that matches the solution proposed by @flrgh , otherwise this PR will lose its original purpose and become bloated.

[sabertobihwy]

I feel it is more direct and intuitive to specify the default value instead of relying on the libraries' default behavior.

Makes sense, should we declare a default precision of 14 in globalpatchers?

[StarlightIbuki]

I feel it is more direct and intuitive to specify the default value instead of relying on the libraries' default behavior.

Makes sense, should we declare a default precision of 14 in globalpatchers?

We can make it the default value of the configuration and always pass the argument.

[sabertobihwy]

We can make it the default value of the configuration and always pass the argument.

That is a breaking change, and I am concerned because not all users require 16-bit precision.

[sabertobihwy]

So now there is a disagreement: whether to upgrade the default precision from 14 digits to 16 digits.
My opinion is no.
What is your opinion? cc @StarlightIbuki @ADD-SP @bungle

[StarlightIbuki]

So now there is a disagreement: whether to upgrade the default precision from 14 digits to 16 digits. My opinion is no. What is your opinion? cc @StarlightIbuki @ADD-SP @bungle

I did not mean we need 16 as default. I mean 14 as the default but explicitly passed to the lib.

[vm-001]

Please add a changelog file

[kikito]

After considernig this in PR queue review, we are closing this. It is not a burning issue, it's controversial and the FTI ticket was resolved in a different way.

Please feel free to reopen.