NIFI-16114 Fix version change bug when controller service property has a parameter reference#11432
NIFI-16114 Fix version change bug when controller service property has a parameter reference#11432esecules wants to merge 1 commit into
Conversation
233d128 to
493021a
Compare
pvillard31
left a comment
pvillard31
left a comment
There was a problem hiding this comment.
Can you make sure that commit/PR follows the rules for the checks to execute properly? Also, did you consider a system test for it?
esecules
changed the title
…arameter dereference during version upgrade
@pvillard31 thanks for having a look. I've added a system test and I fixed up the commit message and PR title to match the rules. |
| || (versionedDescriptor != null && versionedDescriptor.isSensitive()); | ||
|
|
||
| final String proposedValue = proposedProperties.get(propertyName); | ||
| final ParameterParser parameterParser = new ExpressionLanguageAgnosticParameterParser(); |
There was a problem hiding this comment.
This parser is stateless and is created for every property on every synchronized component. Could it be a private static final field, or created once outside the loop?
|
|
||
| final String proposedValue = proposedProperties.get(propertyName); | ||
| final ParameterParser parameterParser = new ExpressionLanguageAgnosticParameterParser(); | ||
| final boolean proposedReferencingParameter = parameterParser.parseTokens(proposedValue).iterator().hasNext(); |
There was a problem hiding this comment.
proposedReferencingParameter is only read inside the referencesService branch. Could the parseTokens call move into that branch so non controller service properties are not parsed?
|
|
||
| final String proposedValue = proposedProperties.get(propertyName); | ||
| final ParameterParser parameterParser = new ExpressionLanguageAgnosticParameterParser(); | ||
| final boolean proposedReferencingParameter = parameterParser.parseTokens(proposedValue).iterator().hasNext(); |
There was a problem hiding this comment.
Would toReferenceList().isEmpty() be more precise here than iterator().hasNext(), since the iterator also returns true for escape only tokens such as ##?
| } | ||
|
|
||
| @Test | ||
| @SuppressWarnings("unchecked") |
There was a problem hiding this comment.
The test class already has a shared type safe propertiesCaptor field built with ArgumentCaptor.captor(). Could the new tests reuse it (or use captor()) so the @SuppressWarnings(unchecked) is not needed?
2 participants
Summary
NIFI-16114 Fix version change bug that caused the controller service properties which have parameter values to have those parameters dereferenced leading to immediate "local changes" in my versioned flow.
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000VerifiedstatusPull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation