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

fix(admin) fix regression of trailing slash in targets uri #2884

Merged
merged 2 commits into from
Sep 13, 2017
Merged

fix(admin) fix regression of trailing slash in targets uri #2884

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
7644acd
fix(admin) fix regression of trailing slash in targets uri
Tieske Sep 12, 2017
58f590b
fix(test) run test with and without trailing slash
Tieske Sep 13, 2017
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
2 changes: 1 addition & 1 deletion kong/api/routes/upstreams.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ return {
end,
},

["/upstreams/:upstream_name_or_id/targets/active/"] = {
Copy link
Member

Choose a reason for hiding this comment

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

I believe (because I tested this yesterday) that removing this makes the endpoint works with both a trailing slash and without. We should also write a new regression test that ensures it is the case (after all, this is a regression we don't want to see happening again in the future).

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, good idea

["/upstreams/:upstream_name_or_id/targets/active"] = {
before = function(self, dao_factory, helpers)
crud.find_upstream_by_name_or_id(self, dao_factory, helpers)
self.params.upstream_id = self.upstream.id
Expand Down
36 changes: 19 additions & 17 deletions spec/02-integration/04-admin_api/08-targets_routes_spec.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,24 +302,26 @@ describe("Admin API", function()
end)

it("only shows active targets", function()
local res = assert(client:send {
method = "GET",
path = "/upstreams/" .. upstream_name3 .. "/targets/active/",
})
assert.response(res).has.status(200)
local json = assert.response(res).has.jsonbody()
for _, append in ipairs({ "", "/" }) do
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker but fwiw, I don't think this is a healthy way of writing tests. If one behavior fails (with or without trailing slash), the developer sees a test named "only shows active targets" fail, which isn't what the test was actually asserting. On top of that, the developer has no easy way of knowing which of the two tests (the one with or without the trailing slash) failed, which is confusing and/or frustrating.

Tests must be atomic and explicit, always. "Meta-programming" testing (generating tests based on loops) should be avoided when it breaks the atomicity of the test.

local res = assert(client:send {
method = "GET",
path = "/upstreams/" .. upstream_name3 .. "/targets/active" .. append,
})
assert.response(res).has.status(200)
local json = assert.response(res).has.jsonbody()

-- we got three active targets for this upstream
assert.equal(3, #json.data)
assert.equal(3, json.total)
-- we got three active targets for this upstream
assert.equal(3, #json.data)
assert.equal(3, json.total)

-- when multiple active targets are present, we only see the last one
assert.equal(apis[4].id, json.data[1].id)
-- when multiple active targets are present, we only see the last one
assert.equal(apis[4].id, json.data[1].id)

-- validate the remaining returned targets
-- note the backwards order, because we walked the targets backwards
assert.equal(apis[3].target, json.data[2].target)
assert.equal(apis[2].target, json.data[3].target)
-- validate the remaining returned targets
-- note the backwards order, because we walked the targets backwards
assert.equal(apis[3].target, json.data[2].target)
assert.equal(apis[2].target, json.data[3].target)
end
end)
end)
end)
Expand Down Expand Up @@ -366,7 +368,7 @@ describe("Admin API", function()

local active = assert(client:send {
method = "GET",
path = "/upstreams/" .. upstream_name4 .. "/targets/active/",
path = "/upstreams/" .. upstream_name4 .. "/targets/active",
})
assert.response(active).has.status(200)
json = assert.response(active).has.jsonbody()
Expand All @@ -393,7 +395,7 @@ describe("Admin API", function()

local active = assert(client:send {
method = "GET",
path = "/upstreams/" .. upstream_name4 .. "/targets/active/",
path = "/upstreams/" .. upstream_name4 .. "/targets/active",
})
assert.response(active).has.status(200)
json = assert.response(active).has.jsonbody()
Expand Down