This is an automated email from the ASF dual-hosted git repository. dahn pushed a commit to branch 4.19 in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/4.19 by this push: new 275abaff6bc Refactor updateDiskOffering API (#8446) 275abaff6bc is described below commit 275abaff6bcc942dd35972e6bd7808166a397391 Author: Henrique Sato <henriquesato2...@gmail.com> AuthorDate: Mon Feb 19 06:29:45 2024 -0300 Refactor updateDiskOffering API (#8446) Co-authored-by: Henrique Sato <henrique.s...@scclouds.com.br> --- .../configuration/ConfigurationManagerImpl.java | 208 +++++++++++++-------- .../ConfigurationManagerImplTest.java | 157 ++++++++++++++++ 2 files changed, 289 insertions(+), 76 deletions(-) diff --git a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java index d84673efd6b..de5311be635 100644 --- a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java +++ b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java @@ -3919,22 +3919,9 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati List<Long> existingZoneIds = diskOfferingDetailsDao.findZoneIds(diskOfferingId); Collections.sort(existingZoneIds); - // check if valid domain - if (CollectionUtils.isNotEmpty(domainIds)) { - for (final Long domainId: domainIds) { - if (_domainDao.findById(domainId) == null) { - throw new InvalidParameterValueException("Please specify a valid domain id"); - } - } - } + validateDomain(domainIds); - // check if valid zone - if (CollectionUtils.isNotEmpty(zoneIds)) { - for (Long zoneId : zoneIds) { - if (_zoneDao.findById(zoneId) == null) - throw new InvalidParameterValueException("Please specify a valid zone id"); - } - } + validateZone(zoneIds); Long userId = CallContext.current().getCallingUserId(); if (userId == null) { @@ -3957,35 +3944,16 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati Collections.sort(filteredZoneIds); if (account.getType() == Account.Type.DOMAIN_ADMIN) { - if (!filteredZoneIds.equals(existingZoneIds)) { // Domain-admins cannot update zone(s) for offerings - throw new InvalidParameterValueException(String.format("Unable to update zone(s) for disk offering: %s by admin: %s as it is domain-admin", diskOfferingHandle.getUuid(), user.getUuid())); - } - if (existingDomainIds.isEmpty()) { - throw new InvalidParameterValueException(String.format("Unable to update public disk offering: %s by user: %s because it is domain-admin", diskOfferingHandle.getUuid(), user.getUuid())); - } else { - if (filteredDomainIds.isEmpty()) { - throw new InvalidParameterValueException(String.format("Unable to update disk offering: %s to a public offering by user: %s because it is domain-admin", diskOfferingHandle.getUuid(), user.getUuid())); - } - } + checkDomainAdminUpdateOfferingRestrictions(diskOfferingHandle, user, filteredZoneIds, existingZoneIds, existingDomainIds, filteredDomainIds); + if (StringUtils.isNotBlank(tags) && !ALLOW_DOMAIN_ADMINS_TO_CREATE_TAGGED_OFFERINGS.valueIn(account.getAccountId())) { throw new InvalidParameterValueException(String.format("User [%s] is unable to update disk offering tags.", user.getUuid())); } - List<Long> nonChildDomains = new ArrayList<>(); - for (Long domainId : existingDomainIds) { - if (!_domainDao.isChildDomain(account.getDomainId(), domainId)) { - if (name != null || displayText != null || sortKey != null) { // Domain-admins cannot update name, display text, sort key for offerings with domain which are not child domains for domain-admin - throw new InvalidParameterValueException(String.format("Unable to update disk offering: %s as it has linked domain(s) which are not child domain for domain-admin: %s", diskOfferingHandle.getUuid(), user.getUuid())); - } - nonChildDomains.add(domainId); - } - } - for (Long domainId : filteredDomainIds) { - if (!_domainDao.isChildDomain(account.getDomainId(), domainId)) { - Domain domain = _entityMgr.findById(Domain.class, domainId); - throw new InvalidParameterValueException(String.format("Unable to update disk offering: %s by domain-admin: %s with domain: %3$s which is not a child domain", diskOfferingHandle.getUuid(), user.getUuid(), domain.getUuid())); - } - } + List<Long> nonChildDomains = getAccountNonChildDomains(diskOfferingHandle, account, user, cmd, existingDomainIds); + + checkIfDomainIsChildDomain(diskOfferingHandle, account, user, filteredDomainIds); + filteredDomainIds.addAll(nonChildDomains); // Final list must include domains which were not child domain for domain-admin but specified for this offering prior to update } else if (account.getType() != Account.Type.ADMIN) { throw new InvalidParameterValueException(String.format("Unable to update disk offering: %s by id user: %s because it is not root-admin or domain-admin", diskOfferingHandle.getUuid(), user.getUuid())); @@ -4001,22 +3969,7 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati } final DiskOfferingVO diskOffering = _diskOfferingDao.createForUpdate(diskOfferingId); - - if (name != null) { - diskOffering.setName(name); - } - - if (displayText != null) { - diskOffering.setDisplayText(displayText); - } - - if (sortKey != null) { - diskOffering.setSortKey(sortKey); - } - - if (displayDiskOffering != null) { - diskOffering.setDisplayOffering(displayDiskOffering); - } + updateDiskOfferingIfCmdAttributeNotNull(diskOffering, cmd); updateOfferingTagsIfIsNotNull(tags, diskOffering); @@ -4039,26 +3992,7 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati } List<DiskOfferingDetailVO> detailsVO = new ArrayList<>(); if(detailsUpdateNeeded) { - SearchBuilder<DiskOfferingDetailVO> sb = diskOfferingDetailsDao.createSearchBuilder(); - sb.and("offeringId", sb.entity().getResourceId(), SearchCriteria.Op.EQ); - sb.and("detailName", sb.entity().getName(), SearchCriteria.Op.EQ); - sb.done(); - SearchCriteria<DiskOfferingDetailVO> sc = sb.create(); - sc.setParameters("offeringId", String.valueOf(diskOfferingId)); - if(!filteredDomainIds.equals(existingDomainIds)) { - sc.setParameters("detailName", ApiConstants.DOMAIN_ID); - diskOfferingDetailsDao.remove(sc); - for (Long domainId : filteredDomainIds) { - detailsVO.add(new DiskOfferingDetailVO(diskOfferingId, ApiConstants.DOMAIN_ID, String.valueOf(domainId), false)); - } - } - if(!filteredZoneIds.equals(existingZoneIds)) { - sc.setParameters("detailName", ApiConstants.ZONE_ID); - diskOfferingDetailsDao.remove(sc); - for (Long zoneId : filteredZoneIds) { - detailsVO.add(new DiskOfferingDetailVO(diskOfferingId, ApiConstants.ZONE_ID, String.valueOf(zoneId), false)); - } - } + updateDiskOfferingDetails(detailsVO, diskOfferingId, filteredDomainIds, existingDomainIds, filteredZoneIds, existingZoneIds); } if (!detailsVO.isEmpty()) { for (DiskOfferingDetailVO detailVO : detailsVO) { @@ -4069,6 +4003,128 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati return _diskOfferingDao.findById(diskOfferingId); } + protected void validateDomain(List<Long> domainIds) { + if (CollectionUtils.isEmpty(domainIds)) { + return; + } + + for (final Long domainId: domainIds) { + if (_domainDao.findById(domainId) == null) { + throw new InvalidParameterValueException("Please specify a valid domain id."); + } + } + } + + protected void validateZone(List<Long> zoneIds) { + if (CollectionUtils.isEmpty(zoneIds)) { + return; + } + + for (Long zoneId : zoneIds) { + if (_zoneDao.findById(zoneId) == null) { + throw new InvalidParameterValueException("Please specify a valid zone id."); + } + } + } + + protected void updateDiskOfferingIfCmdAttributeNotNull(DiskOfferingVO diskOffering, UpdateDiskOfferingCmd cmd) { + if (cmd.getDiskOfferingName() != null) { + diskOffering.setName(cmd.getDiskOfferingName()); + } + + if (cmd.getDisplayText() != null) { + diskOffering.setDisplayText(cmd.getDisplayText()); + } + + if (cmd.getSortKey() != null) { + diskOffering.setSortKey(cmd.getSortKey()); + } + + if (cmd.getDisplayOffering() != null) { + diskOffering.setDisplayOffering(cmd.getDisplayOffering()); + } + } + + protected void updateDiskOfferingDetails(List<DiskOfferingDetailVO> detailsVO, Long diskOfferingId, List<Long> filteredDomainIds, + List<Long> existingDomainIds, List<Long> filteredZoneIds, List<Long> existingZoneIds) { + SearchBuilder<DiskOfferingDetailVO> sb = diskOfferingDetailsDao.createSearchBuilder(); + sb.and("offeringId", sb.entity().getResourceId(), SearchCriteria.Op.EQ); + sb.and("detailName", sb.entity().getName(), SearchCriteria.Op.EQ); + sb.done(); + SearchCriteria<DiskOfferingDetailVO> sc = sb.create(); + sc.setParameters("offeringId", String.valueOf(diskOfferingId)); + + updateDiskOfferingDetailsDomainIds(detailsVO, sc, diskOfferingId, filteredDomainIds, existingDomainIds); + updateDiskOfferingDetailsZoneIds(detailsVO, sc, diskOfferingId, filteredZoneIds, existingZoneIds); + } + + protected void updateDiskOfferingDetailsDomainIds(List<DiskOfferingDetailVO> detailsVO, SearchCriteria<DiskOfferingDetailVO> sc, Long diskOfferingId, List<Long> filteredDomainIds, List<Long> existingDomainIds) { + if (filteredDomainIds.equals(existingDomainIds)) { + return; + } + + sc.setParameters("detailName", ApiConstants.DOMAIN_ID); + diskOfferingDetailsDao.remove(sc); + for (Long domainId : filteredDomainIds) { + detailsVO.add(new DiskOfferingDetailVO(diskOfferingId, ApiConstants.DOMAIN_ID, String.valueOf(domainId), false)); + } + } + + protected void updateDiskOfferingDetailsZoneIds(List<DiskOfferingDetailVO> detailsVO, SearchCriteria<DiskOfferingDetailVO> sc, Long diskOfferingId, List<Long> filteredZoneIds, List<Long> existingZoneIds) { + if (filteredZoneIds.equals(existingZoneIds)) { + return; + } + + sc.setParameters("detailName", ApiConstants.ZONE_ID); + diskOfferingDetailsDao.remove(sc); + for (Long zoneId : filteredZoneIds) { + detailsVO.add(new DiskOfferingDetailVO(diskOfferingId, ApiConstants.ZONE_ID, String.valueOf(zoneId), false)); + } + } + + protected void checkDomainAdminUpdateOfferingRestrictions(DiskOffering diskOffering, User user, List<Long> filteredZoneIds, List<Long> existingZoneIds, + List<Long> existingDomainIds, List<Long> filteredDomainIds) { + if (!filteredZoneIds.equals(existingZoneIds)) { + throw new InvalidParameterValueException(String.format("Unable to update zone(s) for disk offering [%s] by admin [%s] as it is domain-admin.", diskOffering.getUuid(), user.getUuid())); + } + if (existingDomainIds.isEmpty()) { + throw new InvalidParameterValueException(String.format("Unable to update public disk offering [%s] by user [%s] because it is domain-admin.", diskOffering.getUuid(), user.getUuid())); + } + if (filteredDomainIds.isEmpty()) { + throw new InvalidParameterValueException(String.format("Unable to update disk offering [%s] to a public offering by user [%s] because it is domain-admin.", diskOffering.getUuid(), user.getUuid())); + } + } + + protected List<Long> getAccountNonChildDomains(DiskOffering diskOffering, Account account, User user, + UpdateDiskOfferingCmd cmd, List<Long> existingDomainIds) { + List<Long> nonChildDomains = new ArrayList<>(); + String name = cmd.getDiskOfferingName(); + String displayText = cmd.getDisplayText(); + Integer sortKey = cmd.getSortKey(); + for (Long domainId : existingDomainIds) { + if (_domainDao.isChildDomain(account.getDomainId(), domainId)) { + continue; + } + + if (ObjectUtils.anyNotNull(name, displayText, sortKey)) { + throw new InvalidParameterValueException(String.format("Unable to update disk offering [%s] as it has linked domain(s) which are not child domain for domain-admin [%s].", diskOffering.getUuid(), user.getUuid())); + } + nonChildDomains.add(domainId); + } + return nonChildDomains; + } + + protected void checkIfDomainIsChildDomain(DiskOffering diskOffering, Account account, User user, List<Long> filteredDomainIds) { + for (Long domainId : filteredDomainIds) { + if (_domainDao.isChildDomain(account.getDomainId(), domainId)) { + continue; + } + + Domain domain = _entityMgr.findById(Domain.class, domainId); + throw new InvalidParameterValueException(String.format("Unable to update disk offering [%s] by domain-admin [%s] with domain [%3$s] which is not a child domain.", diskOffering.getUuid(), user.getUuid(), domain.getUuid())); + } + } + /** * Check the tags parameters to the disk/service offering * <ul> diff --git a/server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java b/server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java index 38189313a52..958a39be410 100644 --- a/server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java +++ b/server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java @@ -21,6 +21,18 @@ import com.cloud.storage.StorageManager; import com.cloud.utils.net.NetUtils; import org.apache.cloudstack.framework.config.ConfigDepot; import org.apache.cloudstack.framework.config.ConfigKey; +import com.cloud.dc.dao.DataCenterDao; +import com.cloud.domain.Domain; +import com.cloud.domain.dao.DomainDao; +import com.cloud.offering.DiskOffering; +import com.cloud.storage.DiskOfferingVO; +import com.cloud.user.Account; +import com.cloud.user.User; +import com.cloud.utils.db.EntityManager; +import com.cloud.utils.db.SearchCriteria; +import org.apache.cloudstack.api.command.admin.offering.UpdateDiskOfferingCmd; +import org.apache.cloudstack.resourcedetail.DiskOfferingDetailVO; +import org.apache.cloudstack.resourcedetail.dao.DiskOfferingDetailsDao; import org.junit.Assert; import org.junit.Before; import org.junit.Test; @@ -29,7 +41,10 @@ import org.mockito.Mock; import org.mockito.MockedStatic; import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; +import org.mockito.InjectMocks; +import org.mockito.Spy; +import java.util.ArrayList; import java.util.List; @@ -37,7 +52,40 @@ import java.util.List; public class ConfigurationManagerImplTest { @Mock ConfigDepot configDepot; + @InjectMocks ConfigurationManagerImpl configurationManagerImplSpy = Mockito.spy(new ConfigurationManagerImpl()); + @Mock + SearchCriteria<DiskOfferingDetailVO> searchCriteriaDiskOfferingDetailMock; + @Mock + DiskOffering diskOfferingMock; + @Mock + Account accountMock; + @Mock + User userMock; + @Mock + Domain domainMock; + @Mock + DataCenterDao zoneDaoMock; + @Mock + DomainDao domainDaoMock; + @Mock + EntityManager entityManagerMock; + @Mock + DiskOfferingDetailsDao diskOfferingDetailsDao; + @Spy + DiskOfferingVO diskOfferingVOSpy; + @Mock + UpdateDiskOfferingCmd updateDiskOfferingCmdMock; + + Long validId = 1L; + Long invalidId = 100L; + List<Long> filteredZoneIds = List.of(1L, 2L, 3L); + List<Long> existingZoneIds = List.of(1L, 2L, 3L); + List<Long> filteredDomainIds = List.of(1L, 2L, 3L); + List<Long> existingDomainIds = List.of(1L, 2L, 3L); + List<Long> emptyExistingZoneIds = new ArrayList<>(); + List<Long> emptyExistingDomainIds = new ArrayList<>(); + List<Long> emptyFilteredDomainIds = new ArrayList<>(); @Before public void setUp() throws Exception { @@ -50,6 +98,7 @@ public class ConfigurationManagerImplTest { Assert.assertNull(testVariable); } + @Test public void validateIfIntValueIsInRangeTestInvalidValueReturnString() { String testVariable = configurationManagerImplSpy.validateIfIntValueIsInRange("String name", "9", "1-5"); @@ -250,4 +299,112 @@ public class ConfigurationManagerImplTest { Mockito.doReturn(key).when(configurationManagerImplSpy._configDepot).get("config.iprange"); configurationManagerImplSpy.validateIpAddressRelatedConfigValues("config.iprange", "192.168.1.1-192.168.1.100"); } + + @Test + public void validateDomainTestInvalidIdThrowException() { + Mockito.doReturn(null).when(domainDaoMock).findById(invalidId); + Assert.assertThrows(InvalidParameterValueException.class, () -> configurationManagerImplSpy.validateDomain(List.of(invalidId))); + } + + @Test + public void validateZoneTestInvalidIdThrowException() { + Mockito.doReturn(null).when(zoneDaoMock).findById(invalidId); + Assert.assertThrows(InvalidParameterValueException.class, () -> configurationManagerImplSpy.validateZone(List.of(invalidId))); + } + + @Test + public void updateDiskOfferingIfCmdAttributeNotNullTestNotNullValueUpdateOfferingAttribute() { + Mockito.doReturn("DiskOfferingName").when(updateDiskOfferingCmdMock).getDiskOfferingName(); + Mockito.doReturn("DisplayText").when(updateDiskOfferingCmdMock).getDisplayText(); + Mockito.doReturn(1).when(updateDiskOfferingCmdMock).getSortKey(); + Mockito.doReturn(false).when(updateDiskOfferingCmdMock).getDisplayOffering(); + + configurationManagerImplSpy.updateDiskOfferingIfCmdAttributeNotNull(diskOfferingVOSpy, updateDiskOfferingCmdMock); + + Assert.assertEquals(updateDiskOfferingCmdMock.getDiskOfferingName(), diskOfferingVOSpy.getName()); + Assert.assertEquals(updateDiskOfferingCmdMock.getDisplayText(), diskOfferingVOSpy.getDisplayText()); + Assert.assertEquals(updateDiskOfferingCmdMock.getSortKey(), (Integer) diskOfferingVOSpy.getSortKey()); + Assert.assertEquals(updateDiskOfferingCmdMock.getDisplayOffering(), diskOfferingVOSpy.getDisplayOffering()); + } + + @Test + public void updateDiskOfferingIfCmdAttributeNotNullTestNullValueDoesntUpdateOfferingAttribute() { + Mockito.doReturn("Name").when(diskOfferingVOSpy).getName(); + Mockito.doReturn("DisplayText").when(diskOfferingVOSpy).getDisplayText(); + Mockito.doReturn(1).when(diskOfferingVOSpy).getSortKey(); + Mockito.doReturn(true).when(diskOfferingVOSpy).getDisplayOffering(); + + configurationManagerImplSpy.updateDiskOfferingIfCmdAttributeNotNull(diskOfferingVOSpy, updateDiskOfferingCmdMock); + + Assert.assertNotEquals(updateDiskOfferingCmdMock.getDiskOfferingName(), diskOfferingVOSpy.getName()); + Assert.assertNotEquals(updateDiskOfferingCmdMock.getDisplayText(), diskOfferingVOSpy.getDisplayText()); + Assert.assertNotEquals(updateDiskOfferingCmdMock.getSortKey(), (Integer) diskOfferingVOSpy.getSortKey()); + Assert.assertNotEquals(updateDiskOfferingCmdMock.getDisplayOffering(), diskOfferingVOSpy.getDisplayOffering()); + } + + @Test + public void updateDiskOfferingDetailsDomainIdsTestDifferentDomainIdsDiskOfferingDetailsAddDomainIds() { + List<DiskOfferingDetailVO> detailsVO = new ArrayList<>(); + Long diskOfferingId = validId; + + configurationManagerImplSpy.updateDiskOfferingDetailsDomainIds(detailsVO, searchCriteriaDiskOfferingDetailMock, diskOfferingId, filteredDomainIds, existingDomainIds); + + for (int i = 0; i < detailsVO.size(); i++) { + Assert.assertEquals(filteredDomainIds.get(i), (Long) Long.parseLong(detailsVO.get(i).getValue())); + } + } + + @Test + public void checkDomainAdminUpdateOfferingRestrictionsTestDifferentZoneIdsThrowException() { + Assert.assertThrows(InvalidParameterValueException.class, + () -> configurationManagerImplSpy.checkDomainAdminUpdateOfferingRestrictions(diskOfferingMock, userMock, filteredZoneIds, emptyExistingZoneIds, existingDomainIds, filteredDomainIds)); + } + + @Test + public void checkDomainAdminUpdateOfferingRestrictionsTestEmptyExistingDomainIdsThrowException() { + Assert.assertThrows(InvalidParameterValueException.class, + () -> configurationManagerImplSpy.checkDomainAdminUpdateOfferingRestrictions(diskOfferingMock, userMock, filteredZoneIds, existingZoneIds, emptyExistingDomainIds, filteredDomainIds)); + } + + @Test + public void checkDomainAdminUpdateOfferingRestrictionsTestEmptyFilteredDomainIdsThrowException() { + Assert.assertThrows(InvalidParameterValueException.class, + () -> configurationManagerImplSpy.checkDomainAdminUpdateOfferingRestrictions(diskOfferingMock, userMock, filteredZoneIds, existingZoneIds, existingDomainIds, emptyFilteredDomainIds)); + } + + @Test + public void getAccountNonChildDomainsTestValidValuesReturnChildDomains() { + Mockito.doReturn(null).when(updateDiskOfferingCmdMock).getSortKey(); + List<Long> nonChildDomains = configurationManagerImplSpy.getAccountNonChildDomains(diskOfferingMock, accountMock, userMock, updateDiskOfferingCmdMock, existingDomainIds); + + for (int i = 0; i < existingDomainIds.size(); i++) { + Assert.assertEquals(existingDomainIds.get(i), nonChildDomains.get(i)); + } + } + + @Test + public void getAccountNonChildDomainsTestAllDomainsAreChildDomainsReturnEmptyList() { + for (Long existingDomainId : existingDomainIds) { + Mockito.when(domainDaoMock.isChildDomain(accountMock.getDomainId(), existingDomainId)).thenReturn(true); + } + + List<Long> nonChildDomains = configurationManagerImplSpy.getAccountNonChildDomains(diskOfferingMock, accountMock, userMock, updateDiskOfferingCmdMock, existingDomainIds); + + Assert.assertTrue(nonChildDomains.isEmpty()); + } + + @Test + public void getAccountNonChildDomainsTestNotNullCmdAttributeThrowException() { + Mockito.doReturn("name").when(updateDiskOfferingCmdMock).getDiskOfferingName(); + + Assert.assertThrows(InvalidParameterValueException.class, () -> configurationManagerImplSpy.getAccountNonChildDomains(diskOfferingMock, accountMock, userMock, updateDiskOfferingCmdMock, existingDomainIds)); + } + + @Test + public void checkIfDomainIsChildDomainTestNonChildDomainThrowException() { + Mockito.doReturn(false).when(domainDaoMock).isChildDomain(Mockito.anyLong(), Mockito.anyLong()); + Mockito.doReturn(domainMock).when(entityManagerMock).findById(Domain.class, 1L); + + Assert.assertThrows(InvalidParameterValueException.class, () -> configurationManagerImplSpy.checkIfDomainIsChildDomain(diskOfferingMock, accountMock, userMock, filteredDomainIds)); + } }