Eliraz Levi has posted comments on this change. Change subject: engine: integrate CidrValidation to backend ......................................................................
Patch Set 1: (10 comments) http://gerrit.ovirt.org/#/c/32540/1/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/validation/CidrConstraint.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/validation/CidrConstraint.java: Line 12: } Line 13: Line 14: @Override Line 15: public boolean isValid(String cidr, ConstraintValidatorContext context) { Line 16: boolean isCidrFormat = CidrValidator.isCidrFormatValid(cidr); > you can inline the condition Done Line 17: if (!isCidrFormat) { Line 18: return false; Line 19: } Line 20: boolean isNetworkAddress = CidrValidator.isCidrValidNetworkAddress(cidr); Line 16: boolean isCidrFormat = CidrValidator.isCidrFormatValid(cidr); Line 17: if (!isCidrFormat) { Line 18: return false; Line 19: } Line 20: boolean isNetworkAddress = CidrValidator.isCidrValidNetworkAddress(cidr); > same here Done Line 21: if (!isNetworkAddress) { Line 22: context.disableDefaultConstraintViolation(); Line 23: context.buildConstraintViolationWithTemplate("CIDR_NOT_NETWORK_ADDRESS") Line 24: .addNode("cidr") Line 23: context.buildConstraintViolationWithTemplate("CIDR_NOT_NETWORK_ADDRESS") Line 24: .addNode("cidr") Line 25: .addConstraintViolation(); Line 26: return false; Line 27: } > please add a space line after a closing curly bracket. Done Line 28: return true; Line 29: } Line 30: Line 27: } Line 28: return true; Line 29: } Line 30: Line 31: } > or you can make it look like: Done http://gerrit.ovirt.org/#/c/32540/1/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/validation/annotation/Cidr.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/validation/annotation/Cidr.java: Line 13: import org.ovirt.engine.core.common.validation.CidrConstraint; Line 14: Line 15: /*** Line 16: * the annotation valid Line 17: * @author elevi > please remove the 'author' entry. we don't have such a convention. Done Line 18: */ Line 19: @Target({ FIELD }) Line 20: @Retention(RUNTIME) Line 21: @Documented Line 20: @Retention(RUNTIME) Line 21: @Documented Line 22: @Constraint(validatedBy = CidrConstraint.class) Line 23: public @interface Cidr { Line 24: String message() default "BAD_CIDR_FORMAT"; > it will be easier to follow the messages when all are listed inside CidrCon Done Line 25: Line 26: Class<?>[] groups() default {}; Line 27: Line 28: Class<? extends Payload>[] payload() default {}; http://gerrit.ovirt.org/#/c/32540/1/backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/utils/CidrAnnotationTest.java File backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/utils/CidrAnnotationTest.java: Line 40: } Line 41: Line 42: @Parameterized.Parameters Line 43: public static Collection<Object[]> namesParams() { Line 44: return CidrValidatorTest.data(); > since the Cidr validator may produce 2 different message for 2 different fa can be implemented with a cost of not using @Parameterized.Parameters. explanation: say we have input 1.1.1.1/1 for isCidrFormatValid validation, it will be saved in Param's collection as ("1.1.1.1/1",true) but for isCidrValidNetworkAddress validation it should be saved as ("1.1.1.1/1", false) so i think having one test to check them both is worth the simplicity of using @param. note: isCidrValidNetworkAddress assume for valid format. for tests as well , must first ask isCidrFormatValid before calling to isCidrValidNetworkAddress Line 45: } Line 46: Line 47: private class CidrContainer { Line 48: @Cidr Line 48: @Cidr Line 49: private String cidr; Line 50: Line 51: public CidrContainer(String cidr) { Line 52: super(); > the call for the empty c'tor is redundant. Done Line 53: this.cidr = cidr; Line 54: } Line 55: Line 56: public String getCidr() { http://gerrit.ovirt.org/#/c/32540/1/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties: Line 1264: Line 1265: KDUMP_DETECTION_NOT_ENABLED_FOR_VDS=Cannot ${action} ${type}. Kdump integration is not enabled for host '${VdsName}'. Line 1266: KDUMP_DETECTION_NOT_CONFIGURED_ON_VDS=Cannot ${action} ${type}. Kdump is not properly configured on host '${VdsName}'. Line 1267: Line 1268: CIDR_NOT_NETWORK_ADDRESS=Cidr not represnting a network address.\nplease ensure ip and mask are matching to network ip address. \nexample: valid network address: 2.2.0.0/16 \ninvalid: 2.2.0.1/16 > maybe the message should include CIDR instead of Cidr which is an internal Done Line 1269: BAD_CIDR_FORMAT=Cidr bad format, expected:\n x.x.x.x/y where:\n x belongs to [0,255] \n y belongs to [0,32] \n both inclusive Line 1265: KDUMP_DETECTION_NOT_ENABLED_FOR_VDS=Cannot ${action} ${type}. Kdump integration is not enabled for host '${VdsName}'. Line 1266: KDUMP_DETECTION_NOT_CONFIGURED_ON_VDS=Cannot ${action} ${type}. Kdump is not properly configured on host '${VdsName}'. Line 1267: Line 1268: CIDR_NOT_NETWORK_ADDRESS=Cidr not represnting a network address.\nplease ensure ip and mask are matching to network ip address. \nexample: valid network address: 2.2.0.0/16 \ninvalid: 2.2.0.1/16 Line 1269: BAD_CIDR_FORMAT=Cidr bad format, expected:\n x.x.x.x/y where:\n x belongs to [0,255] \n y belongs to [0,32] \n both inclusive > the messages should be added to Done -- To view, visit http://gerrit.ovirt.org/32540 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib4dc94cc350dc03195c3f24ba783036592e3c03e Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eliraz Levi <el...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Eliraz Levi <el...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
