pvillard31 commented on code in PR #11432:
URL: https://github.com/apache/nifi/pull/11432#discussion_r3604304227
##########
nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/flow/synchronization/StandardVersionedComponentSynchronizer.java:
##########
@@ -1653,11 +1655,13 @@ private Map<String, String> populatePropertiesMap(final
ComponentNode componentN
|| (versionedDescriptor != null &&
versionedDescriptor.getIdentifiesControllerService());
final boolean sensitive = (descriptor != null &&
descriptor.isSensitive())
|| (versionedDescriptor != null &&
versionedDescriptor.isSensitive());
-
+ final String proposedValue =
proposedProperties.get(propertyName);
+ final ParameterParser parameterParser = new
ExpressionLanguageAgnosticParameterParser();
+ final boolean proposedReferencingParameter =
parameterParser.parseTokens(proposedValue).iterator().hasNext();
Review Comment:
Would toReferenceList().isEmpty() be more precise here than
iterator().hasNext(), since the iterator also returns true for escape only
tokens such as ##?
##########
nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/flow/synchronization/StandardVersionedComponentSynchronizer.java:
##########
@@ -1653,11 +1655,13 @@ private Map<String, String> populatePropertiesMap(final
ComponentNode componentN
|| (versionedDescriptor != null &&
versionedDescriptor.getIdentifiesControllerService());
final boolean sensitive = (descriptor != null &&
descriptor.isSensitive())
|| (versionedDescriptor != null &&
versionedDescriptor.isSensitive());
-
+ final String proposedValue =
proposedProperties.get(propertyName);
+ final ParameterParser parameterParser = new
ExpressionLanguageAgnosticParameterParser();
+ final boolean proposedReferencingParameter =
parameterParser.parseTokens(proposedValue).iterator().hasNext();
Review 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?
##########
nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/flow/synchronization/StandardVersionedComponentSynchronizer.java:
##########
@@ -1653,11 +1655,13 @@ private Map<String, String> populatePropertiesMap(final
ComponentNode componentN
|| (versionedDescriptor != null &&
versionedDescriptor.getIdentifiesControllerService());
final boolean sensitive = (descriptor != null &&
descriptor.isSensitive())
|| (versionedDescriptor != null &&
versionedDescriptor.isSensitive());
-
+ final String proposedValue =
proposedProperties.get(propertyName);
+ final ParameterParser parameterParser = new
ExpressionLanguageAgnosticParameterParser();
Review 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?
##########
nifi-framework-bundle/nifi-framework/nifi-framework-components/src/test/java/org/apache/nifi/flow/synchronization/StandardVersionedComponentSynchronizerTest.java:
##########
@@ -1021,6 +1021,198 @@ public void
testExternalControllerServiceReferenceRemoved() throws FlowSynchroni
assertNull(properties.get("cs"));
}
+ @Test
+ @SuppressWarnings("unchecked")
Review 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?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
