bobpaulin commented on code in PR #11581:
URL: https://github.com/apache/nifi/pull/11581#discussion_r3833746221
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/components/connector/StandardConnectorNode.java:
##########
@@ -419,11 +420,57 @@ private Map<String, StepConfiguration>
migrateProperties(final List<VersionedCon
initial.put(versionedConfigStep.getName(), new
StepConfiguration(toValueReferenceMap(versionedConfigStep)));
}
+ final Set<String> persistedStepNames = new
LinkedHashSet<>(initial.keySet());
final StandardConnectorPropertyConfiguration propertyConfiguration =
new StandardConnectorPropertyConfiguration(initial, this.toString());
try (final NarCloseable ignored =
NarCloseable.withComponentNarLoader(extensionManager,
getConnector().getClass(), getIdentifier())) {
getConnector().migrateProperties(propertyConfiguration);
+ return
applyMissingRequiredPropertyDefaults(propertyConfiguration.getMutatedProperties(),
persistedStepNames, getConnector().getConfigurationSteps());
}
- return propertyConfiguration.getMutatedProperties();
+ }
+
+ /**
+ * Fills in the default value for any required property that has no value
in the migrated configuration, so a NAR
+ * upgrade that adds a required property with a default does not make the
Connector invalid. Only required
+ * properties are filled, so inheriting a default cannot activate a
dependent property. A step the Connector
Review Comment:
So a dependent property could also be required and in this case wouldn't we
want the dependent property's default value to be set?
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/components/connector/StandardConnectorNode.java:
##########
@@ -419,11 +420,57 @@ private Map<String, StepConfiguration>
migrateProperties(final List<VersionedCon
initial.put(versionedConfigStep.getName(), new
StepConfiguration(toValueReferenceMap(versionedConfigStep)));
}
+ final Set<String> persistedStepNames = new
LinkedHashSet<>(initial.keySet());
final StandardConnectorPropertyConfiguration propertyConfiguration =
new StandardConnectorPropertyConfiguration(initial, this.toString());
try (final NarCloseable ignored =
NarCloseable.withComponentNarLoader(extensionManager,
getConnector().getClass(), getIdentifier())) {
getConnector().migrateProperties(propertyConfiguration);
+ return
applyMissingRequiredPropertyDefaults(propertyConfiguration.getMutatedProperties(),
persistedStepNames, getConnector().getConfigurationSteps());
}
- return propertyConfiguration.getMutatedProperties();
+ }
+
+ /**
+ * Fills in the default value for any required property that has no value
in the migrated configuration, so a NAR
+ * upgrade that adds a required property with a default does not make the
Connector invalid. Only required
+ * properties are filled, so inheriting a default cannot activate a
dependent property. A step the Connector
+ * removed during migration (present in {@code persistedStepNames} but
absent from {@code migratedProperties}) is
+ * not re-created; a declared step in neither is newly added by this
version and is created, but only if at least
+ * one required default applies to it.
+ */
+ private Map<String, StepConfiguration>
applyMissingRequiredPropertyDefaults(final Map<String, StepConfiguration>
migratedProperties,
+ final Set<String> persistedStepNames, final
List<ConfigurationStep> configurationSteps) {
+ if (configurationSteps == null || configurationSteps.isEmpty()) {
+ return migratedProperties;
+ }
+
+ final Map<String, StepConfiguration> propertiesWithDefaults = new
LinkedHashMap<>(migratedProperties);
+ for (final ConfigurationStep configurationStep : configurationSteps) {
Review Comment:
Should we also consider if the step dependency is satisfied?
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/components/connector/StandardConnectorNode.java:
##########
@@ -419,11 +420,57 @@ private Map<String, StepConfiguration>
migrateProperties(final List<VersionedCon
initial.put(versionedConfigStep.getName(), new
StepConfiguration(toValueReferenceMap(versionedConfigStep)));
}
+ final Set<String> persistedStepNames = new
LinkedHashSet<>(initial.keySet());
final StandardConnectorPropertyConfiguration propertyConfiguration =
new StandardConnectorPropertyConfiguration(initial, this.toString());
try (final NarCloseable ignored =
NarCloseable.withComponentNarLoader(extensionManager,
getConnector().getClass(), getIdentifier())) {
getConnector().migrateProperties(propertyConfiguration);
+ return
applyMissingRequiredPropertyDefaults(propertyConfiguration.getMutatedProperties(),
persistedStepNames, getConnector().getConfigurationSteps());
}
- return propertyConfiguration.getMutatedProperties();
+ }
+
+ /**
+ * Fills in the default value for any required property that has no value
in the migrated configuration, so a NAR
+ * upgrade that adds a required property with a default does not make the
Connector invalid. Only required
+ * properties are filled, so inheriting a default cannot activate a
dependent property. A step the Connector
+ * removed during migration (present in {@code persistedStepNames} but
absent from {@code migratedProperties}) is
+ * not re-created; a declared step in neither is newly added by this
version and is created, but only if at least
+ * one required default applies to it.
+ */
+ private Map<String, StepConfiguration>
applyMissingRequiredPropertyDefaults(final Map<String, StepConfiguration>
migratedProperties,
+ final Set<String> persistedStepNames, final
List<ConfigurationStep> configurationSteps) {
+ if (configurationSteps == null || configurationSteps.isEmpty()) {
+ return migratedProperties;
+ }
+
+ final Map<String, StepConfiguration> propertiesWithDefaults = new
LinkedHashMap<>(migratedProperties);
+ for (final ConfigurationStep configurationStep : configurationSteps) {
+ final String stepName = configurationStep.getName();
+ final StepConfiguration existingConfiguration =
propertiesWithDefaults.get(stepName);
+ if (existingConfiguration == null &&
persistedStepNames.contains(stepName)) {
+ continue;
+ }
+
+ final Map<String, ConnectorValueReference> existingValues =
existingConfiguration == null ? null :
existingConfiguration.getPropertyValues();
+ final Map<String, ConnectorValueReference> propertyValues =
existingValues == null ? new LinkedHashMap<>() : new
LinkedHashMap<>(existingValues);
+ boolean appliedMissingDefault = false;
+ for (final ConnectorPropertyGroup propertyGroup :
configurationStep.getPropertyGroups()) {
+ for (final ConnectorPropertyDescriptor descriptor :
propertyGroup.getProperties()) {
+ if (!descriptor.isRequired() ||
descriptor.getDefaultValue() == null ||
propertyValues.containsKey(descriptor.getName())) {
Review Comment:
The above considers if a specific property is required and has a default
value. If the property also has a dependency that is not met I assume it
should not be set.
--
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]
