Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

fix: ref check while deleting proto via Admin API #4575

Merged
merged 11 commits into from
Jul 14, 2021
Merged

Conversation

bluesgao
Copy link
Contributor

@bluesgao bluesgao commented Jul 9, 2021

问题:admin api proto delete 操作 异常。
原因:delete方法调用_M.check_proto_used()方法时,直接返回。如果插件grpc-transcode没有使用需要删除的protoid就会发生错误。
修改点:
1,将proto对象的check_proto_used方法修改为local方法(该方法没有其他地方使用,最好定义为local方法)。
2,对proto对象的delete方法调用check_proto_used方法的地方进行了修改。

Copy link
Member

@spacewander spacewander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test as t/admin/proto.t, like the one in t/admin/upstream2.t?

apisix/admin/proto.lua Outdated Show resolved Hide resolved
apisix/admin/proto.lua Outdated Show resolved Hide resolved
@spacewander spacewander changed the title admin api proto删除操作异常 fix: ref check while deleting proto via Admin API Jul 9, 2021
@bluesgao
Copy link
Contributor Author

one

ok

@bluesgao
Copy link
Contributor Author

Could you add a test as t/admin/proto.t, like the one in t/admin/upstream2.t?

已添加测试用例,路径是 t/admin/proto3.t

apisix/admin/proto.lua Outdated Show resolved Hide resolved
apisix/admin/proto.lua Outdated Show resolved Hide resolved
t/admin/proto3.t Outdated Show resolved Hide resolved
t/admin/proto.t Outdated
GET /t
--- response_body
[push] code: 200 message: passed
[delete] code: 200 message: passed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to refer to the proto in a route/service, so that we can test the path that returns 400.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I will modify proto test case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the problem for this? in Code Lint / lint (pull_request) phase。
The following is the error message:
"
=====bad style=====

echo '=====bad style====='
cat /tmp/error.log
reindex: t/admin/proto.t: done.
echo 'you need to run '''reindex''' to fix them. Read CONTRIBUTING.md for more details.'
you need to run 'reindex' to fix them. Read CONTRIBUTING.md for more details.
exit 1
make: *** [Makefile:121: lint] Error 1
Error: Process completed with exit code 2.
"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

t/admin/proto.t Outdated



=== TEST 3: put proto (id:2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can merge TEST 3~5 into one test? As they both use the Admin API and test for one thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@tokers tokers merged commit bfa70e4 into apache:master Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants