Fix code smells

GutoVeronezi · GutoVeronezi · commit 5e268ce08096 · 2022-08-11T17:01:04.000-03:00
diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java
@@ -346,17 +346,8 @@ protected Boolean deleteSnapshotInfo(SnapshotInfo snapshotInfo, String storage,
 
                 snapshotObject.processEvent(Snapshot.Event.OperationSucceeded);
                 return true;
-            } else {
-                try {
-                    if (snapshotSvr.deleteSnapshot(snapshotInfo)) {
-                        snapshotObject.processEvent(Snapshot.Event.OperationSucceeded);
-                        s_logger.debug(String.format("%s was deleted on %s. We will mark the snapshot as destroyed.", snapshotVo, storageToString));
-                        return true;
-                    }
-                } catch (CloudRuntimeException ex) {
-                    s_logger.warn(String.format("Unable do delete snapshot %s on %s due to [%s]. The reference will be marked as 'Destroying' for future garbage collecting.",
-                        snapshotVo, storageToString, ex.getMessage()), ex);
-                }
+            } else if (deleteSnapshotInPrimaryStorage(snapshotInfo, snapshotVo, storageToString, snapshotObject)) {
+                return true;
             }
 
             s_logger.debug(String.format("Failed to delete %s on %s.", snapshotVo, storageToString));
@@ -368,6 +359,20 @@ protected Boolean deleteSnapshotInfo(SnapshotInfo snapshotInfo, String storage,
         return false;
     }
 
+    protected boolean deleteSnapshotInPrimaryStorage(SnapshotInfo snapshotInfo, SnapshotVO snapshotVo, String storageToString, SnapshotObject snapshotObject) throws NoTransitionException {
+        try {
+            if (snapshotSvr.deleteSnapshot(snapshotInfo)) {
+                snapshotObject.processEvent(Snapshot.Event.OperationSucceeded);
+                s_logger.debug(String.format("%s was deleted on %s. We will mark the snapshot as destroyed.", snapshotVo, storageToString));
+                return true;
+            }
+        } catch (CloudRuntimeException ex) {
+            s_logger.warn(String.format("Unable do delete snapshot %s on %s due to [%s]. The reference will be marked as 'Destroying' for future garbage collecting.",
+                    snapshotVo, storageToString, ex.getMessage()), ex);
+        }
+        return false;
+    }
+
     protected void verifyIfTheSnapshotIsBeingUsedByAnyVolume(SnapshotObject snapshotObject) throws NoTransitionException {
         List<VolumeDetailVO> volumesFromSnapshot = _volumeDetailsDaoImpl.findDetails("SNAPSHOT_ID", String.valueOf(snapshotObject.getSnapshotId()), null);
         if (CollectionUtils.isEmpty(volumesFromSnapshot)) {
diff --git a/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategyTest.java b/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategyTest.java
@@ -226,4 +226,23 @@ public void verifyIfTheSnapshotIsBeingUsedByAnyVolumeTestDetailsIsNotEmptyThrowC
         Mockito.doReturn(List.of(new VolumeDetailVO())).when(volumeDetailsDaoMock).findDetails(Mockito.any(), Mockito.any(), Mockito.any());
         defaultSnapshotStrategySpy.verifyIfTheSnapshotIsBeingUsedByAnyVolume(snapshotObjectMock);
     }
+
+    @Test
+    public void deleteSnapshotInPrimaryStorageTestReturnTrueIfDeleteReturnsTrue() throws NoTransitionException {
+        Mockito.doReturn(true).when(snapshotServiceMock).deleteSnapshot(Mockito.any());
+        Mockito.doNothing().when(snapshotObjectMock).processEvent(Mockito.any(Snapshot.Event.class));
+        Assert.assertTrue(defaultSnapshotStrategySpy.deleteSnapshotInPrimaryStorage(null, null, null, snapshotObjectMock));
+    }
+
+    @Test
+    public void deleteSnapshotInPrimaryStorageTestReturnFalseIfDeleteReturnsFalse() throws NoTransitionException {
+        Mockito.doReturn(false).when(snapshotServiceMock).deleteSnapshot(Mockito.any());
+        Assert.assertFalse(defaultSnapshotStrategySpy.deleteSnapshotInPrimaryStorage(null, null, null, null));
+    }
+
+    @Test
+    public void deleteSnapshotInPrimaryStorageTestReturnFalseIfDeleteThrowsException() throws NoTransitionException {
+        Mockito.doThrow(CloudRuntimeException.class).when(snapshotServiceMock).deleteSnapshot(Mockito.any());
+        Assert.assertFalse(defaultSnapshotStrategySpy.deleteSnapshotInPrimaryStorage(null, null, null, null));
+    }
 }
