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

feature: Add FFI functions for ngx.req.get_post_args #87

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
83 changes: 83 additions & 0 deletions lib/resty/core/request.lua
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,15 @@ ffi.cdef[[
int ngx_http_lua_ffi_req_get_uri_args(ngx_http_request_t *r,
unsigned char *buf, ngx_http_lua_ffi_table_elt_t *out, int count);

int ngx_http_lua_ffi_req_get_post_args_len(ngx_http_request_t *r);

int ngx_http_lua_ffi_req_get_post_args_count(ngx_http_request_t *r,
unsigned char *buf, int len, int max);

int ngx_http_lua_ffi_req_get_post_args(ngx_http_request_t *r,
unsigned char *buf, int len, ngx_http_lua_ffi_table_elt_t *out,
int count);

double ngx_http_lua_ffi_req_start_time(ngx_http_request_t *r);

int ngx_http_lua_ffi_req_get_method(ngx_http_request_t *r);
Expand Down Expand Up @@ -132,6 +141,80 @@ function ngx.req.get_headers(max_headers, raw)
end


function ngx.req.get_post_args(max_args)
local r = getfenv(0).__ngx_req
if not r then
return error("no request found")
end

if not max_args then
max_args = -1
end

local n = C.ngx_http_lua_ffi_req_get_post_args_len(r)
if n == FFI_BAD_CONTEXT then
return error("API disabled in the current context")
end

-- either r->discard_body or r->request_body->bufs == NULL
if n == -1 then
return {}
end

-- r->request_body == NULL
if n == -2 then
return error("no request body found; maybe you should turn on lua_need_request_body?")
end

-- r->request_body->temp_file
if n == -3 then
return nil, "request body in temp file not supported"
end

if n == 0 then
return {}
end

local strbuf = get_string_buf(n)
local kvbuf = ffi_cast(table_elt_type, strbuf + n)

local args_count = C.ngx_http_lua_ffi_req_get_post_args_count(r, strbuf,
n, max_args)

local nargs = C.ngx_http_lua_ffi_req_get_post_args(r, strbuf, n,
kvbuf, args_count)

local args = new_tab(0, nargs)
for i = 0, nargs - 1 do
local arg = kvbuf[i]
Copy link
Member

Choose a reason for hiding this comment

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

Should we go against Mike Pall's advice not to cache table lookup in such a hot code path? I am not sure, just to be clear, just raising a concern we previously talked about :)

This happens a bunch in this function, although, I do agree that it makes the code more readable :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think style consistency is important here; this loop is a duplicate of the one for URI args, so this seems the most appropriate choice. I would be interested in looking at some jit dump output of the two approaches and seeing what the functional difference is.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I'm really unsure too, especially since we're dealing with cdata here and not tables...


local key = arg.key
key = ffi_str(key.data, key.len)

local value = arg.value
local len = value.len
if len == -1 then
value = true
else
value = ffi_str(value.data, len)
end

local existing = args[key]
if existing then
if type(existing) == "table" then
existing[#existing + 1] = value
else
args[key] = {existing, value}
end

else
args[key] = value
end
end
return args
end


function ngx.req.get_uri_args(max_args)
local r = getfenv(0).__ngx_req
if not r then
Expand Down
87 changes: 87 additions & 0 deletions t/request.t
Original file line number Diff line number Diff line change
Expand Up @@ -570,3 +570,90 @@ qr/\[TRACE \d+ content_by_lua\(nginx\.conf:\d+\):4 loop\]/
[error]
bad argument type
stitch



=== TEST 17: ngx.req.get_post_args
--- http_config eval: $::HttpConfig
--- config
location = /t {
set $foo hello;
content_by_lua_block {
ngx.req.read_body()
local args
for i = 1, 200 do
args = ngx.req.get_post_args()
end
if type(args) ~= "table" then
ngx.say("bad args type found: ", args)
return
end
local keys = {}
for k, _ in pairs(args) do
keys[#keys + 1] = k
end
table.sort(keys)
for _, k in ipairs(keys) do
local v = args[k]
if type(v) == "table" then
ngx.say(k, ": ", table.concat(v, ", "))
else
ngx.say(k, ": ", v)
end
end
}
}
--- request
GET /t
a=3%200&foo%20bar=&a=hello&blah
--- response_body
a: 3 0, hello
blah: true
foo bar:
--- error_log eval
qr/\[TRACE \d+ .*? -> \d+\]/
--- no_error_log
[error]
-- NYI:
--- wait: 0.2



=== TEST 18: ngx.req.get_post_args (empty)
--- http_config eval: $::HttpConfig
--- config
location = /t {
set $foo hello;
content_by_lua_block {
ngx.req.read_body()
local args
for i = 1, 200 do
args = ngx.req.get_post_args()
end
if type(args) ~= "table" then
ngx.say("bad args type found: ", args)
return
end
local keys = {}
for k, _ in pairs(args) do
keys[#keys + 1] = k
end
table.sort(keys)
for _, k in ipairs(keys) do
local v = args[k]
if type(v) == "table" then
ngx.say(k, ": ", table.concat(v, ", "))
else
ngx.say(k, ": ", v)
end
end
}
}
--- request
GET /t?
--- response_body
--- error_log eval
qr/\[TRACE \d+ content_by_lua\(nginx\.conf:\d+\):4 loop\]/
--- no_error_log
[error]
-- NYI:
Copy link
Member

Choose a reason for hiding this comment

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

I don't see a test with get_post_args(n) and get_post_args(0). I definitely think we need those :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quoting @agentzh from #78:

The test cases in this library for the FFI-version of ngx_lua's Lua API should focus instead of JIT-ability instead of functionality. Functionality tests are already in the ngx_lua module and ngx_lua's test suite can run with lua-resty-core with the following environment setting, for example:

export TEST_NGINX_INIT_BY_LUA="package.path = (package.path or '') .. ';$PWD/../lua-resty-core/lib/?.lua;$PWD/../lua-resty-lrucache/lib/?.lua;' require 'resty.core'"

Copy link
Member

Choose a reason for hiding this comment

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

Good!