fix(schema): deprecated shorthand fields precedence

The shorthand fields e.g. as used in (the url is a shorthand) take
precedence:
```
http PUT :8001/services/test url=http://test.org/ port=2345
```

That means that `port=2345` will be overwritten by http default
port of `80` that is implicit in shorthand field `url`.

This is how it has been for a long long time.

In PR #12686 we added `deprecation` field to shorthand fields definition.

Some of our automatic migrations without database migrations magic use
this functionality, but the precedence there does not good:

```
http -f PUT :8001/plugins/x name=x config.new=test config.old=should_be_ignored
```

In above the `config.old` is a shorthand field with a deprecation property.
Thus it should not overwrite the `config.new`, but in current code base it
does. This PR changes it so that it doesn't do it anymore.

KAG-5134 and https://kongstrong.slack.com/archives/C07AQH7SAF8/p1722589141558609

Signed-off-by: Aapo Talvensaari <[email protected]>

Author: bungle

changelog/unreleased/fix-deprecate-shorthands-precedence.yml

@@ -0,0 +1,3 @@
+message: Deprecated shorthand fields don't take precedence over replacement fields when both are specified.
+type: bugfix
+scope: Core

kong/api/routes/plugins.lua

@@ -83,28 +83,23 @@ end
 
 local function patch_plugin(self, db, _, parent)
   local post = self.args and self.args.post
+  if post then
+    -- Read-before-write only if necessary
+    if post.name == nil then
+      -- We need the name, otherwise we don't know what type of
+      -- plugin this is and we can't perform *any* validations.
+      local plugin, _, err_t = endpoints.select_entity(self, db, db.plugins.schema)
+      if err_t then
+        return endpoints.handle_error(err_t)
+      end
 
-  -- Read-before-write only if necessary
-  if post and (post.name     == nil or
-               post.route    == nil or
-               post.service  == nil or
-               post.consumer == nil) then
-
-    -- We need the name, otherwise we don't know what type of
-    -- plugin this is and we can't perform *any* validations.
-    local plugin, _, err_t = endpoints.select_entity(self, db, db.plugins.schema)
-    if err_t then
-      return endpoints.handle_error(err_t)
-    end
+      if not plugin then
+        return kong.response.exit(404, { message = "Not found" })
+      end
 
-    if not plugin then
-      return kong.response.exit(404, { message = "Not found" })
+      post.name = plugin.name
     end
 
-    plugin = plugin or {}
-
-    post.name = post.name or plugin.name
-
     -- Only now we can decode the 'config' table for form-encoded values
     local content_type = ngx.var.content_type
     if content_type then
@@ -115,23 +110,6 @@ local function patch_plugin(self, db, _, parent)
       end
     end
 
-    -- While we're at it, get values for composite uniqueness check
-    post.route = post.route or plugin.route
-    post.service = post.service or plugin.service
-    post.consumer = post.consumer or plugin.consumer
-
-    if not post.route and self.params.routes then
-      post.route = { id = self.params.routes }
-    end
-
-    if not post.service and self.params.services then
-      post.service = { id = self.params.services }
-    end
-
-    if not post.consumer and self.params.consumers then
-      post.consumer = { id = self.params.consumers }
-    end
-
     self.args.post = post
   end
 

kong/db/dao/plugins.lua

@@ -23,6 +23,7 @@ local Plugins = {}
 
 
 local fmt = string.format
+local type = type
 local null = ngx.null
 local pairs = pairs
 local tostring = tostring
@@ -75,9 +76,8 @@ local function check_protocols_match(self, plugin)
       })
       return nil, tostring(err_t), err_t
     end
-  end
 
