NIFI-4242 Allow quote and escape chars for CSV to be 'undefined'.#2088
NIFI-4242 Allow quote and escape chars for CSV to be 'undefined'.#2088Wesley-Lawrence wants to merge 2 commits intoapache:masterfrom
Conversation
|
|
||
| @Override | ||
| public PropertyValue getProperty(final PropertyDescriptor property) { | ||
| String value = properties.get(property); |
There was a problem hiding this comment.
@Wesley-Lawrence I don't think this is really what we want here. The concept of a default value is 'use this if the value is null'. So changing this would be inconsistent with how the StandardProcessContext works as well. I think the more desirable approach would be to just update the SingleCharacterValidator being used such that we allow 0 or 1 character. Then, this would allow an empty string to be used as the delimiter, which I believe is equivalent.
|
@markap14 Thanks for the feedback, and my apologies for the delayed response. Hopefully this latest commit address your feedback. Thanks so much for taking the time to review these PRs 🙂 |
| public ValidationResult validate(final String subject, final String input, final ValidationContext context) { | ||
| final String unescaped = CSVUtils.unescape(input); | ||
| if (unescaped.length() != 1) { | ||
| if (allowEmpty && unescaped.length() > 1) { |
There was a problem hiding this comment.
This logic doesn't seem to be exactly right. If allowEmpty == true and unescaped.length() is 0, then this 'if clause' evaluates to false. So we will go to the next line, and unescaped.length() == 0 so it's invalid. As a result, when I tried to set Quote Character to empty string, the processor is invalid.
There was a problem hiding this comment.
Good catch. I'll have to figure out why the unit tests didn't catch that...
There was a problem hiding this comment.
Ah. The unit tests don't trigger SingleCharValidator.
|
@Wesley-Lawrence no problem. I think the logic is a bit off on the latest commit, though. Commented inline. |
|
@markap14 Alright, should be all fixed up now. I made sure to test out a live Flow this time, and make sure the functionality worked outside of just unit tests. |
|
@Wesley-Lawrence can you please rebase to resolve the conflicts? |
Wesley-Lawrence
force-pushed
the
NIFI-4242
branch
from
6ecfcee to
c1a5aa3
Compare
3 participants
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:
For documentation related changes:
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.