ncover21 commented on code in PR #11581:
URL: https://github.com/apache/nifi/pull/11581#discussion_r3833040513
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/components/connector/StandardConnectorNode.java:
##########
@@ -422,8 +422,48 @@ private Map<String, StepConfiguration>
migrateProperties(final List<VersionedCon
final StandardConnectorPropertyConfiguration propertyConfiguration =
new StandardConnectorPropertyConfiguration(initial, this.toString());
try (final NarCloseable ignored =
NarCloseable.withComponentNarLoader(extensionManager,
getConnector().getClass(), getIdentifier())) {
getConnector().migrateProperties(propertyConfiguration);
+ return
applyMissingPropertyDefaults(propertyConfiguration.getMutatedProperties(),
getConnector().getConfigurationSteps());
}
- return propertyConfiguration.getMutatedProperties();
+ }
+
+ /**
+ * For each property declared on the Connector that has a default but no
value in the given configuration,
+ * inserts that default. This is needed when a Connector NAR adds a
property: the saved flow has no entry
+ * for it, so without filling in the default the Connector would be
invalid. Properties that already have a
+ * value, and properties that have no default, are left unchanged.
+ */
+ private Map<String, StepConfiguration> applyMissingPropertyDefaults(final
Map<String, StepConfiguration> migratedProperties, final
List<ConfigurationStep> configurationSteps) {
+ if (configurationSteps == null || configurationSteps.isEmpty()) {
+ return migratedProperties;
+ }
+
+ final Map<String, StepConfiguration> propertiesWithDefaults = new
HashMap<>(migratedProperties);
+ for (final ConfigurationStep configurationStep : configurationSteps) {
+ final Map<String, ConnectorValueReference> propertyValues = new
HashMap<>();
+ final StepConfiguration existingConfiguration =
propertiesWithDefaults.get(configurationStep.getName());
+ if (existingConfiguration != null &&
existingConfiguration.getPropertyValues() != null) {
+
propertyValues.putAll(existingConfiguration.getPropertyValues());
+ }
+
+ boolean appliedMissingDefault = false;
+ for (final ConnectorPropertyGroup propertyGroup :
configurationStep.getPropertyGroups()) {
+ for (final ConnectorPropertyDescriptor descriptor :
propertyGroup.getProperties()) {
+ if (propertyValues.containsKey(descriptor.getName()) ||
descriptor.getDefaultValue() == null) {
+ continue;
+ }
+
+ propertyValues.put(descriptor.getName(), new
StringLiteralValue(descriptor.getDefaultValue()));
+ appliedMissingDefault = true;
+ logger.debug("Applied default value for property [{}] of
configuration step [{}] on {}", descriptor.getName(),
configurationStep.getName(), this);
+ }
+ }
+
+ if (appliedMissingDefault) {
Review Comment:
Correct me if Im wrong here but this puts a brand new entry in when the
persisted flow had no such step at all, and that entry then feeds the
`notifyStepConfigured` loop in `inheritConfiguration`. So the connector gets
`onConfigurationStepConfigured` for a step it was never configured with, and
since the callback exception gets wrapped into a `RuntimeException` further
down, I think that would fail flow loading at startup rather than just leaving
the connector invalid.
Also if a connector calls `removeStep(...)` in `migrateProperties`, this
would put the step straight back with defaults. Would it be safer to only fill
defaults for steps already present in the migrated map?
--
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]
