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

Adding "total" in API responses + database stats in "/status" endpoint #635

Merged
merged 2 commits into from
Oct 16, 2015
Merged

Adding "total" in API responses + database stats in "/status" endpoint #635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
92a37c8
Adding "total" field in API responses
subnetmarco Oct 15, 2015
21d77a5
Adding database stats into status endpoint
subnetmarco Oct 16, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,17 @@
## [Unreleased][unreleased]

### Added

- Added a `total` field in API responses, that counts the total number of entities in the table. [#635](https://github.com/Mashape/kong/pull/635)

### Changed

- The `/status` endpoint now includes `database` statistics, while the previous stats have been moved to a `server` field. [#635](https://github.com/Mashape/kong/pull/635)

### Fixed

- In the API, the `next` link is not being displayed anymore if there are no more entities to return. [#635](https://github.com/Mashape/kong/pull/635)

## [0.5.1] - 2015/10/13

Fixing a few glitches we let out with 0.5.0!
Expand Down
30 changes: 22 additions & 8 deletions kong/api/crud_helpers.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,22 +51,36 @@ function _M.paginated_set(self, dao_collection)
return app_helpers.yield_error(err)
end

local count, err = dao_collection:count_by_keys(self.params)
if err then
return app_helpers.yield_error(err)
end

local next_url
if data.next_page then
next_url = self:build_url(self.req.parsed_url.path, {
port = self.req.parsed_url.port,
query = ngx.encode_args({
offset = ngx.encode_base64(data.next_page),
size = size
})
})
-- Parse next URL, if there are no elements then don't append it
local next_total, err = dao_collection:count_by_keys(self.params, size, data.next_page)
if err then
return app_helpers.yield_error(err)
end

if next_total > 0 then
next_url = self:build_url(self.req.parsed_url.path, {
port = self.req.parsed_url.port,
query = ngx.encode_args({
offset = ngx.encode_base64(data.next_page),
size = size
})
})
end

data.next_page = nil
end

-- This check is required otherwise the response is going to be a
-- JSON Object and not a JSON array. The reason is because an empty Lua array `{}`
-- will not be translated as an empty array by cjson, but as an empty object.
local result = #data == 0 and "{\"data\":[]}" or {data=data, ["next"]=next_url}
local result = #data == 0 and "{\"data\":[],\"total\":0}" or {data=data, ["next"]=next_url, total=count}

return responses.send_HTTP_OK(result, type(result) ~= "table")
end
Expand Down
16 changes: 15 additions & 1 deletion kong/api/routes/kong.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,21 @@ return {
GET = function(self, dao, helpers)
local res = ngx.location.capture("/nginx_status")
if res.status == 200 then
return helpers.responses.send_HTTP_OK(route_helpers.parse_status(res.body))

local status_response = {
server = route_helpers.parse_status(res.body),
database = {}
}

for k, v in pairs(dao.daos) do
local count, err = v:count_by_keys()
if err then
return helpers.responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
end
status_response.database[k] = count
end

return helpers.responses.send_HTTP_OK(status_response)
else
return helpers.responses.send_HTTP_INTERNAL_SERVER_ERROR(res.body)
end
Expand Down
7 changes: 5 additions & 2 deletions kong/dao/cassandra/base_dao.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,9 +553,12 @@ end
-- @return `res`
-- @return `err`
-- @return `filtering` A boolean indicating if ALLOW FILTERING was needed by the query
function BaseDao:count_by_keys(where_t)
function BaseDao:count_by_keys(where_t, page_size, paging_state)
local select_q, where_columns, filtering = query_builder.count(self._table, where_t, self._column_family_details)
Copy link
Member

Choose a reason for hiding this comment

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

actually count_q

Copy link
Member Author

Choose a reason for hiding this comment

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

why?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh you mean instead of select_q - the PR that introduced the count function has been waiting for a while under the dao/count branch, so I merged it. Didn't spot this.

local res, err = self:execute(select_q, where_columns, where_t, {})
local res, err = self:execute(select_q, where_columns, where_t, {
page_size = page_size,
paging_state = paging_state
Copy link
Member

Choose a reason for hiding this comment

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

What is the benefit of giving the page_size and paging_state 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.

Check where it's being used. This avoids showing a next button when it's not required, and since the offset is an UUID and not a number, it's not possible to implement this by simply calculating if (size * offset) > total_count then do_not_show_next() end.

})

return (#res >= 1 and table.remove(res, 1).count or 0), err, filtering
end
Expand Down
7 changes: 5 additions & 2 deletions spec/integration/admin_api/apis_routes_spec.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ describe("Admin API", function()
local body = json.decode(response)
assert.truthy(body.data)
assert.equal(10, table.getn(body.data))
assert.equal(10, body.total)
end)

it("should retrieve a paginated set", function()
Expand All @@ -109,6 +110,7 @@ describe("Admin API", function()
assert.truthy(body_page_1.data)
assert.equal(3, table.getn(body_page_1.data))
assert.truthy(body_page_1.next)
assert.equal(10, body_page_1.total)
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't add that everywhere because it makes changing the tests very painful now (for all entities). Also semantically this test (and the others) is not explaining it is testing the total count. Instead, a describe("total count") with several additions and deletions and specifically testing the total count is better.

Copy link
Member Author

Choose a reason for hiding this comment

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

The function is called "should retrieve a paginated set", so I am checking the response is a valid paginated set, and that also includes the total field.

If the tests themselves need to be changed, well that's another thing, in this specific case I integrated the new feature check with the existing tests.


response, status = http_client.get(BASE_URL, {size=3,offset=body_page_1.next})
assert.equal(200, status)
Expand All @@ -117,14 +119,15 @@ describe("Admin API", function()
assert.equal(3, table.getn(body_page_2.data))
assert.truthy(body_page_2.next)
assert.not_same(body_page_1, body_page_2)
assert.equal(10, body_page_2.total)

response, status = http_client.get(BASE_URL, {size=4,offset=body_page_2.next})
assert.equal(200, status)
local body_page_3 = json.decode(response)
assert.truthy(body_page_3.data)
assert.equal(4, table.getn(body_page_3.data))
-- TODO: fixme
--assert.falsy(body_page_3.next)
assert.equal(10, body_page_3.total)
assert.falsy(body_page_3.next)
assert.not_same(body_page_2, body_page_3)
end)

Expand Down
7 changes: 5 additions & 2 deletions spec/integration/admin_api/consumers_routes_spec.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ describe("Admin API", function()
local body = json.decode(response)
assert.truthy(body.data)
assert.equal(10, table.getn(body.data))
assert.equal(10, body.total)
end)

it("should retrieve a paginated set", function()
Expand All @@ -85,6 +86,7 @@ describe("Admin API", function()
assert.truthy(body_page_1.data)
assert.equal(3, table.getn(body_page_1.data))
assert.truthy(body_page_1.next)
assert.equal(10, body_page_1.total)

response, status = http_client.get(BASE_URL, {size=3,offset=body_page_1.next})
assert.equal(200, status)
Expand All @@ -93,14 +95,15 @@ describe("Admin API", function()
assert.equal(3, table.getn(body_page_2.data))
assert.truthy(body_page_2.next)
assert.not_same(body_page_1, body_page_2)
assert.equal(10, body_page_2.total)

response, status = http_client.get(BASE_URL, {size=4,offset=body_page_2.next})
assert.equal(200, status)
local body_page_3 = json.decode(response)
assert.truthy(body_page_3.data)
assert.equal(4, table.getn(body_page_3.data))
-- TODO: fixme
--assert.falsy(body_page_3.next)
assert.equal(10, body_page_3.total)
assert.falsy(body_page_3.next)
assert.not_same(body_page_2, body_page_3)
end)

Expand Down
28 changes: 20 additions & 8 deletions spec/integration/admin_api/kong_routes_spec.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ local json = require "cjson"
local http_client = require "kong.tools.http_client"
local spec_helper = require "spec.spec_helpers"
local utils = require "kong.tools.utils"
local env = spec_helper.get_env() -- test environment
local dao_factory = env.dao_factory

describe("Admin API", function()

Expand Down Expand Up @@ -60,15 +62,25 @@ describe("Admin API", function()
assert.are.equal(200, status)
local body = json.decode(response)
assert.truthy(body)
assert.are.equal(2, utils.table_size(body))

assert.are.equal(7, utils.table_size(body))
assert.truthy(body.connections_accepted)
assert.truthy(body.connections_active)
assert.truthy(body.connections_handled)
assert.truthy(body.connections_reading)
assert.truthy(body.connections_writing)
assert.truthy(body.connections_waiting)
assert.truthy(body.total_requests)
-- Database stats
-- Removing migrations DAO
dao_factory.daos.migrations = nil
assert.are.equal(utils.table_size(dao_factory.daos), utils.table_size(body.database))
for k, _ in pairs(dao_factory.daos) do
assert.truthy(body.database[k])
end

-- Server stats
assert.are.equal(7, utils.table_size(body.server))
assert.truthy(body.server.connections_accepted)
assert.truthy(body.server.connections_active)
assert.truthy(body.server.connections_handled)
assert.truthy(body.server.connections_reading)
assert.truthy(body.server.connections_writing)
assert.truthy(body.server.connections_waiting)
assert.truthy(body.server.total_requests)
end)
end)
end)