addressed review comments

Author: abh1sar

api/src/main/java/com/cloud/event/EventTypes.java

@@ -610,7 +610,7 @@ public class EventTypes {
     public static final String EVENT_VM_BACKUP_CREATE = "BACKUP.CREATE";
     public static final String EVENT_VM_BACKUP_RESTORE = "BACKUP.RESTORE";
     public static final String EVENT_VM_BACKUP_DELETE = "BACKUP.DELETE";
-    public static final String EVENT_VM_BACKUP_OFFERING_BACKUPS_DELETED = "BACKUP.OFFERING.BACKUPS.DEL";
+    public static final String EVENT_VM_BACKUP_OFFERING_REMOVED_AND_BACKUPS_DELETED = "BACKUP.OFFERING.BACKUPS.DEL";
     public static final String EVENT_VM_BACKUP_RESTORE_VOLUME_TO_VM = "BACKUP.RESTORE.VOLUME.TO.VM";
     public static final String EVENT_VM_BACKUP_SCHEDULE_CONFIGURE = "BACKUP.SCHEDULE.CONFIGURE";
     public static final String EVENT_VM_BACKUP_SCHEDULE_DELETE = "BACKUP.SCHEDULE.DELETE";

api/src/main/java/org/apache/cloudstack/api/ResponseGenerator.java

@@ -534,8 +534,6 @@ List<TemplateResponse> createTemplateResponses(ResponseView view, VirtualMachine
 
     UserDataResponse createUserDataResponse(UserData userData);
 
-    BackupResponse createBackupResponse(Backup backup, Boolean listVmDetails);
-
     BackupScheduleResponse createBackupScheduleResponse(BackupSchedule backup);
 
     BackupOfferingResponse createBackupOfferingResponse(BackupOffering policy);

api/src/main/java/org/apache/cloudstack/api/command/user/backup/ListBackupsCmd.java

@@ -133,7 +133,7 @@ protected void setupResponseBackupList(final List<Backup> backups, final Integer
             if (backup == null) {
                 continue;
             }
-            BackupResponse backupResponse = _responseGenerator.createBackupResponse(backup, this.getListVmDetails());
+            BackupResponse backupResponse = backupManager.createBackupResponse(backup, this.getListVmDetails());
             responses.add(backupResponse);
         }
         final ListResponse<BackupResponse> response = new ListResponse<>();

api/src/main/java/org/apache/cloudstack/backup/BackupManager.java

@@ -29,6 +29,7 @@
 import org.apache.cloudstack.api.command.user.backup.DeleteBackupScheduleCmd;
 import org.apache.cloudstack.api.command.user.backup.ListBackupOfferingsCmd;
 import org.apache.cloudstack.api.command.user.backup.ListBackupsCmd;
+import org.apache.cloudstack.api.response.BackupResponse;
 import org.apache.cloudstack.framework.config.ConfigKey;
 import org.apache.cloudstack.framework.config.Configurable;
 
@@ -270,6 +271,8 @@ public interface BackupManager extends BackupService, Configurable, PluggableSer
 
     String getBackupNameFromVM(VirtualMachine vm);
 
+    BackupResponse createBackupResponse(Backup backup, Boolean listVmDetails);
+
     Capacity getBackupStorageUsedStats(Long zoneId);
 
     void checkAndRemoveBackupOfferingBeforeExpunge(VirtualMachine vm);

engine/schema/src/main/java/org/apache/cloudstack/backup/BackupVO.java

@@ -282,10 +282,6 @@ public void setDetails(Map<String, String> details) {
         this.details = details;
     }
 
-    public void addDetails(Map<String, String> details) {
-        this.details.putAll(details);
-    }
-
     public Date getRemoved() {
         return removed;
     }

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

@@ -19,7 +19,6 @@
 
 import java.util.List;
 
-import org.apache.cloudstack.api.response.BackupResponse;
 import org.apache.cloudstack.backup.Backup;
 import org.apache.cloudstack.backup.BackupVO;
 
@@ -39,7 +38,6 @@ public interface BackupDao extends GenericDao<BackupVO, Long> {
     List<Backup> listByOfferingId(Long backupOfferingId);
     List<BackupVO> listBackupsByVMandIntervalType(Long vmId, Backup.Type backupType);
     List<Long> listVmIdsWithBackupsInZone(Long zoneId);
-    BackupResponse newBackupResponse(Backup backup, Boolean listVmDetails);
     public Long countBackupsForAccount(long accountId);
     public Long calculateBackupStorageForAccount(long accountId);
     void loadDetails(BackupVO backup);

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

@@ -17,48 +17,32 @@
 
 package org.apache.cloudstack.backup.dao;
 
-import java.lang.reflect.Type;
 import java.util.ArrayList;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
 
 import javax.annotation.PostConstruct;
 import javax.inject.Inject;
 
-import com.cloud.offering.ServiceOffering;
 import com.cloud.service.dao.ServiceOfferingDao;
-import com.cloud.storage.Storage;
 import com.cloud.storage.dao.VMTemplateDao;
-import com.cloud.template.VirtualMachineTemplate;
 import com.cloud.utils.db.GenericSearchBuilder;
 
-import org.apache.cloudstack.api.ApiConstants;
-import org.apache.cloudstack.api.response.BackupResponse;
 import org.apache.cloudstack.backup.Backup;
 import org.apache.cloudstack.backup.BackupDetailVO;
-import org.apache.cloudstack.backup.BackupOffering;
 import org.apache.cloudstack.backup.BackupVO;
 import org.apache.commons.collections.CollectionUtils;
 
-import com.cloud.dc.DataCenterVO;
 import com.cloud.dc.dao.DataCenterDao;
-import com.cloud.domain.DomainVO;
 import com.cloud.domain.dao.DomainDao;
-import com.cloud.user.AccountVO;
 import com.cloud.user.dao.AccountDao;
 import com.cloud.utils.db.GenericDaoBase;
 import com.cloud.utils.db.SearchBuilder;
 import com.cloud.utils.db.SearchCriteria;
 import com.cloud.utils.db.Transaction;
 import com.cloud.utils.db.TransactionCallback;
-import com.cloud.vm.VMInstanceVO;
 import com.cloud.vm.dao.VMInstanceDao;
-import com.cloud.network.Network;
 import com.cloud.network.dao.NetworkDao;
-import com.google.gson.Gson;
-import com.google.gson.reflect.TypeToken;
 
 public class BackupDaoImpl extends GenericDaoBase<BackupVO, Long> implements BackupDao {
 
@@ -297,98 +281,4 @@ public List<Long> listVmIdsWithBackupsInZone(Long zoneId) {
         sc.setParameters("zone_id", zoneId);
         return customSearchIncludingRemoved(sc, null);
     }
-
-    Map<String, String> getDetailsFromBackupDetails(Long backupId) {
-        Map<String, String> details = backupDetailsDao.listDetailsKeyPairs(backupId, true);
-        if (details == null) {
-            return null;
-        }
-        if (details.containsKey(ApiConstants.TEMPLATE_ID)) {
-            VirtualMachineTemplate template = templateDao.findByUuid(details.get(ApiConstants.TEMPLATE_ID));
-            if (template != null) {
-                details.put(ApiConstants.TEMPLATE_ID, template.getUuid());
-                details.put(ApiConstants.TEMPLATE_NAME, template.getName());
-                details.put(ApiConstants.IS_ISO, String.valueOf(template.getFormat().equals(Storage.ImageFormat.ISO)));
-            }
-        }
-        if (details.containsKey(ApiConstants.SERVICE_OFFERING_ID)) {
-            ServiceOffering serviceOffering = serviceOfferingDao.findByUuid(details.get(ApiConstants.SERVICE_OFFERING_ID));
-            if (serviceOffering != null) {
-                details.put(ApiConstants.SERVICE_OFFERING_ID, serviceOffering.getUuid());
-                details.put(ApiConstants.SERVICE_OFFERING_NAME, serviceOffering.getName());
-            }
-        }
-        if (details.containsKey(ApiConstants.NICS)) {
-            Type type = new TypeToken<List<Map<String, String>>>() {}.getType();
-            List<Map<String, String>> nics = new Gson().fromJson(details.get(ApiConstants.NICS), type);
-
-            for (Map<String, String> nic : nics) {
-                String networkUuid = nic.get(ApiConstants.NETWORK_ID);
-                if (networkUuid != null) {
-                    Network network = networkDao.findByUuid(networkUuid);
-                    if (network != null) {
-                        nic.put(ApiConstants.NETWORK_NAME, network.getName());
-                    }
-                }
-            }
-            details.put(ApiConstants.NICS, new Gson().toJson(nics));
-        }
-        return details;
-    }
-
-    @Override
-    public BackupResponse newBackupResponse(Backup backup, Boolean listVmDetails) {
-        VMInstanceVO vm = vmInstanceDao.findByIdIncludingRemoved(backup.getVmId());
-        AccountVO account = accountDao.findByIdIncludingRemoved(backup.getAccountId());
-        DomainVO domain = domainDao.findByIdIncludingRemoved(backup.getDomainId());
-        DataCenterVO zone = dataCenterDao.findByIdIncludingRemoved(backup.getZoneId());
-        Long offeringId = backup.getBackupOfferingId();
-        BackupOffering offering = backupOfferingDao.findByIdIncludingRemoved(offeringId);
-
-        BackupResponse response = new BackupResponse();
-        response.setId(backup.getUuid());
-        response.setName(backup.getName());
-        response.setDescription(backup.getDescription());
-        response.setVmName(vm.getHostName());
-        response.setVmId(vm.getUuid());
-        if (vm.getBackupOfferingId() == null || vm.getBackupOfferingId() != backup.getBackupOfferingId()) {
-            response.setVmOfferingRemoved(true);
-        }
-        response.setExternalId(backup.getExternalId());
-        response.setType(backup.getType());
-        response.setDate(backup.getDate());
-        response.setSize(backup.getSize());
-        response.setProtectedSize(backup.getProtectedSize());
-        response.setStatus(backup.getStatus());
-        if (backup.getBackupIntervalType() != null) {
-            response.setIntervalType(Backup.Type.values()[backup.getBackupIntervalType()].toString());
-        }
-        // ACS 4.20: For backups taken prior this release the backup.backed_volumes column would be empty hence use vm_instance.backup_volumes
-        String backedUpVolumes;
-        if (Objects.isNull(backup.getBackedUpVolumes())) {
-            backedUpVolumes = new Gson().toJson(vm.getBackupVolumeList().toArray(), Backup.VolumeInfo[].class);
-        } else {
-            backedUpVolumes = new Gson().toJson(backup.getBackedUpVolumes().toArray(), Backup.VolumeInfo[].class);
-        }
-        response.setVolumes(backedUpVolumes);
-        response.setBackupOfferingId(offering.getUuid());
-        response.setBackupOffering(offering.getName());
-        response.setAccountId(account.getUuid());
-        response.setAccount(account.getAccountName());
-        response.setDomainId(domain.getUuid());
-        response.setDomain(domain.getName());
-        response.setZoneId(zone.getUuid());
-        response.setZone(zone.getName());
-
-        if (Boolean.TRUE.equals(listVmDetails)) {
-            Map<String, String> vmDetails = new HashMap<>();
-            vmDetails.put(ApiConstants.HYPERVISOR, vm.getHypervisorType().toString());
-            Map<String, String> details = getDetailsFromBackupDetails(backup.getId());
-            vmDetails.putAll(details);
-            response.setVmDetails(vmDetails);
-        }
-
-        response.setObjectName("backup");
-        return response;
-    }
 }

engine/schema/src/test/java/org/apache/cloudstack/backup/dao/BackupDaoImplTest.java

@@ -16,16 +16,9 @@
 // under the License.
 package org.apache.cloudstack.backup.dao;
 
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.when;
-
 import java.util.HashMap;
 import java.util.Map;
 
-import org.apache.cloudstack.api.ApiConstants;
-import org.apache.cloudstack.api.response.BackupResponse;
-import org.apache.cloudstack.backup.Backup;
-import org.apache.cloudstack.backup.BackupOfferingVO;
 import org.apache.cloudstack.backup.BackupVO;
 import org.junit.Assert;
 import org.junit.Test;
@@ -37,26 +30,6 @@
 import org.mockito.junit.MockitoJUnitRunner;
 import org.springframework.test.util.ReflectionTestUtils;
 
-import com.cloud.dc.DataCenter;
-import com.cloud.dc.DataCenterVO;
-import com.cloud.dc.dao.DataCenterDao;
-import com.cloud.domain.DomainVO;
-import com.cloud.domain.dao.DomainDao;
-import com.cloud.hypervisor.Hypervisor;
-import com.cloud.network.dao.NetworkDao;
-import com.cloud.network.dao.NetworkVO;
-import com.cloud.service.ServiceOfferingVO;
-import com.cloud.service.dao.ServiceOfferingDao;
-import com.cloud.storage.Storage;
-import com.cloud.storage.VMTemplateVO;
-import com.cloud.storage.dao.VMTemplateDao;
-import com.cloud.user.AccountVO;
-import com.cloud.user.dao.AccountDao;
-import com.cloud.utils.db.SearchBuilder;
-import com.cloud.vm.VMInstanceVO;
-import com.cloud.vm.VirtualMachine;
-import com.cloud.vm.dao.VMInstanceDao;
-
 @RunWith(MockitoJUnitRunner.class)
 public class BackupDaoImplTest {
     @Spy
@@ -66,33 +39,6 @@ public class BackupDaoImplTest {
     @Mock
     BackupDetailsDao backupDetailsDao;
 
-    @Mock
-    SearchBuilder<BackupVO> backupSearch;
-
-    @Mock
-    VMInstanceDao vmInstanceDao;
-
-    @Mock
-    AccountDao accountDao;
-
-    @Mock
-    DomainDao domainDao;
-
-    @Mock
-    DataCenterDao dataCenterDao;
-
-    @Mock
-    BackupOfferingDao backupOfferingDao;
-
-    @Mock
-    private VMTemplateDao templateDao;
-
-    @Mock
-    ServiceOfferingDao serviceOfferingDao;
-
-    @Mock
-    NetworkDao networkDao;
-
     @Test
     public void testLoadDetails() {
         Long backupId = 1L;
@@ -124,95 +70,4 @@ public void testSaveDetails() {
 
         Mockito.verify(backupDetailsDao).saveDetails(Mockito.anyList());
     }
-
-    @Test
-    public void testNewBackupResponse() {
-        Long vmId = 1L;
-        Long accountId = 2L;
-        Long domainId = 3L;
-        Long zoneId = 4L;
-        Long vmOfferingId = 5L;
-        Long backupOfferingId = 6L;
-        Long backupId = 7L;
-        Long templateId = 8L;
-        String templateUuid = "template-uuid1";
-        String serviceOfferingUuid = "service-offering-uuid1";
-
-        BackupVO backup = new BackupVO();
-        ReflectionTestUtils.setField(backup, "id", backupId);
-        ReflectionTestUtils.setField(backup, "uuid", "backup-uuid");
-        backup.setVmId(vmId);
-        backup.setAccountId(accountId);
-        backup.setDomainId(domainId);
-        backup.setZoneId(zoneId);
-        backup.setBackupOfferingId(backupOfferingId);
-        backup.setType("Full");
-        backup.setBackupIntervalType((short) Backup.Type.MANUAL.ordinal());
-
-        VMInstanceVO vm = new VMInstanceVO(vmId, 0L, "test-vm", "test-vm", VirtualMachine.Type.User,
-                0L, Hypervisor.HypervisorType.Simulator, 0L, domainId, accountId, 0L, false);
-        vm.setDataCenterId(zoneId);
-        vm.setBackupOfferingId(vmOfferingId);
-        vm.setTemplateId(templateId);
-
-        AccountVO account = new AccountVO();
-        account.setUuid("account-uuid");
-        account.setAccountName("test-account");
-
-        DomainVO domain = new DomainVO();
-        domain.setUuid("domain-uuid");
-        domain.setName("test-domain");
-
-        DataCenterVO zone = new DataCenterVO(1L, "test-zone", null, null, null, null, null, null, null, null, DataCenter.NetworkType.Advanced, null, null);
-        zone.setUuid("zone-uuid");
-
-        BackupOfferingVO offering = Mockito.mock(BackupOfferingVO.class);
-        Mockito.when(offering.getUuid()).thenReturn("offering-uuid");
-        Mockito.when(offering.getName()).thenReturn("test-offering");
-
-        Mockito.when(vmInstanceDao.findByIdIncludingRemoved(vmId)).thenReturn(vm);
-        Mockito.when(accountDao.findByIdIncludingRemoved(accountId)).thenReturn(account);
-        Mockito.when(domainDao.findByIdIncludingRemoved(domainId)).thenReturn(domain);
-        Mockito.when(dataCenterDao.findByIdIncludingRemoved(zoneId)).thenReturn(zone);
-        Mockito.when(backupOfferingDao.findByIdIncludingRemoved(backupOfferingId)).thenReturn(offering);
-
-        VMTemplateVO template = mock(VMTemplateVO.class);
-        when(template.getFormat()).thenReturn(Storage.ImageFormat.QCOW2);
-        when(template.getUuid()).thenReturn(templateUuid);
-        when(template.getName()).thenReturn("template1");
-        when(templateDao.findByUuid(templateUuid)).thenReturn(template);
-        Map<String, String> details = new HashMap<>();
-        details.put(ApiConstants.TEMPLATE_ID, templateUuid);
-
-        ServiceOfferingVO serviceOffering = mock(ServiceOfferingVO.class);
-        when(serviceOffering.getUuid()).thenReturn(serviceOfferingUuid);
-        when(serviceOffering.getName()).thenReturn("service-offering1");
-        when(serviceOfferingDao.findByUuid(serviceOfferingUuid)).thenReturn(serviceOffering);
-        details.put(ApiConstants.SERVICE_OFFERING_ID, serviceOfferingUuid);
-
-        NetworkVO network = mock(NetworkVO.class);
-        when(network.getName()).thenReturn("network1");
-        when(networkDao.findByUuid("network-uuid1")).thenReturn(network);
-        details.put(ApiConstants.NICS, "[{\"networkid\":\"network-uuid1\"}]");
-
-        Mockito.when(backupDetailsDao.listDetailsKeyPairs(backup.getId(), true)).thenReturn(details);
-
-        BackupResponse response = backupDao.newBackupResponse(backup, true);
-
-        Assert.assertEquals("backup-uuid", response.getId());
-        Assert.assertEquals("test-vm", response.getVmName());
-        Assert.assertEquals("account-uuid", response.getAccountId());
-        Assert.assertEquals("test-account", response.getAccount());
-        Assert.assertEquals("domain-uuid", response.getDomainId());
-        Assert.assertEquals("test-domain", response.getDomain());
-        Assert.assertEquals("zone-uuid", response.getZoneId());
-        Assert.assertEquals("test-zone", response.getZone());
-        Assert.assertEquals("offering-uuid", response.getBackupOfferingId());
-        Assert.assertEquals("test-offering", response.getBackupOffering());
-        Assert.assertEquals("MANUAL", response.getIntervalType());
-        Assert.assertEquals("{serviceofferingid=service-offering-uuid1, isiso=false, hypervisor=Simulator, " +
-                "nics=[{\"networkid\":\"network-uuid1\",\"networkname\":\"network1\"}], serviceofferingname=service-offering1, " +
-                "templatename=template1, templateid=template-uuid1}", response.getVmDetails().toString());
-        Assert.assertEquals(true, response.getVmOfferingRemoved());
-    }
 }