apache / kyuubi

Apache Kyuubi is a distributed and multi-tenant gateway to provide serverless SQL on data warehouses and lakehouses.
https://kyuubi.apache.org/
Apache License 2.0
2.11k stars 915 forks source link

[REST] Using ForbiddenException instead of NotAllowedException #6750

Closed turboFei closed 1 month ago

turboFei commented 1 month ago

:mag: Description

Issue References ๐Ÿ”—

Seems NotAllowedException is used for method not allowed, and currently, we use false constructor, the error message we expected would not be return to client end.

It only told:

{"message":"HTTP 405 Method Not Allowed"}

Because the message we used to build the NotAllowedException was treated as allowed method, not as message.

image

Describe Your Solution ๐Ÿ”ง

We should use the ForbidenException instead, and then the error message we excepted can be visible in client end.

https://github.com/apache/kyuubi/blob/85dd5a52efa790195ed46b713c53938ffd31c5d9/kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/api.scala#L47-L51

Types of changes :bookmark:

Test Plan ๐Ÿงช

Behavior Without This Pull Request :coffin:

Behavior With This Pull Request :tada:

Related Unit Tests

image

Checklist ๐Ÿ“

Be nice. Be informative.

codecov-commenter commented 1 month ago

Codecov Report

Attention: Patch coverage is 0% with 13 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (85dd5a5) to head (4dd6fc1). Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
...rg/apache/kyuubi/server/api/v1/AdminResource.scala 0.00% 12 Missing :warning:
...ache/kyuubi/server/KyuubiRestFrontendService.scala 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #6750 +/- ## ====================================== Coverage 0.00% 0.00% ====================================== Files 684 684 Lines 42281 42281 Branches 5768 5768 ====================================== Misses 42281 42281 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bowenliang123 commented 1 month ago

Thanks, merged to master(1.10.0) and branch-1.9(1.9.3).