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.
---
