Moti Asayag has posted comments on this change. Change subject: engine: integrate CidrValidation to backend ......................................................................
Patch Set 1: (9 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 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 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. Line 28: return true; Line 29: } Line 30: 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. 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 CidrConstraint, and the default will be an empty message. 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 failures, the test should be able to verify the excepted message is thrown for the specific failure (either format violation or ip-mask violation). 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. 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 convention ? s/please/Please ? 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 frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties as well maybe the message should include CIDR instead of Cidr which is an internal convention ? -- 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 <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Eliraz Levi <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
