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(cli) prefix Cassandra connector errors in new DAO #3648

Merged
merged 1 commit into from Jul 27, 2018
Merged

fix(cli) prefix Cassandra connector errors in new DAO #3648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 38 additions & 5 deletions kong/db/init.lua
View file
Open in desktop
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,
Copy link
Contributor

Choose a reason for hiding this comment

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

I had the exact same change done in my codebase here as I work on the Plugins entity. :) We really need to start making smaller PRs with this little "minor tweaks" as we work through major changes!

}

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
Copy link
Member

Choose a reason for hiding this comment

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

considering the repetitive nature of this error checking, could we do something like:

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

  -- and calling it here (and in other places) as:
  return prefix_err(self, self.connector:truncate())

can probably be cleaner if exporting prefix_err as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Considered it, but I don't want to introduce the extra cost of calling a variadic function on every call of the connector's functions. Five occurrences, and it is unlikely that we add more methods to the connector, is not that repetitive is what I told myself.

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 so 😄

end


Expand Down
7 changes: 4 additions & 3 deletions spec/02-integration/04-admin_api/03-consumers_routes_spec.lua
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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