Hello Idan Shaby,
I'd like you to do a code review. Please visit
http://gerrit.ovirt.org/29809
to review the following change.
Change subject: core: UpdateStorageDomain's CDA early return
......................................................................
core: UpdateStorageDomain's CDA early return
Changed UpdateStorageDomainCommand.canDoAction() to use the early return
pattern in order to solve several NullPointerExceptions and enhance
readability.
Tests were added to all the canDoAction() flows to make sure these
changes did not break any functionality.
Change-Id: Iea10984090a759ef07da1fe572c4fdc323ede0e5
Signed-off-by: Allon Mureinik <[email protected]>
Signed-off-by: Idan Shaby <[email protected]>
---
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStorageDomainCommand.java
M
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/UpdateStorageDomainCommandTest.java
2 files changed, 97 insertions(+), 18 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/09/29809/1
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStorageDomainCommand.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStorageDomainCommand.java
index 8b5b049..6ecb36e 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStorageDomainCommand.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStorageDomainCommand.java
@@ -27,15 +27,15 @@
@Override
protected boolean canDoAction() {
- boolean returnValue = super.canDoAction() && checkStorageDomain()
- && checkStorageDomainStatus(StorageDomainStatus.Active) &&
checkStorageDomainNameLengthValid();
- oldDomain =
getStorageDomainStaticDAO().get(getStorageDomain().getId());
+ if (!super.canDoAction() || !checkStorageDomain()
+ || !checkStorageDomainStatus(StorageDomainStatus.Active) ||
!checkStorageDomainNameLengthValid()) {
+ return false;
+ }
// Only after validating the existing of the storage domain in DB, we
set the field lastTimeUsedAsMaster in the
// storage domain which is about to be updated.
- if (returnValue) {
-
getStorageDomain().setLastTimeUsedAsMaster(oldDomain.getLastTimeUsedAsMaster());
- }
+ oldDomain =
getStorageDomainStaticDAO().get(getStorageDomain().getId());
+
getStorageDomain().setLastTimeUsedAsMaster(oldDomain.getLastTimeUsedAsMaster());
// Collect changed fields to update in a list.
List<String> props = ObjectIdentityChecker.GetChangedFields(oldDomain,
getStorageDomain()
@@ -45,26 +45,25 @@
props.remove("storageName");
props.remove("description");
props.remove("comment");
- if (returnValue && props.size() > 0) {
+ if (!props.isEmpty()) {
log.warnFormat("There was an attempt to update the following
fields although they are not allowed to be updated: {0}",
StringUtils.join(props, ","));
-
addCanDoActionMessage(VdcBllMessages.ERROR_CANNOT_CHANGE_STORAGE_DOMAIN_FIELDS);
- returnValue = false;
+ return
failCanDoAction(VdcBllMessages.ERROR_CANNOT_CHANGE_STORAGE_DOMAIN_FIELDS);
}
+
_storageDomainNameChanged =
!StringUtils.equals(oldDomain.getStorageName(),
getStorageDomain().getStorageName());
// if domain is part of pool, and name changed, check that pool is up
in
// order to change description in spm
- if (returnValue
- && _storageDomainNameChanged
- && !validate(new
StoragePoolValidator(getStoragePool()).isUp())) {
- returnValue = false;
+ if (_storageDomainNameChanged && !validate(new
StoragePoolValidator(getStoragePool()).isUp())) {
+ return false;
}
- if (returnValue && _storageDomainNameChanged &&
isStorageWithSameNameExists()) {
-
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_NAME_ALREADY_EXIST);
- returnValue = false;
+
+ if (_storageDomainNameChanged && isStorageWithSameNameExists()) {
+ return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_NAME_ALREADY_EXIST);
}
- return returnValue;
+
+ return true;
}
@Override
diff --git
a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/UpdateStorageDomainCommandTest.java
b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/UpdateStorageDomainCommandTest.java
index 2adc932..a8b3bfe 100644
---
a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/UpdateStorageDomainCommandTest.java
+++
b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/UpdateStorageDomainCommandTest.java
@@ -7,6 +7,7 @@
import static org.ovirt.engine.core.utils.MockConfigRule.mockConfig;
import java.util.List;
+import org.apache.commons.lang.StringUtils;
import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Test;
@@ -20,6 +21,8 @@
import org.ovirt.engine.core.common.businessentities.StorageDomainStatus;
import org.ovirt.engine.core.common.businessentities.StorageDomainType;
import org.ovirt.engine.core.common.businessentities.StorageFormatType;
+import org.ovirt.engine.core.common.businessentities.StoragePool;
+import org.ovirt.engine.core.common.businessentities.StoragePoolStatus;
import org.ovirt.engine.core.common.businessentities.StorageType;
import org.ovirt.engine.core.common.config.ConfigValues;
import org.ovirt.engine.core.common.errors.VdcBllMessages;
@@ -34,13 +37,16 @@
public class UpdateStorageDomainCommandTest {
private Guid sdId;
private StorageDomain sd;
+ private StoragePool sp;
private StorageDomainStatic oldSdStatic;
private StorageDomainStatic newSdStatic;
private UpdateStorageDomainCommand<StorageDomainManagementParameter> cmd;
+ private static final int STORAGE_DOMAIN_NAME_LENGTH_LIMIT = 100;
+
@ClassRule
public static MockConfigRule mcr =
- new
MockConfigRule(mockConfig(ConfigValues.StorageDomainNameSizeLimit, 100));
+ new
MockConfigRule(mockConfig(ConfigValues.StorageDomainNameSizeLimit,
STORAGE_DOMAIN_NAME_LENGTH_LIMIT));
@Mock
private StorageDomainStaticDAO sdsDao;
@@ -50,13 +56,21 @@
sdId = Guid.newGuid();
oldSdStatic = createStorageDomain();
newSdStatic = createStorageDomain();
+ Guid spId = Guid.newGuid();
sd = new StorageDomain();
sd.setStorageStaticData(newSdStatic);
sd.setStatus(StorageDomainStatus.Active);
+ sd.setStoragePoolId(spId);
+
+ sp = new StoragePool();
+ sp.setId(spId);
+ sp.setStatus(StoragePoolStatus.Up);
+ sp.setIsLocal(false);
cmd = spy(new UpdateStorageDomainCommand<>(new
StorageDomainManagementParameter(newSdStatic)));
doReturn(sd).when(cmd).getStorageDomain();
+ doReturn(sp).when(cmd).getStoragePool();
doReturn(sdsDao).when(cmd).getStorageDomainStaticDAO();
when(sdsDao.get(sdId)).thenReturn(oldSdStatic);
@@ -88,4 +102,70 @@
assertTrue("redundant messages " + messages, messages.isEmpty());
}
+ @Test
+ public void canDoActionNoDomain() {
+ doReturn(null).when(cmd).getStorageDomain();
+ CanDoActionTestUtils.runAndAssertCanDoActionFailure(cmd,
+ VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_NOT_EXIST);
+ }
+
+ @Test
+ public void canDoActionWrongStatus() {
+ sd.setStatus(StorageDomainStatus.Maintenance);
+ CanDoActionTestUtils.runAndAssertCanDoActionFailure(cmd,
+
VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_STATUS_ILLEGAL2);
+ }
+
+ @Test
+ public void canDoChangeDescription() {
+ sd.setDescription(StringUtils.reverse(sd.getDescription()));
+ CanDoActionTestUtils.runAndAssertCanDoActionSuccess(cmd);
+ }
+
+ @Test
+ public void canDoChangeComment() {
+ sd.setComment(StringUtils.reverse(sd.getComment()));
+ CanDoActionTestUtils.runAndAssertCanDoActionSuccess(cmd);
+ }
+
+ @Test
+ public void canDoChangeForbiddenField() {
+ sd.setStorageType(StorageType.UNKNOWN);
+ CanDoActionTestUtils.runAndAssertCanDoActionFailure(cmd,
+ VdcBllMessages.ERROR_CANNOT_CHANGE_STORAGE_DOMAIN_FIELDS);
+ }
+
+ @Test
+ public void canDoActionName() {
+ sd.setStorageName(StringUtils.reverse(sd.getStorageName()));
+ CanDoActionTestUtils.runAndAssertCanDoActionSuccess(cmd);
+ }
+
+ @Test
+ public void canDoActionLongName() {
+ // Generate a really long name
+ String longName = StringUtils.leftPad("name",
STORAGE_DOMAIN_NAME_LENGTH_LIMIT * 2, 'X');
+ sd.setStorageName(longName);
+ CanDoActionTestUtils.runAndAssertCanDoActionFailure(cmd,
+ VdcBllMessages.ACTION_TYPE_FAILED_NAME_LENGTH_IS_TOO_LONG);
+ }
+
+ @Test
+ public void canDoChangeNamePoolNotUp() {
+ sd.setStorageName(StringUtils.reverse(sd.getStorageName()));
+ sp.setStatus(StoragePoolStatus.Maintenance);
+
+ CanDoActionTestUtils.runAndAssertCanDoActionFailure(cmd,
+ VdcBllMessages.ACTION_TYPE_FAILED_IMAGE_REPOSITORY_NOT_FOUND);
+ }
+
+ @Test
+ public void canDoChangeNameExists() {
+ String newName = StringUtils.reverse(sd.getStorageName());
+ sd.setStorageName(newName);
+
+ doReturn(new StorageDomainStatic()).when(sdsDao).getByName(newName);
+ CanDoActionTestUtils.runAndAssertCanDoActionFailure(cmd,
+
VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_NAME_ALREADY_EXIST);
+ }
}
--
To view, visit http://gerrit.ovirt.org/29809
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iea10984090a759ef07da1fe572c4fdc323ede0e5
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Idan Shaby <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
