fix(plugins): Request-Transformer rename behavior

Fix KAG-4915

changelog/unreleased/kong/req-trans-rename.yml

@@ -0,0 +1,3 @@
+message: "**Request-Transformer**: Fixed an issue where renamed query parameters, url-encoded body parameters, and json body parameters were not handled properly when target name is the same as the source name in the request."
+type: feature
+scope: Plugin

kong/plugins/request-transformer/access.lua

@@ -147,6 +147,19 @@ local function append_value(current_value, value)
   end
 end
 
+local function rename(tbl, old_name, new_name)
+  if old_name == new_name then
+    return
+  end
+
+  local value = tbl[old_name]
+  if value then
+    tbl[old_name] = nil
+    tbl[new_name] = value
+    return true
+  end
+end
+
 local function transform_headers(conf, template_env)
   local headers = get_headers()
   local headers_to_remove = {}
@@ -164,11 +177,17 @@ local function transform_headers(conf, template_env)
 
   -- Rename headers(s)
   for _, old_name, new_name in iter(conf.rename.headers, template_env) do
-    old_name = old_name:lower()
-    local value = headers[old_name]
-    if value then
-      headers[new_name:lower()] = value
-      headers[old_name] = nil
+    local lower_old_name, lower_new_name = old_name:lower(), new_name:lower()
+    -- headers by default are case-insensitive
+    -- but if we have a case change, we need to handle it as a special case
+    local need_remove
+    if lower_old_name == lower_new_name then
+      need_remove = rename(headers, old_name, new_name)
+    else
+      need_remove = rename(headers, lower_old_name, lower_old_name)
+    end
+
+    if need_remove then
       headers_to_remove[old_name] = true
     end
   end
@@ -229,9 +248,7 @@ local function transform_querystrings(conf, template_env)
 
   -- Rename querystring(s)
   for _, old_name, new_name in iter(conf.rename.querystring, template_env) do
-    local value = querystring[old_name]
-    querystring[new_name] = value
-    querystring[old_name] = nil
+    rename(querystring, old_name, new_name)
   end
 
   for _, name, value in iter(conf.replace.querystring, template_env) do
@@ -274,10 +291,7 @@ local function transform_json_body(conf, body, content_length, template_env)
 
   if content_length > 0 and #conf.rename.body > 0 then
     for _, old_name, new_name in iter(conf.rename.body, template_env) do
-      local value = parameters[old_name]
-      parameters[new_name] = value
-      parameters[old_name] = nil
-      renamed = true
+      renamed = rename(parameters, old_name, new_name) or renamed
     end
   end
 
@@ -325,10 +339,7 @@ local function transform_url_encoded_body(conf, body, content_length, template_e
 
   if content_length > 0 and #conf.rename.body > 0 then
     for _, old_name, new_name in iter(conf.rename.body, template_env) do
-      local value = parameters[old_name]
-      parameters[new_name] = value
-      parameters[old_name] = nil
-      renamed = true
+      renamed = rename(parameters, old_name, new_name) or renamed
     end
   end
 
@@ -369,8 +380,9 @@ local function transform_multipart_body(conf, body, content_length, content_type
 
   if content_length > 0 and #conf.rename.body > 0 then
     for _, old_name, new_name in iter(conf.rename.body, template_env) do
-      if parameters:get(old_name) then
-        local value = parameters:get(old_name).value
+      local para = parameters:get(old_name)
+      if para and old_name ~= new_name then
+        local value = para.value
         parameters:set_simple(new_name, value)
         parameters:delete(old_name)
         renamed = true

spec/03-plugins/36-request-transformer/02-access_spec.lua

@@ -2,6 +2,9 @@ local admin_api = require "spec.fixtures.admin_api"
 local helpers = require "spec.helpers"
 local cjson   = require "cjson"
 local pl_file = require "pl.file"
+local http_mock = require "spec.helpers.http_mock"
+
+local MOCK_PORT = helpers.get_available_port()
 
 local fmt = string.format
 
@@ -16,7 +19,7 @@ end
 
 for _, strategy in helpers.each_strategy() do
 describe("Plugin: request-transformer(access) [#" .. strategy .. "]", function()
-  local client
+  local client, mock_server
 
   lazy_setup(function()
     local bp = helpers.get_db_utils(strategy, {
@@ -487,6 +490,30 @@ describe("Plugin: request-transformer(access) [#" .. strategy .. "]", function()
       }
     }
 
+    do -- rename case tests
+      -- as assert.request(r) does not support case-sensitive header checks
+      -- we need to use a mock server
+      mock_server = http_mock.new(MOCK_PORT)
+      mock_server:start()
+      local mock_service = bp.services:insert {
+        url = "http://localhost:" .. MOCK_PORT
+      }
+      local route = bp.routes:insert {
+        hosts = { "rename.mock" },
+        service = mock_service,
+      }
+      bp.plugins:insert {
+        route = { id = route.id },
+        name = "request-transformer",
+        config = {
+          rename = {
+            headers = { "rename:Rename" },
+            querystring = { "inexist:exist" },
+          }
+        }
+      }
+    end
+
     assert(helpers.start_kong({
       database = strategy,
       plugins = "bundled, request-transformer",
@@ -495,6 +522,7 @@ describe("Plugin: request-transformer(access) [#" .. strategy .. "]", function()
   end)
 
   lazy_teardown(function()
+    mock_server:stop()
     helpers.stop_kong()
   end)
 
@@ -909,6 +937,35 @@ describe("Plugin: request-transformer(access) [#" .. strategy .. "]", function()
       local json = assert.request(r).has.jsonbody()
       assert.equals("{\"emptyarray\":[]}", json.data)
     end)
+    it("rename correctly when only changing capitalization", function()
+      local r = assert(client:send {
+        method = "GET",
+        path = "/request",
+        headers = {
+          host = "rename.mock",
+          ["rename"] = "true",
+        }
+      })
+      assert.response(r).has.status(200)
+      local ret = mock_server:get_request()
+
+      assert.equals("true", ret.headers["Rename"])
+      assert.is_nil(ret.headers["rename"])
+    end)
+    -- but should we override with a value?
+    it("does not override existing value with nil", function()
+      local r = assert(client:send {
+        method = "GET",
+        path = "/request?exist=true",
+        headers = {
+          host = "rename.mock",
+        }
+      })
+      assert.response(r).has.status(200)
+      local ret = mock_server:get_request()
+      print(require"inspect"(ret))
+      assert.equals("/request?exist=true", ret.uri)
+    end)
   end)
 
   describe("replace", function()