Copilot commented on code in PR #14370:
URL: https://github.com/apache/cloudstack/pull/14370#discussion_r4221720848
##########
framework/extensions/src/main/java/org/apache/cloudstack/framework/extensions/manager/ExtensionsManagerImpl.java:
##########
@@ -1306,10 +1314,13 @@ protected void
updateNetworkExtensionServicesOnPhysicalNetworks(ExtensionVO exte
return;
}
for (Service removedService : removedServices) {
- boolean usedByNetworkOffering = CollectionUtils.isNotEmpty(
-
networkOfferingServiceMapDao.listOfferingIdsByServiceAndProvider(removedService,
extension.getName()));
- boolean usedByVpcOffering = CollectionUtils.isNotEmpty(
-
vpcOfferingServiceMapDao.listOfferingIdsByServiceAndProvider(removedService,
extension.getName()));
+ // offering service map rows are kept when an offering is deleted,
so ignore removed offerings
+ boolean usedByNetworkOffering = networkOfferingServiceMapDao
+ .listOfferingIdsByServiceAndProvider(removedService,
extension.getName()).stream()
+ .anyMatch(offeringId ->
networkOfferingDao.findById(offeringId) != null);
+ boolean usedByVpcOffering = vpcOfferingServiceMapDao
+ .listOfferingIdsByServiceAndProvider(removedService,
extension.getName()).stream()
Review Comment:
This change removes the prior null-safety from
`CollectionUtils.isNotEmpty(...)`. If either
`listOfferingIdsByServiceAndProvider(...)` can return `null`, calling
`.stream()` will throw an NPE (a behavior regression). Consider wrapping with a
null-to-empty conversion (e.g., `CollectionUtils.emptyIfNull(...)`) or updating
the DAO contract/implementation to always return an empty list.
##########
framework/extensions/src/main/java/org/apache/cloudstack/framework/extensions/manager/ExtensionsManagerImpl.java:
##########
@@ -1306,10 +1314,13 @@ protected void
updateNetworkExtensionServicesOnPhysicalNetworks(ExtensionVO exte
return;
}
for (Service removedService : removedServices) {
- boolean usedByNetworkOffering = CollectionUtils.isNotEmpty(
-
networkOfferingServiceMapDao.listOfferingIdsByServiceAndProvider(removedService,
extension.getName()));
- boolean usedByVpcOffering = CollectionUtils.isNotEmpty(
-
vpcOfferingServiceMapDao.listOfferingIdsByServiceAndProvider(removedService,
extension.getName()));
+ // offering service map rows are kept when an offering is deleted,
so ignore removed offerings
+ boolean usedByNetworkOffering = networkOfferingServiceMapDao
+ .listOfferingIdsByServiceAndProvider(removedService,
extension.getName()).stream()
+ .anyMatch(offeringId ->
networkOfferingDao.findById(offeringId) != null);
+ boolean usedByVpcOffering = vpcOfferingServiceMapDao
+ .listOfferingIdsByServiceAndProvider(removedService,
extension.getName()).stream()
+ .anyMatch(offeringId ->
vpcOfferingDao.findById(offeringId) != null);
Review Comment:
This introduces an N+1 query pattern: for each removed service, you load
each mapped offering individually via `findById`. If many offerings are mapped,
`updateExtension` could trigger many DB lookups. Prefer a batched/aggregate
approach (e.g., DAO method that returns whether any *non-removed* offering
exists for the service/provider, or a single query that filters out removed
offerings) to keep runtime predictable.
##########
framework/extensions/src/test/java/org/apache/cloudstack/framework/extensions/manager/ExtensionsManagerImplTest.java:
##########
@@ -992,10 +1004,42 @@ public void
testUpdateExtension_RemovingServiceUsedByVpcOfferingThrows() {
.thenReturn(Collections.emptyList());
when(vpcOfferingServiceMapDao.listOfferingIdsByServiceAndProvider(Network.Service.StaticNat,
"MyExt"))
.thenReturn(Collections.singletonList(1L));
+
when(vpcOfferingDao.findById(1L)).thenReturn(mock(VpcOfferingVO.class));
extensionsManager.updateExtension(cmd);
}
+ @Test
+ public void
updateNetworkExtensionServicesOnPhysicalNetworks_IgnoresRemovedOfferings() {
+ ExtensionVO ext = mock(ExtensionVO.class);
+ when(ext.getId()).thenReturn(9L);
+ when(ext.getName()).thenReturn("MyExt");
+
+ // service map rows still reference offerings 1 (network) and 2 (VPC),
but both offerings are removed
+
when(networkOfferingServiceMapDao.listOfferingIdsByServiceAndProvider(Network.Service.CustomAction,
"MyExt"))
+ .thenReturn(Collections.singletonList(1L));
+
when(vpcOfferingServiceMapDao.listOfferingIdsByServiceAndProvider(Network.Service.CustomAction,
"MyExt"))
+ .thenReturn(Collections.singletonList(2L));
+ when(networkOfferingDao.findById(1L)).thenReturn(null);
+ when(vpcOfferingDao.findById(2L)).thenReturn(null);
+
+ when(extensionResourceMapDao.listResourceIdsByExtensionIdAndType(9L,
ExtensionResourceMap.ResourceType.PhysicalNetwork))
+ .thenReturn(Collections.singletonList(100L));
+ PhysicalNetworkServiceProviderVO nsp =
mock(PhysicalNetworkServiceProviderVO.class);
+ when(nsp.getId()).thenReturn(500L);
+ when(nsp.getEnabledServices()).thenReturn(new
ArrayList<>(Collections.singletonList(Network.Service.CustomAction)));
+ when(physicalNetworkServiceProviderDao.findByServiceProvider(100L,
"MyExt")).thenReturn(nsp);
+
+ extensionsManager.updateNetworkExtensionServicesOnPhysicalNetworks(ext,
+ Set.of(Network.Service.CustomAction),
Set.of(Network.Service.UserData));
+
+ ArgumentCaptor<List<Network.Service>> captor =
ArgumentCaptor.forClass(List.class);
Review Comment:
This uses a raw `List.class` captor, which typically triggers unchecked
warnings and can hide type mismatches. Consider using a typed captor pattern
(e.g., `ArgumentCaptor<List<Network.Service>>` with an unchecked cast +
`@SuppressWarnings(\"unchecked\")`, or capturing as `ArgumentCaptor<List>` and
asserting element types explicitly) to keep the test type-safe.
--
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]
