fix(admin): inject updatetime when the requst is PATCH with sub path #4765
Conversation
Fix apache#4763 Signed-off-by: spacewander <spacewanderlzx@gmail.com>
| if code >= 300 then | ||
| ngx.status = code | ||
| end | ||
| ngx.say(message) |
There was a problem hiding this comment.
I suggest add PATCH body is json format like
curl http://127.0.0.1:9080/apisix/admin/upstreams/100/nodes -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -i -X PATCH -d '
{
"39.97.63.215:80": 1,
"39.97.63.215:81": 1
}'
The patch request body support json format
There was a problem hiding this comment.
PATCH with json has been covered by
Line 348 in 183351c
There was a problem hiding this comment.
PATCH with json has been covered by
Line 348 in 183351c
we have auto test cases, why we don't find this issue ?
There was a problem hiding this comment.
This case is not covered by the test cases.
Signed-off-by: spacewander <spacewanderlzx@gmail.com>
apisix/admin/upstreams.lua
Outdated
| @@ -204,12 +204,12 @@ function _M.patch(id, conf, sub_path) | |||
| if code then | |||
| return code, err | |||
| end | |||
| utils.inject_timestamp(new_value, nil, nil) | |||
There was a problem hiding this comment.
Good catch. Fixed in the new commit
| if not conf.update_time or (patch_conf and patch_conf.update_time == nil) then | ||
| if not conf.update_time or | ||
| -- For PATCH request, the modification is passed as 'patch_conf' | ||
| -- If the sub path is used, the 'patch_conf' will be a placeholder `true` |
There was a problem hiding this comment.
I think the place holder is nil
There was a problem hiding this comment.
if the path_conf placeholder is true,
(patch_conf and (patch_conf == true or patch_conf.update_time == nil)) =>
( true and (true == true or true.update_time == nil) ), true.update_time will raise error
There was a problem hiding this comment.
sorry, if path_conf is true, true.update_time doesn't been executed.
| @@ -150,14 +150,14 @@ function _M.patch(id, conf, sub_path) | |||
| if code then | |||
| return code, err | |||
| end | |||
| utils.inject_timestamp(node_value, nil, true) | |||
There was a problem hiding this comment.
The seme as below， only upstream is right
There was a problem hiding this comment.
No. The placeholder is true. See other changed files.
|
@spacewander can it be merged today ？ |
|
@zhendongcmss |
Fix #4763
Signed-off-by: spacewander spacewanderlzx@gmail.com
What this PR does / why we need it:
Pre-submission checklist: