update based on the feedback from the review

Signed-off-by: sabertobihwy <[email protected]>

CHANGELOG/unreleased/kong/11566.yaml

@@ -1,4 +1,4 @@
-message: "use deep copies of Route, Service, and Consumer objects when log serialize"
+message: "use deep copies of Route, Service, and Consumer objects when log serializing"
 type: bugfix
 scope: PDK
 prs:

kong/pdk/log.lua

@@ -16,7 +16,7 @@ local inspect = require "inspect"
 local ngx_ssl = require "ngx.ssl"
 local phase_checker = require "kong.pdk.private.phases"
 local utils = require "kong.tools.utils"
-
+local cycle_aware_deep_copy = utils.cycle_aware_deep_copy
 
 local sub = string.sub
 local type = type
@@ -802,7 +802,7 @@ do
         end
       end
 
-      -- The value of upstream_status is a string, and status codes may be 
+      -- The value of upstream_status is a string, and status codes may be
       -- seperated by comma or grouped by colon, according to
       -- the nginx doc: http://nginx.org/en/docs/http/ngx_http_upstream_module.html#upstream_status
       local upstream_status = var.upstream_status or ""
@@ -832,9 +832,9 @@ do
         },
         tries = (ctx.balancer_data or {}).tries,
         authenticated_entity = build_authenticated_entity(ctx),
-        route = utils.cycle_aware_deep_copy(ctx.route),
-        service = utils.cycle_aware_deep_copy(ctx.service),
-        consumer = utils.cycle_aware_deep_copy(ctx.authenticated_consumer),
+        route = cycle_aware_deep_copy(ctx.route),
+        service = cycle_aware_deep_copy(ctx.service),
+        consumer = cycle_aware_deep_copy(ctx.authenticated_consumer),
         client_ip = var.remote_addr,
         started_at = okong.request.get_start_time(),
       }
@@ -873,9 +873,9 @@ do
         },
         tries = (ctx.balancer_data or {}).tries,
         authenticated_entity = build_authenticated_entity(ctx),
-        route = utils.cycle_aware_deep_copy(ctx.route),
-        service = utils.cycle_aware_deep_copy(ctx.service),
-        consumer = utils.cycle_aware_deep_copy(ctx.authenticated_consumer),
+        route = cycle_aware_deep_copy(ctx.route),
+        service = cycle_aware_deep_copy(ctx.service),
+        consumer = cycle_aware_deep_copy(ctx.authenticated_consumer),
         client_ip = var.remote_addr,
         started_at = okong.request.get_start_time(),
       }

spec/01-unit/10-log_serializer_spec.lua

@@ -31,7 +31,7 @@ describe("kong.log.serialize", function()
           bytes_sent = "99",
           request_time = "2",
           remote_addr = "1.1.1.1",
-          -- may be a non-numeric string, 
+          -- may be a non-numeric string,
           -- see http://nginx.org/en/docs/http/ngx_http_upstream_module.html#var_upstream_addr
           upstream_status = "500, 200 : 200, 200",
         },
@@ -116,7 +116,7 @@ describe("kong.log.serialize", function()
       end)
 
       it("serializes the Consumer object", function()
-        ngx.ctx.authenticated_consumer = {id = "someconsumer"}
+        ngx.ctx.authenticated_consumer = { id = "someconsumer" }
 
         local res = kong.log.serialize({ngx = ngx, kong = kong, })
         assert.is_table(res)
@@ -187,9 +187,9 @@ describe("kong.log.serialize", function()
         assert.equal("/upstream_uri" .. "?" .. args, res.upstream_uri)
       end)
 
-      it("use the deep copies of the Route, Service, Consumer object avoid " .. 
+      it("use the deep copies of the Route, Service, Consumer object avoid " ..
          "modify ctx.authenticated_consumer, ctx.route, ctx.service", function()
-        ngx.ctx.authenticated_consumer = {id = "someconsumer"}
+        ngx.ctx.authenticated_consumer = { id = "someconsumer" }
         ngx.ctx.route = { id = "my_route" }
         ngx.ctx.service = { id = "my_service" }
         local res = kong.log.serialize({ngx = ngx, kong = kong, })
@@ -297,7 +297,7 @@ describe("kong.log.serialize", function()
       end)
 
       it("serializes the Consumer object", function()
-        ngx.ctx.authenticated_consumer = {id = "someconsumer"}
+        ngx.ctx.authenticated_consumer = { id = "someconsumer" }
 
         local res = kong.log.serialize({ngx = ngx, kong = kong, })
         assert.is_table(res)
@@ -356,9 +356,9 @@ describe("kong.log.serialize", function()
         assert.is_nil(res.tries)
       end)
 
-      it("use the deep copies of the Route, Service, Consumer object avoid " .. 
+      it("use the deep copies of the Route, Service, Consumer object avoid " ..
          "modify ctx.authenticated_consumer, ctx.route, ctx.service", function()
-        ngx.ctx.authenticated_consumer = {id = "someconsumer"}
+        ngx.ctx.authenticated_consumer = { id = "someconsumer "}
         ngx.ctx.route = { id = "my_route" }
         ngx.ctx.service = { id = "my_service" }
         local res = kong.log.serialize({ngx = ngx, kong = kong, })

spec/02-integration/07-sdk/02-log_spec.lua

@@ -125,8 +125,9 @@ describe("PDK: kong.log", function()
 end)
 
 for _, strategy in helpers.each_strategy() do
-  describe("PDK: kong.log.serialize [#" .. strategy .. "]", function()
-    describe("http subsystem 1", function()
+  describe("PDK: make sure kong.log.serialize() will not modify ctx which's lifecycle " ..
+           "is across request [#" .. strategy .. "]", function()
+    describe("ctx.authenticated_consumer", function()
       local proxy_client
       local bp
 
@@ -207,7 +208,7 @@ for _, strategy in helpers.each_strategy() do
         helpers.stop_kong()
       end)
 
-      it("use the deep copy of Consumer object in serialize function", function()
+      it("use the deep copy of Consumer object", function()
         for i = 1, 3 do
           local res = proxy_client:send {
             method  = "GET",
@@ -221,7 +222,7 @@ for _, strategy in helpers.each_strategy() do
       end)
     end)
 
-    describe("http subsystem 2", function()
+    describe("ctx.service", function()
       local proxy_client
       local bp
 
@@ -285,7 +286,7 @@ for _, strategy in helpers.each_strategy() do
         helpers.stop_kong()
       end)
 
-      it("use the deep copy of Service object in serialize function", function()
+      it("use the deep copy of Service object", function()
         for i = 1, 3 do
           local res = proxy_client:send {
             method  = "GET",
@@ -299,7 +300,7 @@ for _, strategy in helpers.each_strategy() do
       end)
     end)
 
-    describe("http subsystem 3", function()
+    describe("ctx.route", function()
       local proxy_client
       local bp
 
@@ -366,7 +367,7 @@ for _, strategy in helpers.each_strategy() do
         helpers.stop_kong()
       end)
 
-      it("use the deep copy of Route object in serialize function", function()
+      it("use the deep copy of Route object", function()
         for i = 1, 3 do
           local res = proxy_client:send {
             method  = "GET",
@@ -380,7 +381,7 @@ for _, strategy in helpers.each_strategy() do
       end)
     end)
 
-    describe("strean subsystem", function()
+    describe("in strean subsystem# ctx.authenticated_consumer", function()
       local proxy_client
       local bp
 
@@ -446,7 +447,7 @@ for _, strategy in helpers.each_strategy() do
         helpers.stop_kong()
       end)
 
-      it("use the deep copy of Service object in serialize function", function()
+      it("use the deep copy of Service object", function()
         for i = 1, 3 do
           local tcp_client = ngx.socket.tcp()
           assert(tcp_client:connect(helpers.get_proxy_ip(false), 19000))