hotfix(admin-api) fixes an edge-case and returns 405 on invalid methods #2213
Conversation
|
The better solution would be to add a metatable to the If a lookup fails in that table, it should return a function that executes a 405. |
|
@thibaultcha @thefosk I updated this PR. It seems that Lapis is adding too much magic under the hood for the metatable approach to work properly, hence falling back on the error string checking as implemented by @thefosk. With the exception that the existing error mechanism (that didn't catch custom http methods) is now removed, as it doubled. |
There was a problem hiding this comment.
Ok, noted, that's fine then. A couple points to still address imo:
- Some new code was added which doesn't respect a couple best practices (
elsecoupled with a returning branch andstring.matchperforming a pattern search when not needed) - If we also update this module to use the new code style, then let's fully apply it
Note: Some code in there is returning from branches, but this patch is not touching it (except for style), so I don't feel like we should update them necessarily. Just adding a precision since 1. pointed the same issue, but only in the context of code appearing in this patch.
kong/api/init.lua
Outdated
| if err and err:match("don't know how to respond to") then | ||
| return responses.send_HTTP_METHOD_NOT_ALLOWED() | ||
|
|
||
| else |
There was a problem hiding this comment.
no else here as the previous branch returns
kong/api/init.lua
Outdated
| ngx.log(ngx.ERR, err.."\n"..trace) | ||
| -- We just logged the error so no need to give it to responses and log it twice | ||
| return responses.send_HTTP_INTERNAL_SERVER_ERROR() | ||
|
|
kong/api/init.lua
Outdated
| local app = lapis.Application() | ||
|
|
||
| local needs_body = tablex.readonly({ PUT = 1, POST = 2, PATCH = 3 }) |
There was a problem hiding this comment.
It's good that we are switching to the new code style in this module, but then let's also respect the 2 lines jump here as well: between the require section, the "globals caching section", the "Lapis App creation", and the constants definition for this module.
kong/api/init.lua
Outdated
| local function attach_routes(routes) | ||
|
|
There was a problem hiding this comment.
style: there should be no such blank line when entering a method
kong/api/init.lua
Outdated
| local function parse_params(fn) | ||
|
|
There was a problem hiding this comment.
style: blank line when entering a function is not part of the code style
kong/api/init.lua
Outdated
| -- Loading plugins routes | ||
| if singletons.configuration and singletons.configuration.plugins then | ||
|
|
There was a problem hiding this comment.
style: no such blank lines required when entering a block/function
kong/api/init.lua
Outdated
| for k in pairs(singletons.configuration.plugins) do | ||
|
|
There was a problem hiding this comment.
ditto: blank line entering a block
kong/api/init.lua
Outdated
| else | ||
| ngx.log(ngx.DEBUG, "No API endpoints loaded for plugin: ", k) | ||
| end | ||
|
|
There was a problem hiding this comment.
No such blank line before closing a block either, or at least, not enforced.
kong/api/init.lua
Outdated
| -- We just logged the error so no need to give it to responses and log it twice | ||
| return responses.send_HTTP_INTERNAL_SERVER_ERROR() | ||
|
|
||
| if err and err:match("don't know how to respond to") then |
There was a problem hiding this comment.
string.match should probably just perform a plain text search if no patterns are needed here.
thibaultcha
added
pr/changes requested
|
Can we avoid style changes in the context of this PR? We could open a separate PR if we care deeply for such style, but adding here seems to introduce unnecessary complexity, dirties the git history, and reduces atomicity of the commit. |
thibaultcha
force-pushed
the
hotfix/admin-invalid-method
branch
from
bb5ddab to
fa23ee8
Compare
thibaultcha
force-pushed
the
hotfix/admin-invalid-method
branch
from
fa23ee8 to
232f4f1
Compare
|
Re-worked this patch (new history) to properly introduce those 2 different changes, fix + style. |
thibaultcha
added
pr/ready (but hold merge)
Full changelog