NIFI-4416 CSVRecordReader does not accept escaped character as delimiter #2172

arunma · 2017-09-25T14:35:58Z

Thank you for submitting a contribution to Apache NiFi.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically master)?
Is your initial contribution a single, squashed commit?

For code changes:

Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.

mattyb149 · 2017-09-25T16:45:07Z

...ndle/nifi-record-serialization-services/src/main/java/org/apache/nifi/csv/CSVValidators.java

+
+        @Override
+        public ValidationResult validate(final String subject, final String input, final ValidationContext context) {
+            final String unescaped = CSVUtils.unescape(input);


Currently it appears that the properties that use this validator require a value, which means "input" will never be null. However the PR #2088 aims to allow for these to be undefined, which will cause a NullPointerException here. Best practice here would be to check for null input and return invalid. You might not be able to test that logic from the CSVRecordReader test, but a quick unit test for this class would suffice.

Wow. Wasn't aware of the other PR. Thanks @mattyb149 - Will make the null checks for the cases you mentioned.

mattyb149 · 2017-09-25T16:45:41Z

...ndle/nifi-record-serialization-services/src/main/java/org/apache/nifi/csv/CSVValidators.java

+    public static final Validator UNESCAPED_SINGLE_CHAR_VALIDATOR = new Validator() {
+        @Override
+        public ValidationResult validate(String subject, String input, ValidationContext context) {
+            input = unescapeString(input);


Same comment here for defensive code to avoid NPEs.

Got it Matt, Made this change as well. Figured that I was mutating the argument, avoided that as well now.

mattyb149 · 2017-09-25T16:45:45Z

...ndle/nifi-record-serialization-services/src/main/java/org/apache/nifi/csv/CSVValidators.java

+        }
+
+        private String unescapeString(String input) {
+            if (input.length() > 1) {


Same comment here for defensive code to avoid NPEs.

Added up this as well.

Also added testcases for the above NPE and other validation usecases.

mattyb149 · 2017-09-25T17:02:49Z

...nifi-record-serialization-services/src/test/resources/csv/multi-bank-account_escapedchar.csv

@@ -0,0 +1,3 @@
+idnamebalanceaddresscitystatezipCodecountry


All files need an Apache 2.0 License header, or if that is not feasible due to the nature/format of the file, it must be added to the Apache Rat exclusion list in the nifi-record-serialization-services POM.

Sorry. My bad. I ran an entire contrib check skipping rat checks. Added the new CSV to the exclusion list.

Did that with rat enabled for this bundle alone (for some reason rat checks fail at the parent level level - for nifi-commons).

arunma · 2017-09-25T18:02:28Z

@mattyb149 Squashed the commits with the review changes. Please let me know if I've been sloppy anywhere.

mattyb149 · 2017-09-25T20:01:23Z

...ndle/nifi-record-serialization-services/src/main/java/org/apache/nifi/csv/CSVValidators.java

+                    .subject(subject)
+                    .input(unescapeString)
+                    .explanation("Only non-null single characters are supported")
+                    .valid((input.length() == 1 && input.charAt(0) != 0) || context.isExpressionLanguagePresent(input))


Shouldn't this be checking unescapeString instead of input? If I put "\u0001" as the value of the character, unescapeString() gets the correct value, but then the property is marked invalid because the original input was 6 characters long. The SingleCharacterValidator checks the "unescaped" variable instead of the original input, for this reason.

@mattyb149 Terribly sorry about this. This is super embarrassing. Was wondering how the testcases failed me. It should have been "\\u0001" instead of "\u0001". Fixed the testcase and the class. Will run a manual test on the UI as well before pushing the changes.

ValidationResult result = validator.validate("Delimiter", "\\u0001", mockContext);

I hadn't touched the unit test, and I am having trouble pushing my branch, so please feel free to update and run your tests, and I will use this PR as-is.

Ironically I had been trying to emulate your unit test by pasting in the actual character for \u0001, but that doesn't make it through, an empty string gets passed to the validator. That's when I tried the actual string "\u0001" and how I found the issue.

mattyb149 · 2017-09-25T20:35:35Z

I took the liberty of fixing the unescapeString issue above and verified that the PR's intended functionality is working (tried a Unicode escape character as a delimiter). I will add my commit to the merge. Thanks for the improvement! Merging to master

arunma · 2017-09-25T20:56:41Z

@mattyb149 Tested it against the NiFi UI (after making the changes for your last review comment) and converted a bunch of records from CSV to JSON with \u0001 as the delimiter. Looks good to me.

mattyb149 · 2017-09-25T21:18:18Z

Yes please, let's use yours :)

arunma · 2017-09-26T01:55:57Z

@mattyb149 Hahaha. Thanks a lot, Matt.

Ran a UI check
Ran contrib-check clean install
Fixed the testcase to reflect the last review.
Add a rat exclusion and ran a install without rat skipping for the record-serialization bundle alone. The root build still fails with rat on.

mattyb149 · 2017-09-26T02:33:57Z

+1 LGTM, ran the unit tests and with a live NiFi, verified I could use Unicode and other escaped character(s) as a delimiter. Thanks again! This time I'm really merging to master :)

arunma · 2017-09-26T02:36:27Z

@mattyb149 Really appreciate your time and patience, Matt. I learnt quite a lot from your comments.

mattyb149 reviewed Sep 25, 2017

View reviewed changes

NIFI-4416 CSVRecordReader does not accept escaped character as delimiter

05e002f

asfgit closed this in 3b1b326 Sep 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NIFI-4416 CSVRecordReader does not accept escaped character as delimiter #2172

NIFI-4416 CSVRecordReader does not accept escaped character as delimiter #2172

arunma commented Sep 25, 2017

mattyb149 Sep 25, 2017

arunma Sep 25, 2017

mattyb149 Sep 25, 2017

arunma Sep 25, 2017

mattyb149 Sep 25, 2017

arunma Sep 25, 2017

arunma Sep 25, 2017

mattyb149 Sep 25, 2017

arunma Sep 25, 2017

arunma commented Sep 25, 2017

mattyb149 Sep 25, 2017

arunma Sep 25, 2017 •

edited

mattyb149 Sep 25, 2017

mattyb149 Sep 25, 2017

mattyb149 commented Sep 25, 2017

arunma commented Sep 25, 2017 •

edited

mattyb149 commented Sep 25, 2017

arunma commented Sep 26, 2017

mattyb149 commented Sep 26, 2017

arunma commented Sep 26, 2017

NIFI-4416 CSVRecordReader does not accept escaped character as delimiter #2172

NIFI-4416 CSVRecordReader does not accept escaped character as delimiter #2172

Conversation

arunma commented Sep 25, 2017

For all changes:

For code changes:

For documentation related changes:

Note:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunma commented Sep 25, 2017

Choose a reason for hiding this comment

arunma Sep 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattyb149 commented Sep 25, 2017

arunma commented Sep 25, 2017 • edited

mattyb149 commented Sep 25, 2017

arunma commented Sep 26, 2017

mattyb149 commented Sep 26, 2017

arunma commented Sep 26, 2017

arunma Sep 25, 2017 •

edited

arunma commented Sep 25, 2017 •

edited