Skip to content

Commit

Permalink
fix(cli) prefix connector errors in new DAO
Browse files Browse the repository at this point in the history 
Since recently, we call the new DAO's `assert(db:init_connector())` in
the CLI _before_ instantiating the old DAO (because the latter needs the
former as an attribute).

For Cassandra, this has the effect of retrieving the cluster's topology,
and thus trying to connect to the given contact points. If any error is
encountered, the new DAO does not properly prefix the error with the
`[Cassandra error]` prefix, as the old DAO would do. This goes unnoticed
for the PostgreSQL strategy since the connector initialization is
nopping.

See a related CI failure here:

https://travis-ci.com/Kong/kong-private/jobs/136391637#L1094

We now prefix connector errors (which are always strings).

From #3648
  • Loading branch information
@thibaultcha
thibaultcha committed Jul 27, 2018
1 parent 90096fb commit 9f5102f
Show file tree
Hide file tree
Showing 5 changed files with 100 additions and 53 deletions.
43 changes: 38 additions & 5 deletions kong/db/init.lua
View file
Expand Up @@ -41,6 +41,8 @@ function DB.new(kong_config, strategy)
error("strategy must be a string", 2)
end

strategy = strategy or kong_config.database

-- load errors

local errors = Errors.new(strategy)
Expand Down Expand Up @@ -81,6 +83,7 @@ function DB.new(kong_config, strategy)
daos = daos, -- each of those has the connector singleton
strategies = strategies,
connector = connector,
strategy = strategy,
}

do
Expand All @@ -103,6 +106,11 @@ function DB.new(kong_config, strategy)
end


local function prefix_err(self, err)
return "[" .. self.strategy .. " error] " .. err
end


function DB:init_connector()
-- I/O with the DB connector singleton
-- Implementation up to the strategy's connector. A place for:
Expand All @@ -111,27 +119,52 @@ function DB:init_connector()
-- - prepare statements
-- - nop (default)

return self.connector:init()
local ok, err = self.connector:init()
if not ok then
return nil, prefix_err(self, err)
end

return ok
end


function DB:connect()
return self.connector:connect()
local ok, err = self.connector:connect()
if not ok then
return nil, prefix_err(self, err)
end

return ok
end


function DB:setkeepalive()
return self.connector:setkeepalive()
local ok, err = self.connector:setkeepalive()
if not ok then
return nil, prefix_err(self, err)
end

return ok
end


function DB:reset()
return self.connector:reset()
local ok, err = self.connector:reset()
if not ok then
return nil, prefix_err(self, err)
end

return ok
end


function DB:truncate()
return self.connector:truncate()
local ok, err = self.connector:truncate()
if not ok then
return nil, prefix_err(self, err)
end

return ok
end


