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.

Sign up for GitHub

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(pdk.log): fix a bug that pdk.log.serialize() does not properly handle JSON nulls #13376

Merged
merged 5 commits into from
Jul 22, 2024
Merged

fix(pdk.log): fix a bug that pdk.log.serialize() does not properly handle JSON nulls #13376

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2699ef2
fix(pdk.log): fix a bug that pdk.log.serialize() will throw an error
liverpool8056 Jul 15, 2024
8ea095b
add changelog
liverpool8056 Jul 15, 2024
a6ac9f7
Update changelog/unreleased/kong/pdk-log-error.yml
liverpool8056 Jul 15, 2024
82f3f5e
fix
liverpool8056 Jul 16, 2024
389bf7f
handle cdata null
liverpool8056 Jul 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelog/unreleased/kong/pdk-log-error.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
message: Fixed a bug where `pdk.log.serialize()` threw an error when Json entity set by `kong.log.serialize_value()` contains `cjson.null`.
chronolaw marked this conversation as resolved.
Show resolved Hide resolved
type: bugfix
scope: PDK
3 changes: 2 additions & 1 deletion kong/pdk/log.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ local inspect = require("inspect")
local phase_checker = require("kong.pdk.private.phases")
local constants = require("kong.constants")
local clear_tab = require("table.clear")
local cjson_null = require("cjson").null


local request_id_get = require("kong.observability.tracing.request_id").get
Expand Down Expand Up @@ -640,7 +641,7 @@ do

local function is_valid_value(v, visited)
local t = type(v)
if v == nil or t == "number" or t == "string" or t == "boolean" then
if v == nil or v == cjson_null or t == "number" or t == "string" or t == "boolean" then
Copy link
Contributor

@fffonion fffonion Jul 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

not a blocker for this PR, but we should also exclude FFI null from it

local ffi = require "ffi"

local n = ffi.new("void*")
print(tostring(n))
print(n == nil)

local cjson = require "cjson"

print(cjson.encode(n))

outputs

cdata<void *>: NULL
true
ERROR: /tmp/a.lua:9: Cannot serialise cdata: type not supported

return true
end

Expand Down
8 changes: 8 additions & 0 deletions spec/01-unit/10-log_serializer_spec.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,14 @@ describe("kong.log.serialize", function()
assert.not_equal(tostring(ngx.ctx.service),
tostring(res.service))
end)

it("does not fail when coming across 'cjson.null' in response body", function()
local cjson_null = require "cjson".null
kong.log.set_serialize_value("response.body", cjson_null)
local pok, value = pcall(kong.log.serialize, {})
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why it need be wrap by a pcall

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just have a change to handle exception thrown from the serialize()

assert.is_true(pok)
assert.is_true(type(value) == "table")
end)
end)
end)

Expand Down
Loading