Replace non-public Validators with AccumuloValidators#3370
Replace non-public Validators with AccumuloValidators#3370SethSmucker wants to merge 3 commits intointegrationfrom
Conversation
Create a local AccumuloValidators class to replace usage of non-public org.apache.accumulo.core.util.Validators in InMemoryNamespaceOperations and InMemoryTableOperations.
| public final class AccumuloValidators { | ||
|
|
||
| private AccumuloValidators() { | ||
| // utility class |
There was a problem hiding this comment.
I would throw an UnsupportedOperationException here.
| /** | ||
| * Validator for new table names. Checks that the name is non-null, non-empty, and contains only valid characters. | ||
| */ | ||
| public static final Validator NEW_TABLE_NAME = new Validator("new table name", name -> { |
There was a problem hiding this comment.
This implementation does not fully cover the checks performed by the validator returned from Validators.NEW_TABLE_NAME. That validator also checks for:
- Any table names beginning with a dot e.g.
.tablename - A blank namespace (e.g.
' .tablename') - A table name with a namespace and blank table, (e.g.
'namespace. ') - a table name that is longer that 1024 characters
Please add those checks here as well.
It would also be nice if the exception thrown by the Validator is detailed about what the error is. The Validator class implemented by Accumulo is a good example of how to do this by checking the Optional returned from the validate() function for an error description.
| /** | ||
| * Validator for new namespace names. | ||
| */ | ||
| public static final Validator NEW_NAMESPACE_NAME = new Validator("new namespace name", name -> { |
There was a problem hiding this comment.
Both Validators.NEW_NAMESPACE_NAME and Validators.VALID_NAME_PATTERN also check whether the namespace is more than 1024 characters.
| /** | ||
| * Validators for Accumulo table and namespace names. Replaces non-public class: {@code org.apache.accumulo.core.util.Validators} | ||
| */ | ||
| public final class AccumuloValidators { |
There was a problem hiding this comment.
Would like to see some unit tests for this class.
| /** | ||
| * A simple validator that checks a predicate and throws IllegalArgumentException if validation fails. | ||
| */ | ||
| public static class Validator { |
There was a problem hiding this comment.
Since this is a utility class, I think it would make more sense for Validator to not be a nested class here. We may end up wanting to utilize the Validator class outside of AccumuloValidators down the line.
| } | ||
|
|
||
| // Pattern for valid namespace/table name characters | ||
| private static final Pattern VALID_NAME_PATTERN = Pattern.compile("^[a-zA-Z0-9_]+$"); |
There was a problem hiding this comment.
This is equivalent to the regex pattern \w+, which is simpler.
- Throw UnsupportedOperationException in private constructor - Simplify regex to \w+
2 participants
Summary
org.apache.accumulo.core.util.Validatorsin InMemoryNamespaceOperations and InMemoryTableOperationsFixes #3357
Part of #2443