You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/08/25 02:58:20 UTC
[GitHub] [apisix] bzp2010 opened a new pull request, #7778: fix: admin api v3 count and list length mismatch
bzp2010 opened a new pull request, #7778:
URL: https://github.com/apache/apisix/pull/7778
### Description
After using the new filter feature added to Admin API v3, the filtered out data is not removed from the count in the count field.
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [x] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #7778: feat: remove useless field in Admin API v3
Posted by GitBox <gi...@apache.org>.
spacewander merged PR #7778:
URL: https://github.com/apache/apisix/pull/7778
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] bzp2010 commented on pull request #7778: feat: remove useless field in Admin API v3
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on PR #7778:
URL: https://github.com/apache/apisix/pull/7778#issuecomment-1231277366
@spacewander Currently we don't have a filter for all the data, so I had to filter here to execute the logic separately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #7778: fix: admin api v3 count and list length mismatch
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7778:
URL: https://github.com/apache/apisix/pull/7778#discussion_r954533052
##########
t/admin/filter.t:
##########
@@ -752,3 +752,129 @@ passed
}
--- response_body
passed
+
+
+
+=== TEST 15: filtered data count
+--- config
+ location /t {
+ content_by_lua_block {
+ local json = require("toolkit.json")
+ local core = require("apisix.core")
+ local t = require("lib.test_admin").test
+
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+ if code >= 300 then
+ ngx.status = code
+ ngx.say(body)
+ return
+ end
+
+ code, body = t('/apisix/admin/routes/2',
+ ngx.HTTP_PUT,
+ [[{
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+ if code >= 300 then
+ ngx.status = code
+ ngx.say(body)
+ return
+ end
+
+ ngx.sleep(0.5)
+
+ local code, body, res = t('/apisix/admin/routes', ngx.HTTP_GET)
+ res = json.decode(res)
+ assert(res.count == #res.list)
+
+ local code, body, res = t('/apisix/admin/routes/?label=', ngx.HTTP_GET)
+ res = json.decode(res)
+ assert(res.count == #res.list)
+
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 16: pagination data count
Review Comment:
TEST 16 is equal to 15? And it seems there is no need to recreated the routes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] bzp2010 commented on a diff in pull request #7778: fix: admin api v3 count and list length mismatch
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #7778:
URL: https://github.com/apache/apisix/pull/7778#discussion_r954540506
##########
t/admin/filter.t:
##########
@@ -752,3 +752,129 @@ passed
}
--- response_body
passed
+
+
+
+=== TEST 15: filtered data count
+--- config
+ location /t {
+ content_by_lua_block {
+ local json = require("toolkit.json")
+ local core = require("apisix.core")
+ local t = require("lib.test_admin").test
+
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+ if code >= 300 then
+ ngx.status = code
+ ngx.say(body)
+ return
+ end
+
+ code, body = t('/apisix/admin/routes/2',
+ ngx.HTTP_PUT,
+ [[{
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+ if code >= 300 then
+ ngx.status = code
+ ngx.say(body)
+ return
+ end
+
+ ngx.sleep(0.5)
+
+ local code, body, res = t('/apisix/admin/routes', ngx.HTTP_GET)
+ res = json.decode(res)
+ assert(res.count == #res.list)
+
+ local code, body, res = t('/apisix/admin/routes/?label=', ngx.HTTP_GET)
+ res = json.decode(res)
+ assert(res.count == #res.list)
+
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 16: pagination data count
Review Comment:
changed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] bzp2010 commented on a diff in pull request #7778: fix: admin api v3 count and list length mismatch
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #7778:
URL: https://github.com/apache/apisix/pull/7778#discussion_r955582647
##########
apisix/admin/v3_adapter.lua:
##########
@@ -183,6 +183,9 @@ function _M.filter(body)
pagination(body, args)
filter(body, args)
+
+ -- recalculate the amount of filtered data
+ body.count = body.list and #body.list or 0
Review Comment:
ping @starsz @soulbird
Yes, as I mentioned in the issue, I was thinking the same thing, do you think it would be better to use `count` or add another `total` field?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] starsz commented on a diff in pull request #7778: fix: admin api v3 count and list length mismatch
Posted by GitBox <gi...@apache.org>.
starsz commented on code in PR #7778:
URL: https://github.com/apache/apisix/pull/7778#discussion_r954806738
##########
apisix/admin/v3_adapter.lua:
##########
@@ -183,6 +183,9 @@ function _M.filter(body)
pagination(body, args)
filter(body, args)
+
+ -- recalculate the amount of filtered data
+ body.count = body.list and #body.list or 0
Review Comment:
But I think the count should be the total object of the resource. Not the count of the list.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] bzp2010 commented on a diff in pull request #7778: fix: admin api v3 count and list length mismatch
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #7778:
URL: https://github.com/apache/apisix/pull/7778#discussion_r954538559
##########
t/admin/filter.t:
##########
@@ -752,3 +752,129 @@ passed
}
--- response_body
passed
+
+
+
+=== TEST 15: filtered data count
+--- config
+ location /t {
+ content_by_lua_block {
+ local json = require("toolkit.json")
+ local core = require("apisix.core")
+ local t = require("lib.test_admin").test
+
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+ if code >= 300 then
+ ngx.status = code
+ ngx.say(body)
+ return
+ end
+
+ code, body = t('/apisix/admin/routes/2',
+ ngx.HTTP_PUT,
+ [[{
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+ if code >= 300 then
+ ngx.status = code
+ ngx.say(body)
+ return
+ end
+
+ ngx.sleep(0.5)
+
+ local code, body, res = t('/apisix/admin/routes', ngx.HTTP_GET)
+ res = json.decode(res)
+ assert(res.count == #res.list)
+
+ local code, body, res = t('/apisix/admin/routes/?label=', ngx.HTTP_GET)
+ res = json.decode(res)
+ assert(res.count == #res.list)
+
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 16: pagination data count
Review Comment:
It looks like I made an incorrect copy and overwrote the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] bzp2010 closed pull request #7778: fix: admin api v3 count and list length mismatch
Posted by GitBox <gi...@apache.org>.
bzp2010 closed pull request #7778: fix: admin api v3 count and list length mismatch
URL: https://github.com/apache/apisix/pull/7778
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] bzp2010 commented on a diff in pull request #7778: feat: remove useless field in Admin API v3
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #7778:
URL: https://github.com/apache/apisix/pull/7778#discussion_r957977312
##########
apisix/admin/v3_adapter.lua:
##########
@@ -181,8 +181,31 @@ function _M.filter(body)
local args = request.get_uri_args()
- pagination(body, args)
- filter(body, args)
+ if body.deleted then
+ body.node = nil
+ end
+
+ -- strip node wrapping for single query, create, and update scenarios.
+ if body.node then
+ body = body.node
+ end
+
+ -- filter and paging logic for list query only
+ if body.list then
+ filter(body, args)
+
+ -- calculate the total amount of filtered data
+ body.total = body.list and #body.list or 0
+
+ pagination(body, args)
+
+ -- remove the count field returned by etcd
+ -- we don't need a field that reflects the length of the currently returned data,
+ -- it doesn't make sense
+ body.count = nil
+ end
+
+ return body
Review Comment:
Yes, I also suffer from it, sometimes I do field overwriting and it does not take effect, the problem is strange.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] bzp2010 commented on a diff in pull request #7778: feat: remove useless field in Admin API v3
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #7778:
URL: https://github.com/apache/apisix/pull/7778#discussion_r958094034
##########
apisix/admin/v3_adapter.lua:
##########
@@ -180,9 +181,33 @@ function _M.filter(body)
end
local args = request.get_uri_args()
+ local processed_body = deepcopy(body)
Review Comment:
@spacewander In the comment above, @soulbird points out that we need to copy the body to avoid logical and coding problems.
https://github.com/apache/apisix/pull/7778#discussion_r957974650
Do you think we need it?
##########
apisix/admin/v3_adapter.lua:
##########
@@ -180,9 +181,33 @@ function _M.filter(body)
end
local args = request.get_uri_args()
+ local processed_body = deepcopy(body)
Review Comment:
@spacewander In the comment above, @soulbird points out that we need to copy the body to avoid logical and coding problems. https://github.com/apache/apisix/pull/7778#discussion_r957974650
Do you think we need it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on a diff in pull request #7778: feat: remove useless field in Admin API v3
Posted by GitBox <gi...@apache.org>.
soulbird commented on code in PR #7778:
URL: https://github.com/apache/apisix/pull/7778#discussion_r957974650
##########
apisix/admin/v3_adapter.lua:
##########
@@ -181,8 +181,31 @@ function _M.filter(body)
local args = request.get_uri_args()
- pagination(body, args)
- filter(body, args)
+ if body.deleted then
+ body.node = nil
+ end
+
+ -- strip node wrapping for single query, create, and update scenarios.
+ if body.node then
+ body = body.node
+ end
+
+ -- filter and paging logic for list query only
+ if body.list then
+ filter(body, args)
+
+ -- calculate the total amount of filtered data
+ body.total = body.list and #body.list or 0
+
+ pagination(body, args)
+
+ -- remove the count field returned by etcd
+ -- we don't need a field that reflects the length of the currently returned data,
+ -- it doesn't make sense
+ body.count = nil
+ end
+
+ return body
Review Comment:
Can we create a new body and then populate its fields? I don't think it's intuitive to change fields on the old body structure. I can't see what the final output data structure looks like with the current code, it's really hard to maintain. cc @tzssangglass
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #7778: feat: remove useless field in Admin API v3
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #7778:
URL: https://github.com/apache/apisix/pull/7778#issuecomment-1231248341
Is it possible to move the filter to https://github.com/apache/apisix/blob/7a24ea622b22aa4bf37c3fe19c228505cac003eb/apisix/admin/init.lua#L195?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #7778: feat: remove useless field in Admin API v3
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7778:
URL: https://github.com/apache/apisix/pull/7778#discussion_r958090196
##########
apisix/admin/v3_adapter.lua:
##########
@@ -180,9 +181,33 @@ function _M.filter(body)
end
local args = request.get_uri_args()
+ local processed_body = deepcopy(body)
Review Comment:
Why do we need to copy the body?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] bzp2010 commented on pull request #7778: fix: admin api v3 count and list length mismatch
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on PR #7778:
URL: https://github.com/apache/apisix/pull/7778#issuecomment-1226680737
### Update
When I fix it I find more logical errors that I will describe and fix in new issues and PRs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a diff in pull request #7778: feat: remove useless field in Admin API v3
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7778:
URL: https://github.com/apache/apisix/pull/7778#discussion_r958331543
##########
apisix/admin/v3_adapter.lua:
##########
@@ -181,8 +181,31 @@ function _M.filter(body)
local args = request.get_uri_args()
- pagination(body, args)
- filter(body, args)
+ if body.deleted then
+ body.node = nil
+ end
+
+ -- strip node wrapping for single query, create, and update scenarios.
+ if body.node then
+ body = body.node
+ end
+
+ -- filter and paging logic for list query only
+ if body.list then
+ filter(body, args)
+
+ -- calculate the total amount of filtered data
+ body.total = body.list and #body.list or 0
+
+ pagination(body, args)
+
+ -- remove the count field returned by etcd
+ -- we don't need a field that reflects the length of the currently returned data,
+ -- it doesn't make sense
+ body.count = nil
+ end
+
+ return body
Review Comment:
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #7778: feat: remove useless field in Admin API v3
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7778:
URL: https://github.com/apache/apisix/pull/7778#discussion_r958092107
##########
apisix/admin/v3_adapter.lua:
##########
@@ -180,9 +181,33 @@ function _M.filter(body)
end
local args = request.get_uri_args()
+ local processed_body = deepcopy(body)
Review Comment:
OK, I got it now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] bzp2010 commented on pull request #7778: feat: remove useless field in Admin API v3
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on PR #7778:
URL: https://github.com/apache/apisix/pull/7778#issuecomment-1231325938
### Update
I added a new `need_v3_filter` field to the `_M` exported in the resource module that needs to use the filter, which will be centrally determined in `init.lua`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org