Improve Property Types

[Amemeda]

• Moved required properties out of ServerConfigCheckRunner.java and into Property.java as a final static enum Set REQUIRED_PROPERTIES to match the implementation of FIXED_PROPERTIES, added more detailed description.
• Added validation and PropertyType.URI
• Removed outdated TODO from PropertyType.java

This pr resolves #6112
Refer to discussion over property types on pr #5348

[Amemeda]

@kevinrr888 Checking if you are still available for discussion on this issue, Looking for your guidance/opinion over the expected changes

[Amemeda]

@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:

• Property.RPC_SSL_KEYSTORE_PASSWORD
• Property.RPC_SSL_TRUSTSTORE_PASSWORD
• Property.MONITOR_SSL_KEYSTOREPASS
• Property.MONITOR_SSL_KEYPASS
• Property.MONITOR_SSL_TRUSTSTOREPASS

create new PropertyType.ADDRESS (or IP) for validating if a string is a valid IP format (ex: 0.0.0.0)

• Property.RPC_PROCESS_ADVERTISE_ADDRESS
• Property.GENERAL_PROCESS_BIND_ADDRESS
• Property.RPC_PROCESS_BIND_ADDRESS

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:

• Property.RPC_SSL_CLIENT_PROTOCOL
• Property.RPC_SSL_ENABLED_PROTOCOLS
• Property.MONITOR_SSL_INCLUDE_PROTOCOLS

---

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 all Properties of PropertyType.STRING always returns true to validate a string

[ctubbsii]

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.

[Amemeda]

I reviewed this TODO with Dom, and he said the required properties check in ServerConfigCheckRunner is already doing this/ or something close

[ctubbsii]

Right, but the null is still being allowed here in the property type validation. That's kind of my point. We are allowing all nulls to pass through here, and then check them later. The comment that was removed was suggesting that we could do this better by disallowing nulls here.

Consider the following, which roughly represents what we have today:

// implied validation from the type
MY_PROP_ENUM("key", PropertyType.MyType, "description");

The problem here is that PropertyType.MyType.isValidFormat() must return true if it's null, even for required properties, because the type validation doesn't know if the property is required or not.

Consider this alternative instead:

// explicit validation from the type, with an optional nullable; type no longer has to allow nulls
// alternatively, the type always allows nulls, but we explicitly say that it's not null in the explicit validator
MY_PROP_ENUM("key", PropertyType.MyType, PropertyType.MyType::isValidFormat, "description");
MY_PROP_ENUM2("key2", PropertyType.MyType2, Predicate.isNull().or(PropertyType.MyType::isValidFormat), "description");

Alternatively:

// stored the required bit with the property
MY_PROP_ENUM("key", PropertyType.MyType, /* required = */ true, "description");
// modify the PropertyType.isValidFormat()
public boolean isValidFormat(String string, boolean required) {
  // ensure non-null in here before passing to the type-specific predicate to test the non-null format
}

I think the implication here is that the required set needs to be removed, and replaced with either explicit per-property validation, or an extra per-property "required" boolean parameter to track which properties allow null/empty string.