fix(request-id): we can use different ids with the same request #4479
Conversation
|
@zuiyangqingzhou hi, need to add test cases |
apisix/plugins/request-id.lua
Outdated
| @@ -49,7 +49,7 @@ function _M.rewrite(conf, ctx) | |||
| end | |||
|
|
|||
| if conf.include_in_response then | |||
| ctx.x_request_id = uuid_val | |||
| ctx[conf.header_name] = uuid_val | |||
There was a problem hiding this comment.
I think we need to add a prefix like request-id as conf.header_name may override other value by accident.
There was a problem hiding this comment.
I don’t really understand the intention of adding a prefix, because header_name is originally defined by the user. He should know what the definition of header_name looks like.
There was a problem hiding this comment.
But he doesn't know all the fields in the ctx and the field will be added to ctx in the future.
There was a problem hiding this comment.
Do you mean like this？
if conf.include_in_response then
ctx["request-id" .. conf.header_name] = uuid_val
end
There was a problem hiding this comment.
ctx["request-id-" .. conf.header_name] = uuid_val would be better.
There was a problem hiding this comment.
All right！
In addition, I found that unit testing is not easy to do. Is there any related documentation?
Thank you
t/plugin/request-id.t
Outdated
| } | ||
| }) | ||
|
|
||
| if res.headers["X-Request-Id"] and res.headers["Custom-Header-Name"] and res.headers["X-Request-Id"] ~= res.headers["Custom-Header-Name"] then |
There was a problem hiding this comment.
We should not verify these two values when they are present, actually they must present.
There was a problem hiding this comment.
Okay, got it
t/plugin/request-id.t
Outdated
| }) | ||
|
|
||
| if res.headers["X-Request-Id"] and res.headers["Custom-Header-Name"] and res.headers["X-Request-Id"] ~= res.headers["Custom-Header-Name"] then | ||
| ngx.say("X-Request-Id and Custom-Header-Name is different") |
There was a problem hiding this comment.
| ngx.say("X-Request-Id and Custom-Header-Name is different") | |
| ngx.say("X-Request-Id and Custom-Header-Name are different") |
| --- response_body | ||
| passed | ||
| --- no_error_log | ||
| [error] |
There was a problem hiding this comment.
Please fix the linter: https://github.com/apache/apisix/pull/4479/checks?check_run_id=2929454713
There was a problem hiding this comment.
there should be 3 blank lines between test cases, or you can use the reindex to do. the CI error is about this.
|
What cause the error? https://github.com/apache/apisix/pull/4479/checks?check_run_id=2948088762 |
|
Don't be afraid, known issue: #4503 |
spacewander
changed the title
spacewander
changed the title
What this PR does / why we need it:
fix: 4460
Pre-submission checklist: