You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by David Medinets <da...@gmail.com> on 2012/09/09 03:55:01 UTC
PORT Property Type description does not match regular expression.
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?
Re: PORT Property Type description does not match regular expression.
Posted by Christopher Tubbs <ct...@gmail.com>.
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 <ct...@gmail.com> 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 <da...@gmail.com> 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?
Re: PORT Property Type description does not match regular expression.
Posted by Christopher Tubbs <ct...@gmail.com>.
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 <da...@gmail.com> 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?