Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(dao) cassandra to handle ngx.null with check_*_constraints #3355

Closed
wants to merge 1 commit into from
Closed

fix(dao) cassandra to handle ngx.null with check_*_constraints #3355

wants to merge 1 commit into from

Conversation

bungle
Copy link
Member

@bungle bungle commented Mar 28, 2018 •
edited
Loading

Summary

Fix some issues with C* when doing verifications on constraints while the value was ngx.null.

Full changelog

  • Implement check for ngx.null in check_unique_constraints
  • Implement check for ngx.null in check_foreign_constaints

How to reproduce the problem

KONG_DATABASE=cassandra bin/kong start
http :8001/apis name=test upstream_url=http://www.google.com/ uris=/
http :8001/consumers username=test
http :8001/plugins name=request-transformer api_id=<API_ID> consumer_id=<CONSUMER_ID>
http patch :8001/plugins/<PLUGIN_ID> consumer_id:=null

Issues resolved

Fix #3308

@bungle bungle requested a review from thibaultcha March 28, 2018 21:06
@thibaultcha
Copy link
Member

No regression test case? :/

thibaultcha
thibaultcha reviewed Mar 28, 2018
View reviewed changes
kong/dao/db/cassandra.lua
@@ -357,7 +357,7 @@ local function check_unique_constraints(self, table_name, constraints, values, p

for col, constraint in pairs(constraints.unique) do
-- Only check constraints if value is non-null
if values[col] ~= nil then
if values[col] ~= nil and values[col] ~= ngx.null then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it was the schema_validator's responsibility to nullify the column value if ngx.null (JSON NULL) is given? As per the existing implementation here and here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, here we need to pass something to DAO. If we nil then that field will not be updated (or nullified). If we set it to cassandra.null then the generic implementations needs to know about the database. ngx.null is common way to tell the strategies to do something. Serialize function already handles this:

kong/kong/dao/db/cassandra.lua

Line 314 in 12244ec

if value == nil or value == ngx.null then

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah true. Alright, then I think all we need would be a regression test for this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right. I will be adding it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thibaultcha the test was added.

@bungle bungle added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/please review labels Mar 29, 2018
@bungle bungle force-pushed the fix/cassandra-ngx-null branch from 12244ec to e15802c Compare March 30, 2018 10:08
@bungle bungle added pr/please review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Mar 30, 2018
@bungle bungle force-pushed the fix/cassandra-ngx-null branch from e15802c to 4b1d0e0 Compare March 30, 2018 12:01
bungle
bungle commented Mar 30, 2018
View reviewed changes
kong/dao/schemas_validation.lua
local ok, err = schema.self_check(schema, t, options.dao, options.update)
if ok == false then
return false, nil, err
end
for _, column in ipairs(nil_c) do
t[column] = ngx.null
end
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this, but there is no other way around it. Only way is to update all the self_checks to work with ngx.nulls?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, this change makes sense for backwards-compatibility purposes.

thibaultcha
thibaultcha reviewed Apr 4, 2018
View reviewed changes
kong/dao/schemas_validation.lua
local ok, err = schema.self_check(schema, t, options.dao, options.update)
if ok == false then
return false, nil, err
end
for _, column in ipairs(nil_c) do
t[column] = ngx.null
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, this change makes sense for backwards-compatibility purposes.

spec/02-integration/04-admin_api/04-plugins_routes_spec.lua
assert.is_table(json.enabled_plugins)
assert.True(#json.enabled_plugins > 0)
dao:truncate_tables()
db:truncate()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These operations are now handled by get_db_utils(). Will update locally but good for the next time :)

@thibaultcha thibaultcha mentioned this pull request Apr 4, 2018
Closed
@thibaultcha
Copy link
Member

Merged!

@thibaultcha thibaultcha closed this Apr 4, 2018
thibaultcha pushed a commit that referenced this pull request Apr 4, 2018
@bungle @thibaultcha
fix(dao) prevent self_check from receiving ngx.null
52e39d2 
Because `self_check` for plugins currently does a new DB `:select()` to
check for foreign entities, if the `service_id` or `route_id` fields are
`ngx.null` (from a PATCH with JSON NULL), then Cassandra produces an
error:

[Invalid] Invalid null value in condition for column id

And PostgreSQL as well, for another reason:

runtime error: ./kong/dao/schemas/plugins.lua:101: attempt to concatenate field 'service_id' (a userdata value)

We should avoiding giving `ngx.null` to `self_check` since the entire
old schema was never designed to play nice with it, and because external
plugins might be broken if we do so. This change improves
backwards-compatibility.

Fix #3308
From #3355

Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>
thibaultcha pushed a commit that referenced this pull request Apr 4, 2018
@bungle @thibaultcha
fix(dao) handle ngx.null in C* constraints checks
1b1494f 
We should not select with Cassandra `null` types for these constraints,
so while this change does not come with a reproducible test case, it is
still healthy to have.

See #3308
From #3355

Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>
@thibaultcha thibaultcha deleted the fix/cassandra-ngx-null branch April 4, 2018 02:49
kikito pushed a commit to kikito/kong that referenced this pull request Apr 10, 2018
@bungle @kikito
fix(dao) prevent self_check from receiving ngx.null
b2bfe98 
Because `self_check` for plugins currently does a new DB `:select()` to
check for foreign entities, if the `service_id` or `route_id` fields are
`ngx.null` (from a PATCH with JSON NULL), then Cassandra produces an
error:

[Invalid] Invalid null value in condition for column id

And PostgreSQL as well, for another reason:

runtime error: ./kong/dao/schemas/plugins.lua:101: attempt to concatenate field 'service_id' (a userdata value)

We should avoiding giving `ngx.null` to `self_check` since the entire
old schema was never designed to play nice with it, and because external
plugins might be broken if we do so. This change improves
backwards-compatibility.

Fix Kong#3308
From Kong#3355

Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>
kikito pushed a commit to kikito/kong that referenced this pull request Apr 10, 2018
@bungle @kikito
fix(dao) handle ngx.null in C* constraints checks
d38c9ff 
We should not select with Cassandra `null` types for these constraints,
so while this change does not come with a reproducible test case, it is
still healthy to have.

See Kong#3308
From Kong#3355

Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thibaultcha thibaultcha thibaultcha left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot clear consumer configuration from plugins when using Cassandra
2 participants
@bungle @thibaultcha