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

Conversation

leonzz
Copy link

@leonzz leonzz commented May 14, 2017

Summary

Sort category.apis_by_methods in router.lua to fix a routing problem for APIs defined with method and uri (path)

Full changelog

  • Sort category.apis_by_methods in router.lua to fix a routing problem for APIs defined with method and uri (path). Also add unit test and integration test for it.

Issues resolved

Fix #2522

@leonzz leonzz force-pushed the fix/router-error-method-path branch from 98ef9d8 to 6193124 Compare May 14, 2017 20:57
@leonzz
Copy link
Author

leonzz commented May 14, 2017

Some of the tests seem to fail randomly due to variation of network connectivity condition to external endpoints, not due to changes made in this PR. This PR has been validated manually with the case described in issue #2522

@thibaultcha
Copy link
Member

Hi,

Thanks for looking into this. Could you provide regression tests for this? They exist in both unit and integration suites. Manually validated is not enough... Thank you!

@leonzz
Copy link
Author

leonzz commented May 15, 2017

thanks for the feedback. will make another commit for tests.

@leonzz leonzz force-pushed the fix/router-error-method-path branch from 5716e14 to 5ca51f6 Compare May 15, 2017 18:28
@leonzz
Copy link
Author

leonzz commented May 15, 2017

@thibaultcha can you take a look again or assign to proper reviewer? The travis test failure seems to be caused by httpbin.org timeout again, not related to this PR.

@thibaultcha
Copy link
Member

Please ignore the build failures, we'll take care of those. This will be reviewed when we'll have some time. Thanks.

@thibaultcha
Copy link
Member

Thanks again @leonzz, unfortunately I won't have time to review this today, but we'll certainly consider this very soon, we take such routing issues very seriously.

@bungle Would appreciate to have your insight here as well, if you have some time. Thanks! :)

Copy link
Member

@thibaultcha thibaultcha left a comment

Choose a reason for hiding this comment

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

Hi,

Thanks for spotting this. So, I gave it a try on my side, and I can see that the same scenario happens, when defining an API with uris and plain hosts (not methods). This is related to the same underlying issue: the lack of sorting for category.apis_by_plain_hosts. Would you be willing to update this patch so as to handle this edge-case as well?

@@ -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

@@ -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

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.

@thibaultcha thibaultcha added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels May 17, 2017
@leonzz
Copy link
Author

leonzz commented May 17, 2017

Thanks @thibaultcha . Will do another commit to address comments and add sorting to the case of apis_by_plain_hosts (i noticed that part as well but didn't address it as my team is not defining APIs by hosts. would like to share some comments as well from the usage experience in my team if you guys have time :) )

@thibaultcha
Copy link
Member

@leonzz Great, thank you. Ultimately, this fix should belong to the same commit, as it is the same issue.

would like to share some comments as well from the usage experience in my team if you guys have time

Definitely! That sounds like a good discussion to have on the Kong mailing list :)

@leonzz
Copy link
Author

leonzz commented May 18, 2017

yep agree that they should be one commit. do you want me to put them together and do a force update on this branch?

@thibaultcha thibaultcha modified the milestone: 0.10.3 May 19, 2017
@thibaultcha
Copy link
Member

Merged to master with a few minor edits, thank you very much for the issue + the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

issue with routing order for APIs define with method and uri
2 participants