-  if type(plugin.service) == "table" then
+  elseif type(plugin.service) == "table" then
     if not has_common_protocol_with_service(self, plugin, plugin.service) then
       local err_t = self.errors:schema_violation({
         protocols = "must match the protocols of at least one route " ..
@@ -95,9 +95,9 @@ local function check_ordering_validity(self, entity)
   --[[
     Plugins that are scoped to a consumer can't be a target for reordering
     because they rely on a context (ngx.authenticated_consumer) which is only
-    set during the access phase of an authentacation plugin. This means that
-    we can't influence the order of plugins without running their access phase first
-    which is a catch-22.
+    set during the access phase of an authentication plugin. This means that
+    we can't influence the order of plugins without running their access phase
+    first which is a catch-22.
   --]]
   if type(entity.ordering) ~= "table" then
     -- no reordering requested, no need to validate further
@@ -132,22 +132,38 @@ end
 
 
 function Plugins:update(primary_key, entity, options)
-  options = options or {}
-  options.hide_shorthands = true
-  local rbw_entity = self.super.select(self, primary_key, options) -- ignore errors
-  if rbw_entity then
-    entity = self.schema:merge_values(entity, rbw_entity)
-  end
-  local ok, err, err_t = check_protocols_match(self, entity)
-  if not ok then
-    return nil, err, err_t
-  end
-  local ok_o, err_o, err_t_o = check_ordering_validity(self, entity)
-  if not ok_o then
-    return nil, err_o, err_t_o
+  local rbw_entity
+  if entity.protocols or entity.service or entity.route then
+    if (entity.protocols and not entity.route)
+    or (entity.service and not entity.protocols)
+    or (entity.route and not entity.protocols)
+    then
+      rbw_entity = self.super.select(self, primary_key, options)
+      if rbw_entity then
+        entity.protocols = entity.protocols or rbw_entity.protocols
+        entity.service = entity.service or rbw_entity.service
+        entity.route = entity.route or rbw_entity.route
+      end
+      rbw_entity = rbw_entity or {}
+    end
+    local ok, err, err_t = check_protocols_match(self, entity)
+    if not ok then
+      return nil, err, err_t
+    end
   end
+  if entity.ordering or entity.consumer then
+    if not (rbw_entity or (entity.ordering and entity.consumer)) then
+      rbw_entity = self.super.select(self, primary_key, options) or {}
+    end
+
+    entity.ordering = entity.ordering or rbw_entity.ordering
+    entity.consumer = entity.consumer or rbw_entity.consumer
 
-  options.hide_shorthands = false
+    local ok_o, err_o, err_t_o = check_ordering_validity(self, entity)
+    if not ok_o then
+      return nil, err_o, err_t_o
+    end
+  end
   return self.super.update(self, primary_key, entity, options)
 end
 

kong/db/schema/init.lua

@@ -1758,14 +1758,24 @@ function Schema:process_auto_fields(data, context, nulls, opts)
           local read_only_data = cycle_aware_deep_copy(data)
           local new_values = sdata.func(value, read_only_data)
           if new_values then
+            -- a shorthand field may have a deprecation property, that is used
+            -- to determine whether the shorthand's return value takes precedence
+            -- over the similarly named actual schema fields' value when both
+            -- are present. On deprecated shorthand fields the actual schema
+            -- field value takes the precedence, otherwise the shorthand's
+            -- return value takes the precedence.
+            local deprecation = sdata.deprecation
             for k, v in pairs(new_values) do
               if type(v) == "table" then
                 local source = {}
-                if data[k] and data[k] ~= ngx.null then
+                if data[k] and data[k] ~= null then
                   source = data[k]
                 end
+                data[k] = deprecation and table_merge(v, source)
+                                       or table_merge(source, v)
+
                 data[k] = table_merge(source, v)
-              else
+              elseif not deprecation or data[k] == nil then
                 data[k] = v
               end
             end

spec/01-unit/01-db/01-schema/01-schema_spec.lua

@@ -4251,6 +4251,60 @@ describe("schema", function()
       assert.same({ name = "test1" }, output)
     end)
 
+    it("takes precedence", function()
+      local TestSchema = Schema.new({
+        name = "test",
+        fields = {
+          { name = { type = "string" } },
+        },
+        shorthand_fields = {
+          {
+            username = {
+              type = "string",
+              func = function(value)
+                return {
+                  name = value
+                }
+              end,
+            },
+          },
+        },
+      })
+
+      local input = { username = "test1", name = "ignored" }
+      local output, _ = TestSchema:process_auto_fields(input)
+      assert.same({ name = "test1" }, output)
+    end)
+
+    it("does not take precedence if deprecated", function()
+      local TestSchema = Schema.new({
+        name = "test",
+        fields = {
+          { name = { type = "string" } },
+        },
+        shorthand_fields = {
+          {
+            username = {
+              type = "string",
+              func = function(value)
+                return {
+                  name = value
+                }
+              end,
+              deprecation = {
+                message = "username is deprecated, please use name instead",
+                removal_in_version = "4.0",
+              },
+            },
+          },
+        },
+      })
+
+      local input = { username = "ignored", name = "test1" }
+      local output, _ = TestSchema:process_auto_fields(input)
+      assert.same({ name = "test1" }, output)
+    end)
+
     it("can produce multiple fields", function()
       local TestSchema = Schema.new({
         name = "test",