Martin Mucha has posted comments on this change. Change subject: restapi: Add support for Network Attachements. ......................................................................
Patch Set 38: (7 comments) https://gerrit.ovirt.org/#/c/32775/38/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/NetworkAttachmentValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/NetworkAttachmentValidator.java: Line 64: IpConfiguration ipConfiguration = attachment.getIpConfiguration(); Line 65: return ValidationResult.failWith(VdcBllMessages.NETWORK_ADDR_MANDATORY_IN_STATIC_IP) Line 66: .unless(ipConfiguration == null Line 67: || ipConfiguration.getBootProtocol() != NetworkBootProtocol.STATIC_IP Line 68: || ipConfiguration.hasPrimaryAddressSet() && ipConfiguration.getPrimaryAddress() != null && ipConfiguration.getPrimaryAddress().getNetmask() != null); > 'ipConfiguration.getPrimaryAddress() != null' redundant, Sounds like ipConf Done Line 69: } Line 70: Line 71: public ValidationResult bootProtocolSetForDisplayNetwork() { Line 72: IpConfiguration ipConfiguration = attachment.getIpConfiguration(); https://gerrit.ovirt.org/#/c/32775/38/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/network/IPv4Address.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/network/IPv4Address.java: Line 24: @Pattern(regexp = ValidationUtils.IP_PATTERN, message = "NETWORK_ADDR_IN_GATEWAY_BAD_FORMAT") Line 25: @Size(max = BusinessEntitiesDefinitions.GENERAL_GATEWAY_SIZE) Line 26: private String gateway; Line 27: Line 28: public static long getSerialVersionUID() { > Why do you need this getter? no idea why it's here, removing it. Done. Line 29: return serialVersionUID; Line 30: } Line 31: Line 32: public String getAddress() { https://gerrit.ovirt.org/#/c/32775/38/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/network/IpConfiguration.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/network/IpConfiguration.java: Line 42: } Line 43: Line 44: public IPv4Address getPrimaryAddress() { Line 45: if (!hasPrimaryAddressSet()) { Line 46: throw new IllegalStateException("IpConfiguration does not have set IPv4 address set."); > 1. s/does not have set IPv4 address set/does not have IPv4 address set 1. fixed. 2. API is designed to support multiple IPv4Addresses (see the field), while we do currently support only one. When api is fully implemented/used in future, there will be multiple addresses with one potentially flagged as primary one. So all code dealing with 'getPrimaryAddress' is probably 'wrong' when thinking about supporting multiple addresses. Both this methods exists just to be able to pass multiple addresses from rest to here, so we have as clean as possible rest(juan correctly complained about cherry-picking first element from collection) and at least fields of business entities right. 3. I can alter it, so that it returns null. If someone fails to do it's checking current way he gets exception complaining about illegal state and desc (and it's an illegal state of ip configuration). Returning null will produce NPE. So if we want have same level of self-checking, all callers woud have to check for nullity and complain adequtelly himself. Checking for nullity is optimized for writer (it's more intuitive), hasPrimaryAddressSet is optimized for reader(it's more readable). Thinking about it, it seems that hasPrimaryAddressSet should be private, and if anyone requires primaryAddress it should fail, since it's invalid IpConfiguration state. I tested to remove it (allowing to fail when accessed ipconfiguration without address) and all tests succeeded, but I'm not sure if it's not valid request to send IpConfiguration with only bootProtocol set and without any address set. Line 47: } Line 48: return getIPv4Addresses().get(0); Line 49: } Line 50: Line 48: return getIPv4Addresses().get(0); Line 49: } Line 50: Line 51: public boolean hasPrimaryAddressSet() { Line 52: return IPv4Addresses != null && !IPv4Addresses.isEmpty(); > Please also check the the primaryAddress is not null (a null value can be a Done Line 53: } Line 54: Line 55: public void setIPv4Addresses(List<IPv4Address> IPv4Addresses) { Line 56: this.IPv4Addresses = IPv4Addresses; https://gerrit.ovirt.org/#/c/32775/38/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/network/NetworkAttachmentDaoDbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/network/NetworkAttachmentDaoDbFacadeImpl.java: Line 57: } Line 58: Line 59: private void mapIpConfiguration(MapSqlParameterSource mapper, IpConfiguration ipConfiguration) { Line 60: boolean hasPrimaryAddressSet = ipConfiguration.hasPrimaryAddressSet(); Line 61: IPv4Address primaryAddress = hasPrimaryAddressSet ? ipConfiguration.getPrimaryAddress() : null; > If ipConfiguration.getPrimaryAddress() will return null if it is not set, please see comment in IpConfiguration Line 62: Line 63: mapper.addValue("boot_protocol", EnumUtils.nameOrNull(ipConfiguration.getBootProtocol())) Line 64: .addValue("address", hasPrimaryAddressSet ? primaryAddress.getAddress() : null) Line 65: .addValue("netmask", hasPrimaryAddressSet ? primaryAddress.getNetmask() : null) Line 60: boolean hasPrimaryAddressSet = ipConfiguration.hasPrimaryAddressSet(); Line 61: IPv4Address primaryAddress = hasPrimaryAddressSet ? ipConfiguration.getPrimaryAddress() : null; Line 62: Line 63: mapper.addValue("boot_protocol", EnumUtils.nameOrNull(ipConfiguration.getBootProtocol())) Line 64: .addValue("address", hasPrimaryAddressSet ? primaryAddress.getAddress() : null) > primaryAddress != null instead of hasPrimaryAddressSet is more intuitive. please see comment in IpConfiguration Line 65: .addValue("netmask", hasPrimaryAddressSet ? primaryAddress.getNetmask() : null) Line 66: .addValue("gateway", hasPrimaryAddressSet ? primaryAddress.getGateway() : null); Line 67: } Line 68: https://gerrit.ovirt.org/#/c/32775/38/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/network/NetworkAttachmentDaoTest.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/network/NetworkAttachmentDaoTest.java: Line 82: Line 83: return expected; Line 84: } Line 85: Line 86: public IPv4Address createPrimaryAddress() { > The default values of IPv4Address are null, this method is not needed. Done Line 87: IPv4Address primaryAddress = new IPv4Address(); Line 88: primaryAddress.setAddress(null); Line 89: primaryAddress.setNetmask(null); Line 90: primaryAddress.setGateway(null); -- To view, visit https://gerrit.ovirt.org/32775 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id6ac91eea1000f7fdf6105777343a1ac1c77368d Gerrit-PatchSet: 38 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
