NPE correction when informing removed project in the listProjectRoles#13022
NPE correction when informing removed project in the listProjectRoles#13022Tonitzpp wants to merge 2 commits intoapache:mainfrom
listProjectRoles#13022Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #13022 +/- ##
=============================================
- Coverage 18.02% 3.52% -14.51%
=============================================
Files 5968 464 -5504
Lines 537086 40137 -496949
Branches 65961 7555 -58406
=============================================
- Hits 96820 1415 -95405
+ Misses 429346 38534 -390812
+ Partials 10920 188 -10732
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes a NullPointerException triggered by calling the listProjectRoles API with a projectid that refers to a removed project, changing the behavior to return a parameter error instead of crashing.
Changes:
- Update
ProjectRoleManagerImpl.findProjectRolesto return an empty list whenprojectIdis null (instead of returning null). - Add explicit project existence validation in
ListProjectRolesCmd.execute()and throwInvalidParameterValueExceptionwhen the project cannot be found.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| server/src/main/java/org/apache/cloudstack/acl/ProjectRoleManagerImpl.java | Prevents null list returns from findProjectRoles when projectId is null. |
| api/src/main/java/org/apache/cloudstack/api/command/admin/acl/project/ListProjectRolesCmd.java | Validates the project exists before listing roles and throws a parameter exception when not found. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
listProjectRoles
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent a NullPointerException in the listProjectRoles API when the caller provides a projectid for a removed/non-existent project, changing the behavior to return a clear parameter error instead.
Changes:
- Validate the provided
projectIdinListProjectRolesCmd.execute()and throw anInvalidParameterValueExceptionwhen the project can’t be found. - Avoid per-role project lookup when building
ProjectRoleResponseby passing the project UUID through the response setup method. - Adjust
ProjectRoleManagerImpl.findProjectRoles()to return an empty list whenprojectIdis null (instead of returningnull).
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/cloudstack/acl/ProjectRoleManagerImpl.java |
Returns an empty list when projectId is null to reduce NPE risk from null list returns. |
api/src/main/java/org/apache/cloudstack/api/command/admin/acl/project/ListProjectRolesCmd.java |
Adds project existence validation and avoids NPE when a project has been removed by not dereferencing a null project during response building. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (project == null) { | ||
| throw new InvalidParameterValueException("Unable to find project by ID."); | ||
| } |
| Project project = null; | ||
| if (getProjectId() != null && getProjectId() > 0) { |
Description
Currently, when using the
listProjectRolesAPI, passing theprojectidparameter of a removed project results in an NPE. This PR changes this behavior to return the messageFailed to find project by IDinstead.Types of changes
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
Before the changes
After the changes
How Has This Been Tested?
To perform the tests, a test project was created along with a project role. The project was deleted and the
listProjectRolesAPI was called passing the ID of the project role. This showed the new message.