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/03 10:32:26 UTC
[GitHub] nifi pull request #2672: NIFI-5145 Made MockPropertyValue.evaluateExpression...
GitHub user MikeThomsen opened a pull request:
https://github.com/apache/nifi/pull/2672
NIFI-5145 Made MockPropertyValue.evaluateExpressionLanguage(FlowFile …
…able to accommodate null flowfiles the way live NiFi does.
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/2672.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 #2672
----
commit e717a2c68783815767d0b931f4c0d84fd6e6e0f7
Author: Mike Thomsen <mi...@...>
Date: 2018-05-03T10:28:45Z
NIFI-5145 Made MockPropertyValue.evaluateExpressionLanguage(FlowFile able to accommodate null flowfiles the way live NiFi does.
----
---
[GitHub] nifi pull request #2672: NIFI-5145 Made MockPropertyValue.evaluateExpression...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2672
---
[GitHub] nifi issue #2672: NIFI-5145 Made MockPropertyValue.evaluateExpressionLanguag...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/2672
I think the invalid scopes thing should be done in a separate Jira, there are more than just the ones above (~40 in total) so I think we should tackle them all at once, and have this PR just call evaluateAttributeExpressions with the empty map. I don't think that's a hack, it emulates the real behavior (although the real one has a ValueLookup whose constructor skips flow file processing if null).
---
[GitHub] nifi pull request #2672: NIFI-5145 Made MockPropertyValue.evaluateExpression...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2672#discussion_r188596962
--- Diff: nifi-nar-bundles/nifi-hbase-bundle/nifi-hbase-processors/src/main/java/org/apache/nifi/hbase/GetHBase.java ---
@@ -147,14 +146,6 @@
.allowableValues(NONE, CURRENT_TIME)
.defaultValue(NONE.getValue())
.build();
- static final PropertyDescriptor AUTHORIZATIONS = new PropertyDescriptor.Builder()
--- End diff --
Is this an unused property? I see it being removed but didn't see it moved anywhere
---
[GitHub] nifi issue #2672: NIFI-5145 Made MockPropertyValue.evaluateExpressionLanguag...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2672
@mattyb149 I just changed the override to be the one without a parameter. I agree that there should be a task for squashing the scope bugs, but do the few that got squashed here need to be cherry picked out or should they just stay in?
---
[GitHub] nifi issue #2672: NIFI-5145 Made MockPropertyValue.evaluateExpressionLanguag...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2672
@pvillard31 can you take a look?
---
[GitHub] nifi issue #2672: NIFI-5145 Made MockPropertyValue.evaluateExpressionLanguag...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/nifi/pull/2672
I think making this consistent for testing is a good idea. The fact that the Mock classes in nifi can replicate the runtime behavior is very important to implementors.
I think that the changes to the other processors might be better suited outside of this pr though.
Also, I wouldn't mind seeing a comment or more comments in the Mock class referencing the runtime behavior they are enforcing.
---
[GitHub] nifi pull request #2672: NIFI-5145 Made MockPropertyValue.evaluateExpression...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2672#discussion_r188710133
--- Diff: nifi-mock/src/main/java/org/apache/nifi/util/MockPropertyValue.java ---
@@ -202,6 +202,17 @@ public PropertyValue evaluateAttributeExpressions(final AttributeValueDecorator
@Override
public PropertyValue evaluateAttributeExpressions(final FlowFile flowFile) throws ProcessException {
+ /*
+ * TODO: Come up with a more elegation solution for this.
--- End diff --
I'll remove the TODO on merge
---
[GitHub] nifi pull request #2672: NIFI-5145 Made MockPropertyValue.evaluateExpression...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2672#discussion_r187425793
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/AbstractMongoProcessor.java ---
@@ -268,7 +268,7 @@ protected WriteConcern getWriteConcern(final ProcessContext context) {
protected void writeBatch(String payload, FlowFile parent, ProcessContext context, ProcessSession session,
Map<String, String> extraAttributes, Relationship rel) throws UnsupportedEncodingException {
String charset = parent != null ? context.getProperty(CHARSET).evaluateAttributeExpressions(parent).getValue()
--- End diff --
@MikeThomsen I may be wrong but i think this is supposed to be getting rid of the check for `parent != null`, isn't it? As-is, it's saying if parent != null then evaluate with parent flowfile, otherwise still evaluate with parent flowfile... should just be `String charset = context.getProperty(charset).evaluateAttributeExpressions(parent).getValue()`, no?
---
[GitHub] nifi issue #2672: NIFI-5145 Made MockPropertyValue.evaluateExpressionLanguag...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2672
[Added a Jira ticket](https://issues.apache.org/jira/browse/NIFI-5197) to track the issue with the invalid scope bugs.
---
[GitHub] nifi pull request #2672: NIFI-5145 Made MockPropertyValue.evaluateExpression...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2672#discussion_r188124392
--- Diff: nifi-mock/src/main/java/org/apache/nifi/util/MockPropertyValue.java ---
@@ -202,6 +203,9 @@ public PropertyValue evaluateAttributeExpressions(final AttributeValueDecorator
@Override
public PropertyValue evaluateAttributeExpressions(final FlowFile flowFile) throws ProcessException {
+ if (flowFile == null) {
--- End diff --
Sorry - don't think I was clear in what I was trying to communicate here. I do understand the intent. But it took me a while to understand how the PR differed logically from what already was there. What would seem natural to me is to simply pass the null FlowFile along to an override, filling in other default values, as it already does. But clearly that is a bug because of how this is handled in the actual implementing override. At first look, though, it seemed to me to be the same logic because in many places an empty map is treated the same as a null map.
So while I now understand what it's doing, I think it's a bit confusing at first glance. That's why i suggested passing a boolean down the stack. The PR wouldn't cause me much angst if there were just some inline comments, I think, about how the implementing override checks if flowfile is null and variables is null and if so handles things differently... but at that point it makes it really easy to change the implementation later and leave comments in the code that are no longer valid. but i'd be okay either way.
---
[GitHub] nifi pull request #2672: NIFI-5145 Made MockPropertyValue.evaluateExpression...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2672#discussion_r187470996
--- Diff: nifi-nar-bundles/nifi-mongodb-bundle/nifi-mongodb-processors/src/main/java/org/apache/nifi/processors/mongodb/AbstractMongoProcessor.java ---
@@ -268,7 +268,7 @@ protected WriteConcern getWriteConcern(final ProcessContext context) {
protected void writeBatch(String payload, FlowFile parent, ProcessContext context, ProcessSession session,
Map<String, String> extraAttributes, Relationship rel) throws UnsupportedEncodingException {
String charset = parent != null ? context.getProperty(CHARSET).evaluateAttributeExpressions(parent).getValue()
--- End diff --
Yeah, that was a typo on my parent. It should have been `evaluateAttributeExpressions()`
---
[GitHub] nifi pull request #2672: NIFI-5145 Made MockPropertyValue.evaluateExpression...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2672#discussion_r187427343
--- Diff: nifi-mock/src/main/java/org/apache/nifi/util/MockPropertyValue.java ---
@@ -202,6 +203,9 @@ public PropertyValue evaluateAttributeExpressions(final AttributeValueDecorator
@Override
public PropertyValue evaluateAttributeExpressions(final FlowFile flowFile) throws ProcessException {
+ if (flowFile == null) {
--- End diff --
This feels a little bit odd to me, as it's very unclear just from looking at this method what the intent is here - i think it would be cleaner to pass down a boolean argument that indicates whether or not attributes are available. It would be `true` in all cases except for `evaluateAttributeExpressions()` with no arguments and `evaluateAttributeExpressions(AttributeValueDecorator decorator)` - or otherwise to just have the no-arg version call the one that takes only the decorator and implement that method there instead of calling another override of the method. The actual implementation is only about 4-5 lines of code and quite trivial anyway.
---
[GitHub] nifi pull request #2672: NIFI-5145 Made MockPropertyValue.evaluateExpression...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2672#discussion_r188713507
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ListFileTransfer.java ---
@@ -43,20 +43,20 @@
.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 --
These are not evaluated against FF attributes (see the createAttributes() method below), I'll revert them on merge.
---
[GitHub] nifi issue #2672: NIFI-5145 Made MockPropertyValue.evaluateExpressionLanguag...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2672
@markap14 I added a detailed comment describing why I did what I did and fixed a few processors that had invalid scopes applied to their properties. Build seems totally fine now. Let me know if you need any more changes.
---
[GitHub] nifi issue #2672: NIFI-5145 Made MockPropertyValue.evaluateExpressionLanguag...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2672
@markap14 Any feedback?
---
[GitHub] nifi issue #2672: NIFI-5145 Made MockPropertyValue.evaluateExpressionLanguag...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on the issue:
https://github.com/apache/nifi/pull/2672
@pvillard31 @joewitt Could one of you do a code review on this? It aims to roll back a little of the new testing behavior that breaks evaluateExpressionLanguage(FlowFile) when the input is null. The work around to get the testing behavior to match the live behavior is [pretty hackish](https://issues.apache.org/jira/browse/NIFI-5145).
---
[GitHub] nifi issue #2672: NIFI-5145 Made MockPropertyValue.evaluateExpressionLanguag...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/2672
+1 LGTM, made a couple of edits before merging (see comments), thanks for the improvement! Merging to master
---
[GitHub] nifi pull request #2672: NIFI-5145 Made MockPropertyValue.evaluateExpression...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2672#discussion_r188126334
--- Diff: nifi-mock/src/main/java/org/apache/nifi/util/MockPropertyValue.java ---
@@ -202,6 +203,9 @@ public PropertyValue evaluateAttributeExpressions(final AttributeValueDecorator
@Override
public PropertyValue evaluateAttributeExpressions(final FlowFile flowFile) throws ProcessException {
+ if (flowFile == null) {
--- End diff --
Ok. I'll comment it up and make it clear what it's doing so anyone jumping in there later will understand exactly what's going on there.
---
[GitHub] nifi pull request #2672: NIFI-5145 Made MockPropertyValue.evaluateExpression...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2672#discussion_r187481205
--- Diff: nifi-mock/src/main/java/org/apache/nifi/util/MockPropertyValue.java ---
@@ -202,6 +203,9 @@ public PropertyValue evaluateAttributeExpressions(final AttributeValueDecorator
@Override
public PropertyValue evaluateAttributeExpressions(final FlowFile flowFile) throws ProcessException {
+ if (flowFile == null) {
--- End diff --
> This feels a little bit odd to me, as it's very unclear just from looking at this method what the intent is here
The intent is to mirror the behavior of running NiFi when you pass a null flowfile object into `evaluateExpressionLanguage(FlowFile)`. This works on a running processor:
```
FlowFile parent = null;
String charset = context.getProperty(CHARSET).evaluateAttributeExpressions(parent).getValue();
```
it does not work against the testing framework. So it seems natural to me that the testing framework just handle a null flowfile as an empty set of attributes the way live NiFi does.
---
[GitHub] nifi pull request #2672: NIFI-5145 Made MockPropertyValue.evaluateExpression...
Posted by MikeThomsen <gi...@git.apache.org>.
Github user MikeThomsen commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2672#discussion_r188631087
--- Diff: nifi-nar-bundles/nifi-hbase-bundle/nifi-hbase-processors/src/main/java/org/apache/nifi/hbase/GetHBase.java ---
@@ -147,14 +146,6 @@
.allowableValues(NONE, CURRENT_TIME)
.defaultValue(NONE.getValue())
.build();
- static final PropertyDescriptor AUTHORIZATIONS = new PropertyDescriptor.Builder()
--- End diff --
VisibilityFetchSupport provides a version of it that is fungible.
---