You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "markap14 (via GitHub)" <gi...@apache.org> on 2026/08/21 18:58:37 UTC
[PR] NIFI-16242: Inherit default values for newly added Connector properties [nifi]
markap14 opened a new pull request, #11581:
URL: https://github.com/apache/nifi/pull/11581
## Summary
- After inheriting a saved Connector configuration, apply the NAR's default for any property that has no persisted value.
- This keeps a Connector valid across NAR upgrades that add a required property with a default, matching Processors, Controller Services, and Reporting Tasks.
- Jira: https://issues.apache.org/jira/browse/NIFI-16242
## Test plan
- [x] `TestStandardConnectorNode.testVerifyCanStartAfterInheritingConfigurationMissingRequiredPropertyWithDefault` inherits a saved step that omits a required property with a default, then asserts `verifyCanStart()` succeeds and the default is present
- [x] Confirmed that test fails without the change (`Repeat Count is required`) and passes with it
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-16242: Inherit default values for newly added Connector properties [nifi]
Posted by "markap14 (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-16242: Inherit default values for newly added Connector properties [nifi]
Posted by "bobpaulin (via GitHub)" <gi...@apache.org>.
bobpaulin commented on code in PR #11581:
URL: https://github.com/apache/nifi/pull/11581#discussion_r3845924503
##########
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:
Ok the tests adds clarity. I find the comment confusing then. Why is it important to the reader that "so inheriting a default cannot activate a dependent property"?
Can this comment be more clear that it's just going to set everything and the dependencies are really the irrelevant part?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-16242: Inherit default values for newly added Connector properties [nifi]
Posted by "ncover21 (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-16242: Inherit default values for newly added Connector properties [nifi]
Posted by "bobpaulin (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-16242: Inherit default values for newly added Connector properties [nifi]
Posted by "kevdoran (via GitHub)" <gi...@apache.org>.
kevdoran merged PR #11581:
URL: https://github.com/apache/nifi/pull/11581
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-16242: Inherit default values for newly added Connector properties [nifi]
Posted by "ncover21 (via GitHub)" <gi...@apache.org>.
ncover21 commented on code in PR #11581:
URL: https://github.com/apache/nifi/pull/11581#discussion_r3833054963
##########
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:
One thing im not sure if im reading correctly here, the defaults get applied without looking at property or step dependencies. `AbstractConnector.isDependencySatisfied` reads the controlling value through the name-based `getProperty(stepName, name)` overload, which never returns null, so an unset controlling property reads as null and the dependent property stays gated off.
So for a NAR that adds `SSL Mode` (default `REQUIRED`) plus a required `Truststore Filename` that dependsOn(SSL Mode, "REQUIRED"): before this change the connector stays valid because SSL Mode is unset, after it the default makes the dependency satisfied and `Truststore Filename` reports as required. That would be the opposite of the intent. Does that hold or am I misreading the dependency check?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-16242: Inherit default values for newly added Connector properties [nifi]
Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 commented on code in PR #11581:
URL: https://github.com/apache/nifi/pull/11581#discussion_r3833373889
##########
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:
I can use a LinkedHashMap just to maintain consistency. But in reality, tracing through the code, the assertion made by the comment is entirely false. LinkedHashMap is not needed. Which actually makes sense - if our interface says `Map` and it secretly depends on that `Map` having a specific order that would be a rather critical bug!
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-16242: Inherit default values for newly added Connector properties [nifi]
Posted by "markap14 (via GitHub)" <gi...@apache.org>.
markap14 commented on code in PR #11581:
URL: https://github.com/apache/nifi/pull/11581#discussion_r3846416667
##########
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:
Agreed — that sentence is leftover and the wrong thing to emphasize. This method just fills missing required properties that have a default. Reworded the comment in 7c7ef13 to describe that.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-16242: Inherit default values for newly added Connector properties [nifi]
Posted by "ncover21 (via GitHub)" <gi...@apache.org>.
ncover21 commented on code in PR #11581:
URL: https://github.com/apache/nifi/pull/11581#discussion_r3833029097
##########
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:
I think this needs to be a LinkedHashMap. `migrateProperties` builds initial as a LinkedHashMap with a comment about preserving persisted step order for the `notifyStepConfigured` loop at line 407, and copying into a `HashMap` reorders by hash. So a connector with steps Connection, Filter, Destination would get its onConfigurationStepConfigured callbacks in hash order on every flow load, but in declaration order when configured through the UI. If a later step reads values an earlier one established it would build a different managed flow after a restart.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-16242: Inherit default values for newly added Connector properties [nifi]
Posted by "markap14 (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org