Copilot commented on code in PR #13022:
URL: https://github.com/apache/cloudstack/pull/13022#discussion_r3084316202
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/acl/project/ListProjectRolesCmd.java:
##########
@@ -72,7 +72,10 @@ public String getRoleName() {
@Override
public void execute() {
- List<ProjectRole> projectRoles = new ArrayList<>();
+ if (getProjectId() != null &&
_projectService.getProject(getProjectId()) == null) {
+ throw new InvalidParameterValueException("Failed to find project
by ID.");
Review Comment:
The new error message thrown for a missing/removed project is inconsistent
with other project-related API commands, which typically use "Unable to find
project by ID" (often including the projectId). Consider aligning the
wording/punctuation with existing messages to keep API responses consistent
(and to match the PR description text if that’s meant to be exact).
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/acl/project/ListProjectRolesCmd.java:
##########
@@ -72,7 +72,10 @@ public String getRoleName() {
@Override
public void execute() {
- List<ProjectRole> projectRoles = new ArrayList<>();
+ if (getProjectId() != null &&
_projectService.getProject(getProjectId()) == null) {
+ throw new InvalidParameterValueException("Failed to find project
by ID.");
+ }
Review Comment:
This change adds new behavior (throwing an InvalidParameterValueException
when `projectid` refers to a removed/non-existent project) but there doesn't
appear to be a unit test covering it. Adding a focused test for
`ListProjectRolesCmd.execute()` that asserts the exception type/message would
help prevent regressions back to an NPE.
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/acl/project/ListProjectRolesCmd.java:
##########
@@ -72,7 +72,10 @@ public String getRoleName() {
@Override
public void execute() {
- List<ProjectRole> projectRoles = new ArrayList<>();
+ if (getProjectId() != null &&
_projectService.getProject(getProjectId()) == null) {
+ throw new InvalidParameterValueException("Failed to find project
by ID.");
+ }
Review Comment:
`_projectService.getProject(getProjectId())` is looked up here for
validation, but the project is looked up again later when building each
response. Consider fetching the Project once (or at least its UUID) and reusing
it to avoid repeated DB lookups and to prevent a potential NPE if the project
is deleted between the initial validation and response construction.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
