You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2020/01/23 14:46:42 UTC
[GitHub] [nifi] ottobackwards opened a new pull request #4012: NIFI-7055
createListValidator should treat ", " as invalid
ottobackwards opened a new pull request #4012: NIFI-7055 createListValidator should treat "," as invalid
URL: https://github.com/apache/nifi/pull/4012
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
#### Description of PR
fixes bug NIFI-7055 -
An empty set returned by split of "," was returning as valid.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] 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.
- [x] Has your PR been rebased against the latest commit within the target branch (typically `master`)?
- [x] Is your initial contribution a single, squashed commit? _Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not `squash` or use `--force` when pushing to allow for clean monitoring of changes._
### For code changes:
- [x] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [x] Have you written or updated unit tests to verify your changes?
- [ ] Have you verified that the full build is successful on both JDK 8 and JDK 11?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] ottobackwards commented on issue #4012: NIFI-7055
createListValidator should treat ", " as invalid
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on issue #4012: NIFI-7055 createListValidator should treat "," as invalid
URL: https://github.com/apache/nifi/pull/4012#issuecomment-582725361
No, @jfrazee, the issue is that "," will return valid, and never be passed to the empty validator, or any validator you have specified.
This PR changes it so that it treats ",", which splits to a 0 length array ( IE Nothing to validate ) as if it returned nothing
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] ottobackwards commented on issue #4012: NIFI-7055
createListValidator should treat ", " as invalid
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on issue #4012: NIFI-7055 createListValidator should treat "," as invalid
URL: https://github.com/apache/nifi/pull/4012#issuecomment-582897267
Ok, so " , " evaluates to a list of two, so this fix will not effect it
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] ottobackwards commented on a change in pull request #4012:
NIFI-7055 createListValidator should treat ", " as invalid
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on a change in pull request #4012: NIFI-7055 createListValidator should treat "," as invalid
URL: https://github.com/apache/nifi/pull/4012#discussion_r384593057
##########
File path: nifi-commons/nifi-utils/src/test/java/org/apache/nifi/util/validator/TestStandardValidators.java
##########
@@ -184,6 +184,25 @@ public void testListValidator() {
assertFalse(vr.isValid());
assertEquals(1, mockValidator.getValidateCallCount());
+ // An empty list is the same as null, "" or " "
+ vr = val.validate("List", ",", validationContext);
+ assertFalse(vr.isValid());
+ assertEquals(0, mockValidator.getValidateCallCount());
+
+ vr = val.validate("List", " , ", validationContext);
+ assertFalse(vr.isValid());
+ assertEquals(1, mockValidator.getValidateCallCount());
+
+ // will evaluate to no entry
+ vr = val.validate("List", ",,,,", validationContext);
+ assertFalse(vr.isValid());
+ assertEquals(0, mockValidator.getValidateCallCount());
+
+ // will evaluate to an empty element
+ vr = val.validate("List", ",foo", validationContext);
Review comment:
No. The way it works is each item in a produced list is passed to the embedded validator.
In this case, the split produces ["","foo"]. 2 elements.
The first element is passed to the non-empty validator and fails.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] ottobackwards commented on issue #4012: NIFI-7055
createListValidator should treat ", " as invalid
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on issue #4012: NIFI-7055 createListValidator should treat "," as invalid
URL: https://github.com/apache/nifi/pull/4012#issuecomment-582900887
https://issues.apache.org/jira/browse/NIFI-7109
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] mattyb149 commented on a change in pull request #4012:
NIFI-7055 createListValidator should treat ", " as invalid
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on a change in pull request #4012: NIFI-7055 createListValidator should treat "," as invalid
URL: https://github.com/apache/nifi/pull/4012#discussion_r384597165
##########
File path: nifi-commons/nifi-utils/src/test/java/org/apache/nifi/util/validator/TestStandardValidators.java
##########
@@ -184,6 +184,25 @@ public void testListValidator() {
assertFalse(vr.isValid());
assertEquals(1, mockValidator.getValidateCallCount());
+ // An empty list is the same as null, "" or " "
+ vr = val.validate("List", ",", validationContext);
+ assertFalse(vr.isValid());
+ assertEquals(0, mockValidator.getValidateCallCount());
+
+ vr = val.validate("List", " , ", validationContext);
+ assertFalse(vr.isValid());
+ assertEquals(1, mockValidator.getValidateCallCount());
+
+ // will evaluate to no entry
+ vr = val.validate("List", ",,,,", validationContext);
+ assertFalse(vr.isValid());
+ assertEquals(0, mockValidator.getValidateCallCount());
+
+ // will evaluate to an empty element
+ vr = val.validate("List", ",foo", validationContext);
Review comment:
Ah I see, it's because you're using a non-empty validator for each element.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] ottobackwards commented on issue #4012: NIFI-7055
createListValidator should treat ", " as invalid
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on issue #4012: NIFI-7055 createListValidator should treat "," as invalid
URL: https://github.com/apache/nifi/pull/4012#issuecomment-582899667
part of the issue here is our testing framework. we can't tell in the tests if something is not valid because of the validator or because of other reasons. IE> if the validator for items was called.
I'm going to create a jira for that
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] jfrazee commented on issue #4012: NIFI-7055
createListValidator should treat ", " as invalid
Posted by GitBox <gi...@apache.org>.
jfrazee commented on issue #4012: NIFI-7055 createListValidator should treat "," as invalid
URL: https://github.com/apache/nifi/pull/4012#issuecomment-582734346
I just wanted to confirm that you're leaving " , " (with spaces) to be handled inside the loop according to the the trim and exclude?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] ottobackwards commented on a change in pull request #4012:
NIFI-7055 createListValidator should treat ", " as invalid
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on a change in pull request #4012: NIFI-7055 createListValidator should treat "," as invalid
URL: https://github.com/apache/nifi/pull/4012#discussion_r384669241
##########
File path: nifi-commons/nifi-utils/src/test/java/org/apache/nifi/util/validator/TestStandardValidators.java
##########
@@ -184,6 +184,25 @@ public void testListValidator() {
assertFalse(vr.isValid());
assertEquals(1, mockValidator.getValidateCallCount());
+ // An empty list is the same as null, "" or " "
+ vr = val.validate("List", ",", validationContext);
+ assertFalse(vr.isValid());
+ assertEquals(0, mockValidator.getValidateCallCount());
+
+ vr = val.validate("List", " , ", validationContext);
+ assertFalse(vr.isValid());
+ assertEquals(1, mockValidator.getValidateCallCount());
+
+ // will evaluate to no entry
+ vr = val.validate("List", ",,,,", validationContext);
+ assertFalse(vr.isValid());
+ assertEquals(0, mockValidator.getValidateCallCount());
+
+ // will evaluate to an empty element
+ vr = val.validate("List", ",foo", validationContext);
Review comment:
Yes, that is how the tests were already
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] mattyb149 commented on a change in pull request #4012:
NIFI-7055 createListValidator should treat ", " as invalid
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on a change in pull request #4012: NIFI-7055 createListValidator should treat "," as invalid
URL: https://github.com/apache/nifi/pull/4012#discussion_r384584531
##########
File path: nifi-commons/nifi-utils/src/test/java/org/apache/nifi/util/validator/TestStandardValidators.java
##########
@@ -184,6 +184,25 @@ public void testListValidator() {
assertFalse(vr.isValid());
assertEquals(1, mockValidator.getValidateCallCount());
+ // An empty list is the same as null, "" or " "
+ vr = val.validate("List", ",", validationContext);
+ assertFalse(vr.isValid());
+ assertEquals(0, mockValidator.getValidateCallCount());
+
+ vr = val.validate("List", " , ", validationContext);
+ assertFalse(vr.isValid());
+ assertEquals(1, mockValidator.getValidateCallCount());
+
+ // will evaluate to no entry
+ vr = val.validate("List", ",,,,", validationContext);
+ assertFalse(vr.isValid());
+ assertEquals(0, mockValidator.getValidateCallCount());
+
+ // will evaluate to an empty element
+ vr = val.validate("List", ",foo", validationContext);
Review comment:
Since this list has one non-empty element, should it be valid?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] ottobackwards commented on issue #4012: NIFI-7055
createListValidator should treat ", " as invalid
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on issue #4012: NIFI-7055 createListValidator should treat "," as invalid
URL: https://github.com/apache/nifi/pull/4012#issuecomment-582725676
In other words, even if you had trim entries, and exclude empties, that is not honored in the case of ",", because that code executes inside the for loop of split results.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] woutifier-t commented on issue #4012: NIFI-7055
createListValidator should treat ", " as invalid
Posted by GitBox <gi...@apache.org>.
woutifier-t commented on issue #4012: NIFI-7055 createListValidator should treat "," as invalid
URL: https://github.com/apache/nifi/pull/4012#issuecomment-578808674
Looks like a sane, minimal change.
(The idea is to add a separate new createListValidator employing the .split(",", -1) fix under NIFI-7063.)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] mattyb149 commented on issue #4012: NIFI-7055
createListValidator should treat ", " as invalid
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on issue #4012: NIFI-7055 createListValidator should treat "," as invalid
URL: https://github.com/apache/nifi/pull/4012#issuecomment-594936311
+1 LGTM, ran contrib-check and tested with various list entries. Thanks for the fix! Merging to master
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] jfrazee commented on issue #4012: NIFI-7055
createListValidator should treat ", " as invalid
Posted by GitBox <gi...@apache.org>.
jfrazee commented on issue #4012: NIFI-7055 createListValidator should treat "," as invalid
URL: https://github.com/apache/nifi/pull/4012#issuecomment-582626469
@ottobackwards If I read the JIRA correctly " , " is supposed to stay valid for this PR, is that right? If that's the case then LGTM +1
I read through every use of it in the framework to see what the impact would be. The only uses of the validator in the framework have trimEntries and excludeEmptyEntries set to true. And most uses (CaptureChangeMySQL, LivySessionController, DBCPConnectionPool, AbstractAzureLogAnalyticsReportingTask/AzureLogAnalyticsReportingTask) re-validate or re-split and re-validate before using the value.
Only JMSConnectionFactoryProvider and JndiJmsConnectionFactoryProvider don't re-validate.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] ottobackwards commented on issue #4012: NIFI-7055
createListValidator should treat ", " as invalid
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on issue #4012: NIFI-7055 createListValidator should treat "," as invalid
URL: https://github.com/apache/nifi/pull/4012#issuecomment-582868177
I'll write an explicit test for that " , ". I just handled the empty list, so if that did not produce an empty list before, it won't change the behavior. If it _did_ produce and empty list before, then yes, it will not be not valid
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] asfgit closed pull request #4012: NIFI-7055
createListValidator should treat ", " as invalid
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4012: NIFI-7055 createListValidator should treat "," as invalid
URL: https://github.com/apache/nifi/pull/4012
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services