From 21bc89e07270fbcd7701976e74e1da2df236878c Mon Sep 17 00:00:00 2001 From: Pearl Dsilva Date: Thu, 4 Dec 2025 10:38:31 -0500 Subject: [PATCH] Add tests and UI support and update response params --- .../admin/backup/UpdateBackupOfferingCmd.java | 11 ++- .../api/response/BackupOfferingResponse.java | 17 ++++ .../backup/dao/BackupOfferingDaoImpl.java | 23 +++++- .../java/com/cloud/acl/DomainChecker.java | 3 - .../cloudstack/backup/BackupManagerImpl.java | 50 ++++++++++-- .../cloudstack/backup/BackupManagerTest.java | 78 ++++++++++++++++++- ui/src/config/section/offering.js | 6 +- .../views/offering/ImportBackupOffering.vue | 69 +++++++++++++++- 8 files changed, 241 insertions(+), 16 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/backup/UpdateBackupOfferingCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/backup/UpdateBackupOfferingCmd.java index 682f2f595ad..2b8643819d4 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/backup/UpdateBackupOfferingCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/backup/UpdateBackupOfferingCmd.java @@ -37,6 +37,7 @@ import com.cloud.user.Account; import com.cloud.utils.exception.CloudRuntimeException; import java.util.List; +import java.util.function.LongFunction; @APICommand(name = "updateBackupOffering", description = "Updates a backup offering.", responseObject = BackupOfferingResponse.class, requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, since = "4.16.0") @@ -114,7 +115,15 @@ public class UpdateBackupOfferingCmd extends BaseCmd implements DomainAndZoneIdR } public List getDomainIds() { - return resolveDomainIds(domainIds, id, backupManager::getBackupOfferingDomains, "backup offering"); + // backupManager may be null in unit tests where the command is spied without injection. + // Avoid creating a method reference to a null receiver which causes NPE. When backupManager + // is null, pass null as the defaultDomainsProvider so resolveDomainIds will simply return + // an empty list or parse the explicit domainIds string. + LongFunction> defaultDomainsProvider = null; + if (backupManager != null) { + defaultDomainsProvider = backupManager::getBackupOfferingDomains; + } + return resolveDomainIds(domainIds, id, defaultDomainsProvider, "backup offering"); } @Override diff --git a/api/src/main/java/org/apache/cloudstack/api/response/BackupOfferingResponse.java b/api/src/main/java/org/apache/cloudstack/api/response/BackupOfferingResponse.java index 4120f68d9da..0669afa604b 100644 --- a/api/src/main/java/org/apache/cloudstack/api/response/BackupOfferingResponse.java +++ b/api/src/main/java/org/apache/cloudstack/api/response/BackupOfferingResponse.java @@ -61,6 +61,14 @@ public class BackupOfferingResponse extends BaseResponse { @Param(description = "zone name") private String zoneName; + @SerializedName(ApiConstants.DOMAIN_ID) + @Param(description = "the domain ID(s) this disk offering belongs to. Ignore this information as it is not currently applicable.") + private String domainId; + + @SerializedName(ApiConstants.DOMAIN) + @Param(description = "the domain name(s) this disk offering belongs to. Ignore this information as it is not currently applicable.") + private String domain; + @SerializedName(ApiConstants.CROSS_ZONE_INSTANCE_CREATION) @Param(description = "the backups with this offering can be used to create Instances on all Zones", since = "4.22.0") private Boolean crossZoneInstanceCreation; @@ -108,4 +116,13 @@ public class BackupOfferingResponse extends BaseResponse { public void setCreated(Date created) { this.created = created; } + + public void setDomainId(String domainId) { + this.domainId = domainId; + } + + public void setDomain(String domain) { + this.domain = domain; + } + } diff --git a/engine/schema/src/main/java/org/apache/cloudstack/backup/dao/BackupOfferingDaoImpl.java b/engine/schema/src/main/java/org/apache/cloudstack/backup/dao/BackupOfferingDaoImpl.java index a41e4e70d33..708faeef464 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/backup/dao/BackupOfferingDaoImpl.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/backup/dao/BackupOfferingDaoImpl.java @@ -20,6 +20,8 @@ package org.apache.cloudstack.backup.dao; import javax.annotation.PostConstruct; import javax.inject.Inject; +import com.cloud.domain.DomainVO; +import com.cloud.domain.dao.DomainDao; import org.apache.cloudstack.api.response.BackupOfferingResponse; import org.apache.cloudstack.backup.BackupOffering; import org.apache.cloudstack.backup.BackupOfferingVO; @@ -30,10 +32,16 @@ import com.cloud.utils.db.GenericDaoBase; import com.cloud.utils.db.SearchBuilder; import com.cloud.utils.db.SearchCriteria; +import java.util.List; + public class BackupOfferingDaoImpl extends GenericDaoBase implements BackupOfferingDao { @Inject DataCenterDao dataCenterDao; + @Inject + BackupOfferingDetailsDao backupOfferingDetailsDao; + @Inject + DomainDao domainDao; private SearchBuilder backupPoliciesSearch; @@ -51,8 +59,9 @@ public class BackupOfferingDaoImpl extends GenericDaoBase domainIds = backupOfferingDetailsDao.findDomainIds(offering.getId()); BackupOfferingResponse response = new BackupOfferingResponse(); response.setId(offering.getUuid()); response.setName(offering.getName()); @@ -64,6 +73,18 @@ public class BackupOfferingDaoImpl extends GenericDaoBase { + DomainVO domain = domainDao.findById(domainId); + return domain != null ? domain.getUuid() : ""; + }).filter(name -> !name.isEmpty()).reduce((a, b) -> a + "," + b).orElse(""); + String domainNames = domainIds.stream().map(Long::valueOf).map(domainId -> { + DomainVO domain = domainDao.findById(domainId); + return domain != null ? domain.getName() : ""; + }).filter(name -> !name.isEmpty()).reduce((a, b) -> a + "," + b).orElse(""); + response.setDomain(domainNames); + response.setDomainId(domainUUIDs); + } if (crossZoneInstanceCreation) { response.setCrossZoneInstanceCreation(true); } diff --git a/server/src/main/java/com/cloud/acl/DomainChecker.java b/server/src/main/java/com/cloud/acl/DomainChecker.java index b436d0bdcf3..99a1a4d5a20 100644 --- a/server/src/main/java/com/cloud/acl/DomainChecker.java +++ b/server/src/main/java/com/cloud/acl/DomainChecker.java @@ -481,15 +481,12 @@ public class DomainChecker extends AdapterBase implements SecurityChecker { @Override public boolean checkAccess(Account account, BackupOffering backupOffering) throws PermissionDeniedException { boolean hasAccess = false; - // Check for domains if (account == null || backupOffering == null) { hasAccess = true; } else { - // admin has all permissions if (_accountService.isRootAdmin(account.getId())) { hasAccess = true; } - // if account is normal user or domain admin or project else if (_accountService.isNormalUser(account.getId()) || account.getType() == Account.Type.RESOURCE_DOMAIN_ADMIN || _accountService.isDomainAdmin(account.getId()) diff --git a/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java b/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java index e1c51813ad6..bc40719c7ec 100644 --- a/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java @@ -291,7 +291,6 @@ public class BackupManagerImpl extends ManagerBase implements BackupManager { } } - // enforce account permissions: domain-admins cannot create public offerings final Account caller = CallContext.current().getCallingAccount(); List filteredDomainIds = cmd.getDomainIds() == null ? new ArrayList<>() : new ArrayList<>(cmd.getDomainIds()); if (filteredDomainIds.size() > 1) { @@ -310,11 +309,10 @@ public class BackupManagerImpl extends ManagerBase implements BackupManager { if (savedOffering == null) { throw new CloudRuntimeException("Unable to create backup offering: " + cmd.getExternalId() + ", name: " + cmd.getName()); } - // Persist domain dedication details (if any) if (org.apache.commons.collections.CollectionUtils.isNotEmpty(filteredDomainIds)) { List detailsVOList = new ArrayList<>(); for (Long domainId : filteredDomainIds) { - detailsVOList.add(new BackupOfferingDetailsVO(savedOffering.getId(), org.apache.cloudstack.api.ApiConstants.DOMAIN_ID, String.valueOf(domainId), false)); + detailsVOList.add(new BackupOfferingDetailsVO(savedOffering.getId(), ApiConstants.DOMAIN_ID, String.valueOf(domainId), false)); } if (!detailsVOList.isEmpty()) { backupOfferingDetailsDao.saveDetails(detailsVOList); @@ -400,7 +398,6 @@ public class BackupManagerImpl extends ManagerBase implements BackupManager { throw new CloudRuntimeException("Could not find a backup offering with id: " + offeringId); } - // Ensure caller has permission to delete this offering accountManager.checkAccess(CallContext.current().getCallingAccount(), offering); if (backupDao.listByOfferingId(offering.getId()).size() > 0) { @@ -2179,6 +2176,7 @@ public class BackupManagerImpl extends ManagerBase implements BackupManager { String name = updateBackupOfferingCmd.getName(); String description = updateBackupOfferingCmd.getDescription(); Boolean allowUserDrivenBackups = updateBackupOfferingCmd.getAllowUserDrivenBackups(); + List domainIds = updateBackupOfferingCmd.getDomainIds(); BackupOfferingVO backupOfferingVO = backupOfferingDao.findById(id); if (backupOfferingVO == null) { @@ -2209,16 +2207,58 @@ public class BackupManagerImpl extends ManagerBase implements BackupManager { fields.add("allowUserDrivenBackups: " + allowUserDrivenBackups); } - if (!backupOfferingDao.update(id, offering)) { + if (org.apache.commons.collections.CollectionUtils.isNotEmpty(domainIds)) { + for (final Long domainId: domainIds) { + if (domainDao.findById(domainId) == null) { + throw new InvalidParameterValueException("Please specify a valid domain id"); + } + } + } + List filteredDomainIds = filterChildSubDomains(domainIds); + Collections.sort(filteredDomainIds); + + boolean success =backupOfferingDao.update(id, offering); + if (!success) { logger.warn(String.format("Couldn't update Backup offering (%s) with [%s].", backupOfferingVO, String.join(", ", fields))); } + if (success) { + List existingDomainIds = backupOfferingDetailsDao.findDomainIds(id); + Collections.sort(existingDomainIds); + updateBackupOfferingDomainDetails(id, filteredDomainIds, existingDomainIds); + } + BackupOfferingVO response = backupOfferingDao.findById(id); CallContext.current().setEventDetails(String.format("Backup Offering updated [%s].", ReflectionToStringBuilderUtils.reflectOnlySelectedFields(response, "id", "name", "description", "userDrivenBackupAllowed", "externalId"))); return response; } + private void updateBackupOfferingDomainDetails(Long id, List filteredDomainIds, List existingDomainIds) { + if (existingDomainIds == null) { + existingDomainIds = new ArrayList<>(); + } + List detailsVO = new ArrayList<>(); + if(!filteredDomainIds.equals(existingDomainIds)) { + SearchBuilder sb = backupOfferingDetailsDao.createSearchBuilder(); + sb.and("offeringId", sb.entity().getResourceId(), SearchCriteria.Op.EQ); + sb.and("detailName", sb.entity().getName(), SearchCriteria.Op.EQ); + sb.done(); + SearchCriteria sc = sb.create(); + sc.setParameters("offeringId", String.valueOf(id)); + sc.setParameters("detailName", ApiConstants.DOMAIN_ID); + backupOfferingDetailsDao.remove(sc); + for (Long domainId : filteredDomainIds) { + detailsVO.add(new BackupOfferingDetailsVO(id, ApiConstants.DOMAIN_ID, String.valueOf(domainId), false)); + } + } + if (!detailsVO.isEmpty()) { + for (BackupOfferingDetailsVO detailVO : detailsVO) { + backupOfferingDetailsDao.persist(detailVO); + } + } + } + Map getDetailsFromBackupDetails(Long backupId) { Map details = backupDetailsDao.listDetailsKeyPairs(backupId, true); if (details == null) { diff --git a/server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java b/server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java index 6f6879b6fe3..47c3c0d5d4c 100644 --- a/server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java +++ b/server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java @@ -85,6 +85,7 @@ import org.apache.cloudstack.api.response.BackupResponse; import org.apache.cloudstack.backup.dao.BackupDao; import org.apache.cloudstack.backup.dao.BackupDetailsDao; import org.apache.cloudstack.backup.dao.BackupOfferingDao; +import org.apache.cloudstack.backup.dao.BackupOfferingDetailsDao; import org.apache.cloudstack.backup.dao.BackupScheduleDao; import org.apache.cloudstack.context.CallContext; import org.apache.cloudstack.framework.config.ConfigKey; @@ -241,6 +242,9 @@ public class BackupManagerTest { @Mock private GuestOSDao _guestOSDao; + @Mock + private BackupOfferingDetailsDao backupOfferingDetailsDao; + private Gson gson; private String[] hostPossibleValues = {"127.0.0.1", "hostname"}; @@ -352,6 +356,7 @@ public class BackupManagerTest { when(cmd.getName()).thenReturn("New name"); when(cmd.getDescription()).thenReturn("New description"); when(cmd.getAllowUserDrivenBackups()).thenReturn(true); + when(backupOfferingDetailsDao.findDomainIds(id)).thenReturn(Collections.emptyList()); BackupOffering updated = backupManager.updateBackupOffering(cmd); assertEquals("New name", updated.getName()); @@ -1081,7 +1086,7 @@ public class BackupManagerTest { assertEquals("root-disk-offering-uuid", VmDiskInfo.getDiskOffering().getUuid()); assertEquals(Long.valueOf(5), VmDiskInfo.getSize()); -// assertNull(com.cloud.vm.VmDiskInfo.getDeviceId()); + assertNull(com.cloud.vm.VmDiskInfo.getDeviceId()); } @Test @@ -2156,4 +2161,75 @@ public class BackupManagerTest { verify(vmInstanceDao, times(1)).findByIdIncludingRemoved(vmId); verify(volumeDao, times(1)).findByInstance(vmId); } + + @Test + public void getBackupOfferingDomainsTestOfferingNotFound() { + Long offeringId = 1L; + when(backupOfferingDao.findById(offeringId)).thenReturn(null); + + InvalidParameterValueException exception = Assert.assertThrows(InvalidParameterValueException.class, + () -> backupManager.getBackupOfferingDomains(offeringId)); + assertEquals("Unable to find backup offering null", exception.getMessage()); + } + + @Test + public void getBackupOfferingDomainsTestReturnsDomains() { + Long offeringId = 1L; + BackupOfferingVO offering = Mockito.mock(BackupOfferingVO.class); + when(backupOfferingDao.findById(offeringId)).thenReturn(offering); + when(backupOfferingDetailsDao.findDomainIds(offeringId)).thenReturn(List.of(10L, 20L)); + + List result = backupManager.getBackupOfferingDomains(offeringId); + + assertEquals(2, result.size()); + assertTrue(result.contains(10L)); + assertTrue(result.contains(20L)); + } + + @Test + public void testUpdateBackupOfferingThrowsWhenDomainIdInvalid() { + Long id = 1234L; + UpdateBackupOfferingCmd cmd = Mockito.spy(UpdateBackupOfferingCmd.class); + when(cmd.getId()).thenReturn(id); + when(cmd.getDomainIds()).thenReturn(List.of(99L)); + + when(domainDao.findById(99L)).thenReturn(null); + + InvalidParameterValueException exception = Assert.assertThrows(InvalidParameterValueException.class, + () -> backupManager.updateBackupOffering(cmd)); + assertEquals("Please specify a valid domain id", exception.getMessage()); + } + + @Test + public void testUpdateBackupOfferingPersistsDomainDetailsWhenProvided() { + Long id = 1234L; + Long domainId = 11L; + UpdateBackupOfferingCmd cmd = Mockito.spy(UpdateBackupOfferingCmd.class); + when(cmd.getId()).thenReturn(id); + when(cmd.getDomainIds()).thenReturn(List.of(domainId)); + + DomainVO domain = Mockito.mock(DomainVO.class); + when(domainDao.findById(domainId)).thenReturn(domain); + + when(backupOfferingDetailsDao.findDomainIds(id)).thenReturn(Collections.emptyList()); + + SearchBuilder sb = Mockito.mock(SearchBuilder.class); + SearchCriteria sc = Mockito.mock(SearchCriteria.class); + BackupOfferingDetailsVO entity = Mockito.mock(BackupOfferingDetailsVO.class); + when(backupOfferingDetailsDao.createSearchBuilder()).thenReturn(sb); + when(sb.entity()).thenReturn(entity); + when(sb.and(Mockito.anyString(), (Object) any(), Mockito.any())).thenReturn(sb); + when(sb.create()).thenReturn(sc); + + BackupOfferingVO offering = Mockito.mock(BackupOfferingVO.class); + BackupOfferingVO offeringUpdate = Mockito.mock(BackupOfferingVO.class); + when(backupOfferingDao.findById(id)).thenReturn(offering); + when(backupOfferingDao.createForUpdate(id)).thenReturn(offeringUpdate); + when(backupOfferingDao.update(id, offeringUpdate)).thenReturn(true); + + BackupOffering updated = backupManager.updateBackupOffering(cmd); + + verify(backupOfferingDetailsDao, times(1)).persist(Mockito.any(BackupOfferingDetailsVO.class)); + } + } diff --git a/ui/src/config/section/offering.js b/ui/src/config/section/offering.js index 4a32619b8c2..bc95772d6f7 100644 --- a/ui/src/config/section/offering.js +++ b/ui/src/config/section/offering.js @@ -340,9 +340,9 @@ export default { icon: 'cloud-upload-outlined', docHelp: 'adminguide/virtual_machines.html#backup-offerings', permission: ['listBackupOfferings'], - searchFilters: ['zoneid'], - columns: ['name', 'description', 'zonename'], - details: ['name', 'id', 'description', 'externalid', 'zone', 'allowuserdrivenbackups', 'created'], + searchFilters: ['zoneid', 'domainid'], + columns: ['name', 'description', 'domain', 'zonename'], + details: ['name', 'id', 'description', 'externalid', 'domain', 'zone', 'allowuserdrivenbackups', 'created'], related: [{ name: 'vm', title: 'label.instances', diff --git a/ui/src/views/offering/ImportBackupOffering.vue b/ui/src/views/offering/ImportBackupOffering.vue index b8ac7d8e8e6..f680eacd4a7 100644 --- a/ui/src/views/offering/ImportBackupOffering.vue +++ b/ui/src/views/offering/ImportBackupOffering.vue @@ -85,6 +85,33 @@ + + + + + + + + + + + {{ opt.path || opt.name || opt.description }} + + + +
{{ this.$t('label.cancel') }} {{ this.$t('label.ok') }} @@ -96,6 +123,7 @@