NPE correction when informing removed project in the 'listProjectRoles'#13022
NPE correction when informing removed project in the 'listProjectRoles'#13022Tonitzpp wants to merge 1 commit intoapache:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13022 +/- ##
============================================
- Coverage 18.02% 17.94% -0.08%
- Complexity 16450 16499 +49
============================================
Files 5968 6019 +51
Lines 537086 540750 +3664
Branches 65961 66255 +294
============================================
+ Hits 96820 97061 +241
- Misses 429346 432749 +3403
- Partials 10920 10940 +20
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:
|
| @Override | ||
| public void execute() { | ||
| List<ProjectRole> projectRoles = new ArrayList<>(); | ||
| if (getProjectId() != null && _projectService.getProject(getProjectId()) == null) { |
There was a problem hiding this comment.
I think the special case projectId = -1 is neglected this way. It looks like it is not applicable for ListProjectRoles, but in general that value is allowed. Can you check how to handle it?
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.
| public void execute() { | ||
| List<ProjectRole> projectRoles = new ArrayList<>(); | ||
| if (getProjectId() != null && _projectService.getProject(getProjectId()) == null) { | ||
| throw new InvalidParameterValueException("Failed to find project by ID."); |
| if (getProjectId() != null && _projectService.getProject(getProjectId()) == null) { | ||
| throw new InvalidParameterValueException("Failed to find project by ID."); | ||
| } |
| if (getProjectId() != null && _projectService.getProject(getProjectId()) == null) { | ||
| throw new InvalidParameterValueException("Failed to find project by ID."); | ||
| } |
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.