pgyori commented on a change in pull request #5249:
URL: https://github.com/apache/nifi/pull/5249#discussion_r689668781
##########
File path:
nifi-nar-bundles/nifi-extension-utils/nifi-record-utils/nifi-standard-record-utils/src/main/java/org/apache/nifi/csv/CSVValidators.java
##########
@@ -25,15 +25,18 @@
import java.util.Set;
public class CSVValidators {
+ private static final Set<String> illegalChars = new HashSet<>();
- public static class SingleCharacterValidator implements Validator {
- private static final Set<String> illegalChars = new HashSet<>();
+ static {
+ illegalChars.add("\r");
+ illegalChars.add("\n");
+ }
- static {
- illegalChars.add("\r");
- illegalChars.add("\n");
- }
+ public static final Validator SINGLE_CHAR_VALIDATOR =
createSingleCharValidator(false);
+
+ public static final Validator EMPTY_AND_SINGLE_CHAR_VALIDATOR =
createSingleCharValidator(true);
Review comment:
I recommend naming it EMPTY_OR_SINGLE_CHAR_VALIDATOR as it contains a
logical OR in the implementation. When reading the code of CSVUtils, at
`.addValidator(CSVValidators.EMPTY_AND_SINGLE_CHAR_VALIDATOR)` I got a little
confused. My first impression was that the property is required to be empty and
contain a single character at the same time.
##########
File path:
nifi-nar-bundles/nifi-standard-services/nifi-record-serialization-services-bundle/nifi-record-serialization-services/src/test/java/org/apache/nifi/csv/TestCSVValidators.java
##########
@@ -33,48 +33,109 @@
/*** SingleCharValidator **/
@Test
public void testSingleCharNullValue() {
- CSVValidators.SingleCharacterValidator validator = new
CSVValidators.SingleCharacterValidator();
+ Validator validator = CSVValidators.SINGLE_CHAR_VALIDATOR;
ValidationContext mockContext = Mockito.mock(ValidationContext.class);
ValidationResult result = validator.validate("EscapeChar", null,
mockContext);
assertEquals("Input is null for this property",
result.getExplanation());
assertFalse(result.isValid());
}
+ @Test
+ public void testSingleCharEmptyValue() {
+ Validator validator = CSVValidators.SINGLE_CHAR_VALIDATOR;
+ ValidationContext mockContext = Mockito.mock(ValidationContext.class);
+ ValidationResult result = validator.validate("EscapeChar", "",
mockContext);
+ assertEquals("Value must be exactly 1 character but was 0 in length",
result.getExplanation());
+ assertFalse(result.isValid());
+ }
+
@Test
public void testSingleCharTab() {
- CSVValidators.SingleCharacterValidator validator = new
CSVValidators.SingleCharacterValidator();
+ Validator validator = CSVValidators.SINGLE_CHAR_VALIDATOR;
ValidationContext mockContext = Mockito.mock(ValidationContext.class);
ValidationResult result = validator.validate("EscapeChar", "\\t",
mockContext);
assertTrue(result.isValid());
}
@Test
public void testSingleCharIllegalChar() {
- CSVValidators.SingleCharacterValidator validator = new
CSVValidators.SingleCharacterValidator();
+ Validator validator = CSVValidators.SINGLE_CHAR_VALIDATOR;
ValidationContext mockContext = Mockito.mock(ValidationContext.class);
ValidationResult result = validator.validate("EscapeChar", "\\r",
mockContext);
assertEquals("\\r is not a valid character for this property",
result.getExplanation());
assertFalse(result.isValid());
}
+ @Test
+ public void testSingleCharExpressionLanguage() {
+ Validator validator = CSVValidators.SINGLE_CHAR_VALIDATOR;
+ ValidationContext mockContext = Mockito.mock(ValidationContext.class);
+
Mockito.when(mockContext.isExpressionLanguageSupported(Mockito.any())).thenReturn(true);
+
Mockito.when(mockContext.isExpressionLanguagePresent(Mockito.any())).thenReturn(true);
+ ValidationResult result = validator.validate("EscapeChar",
"${csv.escape}", mockContext);
+ assertTrue(result.isValid());
+ }
+
@Test
public void testSingleCharGoodChar() {
- CSVValidators.SingleCharacterValidator validator = new
CSVValidators.SingleCharacterValidator();
+ Validator validator = CSVValidators.SINGLE_CHAR_VALIDATOR;
ValidationContext mockContext = Mockito.mock(ValidationContext.class);
ValidationResult result = validator.validate("EscapeChar", "'",
mockContext);
assertTrue(result.isValid());
}
+ /*** Empty And SingleCharValidator **/
Review comment:
Being consistent with my other comment, I would replace "And" with "Or".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
