Skip to content

Commit 21bc89e

Browse files
Pearl1594Pearl1594
committed
Add tests and UI support and update response params
1 parent 7385d8c commit 21bc89e

File tree

8 files changed

+241
-16
lines changed

8 files changed

+241
-16
lines changed

‎api/src/main/java/org/apache/cloudstack/api/command/admin/backup/UpdateBackupOfferingCmd.java‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
import com.cloud.utils.exception.CloudRuntimeException;
3838

3939
import java.util.List;
40+
import java.util.function.LongFunction;
4041

4142
@APICommand(name = "updateBackupOffering", description = "Updates a backup offering.", responseObject = BackupOfferingResponse.class,
4243
requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, since = "4.16.0")
@@ -114,7 +115,15 @@ public void execute() {
114115
}
115116

116117
public List<Long> getDomainIds() {
117-
return resolveDomainIds(domainIds, id, backupManager::getBackupOfferingDomains, "backup offering");
118+
// backupManager may be null in unit tests where the command is spied without injection.
119+
// Avoid creating a method reference to a null receiver which causes NPE. When backupManager
120+
// is null, pass null as the defaultDomainsProvider so resolveDomainIds will simply return
121+
// an empty list or parse the explicit domainIds string.
122+
LongFunction<List<Long>> defaultDomainsProvider = null;
123+
if (backupManager != null) {
124+
defaultDomainsProvider = backupManager::getBackupOfferingDomains;
125+
}
126+
return resolveDomainIds(domainIds, id, defaultDomainsProvider, "backup offering");
118127
}
119128

120129
@Override

