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

feat: add schema validate admin API #10065

Merged
merged 7 commits into from
Aug 22, 2023

Conversation

kingluo
Copy link
Contributor

@kingluo kingluo commented Aug 21, 2023

Description

https://lists.apache.org/thread/6lsbhh4c9ytjmrkr0jt66j5s4vyh482k

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@kingluo kingluo marked this pull request as ready for review August 21, 2023 09:15
Copy link
Member

@moonming moonming left a comment

Choose a reason for hiding this comment

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

we need more test cases to cover invalid requests.

"scheme": "https",
"type": "roundrobin",
"nodes": {
"nghttp2.org": 1
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"nghttp2.org": 1
"httpbin.org": 1

Copy link
Contributor Author

@kingluo kingluo Aug 21, 2023

Choose a reason for hiding this comment

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

httpbin has been less stable in recent months, and now the community is using nghttp2 more.
Also, regarding schema checking, what matters here is not the actual value, but the type.

Copy link
Member

@tao12345666333 tao12345666333 Aug 22, 2023

Choose a reason for hiding this comment

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

yes.

FYI We can use httpbun.org also

This does not affect this PR

Copy link
Member

Choose a reason for hiding this comment

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

how about https://mock.api7.ai/?

Copy link
Member

Choose a reason for hiding this comment

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

@moonming SGTM.

"scheme": "https",
"type": "roundrobin",
"nodes": {
"nghttp2.org": 1
Copy link
Member

Choose a reason for hiding this comment

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

ditto

"scheme": "https",
"type": "roundrobin",
"nodes": {
"nghttp2.org": 1
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Revolyssup
Revolyssup previously approved these changes Aug 21, 2023
Copy link
Contributor

@Revolyssup Revolyssup left a comment

Choose a reason for hiding this comment

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

LGTM

docs/en/latest/admin-api.md Outdated Show resolved Hide resolved
docs/en/latest/admin-api.md Outdated Show resolved Hide resolved
docs/en/latest/admin-api.md Outdated Show resolved Hide resolved
Co-authored-by: Abhishek Choudhary <shreemaan.abhishek@gmail.com>
kingluo and others added 2 commits August 21, 2023 23:54
Co-authored-by: Abhishek Choudhary <shreemaan.abhishek@gmail.com>
Co-authored-by: Abhishek Choudhary <shreemaan.abhishek@gmail.com>
@kayx23
Copy link
Member

kayx23 commented Aug 22, 2023

Just a question: why not doing the schema check automatically before users sending requests to any Admin API endpoints? The current approach, that is to ask users to send requests to a specific /validate endpoint, feels rather manual.

Copy link
Member

@moonming moonming left a comment

Choose a reason for hiding this comment

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

we need more test cases not only in routes

@moonming
Copy link
Member

Just a question: why not doing the schema check automatically before users sending requests to any Admin API endpoints? The current approach, that is to ask users to send requests to a specific /validate endpoint, feels rather manual.

The user's request already run the schema check.
The purpose of this API is only to check the schema of requests, no more no less.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants