Sorry, that was long. A shorter response: It should do better than that, but it was only intended as a quick sanity check, deferring more robust checks to the code that uses the property.
On Sun, Sep 9, 2012 at 1:49 AM, Christopher Tubbs <[email protected]> wrote: > The regular expression was originally just a simple, quick sanity > check, not a full-fledged validation. It should probably check for > range validity, either in the regular expression or in the > isValidFormat implementation. What I'd like to do with that is replace > it with a more generic validator. So, instead of just passing in a > regex to validate, it would pass in a function. > > One reason why just a quick and dirty check was done was because every > property has a separate range of validity, and we can only generalize > so much by validating the type outside of the context of a particular > property. The intent was to do a dirty check on the type for sanity, > then fail more robustly when the property actually needed to be used. > > My only concern with such a change (getting rid of regexes for more > robust validators), is that I don't remember whether the regex showed > up in the config documentation during the build... if it does, the > regex still has some utility for documentation. > > At the very least, that's a horrible regex for PORT, and I should > apologize for being so lazy. A better one have at least been > "\\d{4,5}". Better yet, it should be something like > "102[4-9]|10[3-9]\\d|1[1-9]\\d{2}|[2-9]\\d{3}|[1-5]\\d{4}|6[0-4]\\d{3}|65[0-4]\\d{2}|655[0-2]\\d|6553[0-5]" > (untested against java-specific syntax, but I think it is complete in > principle). > > If the regex is being shown on any generated configuration > documentation still, this longer one wouldn't be very useful to read. > I still like the validator concept to replace the regex. Descriptive > sentences are probably best for documentation, anyway, not regexes. > > If you create a ticket, feel free to assign it to me. I can create a > patch for 1.5.0 to improve the validation. I had some other ideas > about improving config anyway (possible discussions to follow). > > On Sat, Sep 8, 2012 at 9:55 PM, David Medinets <[email protected]> > wrote: >> PropertyType >> >> PORT("port", "\\d{1,5}", "An positive integer in the range >> 1024-65535, not already in use or specified elsewhere in the >> configuration"), >> >> I was writing some unit tests for PropertyType and ran across PORT: >> >> case PORT: >> assertFalse(pt.name(), pt.isValidFormat(null)); >> assertFalse(pt.name(), pt.isValidFormat("1023")); >> for (int i = 1024; i < 65536; i++) { >> assertTrue(pt.name(), pt.isValidFormat(new >> Integer(i).toString())); >> } >> assertFalse(pt.name(), pt.isValidFormat("65536")); >> >> The second test failed. And, of course, when I checked the regular >> expression it allows any number from "0" to "99999". Should the >> isValidFormat method actually test the port range?