‎api/src/main/java/org/apache/cloudstack/api/response/BackupOfferingResponse.java‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,14 @@ public class BackupOfferingResponse extends BaseResponse {
6161
@Param(description = "zone name")
6262
private String zoneName;
6363

64+
@SerializedName(ApiConstants.DOMAIN_ID)
65+
@Param(description = "the domain ID(s) this disk offering belongs to. Ignore this information as it is not currently applicable.")
66+
private String domainId;
67+
68+
@SerializedName(ApiConstants.DOMAIN)
69+
@Param(description = "the domain name(s) this disk offering belongs to. Ignore this information as it is not currently applicable.")
70+
private String domain;
71+
6472
@SerializedName(ApiConstants.CROSS_ZONE_INSTANCE_CREATION)
6573
@Param(description = "the backups with this offering can be used to create Instances on all Zones", since = "4.22.0")
6674
private Boolean crossZoneInstanceCreation;
@@ -108,4 +116,13 @@ public void setCrossZoneInstanceCreation(Boolean crossZoneInstanceCreation) {
108116
public void setCreated(Date created) {
109117
this.created = created;
110118
}
119+
120+
public void setDomainId(String domainId) {
121+
this.domainId = domainId;
122+
}
123+
124+
public void setDomain(String domain) {
125+
this.domain = domain;
126+
}
127+
111128
}

‎engine/schema/src/main/java/org/apache/cloudstack/backup/dao/BackupOfferingDaoImpl.java‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@
2020
import javax.annotation.PostConstruct;
2121
import javax.inject.Inject;
2222

23+
import com.cloud.domain.DomainVO;
24+
import com.cloud.domain.dao.DomainDao;
2325
import org.apache.cloudstack.api.response.BackupOfferingResponse;
2426
import org.apache.cloudstack.backup.BackupOffering;
2527
import org.apache.cloudstack.backup.BackupOfferingVO;
@@ -30,10 +32,16 @@
3032
import com.cloud.utils.db.SearchBuilder;
3133
import com.cloud.utils.db.SearchCriteria;
3234

35+
import java.util.List;
36+
3337
public class BackupOfferingDaoImpl extends GenericDaoBase<BackupOfferingVO, Long> implements BackupOfferingDao {
3438

3539
@Inject
3640
DataCenterDao dataCenterDao;
41+
@Inject
42+
BackupOfferingDetailsDao backupOfferingDetailsDao;
43+
@Inject
44+
DomainDao domainDao;
3745

3846
private SearchBuilder<BackupOfferingVO> backupPoliciesSearch;
3947

@@ -51,8 +59,9 @@ protected void init() {
5159

5260
@Override
5361
public BackupOfferingResponse newBackupOfferingResponse(BackupOffering offering, Boolean crossZoneInstanceCreation) {
54-
DataCenterVO zone = dataCenterDao.findById(offering.getZoneId());
5562

63+
DataCenterVO zone = dataCenterDao.findById(offering.getZoneId());
64+
List<Long> domainIds = backupOfferingDetailsDao.findDomainIds(offering.getId());
5665
BackupOfferingResponse response = new BackupOfferingResponse();
5766
response.setId(offering.getUuid());
5867
response.setName(offering.getName());
@@ -64,6 +73,18 @@ public BackupOfferingResponse newBackupOfferingResponse(BackupOffering offering,
6473
response.setZoneId(zone.getUuid());
6574
response.setZoneName(zone.getName());
6675
}
76+
if (domainIds != null && !domainIds.isEmpty()) {
77+
String domainUUIDs = domainIds.stream().map(Long::valueOf).map(domainId -> {
78+
DomainVO domain = domainDao.findById(domainId);
79+
return domain != null ? domain.getUuid() : "";
80+
}).filter(name -> !name.isEmpty()).reduce((a, b) -> a + "," + b).orElse("");
81+
String domainNames = domainIds.stream().map(Long::valueOf).map(domainId -> {
82+
DomainVO domain = domainDao.findById(domainId);
83+
return domain != null ? domain.getName() : "";
84+
}).filter(name -> !name.isEmpty()).reduce((a, b) -> a + "," + b).orElse("");
85+
response.setDomain(domainNames);
86+
response.setDomainId(domainUUIDs);
87+
}
6788
if (crossZoneInstanceCreation) {
6889
response.setCrossZoneInstanceCreation(true);
6990
}

‎server/src/main/java/com/cloud/acl/DomainChecker.java‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -481,15 +481,12 @@ else if (_accountService.isNormalUser(account.getId())
481481
@Override
482482
public boolean checkAccess(Account account, BackupOffering backupOffering) throws PermissionDeniedException {
483483
boolean hasAccess = false;
484-
// Check for domains
485484
if (account == null || backupOffering == null) {
486485
hasAccess = true;
487486
} else {
488-
// admin has all permissions
489487
if (_accountService.isRootAdmin(account.getId())) {
490488
hasAccess = true;
491489
}
492-
// if account is normal user or domain admin or project
493490
else if (_accountService.isNormalUser(account.getId())
494491
|| account.getType() == Account.Type.RESOURCE_DOMAIN_ADMIN
495492
|| _accountService.isDomainAdmin(account.getId())

‎server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java‎

Lines changed: 45 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -291,7 +291,6 @@ public BackupOffering importBackupOffering(final ImportBackupOfferingCmd cmd) {
291291
}
292292
}
293293

294-
// enforce account permissions: domain-admins cannot create public offerings
295294
final Account caller = CallContext.current().getCallingAccount();
296295
List<Long> filteredDomainIds = cmd.getDomainIds() == null ? new ArrayList<>() : new ArrayList<>(cmd.getDomainIds());
297296
if (filteredDomainIds.size() > 1) {
@@ -310,11 +309,10 @@ public BackupOffering importBackupOffering(final ImportBackupOfferingCmd cmd) {
310309
if (savedOffering == null) {
311310
throw new CloudRuntimeException("Unable to create backup offering: " + cmd.getExternalId() + ", name: " + cmd.getName());
312311
}
313-
// Persist domain dedication details (if any)
314312
if (org.apache.commons.collections.CollectionUtils.isNotEmpty(filteredDomainIds)) {
315313
List<BackupOfferingDetailsVO> detailsVOList = new ArrayList<>();
316314
for (Long domainId : filteredDomainIds) {
317-
detailsVOList.add(new BackupOfferingDetailsVO(savedOffering.getId(), org.apache.cloudstack.api.ApiConstants.DOMAIN_ID, String.valueOf(domainId), false));
315+
detailsVOList.add(new BackupOfferingDetailsVO(savedOffering.getId(), ApiConstants.DOMAIN_ID, String.valueOf(domainId), false));
318316
}
319317
if (!detailsVOList.isEmpty()) {
320318
backupOfferingDetailsDao.saveDetails(detailsVOList);
@@ -400,7 +398,6 @@ public boolean deleteBackupOffering(final Long offeringId) {
400398
throw new CloudRuntimeException("Could not find a backup offering with id: " + offeringId);
401399
}
402400

403-
// Ensure caller has permission to delete this offering
404401
accountManager.checkAccess(CallContext.current().getCallingAccount(), offering);
405402

406403
if (backupDao.listByOfferingId(offering.getId()).size() > 0) {
@@ -2179,6 +2176,7 @@ public BackupOffering updateBackupOffering(UpdateBackupOfferingCmd updateBackupO
21792176
String name = updateBackupOfferingCmd.getName();
21802177
String description = updateBackupOfferingCmd.getDescription();
21812178
Boolean allowUserDrivenBackups = updateBackupOfferingCmd.getAllowUserDrivenBackups();
2179+
List<Long> domainIds = updateBackupOfferingCmd.getDomainIds();
21822180

21832181
BackupOfferingVO backupOfferingVO = backupOfferingDao.findById(id);
21842182
if (backupOfferingVO == null) {
@@ -2209,16 +2207,58 @@ public BackupOffering updateBackupOffering(UpdateBackupOfferingCmd updateBackupO
22092207
fields.add("allowUserDrivenBackups: " + allowUserDrivenBackups);
22102208
}
22112209

2212-
if (!backupOfferingDao.update(id, offering)) {
2210+
if (org.apache.commons.collections.CollectionUtils.isNotEmpty(domainIds)) {
2211+
for (final Long domainId: domainIds) {
2212+
if (domainDao.findById(domainId) == null) {
2213+
throw new InvalidParameterValueException("Please specify a valid domain id");
2214+
}
2215+
}
2216+
}
2217+
List<Long> filteredDomainIds = filterChildSubDomains(domainIds);
2218+
Collections.sort(filteredDomainIds);
2219+
2220+
boolean success =backupOfferingDao.update(id, offering);
2221+
if (!success) {
22132222
logger.warn(String.format("Couldn't update Backup offering (%s) with [%s].", backupOfferingVO, String.join(", ", fields)));
22142223
}
22152224

2225+
if (success) {
2226+
List<Long> existingDomainIds = backupOfferingDetailsDao.findDomainIds(id);
2227+
Collections.sort(existingDomainIds);
2228+
updateBackupOfferingDomainDetails(id, filteredDomainIds, existingDomainIds);
2229+
}
2230+
22162231
BackupOfferingVO response = backupOfferingDao.findById(id);
22172232
CallContext.current().setEventDetails(String.format("Backup Offering updated [%s].",
22182233
ReflectionToStringBuilderUtils.reflectOnlySelectedFields(response, "id", "name", "description", "userDrivenBackupAllowed", "externalId")));
22192234
return response;
22202235
}
22212236

2237+
private void updateBackupOfferingDomainDetails(Long id, List<Long> filteredDomainIds, List<Long> existingDomainIds) {
2238+
if (existingDomainIds == null) {
2239+
existingDomainIds = new ArrayList<>();
2240+
}
2241+
List<BackupOfferingDetailsVO> detailsVO = new ArrayList<>();
2242+
if(!filteredDomainIds.equals(existingDomainIds)) {
2243+
SearchBuilder<BackupOfferingDetailsVO> sb = backupOfferingDetailsDao.createSearchBuilder();
2244+
sb.and("offeringId", sb.entity().getResourceId(), SearchCriteria.Op.EQ);
2245+
sb.and("detailName", sb.entity().getName(), SearchCriteria.Op.EQ);
2246+
sb.done();
2247+
SearchCriteria<BackupOfferingDetailsVO> sc = sb.create();
2248+
sc.setParameters("offeringId", String.valueOf(id));
2249+
sc.setParameters("detailName", ApiConstants.DOMAIN_ID);
2250+
backupOfferingDetailsDao.remove(sc);
2251+
for (Long domainId : filteredDomainIds) {
2252+
detailsVO.add(new BackupOfferingDetailsVO(id, ApiConstants.DOMAIN_ID, String.valueOf(domainId), false));
2253+
}
2254+
}
2255+
if (!detailsVO.isEmpty()) {
2256+
for (BackupOfferingDetailsVO detailVO : detailsVO) {
2257+
backupOfferingDetailsDao.persist(detailVO);
2258+
}
2259+
}
2260+
}
2261+
22222262
Map<String, String> getDetailsFromBackupDetails(Long backupId) {
22232263
Map<String, String> details = backupDetailsDao.listDetailsKeyPairs(backupId, true);
22242264
if (details == null) {

‎server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java‎

Lines changed: 77 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@
8585
import org.apache.cloudstack.backup.dao.BackupDao;
8686
import org.apache.cloudstack.backup.dao.BackupDetailsDao;
8787
import org.apache.cloudstack.backup.dao.BackupOfferingDao;
88+
import org.apache.cloudstack.backup.dao.BackupOfferingDetailsDao;
8889
import org.apache.cloudstack.backup.dao.BackupScheduleDao;
8990
import org.apache.cloudstack.context.CallContext;
9091
import org.apache.cloudstack.framework.config.ConfigKey;
@@ -241,6 +242,9 @@ public class BackupManagerTest {
241242
@Mock
242243
private GuestOSDao _guestOSDao;
243244

245+
@Mock
246+
private BackupOfferingDetailsDao backupOfferingDetailsDao;
247+
244248
private Gson gson;
245249

246250
private String[] hostPossibleValues = {"127.0.0.1", "hostname"};
@@ -352,6 +356,7 @@ public void testUpdateBackupOfferingSuccess() {
352356
when(cmd.getName()).thenReturn("New name");
353357
when(cmd.getDescription()).thenReturn("New description");
354358
when(cmd.getAllowUserDrivenBackups()).thenReturn(true);
359+
when(backupOfferingDetailsDao.findDomainIds(id)).thenReturn(Collections.emptyList());
355360

356361
BackupOffering updated = backupManager.updateBackupOffering(cmd);
357362
assertEquals("New name", updated.getName());
@@ -1081,7 +1086,7 @@ public void testGetRootDiskInfoFromBackup() {
10811086

10821087
assertEquals("root-disk-offering-uuid", VmDiskInfo.getDiskOffering().getUuid());
10831088
assertEquals(Long.valueOf(5), VmDiskInfo.getSize());
1084-
// assertNull(com.cloud.vm.VmDiskInfo.getDeviceId());
1089+
assertNull(com.cloud.vm.VmDiskInfo.getDeviceId());
10851090
}
10861091

10871092
@Test
@@ -2156,4 +2161,75 @@ public void testRestoreBackupVolumeMismatch() {
21562161
verify(vmInstanceDao, times(1)).findByIdIncludingRemoved(vmId);
21572162
verify(volumeDao, times(1)).findByInstance(vmId);
21582163
}
2164+
2165+
@Test
2166+
public void getBackupOfferingDomainsTestOfferingNotFound() {
2167+
Long offeringId = 1L;
2168+
when(backupOfferingDao.findById(offeringId)).thenReturn(null);
2169+
2170+
InvalidParameterValueException exception = Assert.assertThrows(InvalidParameterValueException.class,
2171+
() -> backupManager.getBackupOfferingDomains(offeringId));
2172+
assertEquals("Unable to find backup offering null", exception.getMessage());
2173+
}
2174+
2175+
@Test
2176+
public void getBackupOfferingDomainsTestReturnsDomains() {
2177+
Long offeringId = 1L;
2178+
BackupOfferingVO offering = Mockito.mock(BackupOfferingVO.class);
2179+
when(backupOfferingDao.findById(offeringId)).thenReturn(offering);
2180+
when(backupOfferingDetailsDao.findDomainIds(offeringId)).thenReturn(List.of(10L, 20L));
2181+
2182+
List<Long> result = backupManager.getBackupOfferingDomains(offeringId);
2183+
2184+
assertEquals(2, result.size());
2185+
assertTrue(result.contains(10L));
2186+
assertTrue(result.contains(20L));
2187+
}
2188+
2189+
@Test
2190+
public void testUpdateBackupOfferingThrowsWhenDomainIdInvalid() {
2191+
Long id = 1234L;
2192+
UpdateBackupOfferingCmd cmd = Mockito.spy(UpdateBackupOfferingCmd.class);
2193+
when(cmd.getId()).thenReturn(id);
2194+
when(cmd.getDomainIds()).thenReturn(List.of(99L));
2195+
2196+
when(domainDao.findById(99L)).thenReturn(null);
2197+
2198+
InvalidParameterValueException exception = Assert.assertThrows(InvalidParameterValueException.class,
2199+
() -> backupManager.updateBackupOffering(cmd));
2200+
assertEquals("Please specify a valid domain id", exception.getMessage());
2201+
}
2202+
2203+
@Test
2204+
public void testUpdateBackupOfferingPersistsDomainDetailsWhenProvided() {
2205+
Long id = 1234L;
2206+
Long domainId = 11L;
2207+
UpdateBackupOfferingCmd cmd = Mockito.spy(UpdateBackupOfferingCmd.class);
2208+
when(cmd.getId()).thenReturn(id);
2209+
when(cmd.getDomainIds()).thenReturn(List.of(domainId));
2210+
2211+
DomainVO domain = Mockito.mock(DomainVO.class);
2212+
when(domainDao.findById(domainId)).thenReturn(domain);
2213+
2214+
when(backupOfferingDetailsDao.findDomainIds(id)).thenReturn(Collections.emptyList());
2215+
2216+
SearchBuilder<BackupOfferingDetailsVO> sb = Mockito.mock(SearchBuilder.class);
2217+
SearchCriteria<BackupOfferingDetailsVO> sc = Mockito.mock(SearchCriteria.class);
2218+
BackupOfferingDetailsVO entity = Mockito.mock(BackupOfferingDetailsVO.class);
2219+
when(backupOfferingDetailsDao.createSearchBuilder()).thenReturn(sb);
2220+
when(sb.entity()).thenReturn(entity);
2221+
when(sb.and(Mockito.anyString(), (Object) any(), Mockito.any())).thenReturn(sb);
2222+
when(sb.create()).thenReturn(sc);
2223+
2224+
BackupOfferingVO offering = Mockito.mock(BackupOfferingVO.class);
2225+
BackupOfferingVO offeringUpdate = Mockito.mock(BackupOfferingVO.class);
2226+
when(backupOfferingDao.findById(id)).thenReturn(offering);
2227+
when(backupOfferingDao.createForUpdate(id)).thenReturn(offeringUpdate);
2228+
when(backupOfferingDao.update(id, offeringUpdate)).thenReturn(true);
2229+
2230+
BackupOffering updated = backupManager.updateBackupOffering(cmd);
2231+
2232+
verify(backupOfferingDetailsDao, times(1)).persist(Mockito.any(BackupOfferingDetailsVO.class));
2233+
}
2234+
21592235
}

‎ui/src/config/section/offering.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -340,9 +340,9 @@ export default {
340340
icon: 'cloud-upload-outlined',
341341
docHelp: 'adminguide/virtual_machines.html#backup-offerings',
342342
permission: ['listBackupOfferings'],
343-
searchFilters: ['zoneid'],
344-
columns: ['name', 'description', 'zonename'],
345-
details: ['name', 'id', 'description', 'externalid', 'zone', 'allowuserdrivenbackups', 'created'],
343+
searchFilters: ['zoneid', 'domainid'],
344+
columns: ['name', 'description', 'domain', 'zonename'],
345+
details: ['name', 'id', 'description', 'externalid', 'domain', 'zone', 'allowuserdrivenbackups', 'created'],
346346
related: [{
347347
name: 'vm',
348348
title: 'label.instances',

0 commit comments

Comments
 (0)