markap14 commented on code in PR #11581:
URL: https://github.com/apache/nifi/pull/11581#discussion_r3833568209
##########
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) {
Review Comment:
You read it correctly. Fixed in 9e5afff: defaults are now filled only for
required properties, so an optional controlling property like `SSL Mode` stays
unset and its dependent `Truststore Filename` remains gated off. Added
`testInheritingConfigurationDoesNotApplyOptionalPropertyDefault` covering
exactly the `SSL Mode` / `Truststore Filename` case.
##########
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:
Fixed in 9e5afff. `migrateProperties` now captures the persisted step names
and passes them in, so two cases can be told apart:
- A step that was persisted but dropped from the migrated map (the connector
called `removeStep(...)`) is not re-created.
- A declared step that appears in neither the persisted flow nor the
migrated map is genuinely new in this NAR version, so it is created with its
required defaults. Otherwise a NAR that adds a new required-with-default step
would leave the connector invalid. A new step is only created when at least one
required default actually applies, so we never materialize an empty step just
to fire a callback.
The `onConfigurationStepConfigured` callback that fires for a newly added
step is the same path a normally-configured step takes. Added tests for both
the removed-step and new-step cases.
##########
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);
Review Comment:
Kept it as a `LinkedHashMap` for consistency with `migrateProperties` and
`StandardConnectorPropertyConfiguration` so the migration path stays
deterministic. As noted in the other thread, the ordering isn't actually
load-bearing here since `ConnectorConfiguration` stores steps in a `HashSet`,
so nothing downstream depends on step order regardless.
--
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]
