Improve Property Types - #6487
Conversation
|
@kevinrr888 Checking if you are still available for discussion on this issue, Looking for your guidance/opinion over the expected changes |
…cond version of validPath()
|
@ctubbsii Proposal for additional PropertyType.STRING validation: Found groups of Properties of PropertyType.STRING that could potentially have new PropertyType: create new PropertyType.PASSWORD (is password unique enough to need its own validation? Can a password be empty/null) for validating a password for these Properties:
create new PropertyType.ADDRESS (or IP) for validating if a string is a valid IP format (ex: 0.0.0.0)
Probably unnecessary since it looks like there is a list of enabled protocols server side that the protocol must match. Create new PropertyType.PROTOCOL (what does protocol look like) for validating a protocol for this group of Property:
Alternative validation for these Properties that are PropertyType.STRING can be done by creating a new ValidString() method for the PropertyType.STRING. ValidString() can check for the specific string cases mentioned above, and return true for all other strings, currently PropertyType.STRING always returns true to validate any string |
| } else if (new Path(path.trim()).isAbsolute()) { | ||
| return true; |
There was a problem hiding this comment.
If this fails to parse and throws an exception, it will not get to the next else if block.
| } else if (path.matches("/?[A-Za-z+/?]+")) { | ||
| return true; |
There was a problem hiding this comment.
I don't think we can match using a regex. Files can contain nearly any character.... emojis, kanji, cyrillic, whitespace, special characters... really anything.
The original validation for paths before this PR allowed any string. I think that's right. Unless we know more about the file and what its purpose is, we really can't validate anything about it. The PATH type in this case is only to communicate documentation to the user that it should represent a PATH. Other than that, we can do no more validation than anything of STRING type.
| new URI(uri); | ||
| return true; | ||
| } catch (URISyntaxException e) { | ||
| log.error("provided uri string is not valid"); |
There was a problem hiding this comment.
This validation for URI type is good, but I don't think we need to log the error. Returning false achieves the same effect, and will report to the user that it is not a valid URI type.
| // TODO when the input is null, it just means that the property wasn't set | ||
| // we can add checks for not null for required properties with | ||
| // Predicates.and(Predicates.notNull(), ...), | ||
| // or we can stop assuming that null is always okay for a Matches predicate, and do that | ||
| // explicitly with Predicates.or(Predicates.isNull(), ...) |
There was a problem hiding this comment.
This comment that was removed suggests that we should not check input == null here, but should use Predicates.or(Predicates.isNull(), ...) for any patterns where we want to allow null.
If we leave the input == null here, then we need to do something like Predicates.and(Predicates.isNull().negate(), ...) for required properties.
I'm not sure if we've done either, or which would be easier to do if we haven't.
2 participants
ServerConfigCheckRunner.javaand intoProperty.javaas a static method that returns a Set of the required properties, added more detailed Javadoc to the method.PropertyType.PATHandPropertyType.URIPropertyType.javaThis pr resolves #6112
TODO:
PropertyType.STRING, determine which ones need additional validationRefer to discussion over property types on pr #5348