Moti Asayag has posted comments on this change.
Change subject: engine: Refactored code into NetworkValidator class
......................................................................
Patch Set 3: (16 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/dc/AddNetworkCommand.java
Line 82
Line 83
Line 84
Line 85
Line 86
Since this method is being used by AddNetworkCommand, there is an option to
extend the NetworkValidator by this class as "AddNetworkValidator" that will
contain the validation below.
However i'm not sure at the moment that it worth introducing new class just for
the sake a single validation.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/dc/NetworkCommon.java
Line 17: @CustomLogFields({ @CustomLogField("NetworkName") })
Line 18: public abstract class NetworkCommon<T extends
AddNetworkStoragePoolParameters> extends CommandBase<T> {
Line 19: public NetworkCommon(T parameters) {
Line 20: super(parameters);
Line 21: this.setStoragePoolId(getNetwork().getDataCenterId());
The changes make this line relevant only for AddNetworkCommand.
Please remove it and modify:
AddNetworkCommand.getPermissionCheckSubjects() to use getNetwork() instead in
a safe way.
It can come as a follower patch.
Line 22: }
Line 23:
Line 24: protected Network getNetwork() {
Line 25: return getParameters().getNetwork();
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/dc/RemoveNetworkCommand.java
Line 39: @Override
Line 40: public AuditLogType getAuditLogTypeValue() {
Line 41: return getSucceeded() ? AuditLogType.NETWORK_REMOVE_NETWORK :
AuditLogType.NETWORK_REMOVE_NETWORK_FAILED;
Line 42: }
Line 43:
this method can be removed, same as the network data member since they won't be
called more than once and inline them when creating the NetworkValidator.
Can be done in a separate patch.
Line 44: protected Network getRemovedNetwork() {
Line 45: if (network == null) {
Line 46: network = getNetworkDAO().get(super.getNetwork().getId());
Line 47: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/dc/UpdateNetworkCommand.java
Line 48: && validate(validatorNew.mtuValid())
Line 49: && validate(validatorNew.networkPrefixValid())
Line 50: && validate(validatorNew.vlanIdNotUsed())
Line 51: && validate(validatorOld.networkExists())
Line 52: && validate(validatorOld.notManagementNetwork())
This validation prevents any updates to the management network. User should be
able to change the network management configuration except its name.
Please preserve previous validation.
Line 53: && validate(validatorNew.networkNameNotUsed())
Line 54: && validate(validatorOld.networkNotUsedByVms())
Line 55: && validate(validatorOld.networkNotUsedByTemplates())
Line 56: && validate(validatorOld.networkNotUsedByHosts());
Line 81
Line 82
Line 83
Line 84
Line 85
Same as AddNetworkValidator comment - you can introduce a validator
UpdateNetworkValidator inherits from NetworkValidator which extends it with the
method below (probably only notChangingManagementNetworkName())
Yet again, not sure how worthy it is.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/NetworkValidator.java
Line 14: import org.ovirt.engine.core.dal.dbbroker.DbFacade;
Line 15: import org.ovirt.engine.core.utils.ReplacementUtils;
Line 16:
Line 17: /**
Line 18: * @author Lior Vernia ([email protected]) <br>
please remove the @author, there is no such convention in the project.
Line 19: * <br>
Line 20: * This class is used to validate different traits of a
network on the data center level. <br>
Line 21: * <br>
Line 22: * Usage: instantiate on a per-network basis, passing the
network to be validated as an argument to the
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/NetworkValidatorTest.java
Line 54: public void setup() {
Line 55:
Line 56: // spy on attempts to access the database
Line 57: validator = spy(new NetworkValidator(network));
Line 58: doReturn(dataCenter).when(validator).getDataCenter();
There preferred method is using
when(validator.getDataCenter()).thenReturn(dataCenter);
rather than doReturn() as the javadoc of doReturn explains.
Note that changing it requires mocking the getDbFacade(), getNetworkDao() and
getStoragePoolDao().
Line 59: doReturn(networks).when(validator).getNetworks();
Line 60:
Line 61: // mock version checking
Line 62: Version version = mock(Version.class);
Line 61: // mock version checking
Line 62: Version version = mock(Version.class);
Line 63: when(version.getValue()).thenReturn(null);
Line 64:
when(dataCenter.getcompatibility_version()).thenReturn(version);
Line 65:
please remove this space line.
Line 66: }
Line 67:
Line 68: @Test
Line 69: public void networkExists() throws Exception {
Line 66: }
Line 67:
Line 68: @Test
Line 69: public void networkExists() throws Exception {
Line 70: assertEquals(ValidationResult.VALID.getMessage(),
validator.networkExists().getMessage());
Since using assertEquals to compare objects, you can compare the
ValidationResult objects:
assertEquals(ValidationResult.VALID, validator.networkExists());
here and elsewhere in the file.
Line 71: }
Line 72:
Line 73: @Test
Line 74: public void networkNull() throws Exception {
Line 71: }
Line 72:
Line 73: @Test
Line 74: public void networkNull() throws Exception {
Line 75: validator = spy(new NetworkValidator(null)); // replace
default validator with one corresponding to null network
1. no need for spy here, since you don't plan changing any of the validator
behaviour
2. you reassign the validator value - since this is a test, I don't mind
leaving it here, else it's going to be pain adding the validator initialization
per test.
Line 76: assertEquals(new
ValidationResult(VdcBllMessages.NETWORK_NOT_EXISTS), validator.networkExists());
Line 77: }
Line 78:
Line 79: @Test
Line 71: }
Line 72:
Line 73: @Test
Line 74: public void networkNull() throws Exception {
Line 75: validator = spy(new NetworkValidator(null)); // replace
default validator with one corresponding to null network
the spy is redundant here
Line 76: assertEquals(new
ValidationResult(VdcBllMessages.NETWORK_NOT_EXISTS), validator.networkExists());
Line 77: }
Line 78:
Line 79: @Test
Line 77: }
Line 78:
Line 79: @Test
Line 80: public void dataCenterDoesntExist() throws Exception {
Line 81: doReturn(null).when(validator).getDataCenter(); // replace
default mock data center with null data center
Please replace the doReturn() with when() around the entire file, i.e.
when(validator.getDataCenter()).thenReturn(null);
Line 82: assertEquals(new
ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_POOL_NOT_EXIST).getMessage(),
Line 83: validator.dataCenterExists().getMessage());
Line 84: }
Line 85:
Line 241: otherNetwork.setVlanId(DEFAULT_VLAN_ID);
Line 242: otherNetwork.setId(new Guid(OTHER_GUID));
Line 243:
Line 244: List<Network> networks = new ArrayList<Network>();
Line 245: networks.add(otherNetwork);
you can replace the two lines above and the need for networks variable with a
single call to:
Collections.singletonList(otherNetwork)
Here and elsewhere if the file that perform a list with single item for read.
Line 246:
Line 247: vlanIdAvailableTest(new
ValidationResult(VdcBllMessages.NETWORK_VLAN_IN_USE), networks);
Line 248: }
Line 249:
Line 325: }
Line 326:
Line 327: @Test
Line 328: public void networkNotInUse() throws Exception {
Line 329: networkNotUsedTest(ValidationResult.VALID, new
ArrayList<Nameable>());
please replace with Collections.<Network> emptyList()
Line 330: }
Line 331:
Line 332: @Test
Line 333: public void networkInUse() throws Exception {
Line 330: }
Line 331:
Line 332: @Test
Line 333: public void networkInUse() throws Exception {
Line 334: Nameable nameable = new VM();
Please replace instantiation with independent Nameable implementation, e.g.:
Nameable nameable = new Nameable() {
@Override
public String getName() {
return null;
}
};
Line 335: List<Nameable> nameables = new ArrayList<Nameable>();
Line 336: nameables.add(nameable);
Line 337: networkNotUsedTest(new
ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_NETWORK_IN_USE), nameables);
Line 338: }
Line 332: @Test
Line 333: public void networkInUse() throws Exception {
Line 334: Nameable nameable = new VM();
Line 335: List<Nameable> nameables = new ArrayList<Nameable>();
Line 336: nameables.add(nameable);
Please replace the nameables instantiation with a call to
Collections.singletonList(nameable)
Line 337: networkNotUsedTest(new
ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_NETWORK_IN_USE), nameables);
Line 338: }
Line 339:
--
To view, visit http://gerrit.ovirt.org/10940
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf40d81f0481c6b6dec141a71363888dc9e9a941
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Lior Vernia <[email protected]>
Gerrit-Reviewer: Lior Vernia <[email protected]>
Gerrit-Reviewer: Livnat Peer <[email protected]>
Gerrit-Reviewer: Mike Kolesnik <[email protected]>
Gerrit-Reviewer: Moti Asayag <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
