You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by MikeThomsen <gi...@git.apache.org> on 2018/05/31 23:40:17 UTC
[GitHub] nifi pull request #2749: NIFI-5145 Fixed evaluateAttributeExpressions in moc...
GitHub user MikeThomsen opened a pull request:
https://github.com/apache/nifi/pull/2749
NIFI-5145 Fixed evaluateAttributeExpressions in mockpropertyvalue to …
…make it handle null flowfiles.
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](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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/MikeThomsen/nifi NIFI-5145
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2749.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2749
----
commit db4114a653474d55a0bd8472e8f21bc755b8e2be
Author: Mike Thomsen <mi...@...>
Date: 2018-05-31T23:39:05Z
NIFI-5145 Fixed evaluateAttributeExpressions in mockpropertyvalue to make it handle null flowfiles.
----
---
[GitHub] nifi issue #2749: NIFI-5145 Fixed evaluateAttributeExpressions in mockproper...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/2749
Sorry, I didn't get a notification about this one. I've been working on some other things but will see if I can look at this tomorrow. It's 50/50 right now.
---
[GitHub] nifi issue #2749: NIFI-5145 Fixed evaluateAttributeExpressions in mockproper...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2749
@markap14 can you review? Since you brought this up on the dev list, here's the simple summary:
The test framework is too strict on processors where input is optional. In those cases, it's common to just call `evaluateExpressionLanguage` with a null flowfile, but the test framework won't allow that. So it reports false negatives on execution that would go through with live execution of the processors.
---
[GitHub] nifi issue #2749: NIFI-5145 Fixed evaluateAttributeExpressions in mockproper...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2749
@joewitt can you review?
---
[GitHub] nifi issue #2749: NIFI-5145 Fixed evaluateAttributeExpressions in mockproper...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2749
@mattyb149 @alopresto I backed out the FTP-related changes and put a temporary fix in there that handles the edge case that was impacting the FTP processors. There are two cases that cause trouble right now:
1. You pass a null flowfile to the `evaluateExpressionLanguage(FlowFile)` method while it is set to use flowfile attributes.
2. You pass a null flowfile to that method when it is set to use the variable registry.
Since, for the time being, the execution framework doesn't care about these things, I think this should work as a solution until that changes.
---
[GitHub] nifi pull request #2749: NIFI-5145 Fixed evaluateAttributeExpressions in moc...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2749#discussion_r192440580
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ListFileTransfer.java ---
@@ -43,7 +43,7 @@
.description("The fully qualified hostname or IP address of the remote system")
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.required(true)
- .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
--- End diff --
Your first link raises a good point about the FTP issue. Do you want to separate this into two tickets with the understanding that the immediate fix will break the master build (due to the FTP processors) but address the issue or do it all here?
@joewitt
---
[GitHub] nifi issue #2749: NIFI-5145 Fixed evaluateAttributeExpressions in mockproper...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2749
@alopresto can you review? See comment above.
---
[GitHub] nifi pull request #2749: NIFI-5145 Fixed evaluateAttributeExpressions in moc...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2749#discussion_r192455843
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ListFileTransfer.java ---
@@ -43,7 +43,7 @@
.description("The fully qualified hostname or IP address of the remote system")
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.required(true)
- .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
--- End diff --
I spent some time trying to untangle it and found that the reuse of some of these property descriptors between the FTP processors, combine with the processors having different input requirements wrecked havoc on the test framework. So I think we're going to need to commit the bare minimum fix here and do a separate ticket to refactor the FTP processors.
---
[GitHub] nifi issue #2749: NIFI-5145 Fixed evaluateAttributeExpressions in mockproper...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/2749
+1 LGTM, let's tackle the FTP stuff in a different Jira. Thanks for the improvement! Merging to master
---
[GitHub] nifi issue #2749: NIFI-5145 Fixed evaluateAttributeExpressions in mockproper...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2749
@mattyb149 Regarding your question about what is blocked by this PR, there are some integration tests in the Mongo package that break without this because the Mongo processors have optional input. Anything with optional input would break without this because you have to have the test framework take a laissez faire attitude toward null flowfiles just like the execution framework does. When no connection is there, all flowfiles passed in will be null and I don't think forcing a null check in each processor is as clean as funneling that null check into the evaluateExpressionLanguage methods.
---
[GitHub] nifi issue #2749: NIFI-5145 Fixed evaluateAttributeExpressions in mockproper...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/2749
What is the current state of this PR? The tests are passing but they were before that. Does ListFTP or some processor break without this fix? If not, one comment above implies something would be broken without another Jira/PR on its heels. I thought because of the issue between FTPTransfer and ListFTP's properties, the flow file attributes won't be honored?
---
[GitHub] nifi pull request #2749: NIFI-5145 Fixed evaluateAttributeExpressions in moc...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2749#discussion_r192408467
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ListFileTransfer.java ---
@@ -43,7 +43,7 @@
.description("The fully qualified hostname or IP address of the remote system")
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.required(true)
- .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
--- End diff --
According to [this](https://github.com/apache/nifi/pull/2749/files#diff-a68a345757d54ad30f79658062d6e794R76), the attributes will not be populated with the right information if the attributes come in from flow files as no FlowFile is passed in.
This is a weird thing because ListFTP declares a property HOSTNAME that had EL scope VARIABLE_REGISTRY. But it also creates an FTPTransfer object which declares a HOSTNAME property that has EL scope FLOWFILES. Because they have the same name, the context fetches the FTPTransfer property but gets ListFTP's property instead. Then it calls evaluateAttributeExpressions(flowFile), but that violates ListFTP's HOSTNAME EL scope.
This fix prevents the violation, but the attributes would still be incorrect right? I mention that because I thought of applying this same fix but instead put in the one from #2717
---
[GitHub] nifi issue #2749: NIFI-5145 Fixed evaluateAttributeExpressions in mockproper...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2749
@mattyb149 can we close this out?
---
[GitHub] nifi issue #2749: NIFI-5145 Fixed evaluateAttributeExpressions in mockproper...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2749
@joewitt can you review this commit?
---
[GitHub] nifi pull request #2749: NIFI-5145 Fixed evaluateAttributeExpressions in moc...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2749
---