Copilot commented on code in PR #12372:
URL: https://github.com/apache/cloudstack/pull/12372#discussion_r2662370766
##########
server/src/main/java/com/cloud/server/ManagementServerImpl.java:
##########
@@ -2648,12 +2649,21 @@ public Pair<List<? extends IpAddress>, Integer>
searchForIPAddresses(final ListP
}
if (associatedNetworkId != null) {
- _accountMgr.checkAccess(caller, null, false,
networkDao.findById(associatedNetworkId));
- sc.setParameters("associatedNetworkIdEq", associatedNetworkId);
+ NetworkVO associatedNetwork =
networkDao.findById(associatedNetworkId);
+
+ if (associatedNetwork != null) {
+ _accountMgr.checkAccess(caller, null, false,
associatedNetwork);
+ sc.setParameters("associatedNetworkIdEq", associatedNetworkId);
+ }
}
+
if (vpcId != null) {
- _accountMgr.checkAccess(caller, null, false,
_vpcDao.findById(vpcId));
- sc.setParameters("vpcId", vpcId);
+ VpcVO vpc = _vpcDao.findById(vpcId);
+
+ if (vpc != null) {
+ _accountMgr.checkAccess(caller, null, false, vpc);
+ sc.setParameters("vpcId", vpcId);
+ }
}
Review Comment:
The fix for the NPE is incomplete. While null checks are added for
associatedNetworkId and vpcId here, there's no test coverage for these specific
scenarios. Since ManagementServerImplTest already has tests for the
setParameters method with ListPublicIpAddressesCmd, consider adding tests that
verify the behavior when removed network/VPC IDs are provided.
##########
server/src/main/java/com/cloud/server/ManagementServerImpl.java:
##########
@@ -2648,12 +2649,21 @@ public Pair<List<? extends IpAddress>, Integer>
searchForIPAddresses(final ListP
}
if (associatedNetworkId != null) {
- _accountMgr.checkAccess(caller, null, false,
networkDao.findById(associatedNetworkId));
- sc.setParameters("associatedNetworkIdEq", associatedNetworkId);
+ NetworkVO associatedNetwork =
networkDao.findById(associatedNetworkId);
+
+ if (associatedNetwork != null) {
+ _accountMgr.checkAccess(caller, null, false,
associatedNetwork);
+ sc.setParameters("associatedNetworkIdEq", associatedNetworkId);
+ }
}
+
if (vpcId != null) {
- _accountMgr.checkAccess(caller, null, false,
_vpcDao.findById(vpcId));
- sc.setParameters("vpcId", vpcId);
+ VpcVO vpc = _vpcDao.findById(vpcId);
+
+ if (vpc != null) {
+ _accountMgr.checkAccess(caller, null, false, vpc);
+ sc.setParameters("vpcId", vpcId);
+ }
Review Comment:
When a removed VPC ID is provided, the code silently skips the search
parameter without informing the user. This creates inconsistent API behavior
where a valid but removed VPC ID returns all public IPs instead of an empty
result or an error. Consider throwing an InvalidParameterValueException when
the VPC is not found to provide clearer feedback to the API caller.
```suggestion
if (vpc == null) {
throw new InvalidParameterValueException("Unable to find VPC
by id " + vpcId);
}
_accountMgr.checkAccess(caller, null, false, vpc);
sc.setParameters("vpcId", vpcId);
```
##########
server/src/main/java/com/cloud/server/ManagementServerImpl.java:
##########
@@ -2648,12 +2649,21 @@ public Pair<List<? extends IpAddress>, Integer>
searchForIPAddresses(final ListP
}
if (associatedNetworkId != null) {
- _accountMgr.checkAccess(caller, null, false,
networkDao.findById(associatedNetworkId));
- sc.setParameters("associatedNetworkIdEq", associatedNetworkId);
+ NetworkVO associatedNetwork =
networkDao.findById(associatedNetworkId);
+
+ if (associatedNetwork != null) {
+ _accountMgr.checkAccess(caller, null, false,
associatedNetwork);
+ sc.setParameters("associatedNetworkIdEq", associatedNetworkId);
Review Comment:
When a removed network ID is provided, the code silently skips the search
parameter without informing the user. This creates inconsistent API behavior
where a valid but removed network ID returns all public IPs instead of an empty
result or an error. Consider throwing an InvalidParameterValueException when
the network is not found to provide clearer feedback to the API caller.
```suggestion
sc.setParameters("associatedNetworkIdEq",
associatedNetworkId);
} else {
throw new InvalidParameterValueException("Unable to find
network by id " + associatedNetworkId);
```
--
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]
