-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
feat(request-transformer) change HTTP method of upstream request #1635
Conversation
60e213a
to
bd8f0f0
Compare
bd8f0f0
to
1f813c9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reluctant to adding a new feature to 0.9.2
. Should be targeted for 0.10
.
local content_type_value = req_get_headers()[CONTENT_TYPE] | ||
local content_type = get_content_type(content_type_value) | ||
if content_type == ENCODED then | ||
-- Also put the body into querystring |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such behavior should be configurable.
@@ -303,7 +303,33 @@ local function transform_body(conf) | |||
end | |||
end | |||
|
|||
local function transform_method(conf) | |||
if conf.http_method then | |||
ngx.req.set_method(ngx["HTTP_"..conf.http_method:upper()]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All ngx
methods are cached except this one. It should be too for consistency and efficiency.
local method = value:upper() | ||
local ngx_method = ngx["HTTP_"..method] | ||
if not ngx_method then | ||
return false, method.." is not supported" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By convention our codebase returns nil
on error. Some parts are not yet updated, but new code should comply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think of this value as being stored into a is_valid
property, that's why it's true
or false
.
local function check_method(value)
if not value then return true end
local method = value:upper()
local ngx_method = ngx["HTTP_"..method]
if not ngx_method then
return false, method.." is not supported"
end
return true
end
local is_valid, err = check_method("hello")
Makes sense that the return value is boolean
, with an optional error message into err
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that's not how this method is being used. It's passed to func
, and it is a common Lua idiom when the second parameter is an error to return nil
as the first one:
local ok, err = check_method("hello")
if not ok then
end
Returning false
would make sense if, say, there was no error as the 2nd return value.
local function transform_method(conf) | ||
if conf.http_method then | ||
ngx.req.set_method(ngx["HTTP_"..conf.http_method:upper()]) | ||
if conf.http_method == "GET" then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe HEAD
too? Should probably be configurable as well.
Issues resolved
Fix #1631.