Skip to content

Commit

Permalink
fix(schema): deprecated shorthand fields precedence
Browse files Browse the repository at this point in the history
### 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 <[email protected]>
  • Loading branch information
bungle committed Aug 12, 2024
1 parent b50cf86 commit df32e55
Show file tree
Hide file tree
Showing 5 changed files with 98 additions and 49 deletions.
3 changes: 3 additions & 0 deletions changelog/unreleased/fix-deprecate-shorthands-precedence.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
message: Deprecated shorthand fields don't take precedence over replacement fields when both are specified.
type: bugfix
scope: Core
47 changes: 13 additions & 34 deletions kong/api/routes/plugins.lua
Original file line number Diff line number Diff line change
Expand Up @@ -78,25 +78,21 @@ local function patch_plugin(self, db, _, parent)
local post = self.args and self.args.post

-- 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 post then
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

plugin = plugin or {}
if not plugin then
return kong.response.exit(404, { message = "Not found" })
end

post.name = post.name or plugin.name
post.name = plugin.name
end

-- Only now we can decode the 'config' table for form-encoded values
local content_type = ngx.var.content_type
Expand All @@ -108,23 +104,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

Expand Down
31 changes: 18 additions & 13 deletions kong/db/dao/plugins.lua
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ local Plugins = {}


local fmt = string.format
local type = type
local null = ngx.null
local pairs = pairs
local tostring = tostring
Expand Down Expand Up @@ -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 " ..
Expand All @@ -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

Expand Down
12 changes: 10 additions & 2 deletions kong/db/schema/init.lua
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
54 changes: 54 additions & 0 deletions spec/01-unit/01-db/01-schema/01-schema_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down

0 comments on commit df32e55

Please sign in to comment.