Lior Vernia has posted comments on this change. Change subject: common: adding maskValidator util ......................................................................
Patch Set 8: (6 comments) http://gerrit.ovirt.org/#/c/35144/8/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/ValidationUtils.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/ValidationUtils.java: Line 21: public static final String ONLY_I18N_ASCII_OR_NONE = "[\\p{ASCII}\\p{L}]*"; Line 22: public static final String ONLY_ASCII_OR_NONE = "[\\p{ASCII}]*"; Line 23: public static final String NO_SPECIAL_CHARACTERS_WITH_DOT = "[0-9a-zA-Z-_\\.]+"; Line 24: public static final String NO_TRIMMING_WHITE_SPACES_PATTERN = "^$|\\S.*\\S"; Line 25: public static final String IP_PATTERN = This should be change to "^" + IP_PATTERN_NON_EMPTY + "$|^$". Line 26: "^\\b((25[0-5]|2[0-4]\\d|[01]\\d\\d|\\d?\\d)\\.){3}(25[0-5]|2[0-4]\\d|[01]\\d\\d|\\d?\\d)\\b$|^$"; Line 27: public static final String IP_PATTERN_NON_EMPTY = Line 28: "\\b((25[0-5]|2[0-4]\\d|[01]\\d\\d|\\d?\\d)\\.){3}(25[0-5]|2[0-4]\\d|[01]\\d\\d|\\d?\\d)"; Line 29: public static final String SUB_NET_PREFIX_PATTERN = "(?:3[0-2]|[12]?[0-9])"; Line 25: public static final String IP_PATTERN = Line 26: "^\\b((25[0-5]|2[0-4]\\d|[01]\\d\\d|\\d?\\d)\\.){3}(25[0-5]|2[0-4]\\d|[01]\\d\\d|\\d?\\d)\\b$|^$"; Line 27: public static final String IP_PATTERN_NON_EMPTY = Line 28: "\\b((25[0-5]|2[0-4]\\d|[01]\\d\\d|\\d?\\d)\\.){3}(25[0-5]|2[0-4]\\d|[01]\\d\\d|\\d?\\d)"; Line 29: public static final String SUB_NET_PREFIX_PATTERN = "(?:3[0-2]|[12]?[0-9])"; Subnet is usually one word. Line 30: public static final String CIDR_FORMAT_PATTERN = "^" + IP_PATTERN_NON_EMPTY + "/" + SUB_NET_PREFIX_PATTERN + "$"; Line 31: public static final String ISO_SUFFIX = ".iso"; Line 32: public static final String ISO_SUFFIX_PATTERN = "^$|^.+\\.iso$"; Line 33: http://gerrit.ovirt.org/#/c/35144/8/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/validation/MaskValidator.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/validation/MaskValidator.java: Line 27: * @param isPrefixAllowed Line 28: * indicates if mask in prefix format is allowed Line 29: * @return true if correct IPv4 format or string [0-32] (possible with /), false otherwise. Line 30: */ Line 31: public boolean isMaskFormatValid(String mask, boolean isPrefixAllowed) { I apologize, unifying the two formats isn't a good idea. There should still be one public method that queries whether this is in valid netmask format, and one that queries whether this is in valid prefix format. See my comment below on isMaskValid(). Line 32: return mask != null && (isPrefixAllowed ? mask.matches("(^" + ValidationUtils.IP_PATTERN_NON_EMPTY + "$)" Line 33: + "|" + "/?" + ValidationUtils.SUB_NET_PREFIX_PATTERN) Line 34: : mask.matches(ValidationUtils.IP_PATTERN_NON_EMPTY)); Line 35: } Line 30: */ Line 31: public boolean isMaskFormatValid(String mask, boolean isPrefixAllowed) { Line 32: return mask != null && (isPrefixAllowed ? mask.matches("(^" + ValidationUtils.IP_PATTERN_NON_EMPTY + "$)" Line 33: + "|" + "/?" + ValidationUtils.SUB_NET_PREFIX_PATTERN) Line 34: : mask.matches(ValidationUtils.IP_PATTERN_NON_EMPTY)); I think you also want the "^" and "$" here. Line 35: } Line 36: Line 37: /*** Line 38: * check if mask value is valid and return true if does Line 41: * indicates if mask in prefix format is allowed Line 42: * @return true for valid mask value, false otherwise Line 43: */ Line 44: public boolean isMaskValid(String mask, boolean isPrefixAllowed) { Line 45: if (isPrefixAllowed) { This is weird - again the method isn't granular enough. It doesn't make sense to ask whether the netmask value is valid if it's in prefix format - this only makes sense if it's in netmask format, and therefore should only be called in that case. For that to happen, you need code calling this method to first know whether what it has as netmask is in prefix format or not (and only call this if not) - that's why you need the two distinct query methods. If this is done, then this method will not be needed and the method below needs to be made public again. Line 46: if (mask.matches("/?" + ValidationUtils.SUB_NET_PREFIX_PATTERN)) { Line 47: return true; Line 48: } Line 49: http://gerrit.ovirt.org/#/c/35144/8/backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/utils/IpAddressConverterTest.java File backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/utils/IpAddressConverterTest.java: Line 36: { "1.255.4.255", 33490175 }, Line 37: { "0.128.0.7", 8388615 }, Line 38: { "1.2.3.4", 16909060 }, Line 39: { "127.63.31.7", 2134843143 }, Line 40: { "0.0.0.1", 1 }, }); You haven't taken into consideration my comment from the last patchset. Line 41: } Line 42: -- To view, visit http://gerrit.ovirt.org/35144 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I04fb0aa2193801688f1bfc5dc7684cabdfa2827d Gerrit-PatchSet: 8 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: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@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
