[#10166] Resolve NPE problem in grant/revoke permission's error handling when request body is null#10258
Conversation
4c3203f to
f50e034
Compare
There was a problem hiding this comment.
Pull request overview
Fixes a NullPointerException risk in PermissionOperations when role grant/revoke endpoints receive a null request body, ensuring error handling can still build the role-names string safely and return a proper error response.
Changes:
- Add null-guarded
roleNamesconstruction in catch blocks for user/group role grant/revoke endpoints. - Add unit tests covering
nullrequest bodies for grant/revoke role endpoints.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
server/src/main/java/org/apache/gravitino/server/web/rest/PermissionOperations.java |
Makes catch-block role name formatting null-safe for grant/revoke role operations. |
server/src/test/java/org/apache/gravitino/server/web/rest/TestPermissionOperations.java |
Adds tests asserting endpoints don’t NPE when the request object is null. |
server/src/test/java/org/apache/gravitino/server/web/rest/TestPermissionOperations.java
Show resolved
Hide resolved
server/src/test/java/org/apache/gravitino/server/web/rest/TestPermissionOperations.java
Show resolved
Hide resolved
f50e034 to
d15a2b0
Compare
… handling when request body is null
d15a2b0 to
2dec471
Compare
Code Coverage Report
Files
|
jerryshao
left a comment
There was a problem hiding this comment.
Code Review
The fix is correct — creating a local roleNames variable before the return in each catch block avoids the secondary NPE when request is null.
Important: PR #10183 covers the exact same catch blocks in PermissionOperations.java (and 15 other files) using the same inline ternary null-check pattern. These two PRs will conflict. Please coordinate with the author of #10183 — one of these PRs should be closed to avoid a merge conflict. Since #10183 is a broader, more comprehensive fix, consider whether this PR should be closed in its favor.
The unit tests added are good — they cover all four grant/revoke methods. If this PR proceeds independently, please ensure it is rebased on top of #10183 (or vice versa).
|
I'll close the PR for now. |
What changes were proposed in this pull request?
Build role-name string with a null guard in grant/revoke permission operation error handling logics
Why are the changes needed?
To avoid NPE
Fix: #10166
Does this PR introduce any user-facing change?
No
How was this patch tested?
UTs