markap14 commented on code in PR #11581:
URL: https://github.com/apache/nifi/pull/11581#discussion_r3834399378
##########
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:
Good question — rather than reason about it I wrote a test, and the current
behavior turns out to be safe because dependency evaluation is transitive. I
set up a step with an optional `Authentication`, a required `Username` (default
`admin`) that `dependsOn(Authentication, "Basic")`, and a required `Password`
(no default) that `dependsOn(Username)`. With `Authentication` unset, the
back-fill still materializes `Username=admin`, but `Password` stays irrelevant
and the connector remains valid.
The reason is `AbstractConnector.isDependencySatisfied`: when it evaluates
`Password` it recurses into `Username`'s own dependency, and since
`Authentication` is unset (the name-based `getProperty` returns null, no
default fallback), `Username` isn't relevant, so `Password` isn't either.
Materializing a gated-off property's default can't activate anything
downstream, because the downstream property is gated by the same unsatisfied
ancestor.
So filling a required-with-default property that's currently gated off is
harmless — validation ignores it and everything beneath it. Added
`testInheritingConfigurationKeepsTransitivelyGatedRequiredPropertyIrrelevant`
(5a5a34b) to lock this in.
##########
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:
Same answer as the property-dependency thread below:
`isStepDependencySatisfied` gates the whole step the same transitive way, so a
step whose dependency is unmet has all of its properties treated as irrelevant,
and materializing their defaults changes nothing. Covered by the test added in
5a5a34b.
##########
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:
Yes — a dependent property that is required, has a default, and is actually
relevant does get its default materialized, since the back-fill fills every
required-with-default property. If it's gated off it's irrelevant and doesn't
need a value. Both directions are covered by the test in 5a5a34b.
--
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]
