Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: support dash in args (#4519) #4676

Merged
merged 19 commits into from
Aug 6, 2021

Conversation

nanamikon
Copy link
Contributor

What this PR does / why we need it:

Support fetch value from arg name with dash , see #4519

Pre-submission checklist:

  • [ 1] Did you explain what problem does this PR solve? Or what new features have been added?
  • [ 1] Have you added corresponding test cases?
  • [ 1] Have you modified the corresponding document?
  • [ 1] Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

nanamikon and others added 3 commits July 27, 2021 09:37
Co-authored-by: Alex Zhang <tokers@apache.org>
Co-authored-by: Alex Zhang <tokers@apache.org>
@spacewander
Copy link
Member

Please merge master to make CI pass.

@nanamikon
Copy link
Contributor Author

Some conflicts should be solved, i will fix it

@nanamikon
Copy link
Contributor Author

Please merge master to make CI pass.

CI fail refering to error log

2021/08/03 22:55:24 [error] 21865#948770: *10 lua entry thread aborted: runtime error: /Users/jianzhou/workspace/apisix/apisix/core/request.lua:132: attempt to index local 'ctx' (a nil value)
stack traceback:
coroutine 0:
        /Users/jianzhou/workspace/apisix/apisix/core/request.lua: in function 'get_uri_args'
        /Users/jianzhou/workspace/apisix/apisix/core/ctx.lua:169: in function '__index'
        /Users/jianzhou/workspace/apisix/apisix/admin/init.lua:71: in function 'check_token'
        /Users/jianzhou/workspace/apisix/apisix/admin/init.lua:123: in function 'handler'
        ...workspace/apisix//deps/share/lua/5.1/resty/radixtree.lua:722: in function 'dispatch'
        /Users/jianzhou/workspace/apisix/apisix/init.lua:765: in function 'http_admin'
        content_by_lua(nginx.conf:222):2: in main chunk, client: 127.0.0.1, server: , request: "GET /apisix/admin/routes HTTP/1.1", host: "127.0.0.1:9180"

I think it should pass api_ctx to ngx.ctx in the /admin/init.lua

local function run()
    local api_ctx = {}
    core.ctx.set_vars_meta(api_ctx)

    local ok, err = check_token(api_ctx)

Do I need to fit this in this pr?

@spacewander
Copy link
Member

@nanamikon
Yes, let's do it.

@nanamikon
Copy link
Contributor Author

All tests pass now

@spacewander
Copy link
Member

We can remove the 'arg' part from 'TEST 2: http header + arg'

@nanamikon
Copy link
Contributor Author

Get it

@nanamikon nanamikon requested a review from spacewander August 4, 2021 06:39
@spacewander
Copy link
Member

Err... We still need the "TEST 2: http header" but remove the arg part of it.

@nanamikon
Copy link
Contributor Author

Err... We still need the "TEST 2: http header" but remove the arg part of it.

Sorry for misunderstanding, I will fix it

Co-authored-by: tzssangglass <tzssangglass@gmail.com>
@spacewander spacewander merged commit 88b92ae into apache:master Aug 6, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants