Skip to content

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

Merged
spacewander merged 19 commits into
apache:masterfrom
nanamikon:support-dash-in-args
Aug 6, 2021
Merged

feat: support dash in args (#4519)#4676
spacewander merged 19 commits into
apache:masterfrom
nanamikon:support-dash-in-args

Conversation

@nanamikon

@nanamikon nanamikon commented Jul 26, 2021

Copy link
Copy Markdown
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

spacewander
spacewander reviewed Jul 27, 2021
View reviewed changes
Comment thread t/core/ctx.t Outdated
tokers
tokers reviewed Jul 27, 2021
View reviewed changes
Comment thread apisix/core/ctx.lua Outdated
Comment thread apisix/core/ctx.lua Outdated
Comment thread apisix/core/ctx.lua Outdated
spacewander
spacewander reviewed Jul 28, 2021
View reviewed changes
Comment thread apisix/core/ctx.lua Outdated
spacewander
spacewander reviewed Jul 30, 2021
View reviewed changes
Comment thread apisix/core/ctx.lua Outdated
Comment thread apisix/core/ctx.lua Outdated
spacewander
spacewander reviewed Jul 30, 2021
View reviewed changes
Comment thread t/core/ctx2.t Outdated
spacewander
spacewander reviewed Jul 30, 2021
View reviewed changes
Comment thread apisix/core/ctx.lua Outdated
spacewander
spacewander reviewed Aug 3, 2021
View reviewed changes
Comment thread t/core/ctx2.t Outdated
@spacewander

spacewander commented Aug 3, 2021

Copy link
Copy Markdown
Member

Please merge master to make CI pass.

@nanamikon

nanamikon commented Aug 3, 2021

Copy link
Copy Markdown
Contributor Author

Some conflicts should be solved, i will fix it

@nanamikon

nanamikon commented Aug 3, 2021

Copy link
Copy Markdown
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

spacewander commented Aug 4, 2021

Copy link
Copy Markdown
Member

@nanamikon
Yes, let's do it.

@nanamikon

nanamikon commented Aug 4, 2021

Copy link
Copy Markdown
Contributor Author

All tests pass now

@spacewander

spacewander commented Aug 4, 2021

Copy link
Copy Markdown
Member

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

@nanamikon

nanamikon commented Aug 4, 2021

Copy link
Copy Markdown
Contributor Author

Get it

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

spacewander commented Aug 4, 2021

Copy link
Copy Markdown
Member

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

@nanamikon

nanamikon commented Aug 5, 2021

Copy link
Copy Markdown
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

@nanamikon @tzssangglass
Update apisix/core/ctx.lua
3113cd0 
Co-authored-by: tzssangglass <[email protected]>
@spacewander
spacewander merged commit 88b92ae into apache:master Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@spacewander spacewander spacewander approved these changes
@membphis membphis membphis approved these changes
@tokers tokers Awaiting requested review from tokers
@tzssangglass tzssangglass Awaiting requested review from tzssangglass

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@nanamikon @spacewander @membphis @tokers @tzssangglass