Expand Down
7 changes: 4 additions & 3 deletions spec/02-integration/04-admin_api/03-consumers_routes_spec.lua
View file
Expand Up @@ -493,9 +493,10 @@ describe("Admin API (" .. strategy .. "): ", function()
})
local body = assert.res_status(400, res)
assert.same({
code = Errors.codes.SCHEMA_VIOLATION,
name = "schema violation",
fields = {
code = Errors.codes.SCHEMA_VIOLATION,
name = "schema violation",
strategy = strategy,
fields = {
["@entity"] = {
"at least one of these fields must be non-empty: 'custom_id', 'username'"
}
Expand Down
16 changes: 9 additions & 7 deletions spec/02-integration/04-admin_api/06-certificates_routes_spec.lua
View file
Expand Up @@ -296,9 +296,10 @@ describe("Admin API: #" .. strategy, function()
})
local body = assert.res_status(400, res)
assert.same({
code = Errors.codes.SCHEMA_VIOLATION,
name = "schema violation",
message = "2 schema violations (cert: required field missing; key: required field missing)",
code = Errors.codes.SCHEMA_VIOLATION,
name = "schema violation",
strategy = strategy,
message = "2 schema violations (cert: required field missing; key: required field missing)",
fields = {
cert = "required field missing",
key = "required field missing",
Expand Down Expand Up @@ -716,10 +717,11 @@ describe("Admin API: #" .. strategy, function()
})
local body = assert.res_status(400, res)
assert.same({
code = Errors.codes.SCHEMA_VIOLATION,
name = "schema violation",
message = "2 schema violations (certificate: required field missing; name: required field missing)",
fields = {
code = Errors.codes.SCHEMA_VIOLATION,
name = "schema violation",
strategy = strategy,
message = "2 schema violations (certificate: required field missing; name: required field missing)",
fields = {
certificate = "required field missing",
name = "required field missing",
}
Expand Down
80 changes: 45 additions & 35 deletions spec/02-integration/04-admin_api/20-routes_routes_spec.lua
View file
Expand Up @@ -117,14 +117,15 @@ for _, strategy in helpers.each_strategy() do
})
local body = assert.res_status(400, res)
assert.same({
code = Errors.codes.SCHEMA_VIOLATION,
name = "schema violation",
message = unindent([[
code = Errors.codes.SCHEMA_VIOLATION,
name = "schema violation",
strategy = strategy,
message = unindent([[
2 schema violations
(at least one of these fields must be non-empty: 'methods', 'hosts', 'paths';
service: required field missing)
]], true, true),
fields = {
fields = {
service = "required field missing",
["@entity"] = {
"at least one of these fields must be non-empty: 'methods', 'hosts', 'paths'"
Expand All @@ -142,12 +143,13 @@ for _, strategy in helpers.each_strategy() do
})
body = assert.res_status(400, res)
assert.same({
code = Errors.codes.SCHEMA_VIOLATION,
name = "schema violation",
message = "2 schema violations " ..
code = Errors.codes.SCHEMA_VIOLATION,
name = "schema violation",
strategy = strategy,
message = "2 schema violations " ..
"(protocols: expected one of: http, https; " ..
"service: required field missing)",
fields = {
fields = {
protocols = "expected one of: http, https",
service = "required field missing",
}
Expand Down Expand Up @@ -234,9 +236,10 @@ for _, strategy in helpers.each_strategy() do
local res = client:get("/routes", { query = { offset = "x" } })
local body = assert.res_status(400, res)
assert.same({
code = Errors.codes.INVALID_OFFSET,
name = "invalid offset",
message = "'x' is not a valid offset for this strategy: bad base64 encoding"
code = Errors.codes.INVALID_OFFSET,
name = "invalid offset",
strategy = strategy,
message = "'x' is not a valid offset for this strategy: bad base64 encoding"
}, cjson.decode(body))

res = client:get("/routes", { query = { offset = "potato" } })
Expand All @@ -246,8 +249,9 @@ for _, strategy in helpers.each_strategy() do
json.message = nil

assert.same({
code = Errors.codes.INVALID_OFFSET,
name = "invalid offset",
code = Errors.codes.INVALID_OFFSET,
name = "invalid offset",
strategy = strategy,
}, json)
end)

Expand All @@ -268,10 +272,11 @@ for _, strategy in helpers.each_strategy() do
local body = assert.res_status(400, res)
local pk = { id = "expected a valid UUID" }
assert.same({
code = Errors.codes.INVALID_PRIMARY_KEY,
name = "invalid primary key",
message = [[invalid primary key: '{id="expected a valid UUID"}']],
fields = pk
code = Errors.codes.INVALID_PRIMARY_KEY,
name = "invalid primary key",
strategy = strategy,
message = [[invalid primary key: '{id="expected a valid UUID"}']],
fields = pk
}, cjson.decode(body))
end)
end)
Expand Down Expand Up @@ -394,9 +399,10 @@ for _, strategy in helpers.each_strategy() do
})
local body = assert.res_status(400, res)
assert.same({
code = Errors.codes.SCHEMA_VIOLATION,
name = "schema violation",
message = unindent([[
code = Errors.codes.SCHEMA_VIOLATION,
name = "schema violation",
strategy = strategy,
message = unindent([[
2 schema violations
(at least one of these fields must be non-empty: 'methods', 'hosts', 'paths';
service: required field missing)
Expand All @@ -419,9 +425,10 @@ for _, strategy in helpers.each_strategy() do
})
body = assert.res_status(400, res)
assert.same({
code = Errors.codes.SCHEMA_VIOLATION,
name = "schema violation",
message = "2 schema violations " ..
code = Errors.codes.SCHEMA_VIOLATION,
name = "schema violation",
strategy = strategy,
message = "2 schema violations " ..
"(protocols: expected one of: http, https; " ..
"service: required field missing)",
fields = {
Expand All @@ -442,10 +449,11 @@ for _, strategy in helpers.each_strategy() do
})
local body = assert.res_status(400, res)
assert.same({
code = Errors.codes.SCHEMA_VIOLATION,
name = "schema violation",
message = "schema violation (regex_priority: expected an integer)",
fields = {
code = Errors.codes.SCHEMA_VIOLATION,
name = "schema violation",
strategy = strategy,
message = "schema violation (regex_priority: expected an integer)",
fields = {
regex_priority = "expected an integer"
},
}, cjson.decode(body))
Expand Down Expand Up @@ -604,10 +612,11 @@ for _, strategy in helpers.each_strategy() do
})
local body = assert.res_status(400, res)
assert.same({
code = Errors.codes.SCHEMA_VIOLATION,
name = "schema violation",
message = "schema violation (regex_priority: expected an integer)",
fields = {
code = Errors.codes.SCHEMA_VIOLATION,
name = "schema violation",
strategy = strategy,
message = "schema violation (regex_priority: expected an integer)",
fields = {
regex_priority = "expected an integer"
},
}, cjson.decode(body))
Expand Down Expand Up @@ -746,10 +755,11 @@ for _, strategy in helpers.each_strategy() do
})
local body = assert.res_status(400, res)
assert.same({
code = Errors.codes.SCHEMA_VIOLATION,
name = "schema violation",
message = "schema violation (connect_timeout: expected an integer)",
fields = {
code = Errors.codes.SCHEMA_VIOLATION,
name = "schema violation",
strategy = strategy,
message = "schema violation (connect_timeout: expected an integer)",
fields = {
connect_timeout = "expected an integer",
},
}, cjson.decode(body))
Expand Down
7 changes: 4 additions & 3 deletions spec/02-integration/04-admin_api/21-services_routes_spec.lua
View file
Expand Up @@ -752,9 +752,10 @@ for _, strategy in helpers.each_strategy() do
local json = cjson.decode(body)
assert.same(
{
name = "schema violation",
code = Errors.codes.SCHEMA_VIOLATION,
message = unindent([[
name = "schema violation",
strategy = strategy,
code = Errors.codes.SCHEMA_VIOLATION,
message = unindent([[
2 schema violations
(host: required field missing;
path: should start with: /)
Expand Down

0 comments on commit 9f5102f

Please sign in to comment.