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.

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

sort category.apis_by_methods to fix a routing problem #2523

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
42 changes: 25 additions & 17 deletions kong/core/router.lua
Original file line number Diff line number Diff line change
Expand Up @@ -433,30 +433,38 @@ function _M.new(apis)
categorize_api_t(api_t, categories)
end

local compare = function(a,b, category_bit)
Copy link
Member

Choose a reason for hiding this comment

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

style: missing a space after the first argument a,. We also favor declaration of local function with the following form:

local function compare(a, b, category_bit)

end

if not band(category_bit, MATCH_RULES.URI) then
return
end

for category_bit, category in pairs(categories) do
table.sort(category.apis, function(a, b)
if not band(category_bit, MATCH_RULES.URI) then
return
end

local max_uri_a = 0
local max_uri_b = 0
local max_uri_a = 0
local max_uri_b = 0

for _, prefix in ipairs(a.uris_prefixes_regexes) do
if #prefix.regex > max_uri_a then
max_uri_a = #prefix.regex
end
for _, prefix in ipairs(a.uris_prefixes_regexes) do
if #prefix.regex > max_uri_a then
max_uri_a = #prefix.regex
end
end

for _, prefix in ipairs(b.uris_prefixes_regexes) do
if #prefix.regex > max_uri_b then
max_uri_b = #prefix.regex
end
for _, prefix in ipairs(b.uris_prefixes_regexes) do
if #prefix.regex > max_uri_b then
max_uri_b = #prefix.regex
end
end

return max_uri_a > max_uri_b
end

return max_uri_a > max_uri_b
for category_bit, category in pairs(categories) do
table.sort(category.apis, function(a,b)
return compare(a,b,category_bit)
end)
for _, apilist_by_method in pairs(category.apis_by_methods) do
table.sort(apilist_by_method, function(a,b)
return compare(a,b,category_bit)
end)
end
end


Expand Down
33 changes: 33 additions & 0 deletions spec/01-unit/11-router_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,39 @@ describe("Router", function()
assert.same(use_case[2], api_t.api)
end)

it("does not superseds another API with a longer URI prefix while methods are also defined", function()
local use_case = {
{
name = "api-1",
methods = {"POST", "PUT", "GET"},
uris = { "/my-api" },
},
{
name = "api-2",
methods = {"POST", "PUT", "GET"},
uris = { "/my-api/hello" },
}
}

local router = assert(Router.new(use_case))

local api_t = router.select("GET", "/my-api/hello")
assert.truthy(api_t)
assert.same(use_case[2], api_t.api)

api_t = router.select("GET", "/my-api/hello/world")
assert.truthy(api_t)
assert.same(use_case[2], api_t.api)

api_t = router.select("GET", "/my-api")
assert.truthy(api_t)
assert.same(use_case[1], api_t.api)

api_t = router.select("GET", "/my-api/world")
assert.truthy(api_t)
assert.same(use_case[1], api_t.api)
end)
Copy link
Member

Choose a reason for hiding this comment

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

We need the same test for APIs defined with both a URI and a plain host.


it("only matches URI as a prefix (anchored mode)", function()
local use_case = {
{
Expand Down
39 changes: 38 additions & 1 deletion spec/02-integration/05-proxy/01-router_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ describe("Router", function()
end)
end)
end)

Copy link
Member

Choose a reason for hiding this comment

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

this seems like an undesired change

describe("edge-cases", function()

setup(function()
Expand Down Expand Up @@ -435,6 +435,43 @@ describe("Router", function()
end)
end)

describe("edge-cases where longer uris should be matched first while method and uri are both defined", function()

setup(function()
insert_apis {
{
name = "root-api",
methods = {"GET"},
upstream_url = "http://httpbin.org",
uris = "/root",
},
{
name = "fixture-api",
methods = {"GET"},
upstream_url = "http://httpbin.org",
uris = "/root/fixture",
},
}

assert(helpers.start_kong())
end)

teardown(function()
helpers.stop_kong()
end)

it("route to the correct api when requested with a subpath", function()
local res = assert(client:send {
method = "GET",
path = "/root/fixture/get",
headers = { ["kong-debug"] = 1 }
})

assert.response(res).has_status(200)
assert.equal("fixture-api", res.headers["kong-api-name"])
end)
end)

describe("trailing slash", function()
local checks = {
-- upstream url uris request path expected path strip uri
Expand Down