From c2e9f99a7050dcb78b004a7534361442fdd03c16 Mon Sep 17 00:00:00 2001 From: Aapo Talvensaari Date: Fri, 9 Aug 2024 16:11:03 +0300 Subject: [PATCH] fix(schema): deprecated shorthand fields precedence ### Summary 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 --- .../fix-deprecate-shorthands-precedence.yml | 3 ++ kong/api/routes/plugins.lua | 48 +++++------------ kong/db/dao/plugins.lua | 31 ++++++----- kong/db/schema/init.lua | 12 ++++- .../01-db/01-schema/01-schema_spec.lua | 54 +++++++++++++++++++ 5 files changed, 98 insertions(+), 50 deletions(-) create mode 100644 changelog/unreleased/fix-deprecate-shorthands-precedence.yml diff --git a/changelog/unreleased/fix-deprecate-shorthands-precedence.yml b/changelog/unreleased/fix-deprecate-shorthands-precedence.yml new file mode 100644 index 000000000000..5053cbca2748 --- /dev/null +++ b/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 diff --git a/kong/api/routes/plugins.lua b/kong/api/routes/plugins.lua index 38a7f9485082..6f4989f325c6 100644 --- a/kong/api/routes/plugins.lua +++ b/kong/api/routes/plugins.lua @@ -76,28 +76,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 @@ -108,23 +103,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 diff --git a/kong/db/dao/plugins.lua b/kong/db/dao/plugins.lua index bdb8e0c37c1b..e1b198b51d38 100644 --- a/kong/db/dao/plugins.lua +++ b/kong/db/dao/plugins.lua @@ -11,6 +11,7 @@ local Plugins = {} local fmt = string.format +local type = type local null = ngx.null local pairs = pairs local tostring = tostring @@ -64,9 +65,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 " .. @@ -89,18 +89,23 @@ 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 + 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 + local 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 + end + local ok, err, err_t = check_protocols_match(self, entity) + if not ok then + return nil, err, err_t + end end - - options.hide_shorthands = false return self.super.update(self, primary_key, entity, options) end diff --git a/kong/db/schema/init.lua b/kong/db/schema/init.lua index 899349ba75fe..32578af5efae 100644 --- a/kong/db/schema/init.lua +++ b/kong/db/schema/init.lua @@ -1743,10 +1743,18 @@ function Schema:process_auto_fields(data, context, nulls, opts) data[sname] = nil local new_values = sdata.func(value) 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 - data[k] = table_merge(data[k] or {}, v) - else + data[k] = deprecation and table_merge(v, data[k]) + or table_merge(data[k] or {}, v) + elseif not deprecation or data[k] == nil then data[k] = v end end diff --git a/spec/01-unit/01-db/01-schema/01-schema_spec.lua b/spec/01-unit/01-db/01-schema/01-schema_spec.lua index d8d669210ff4..5981e1dd096e 100644 --- a/spec/01-unit/01-db/01-schema/01-schema_spec.lua +++ b/spec/01-unit/01-db/01-schema/01-schema_spec.lua @@ -4110,6 +4110,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",