kevdoran commented on code in PR #11789:
URL: https://github.com/apache/nifi/pull/11789#discussion_r4224165766
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/components/connector/TestStandardConnectorRepository.java:
##########
@@ -1717,6 +1719,475 @@ public void
testSyncConnectorProviderAllowWithWorkingConfig() throws Exception {
verify(connector).inheritConfiguration(any(), any(), any());
}
+ @Test
+ public void testSyncConnectorSavesMigratedProviderConfiguration() throws
Exception {
+ final ConnectorConfigurationProvider provider =
mock(ConnectorConfigurationProvider.class);
+ final VersionedConfigurationStep persistedStep =
createVersionedStep("step1",
+ Map.of("Legacy Property",
createStringLiteralRef("configured-value")));
+ final VersionedConfigurationStep migratedStep =
createVersionedStep("step1",
+ Map.of("Current Property",
createStringLiteralRef("configured-value")));
+ final ConnectorWorkingConfiguration providerConfig = new
ConnectorWorkingConfiguration();
+ providerConfig.setName("Provider Name");
+ providerConfig.setWorkingFlowConfiguration(List.of(persistedStep));
+ final ConnectorWorkingConfiguration migratedProviderConfig = new
ConnectorWorkingConfiguration();
+ migratedProviderConfig.setName("Provider Name");
+
migratedProviderConfig.setWorkingFlowConfiguration(List.of(migratedStep));
+ when(provider.getSyncDirective(eq("connector-1"), any()))
+ .thenReturn(ConnectorSyncDirective.allow(providerConfig),
ConnectorSyncDirective.allow(migratedProviderConfig));
+ final StandardConnectorRepository repository =
createRepositoryWithProvider(provider);
+
+ final ConnectorNode connector =
createConnectorNodeWithEmptyWorkingConfig("connector-1", "Test Connector");
+ when(connector.getCurrentState()).thenReturn(ConnectorState.STOPPED);
+
when(connector.migrateConfiguration(List.of(persistedStep))).thenReturn(List.of(migratedStep));
+ repository.restoreConnector(connector);
+
+ final VersionedConnector versioned =
createVersionedConnector("connector-1", "Test Connector",
VersionedConnectorState.ENABLED, List.of());
+ final ConnectorSyncResult result = repository.syncConnector(versioned);
+ verify(connector).inheritConfiguration(any(),
eq(List.of(persistedStep)), any());
+
+ repository.syncConnector(versioned);
+
+ assertEquals(ConnectorSyncResult.Outcome.SYNCED, result.getOutcome());
+ final ArgumentCaptor<ConnectorWorkingConfiguration>
configurationCaptor =
ArgumentCaptor.forClass(ConnectorWorkingConfiguration.class);
+ verify(provider).save(eq("connector-1"),
configurationCaptor.capture());
Review Comment:
[openai-gpt-6-astra] With the read-only synchronization direction, this
assertion and the migration-save retry test below encode the behavior we are
removing. Please replace them with a sequence that leaves the provider document
unchanged across repeated reads/synchronization while checking that the
in-memory configuration is migrated and usable, and verifies `provider.save()`
is never called. Then perform an explicit configuration save and assert the
provider receives the migrated/filtered document, including when the user edits
a different step. Reload the saved document to verify the old name does not
reappear. The existing provider-backed system-test fixture in #11776 looks like
a useful place to exercise this beyond mocked save calls.
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/components/connector/StandardConnectorNode.java:
##########
@@ -479,26 +479,43 @@ private WorkingFlowContextState
installReplacementWorkingFlowContext(final Mutab
private Map<String, StepConfiguration> migrateProperties(final
List<VersionedConfigurationStep> flowConfiguration) {
// Preserve persisted step order so the notifyStepConfigured loop in
inheritConfiguration fires in a
- // deterministic order matching the flow definition.
+ // deterministic order matching the flow definition. A serialized flow
omits a configuration list that was
+ // never written, so a missing list is an empty configuration.
+ final List<VersionedConfigurationStep> persistedConfiguration =
flowConfiguration == null ? List.of() : flowConfiguration;
final Map<String, StepConfiguration> initial = new LinkedHashMap<>();
- for (final VersionedConfigurationStep versionedConfigStep :
flowConfiguration) {
+ for (final VersionedConfigurationStep versionedConfigStep :
persistedConfiguration) {
initial.put(versionedConfigStep.getName(), new
StepConfiguration(toValueReferenceMap(versionedConfigStep)));
}
final Set<String> persistedStepNames = new
LinkedHashSet<>(initial.keySet());
+ final Map<String, StepConfiguration> migratedProperties =
migratePersistedProperties(initial);
+ return applyMissingRequiredPropertyDefaults(migratedProperties,
persistedStepNames, getConnector().getConfigurationSteps());
+ }
+
+ private Map<String, StepConfiguration> migratePersistedProperties(final
Map<String, StepConfiguration> initial) {
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
retainDeclaredProperties(propertyConfiguration.getMutatedProperties(),
getConnector().getConfigurationSteps());
}
}
+ @Override
+ public List<VersionedConfigurationStep> migrateConfiguration(final
List<VersionedConfigurationStep> flowConfiguration) {
+ final List<VersionedConfigurationStep> persistedConfiguration =
flowConfiguration == null ? List.of() : flowConfiguration;
+ final Map<String, StepConfiguration> initial = new LinkedHashMap<>();
+ for (final VersionedConfigurationStep versionedConfigStep :
persistedConfiguration) {
+ initial.put(versionedConfigStep.getName(), new
StepConfiguration(toValueReferenceMap(versionedConfigStep)));
+ }
+
+ return
toVersionedConfigurationSteps(migratePersistedProperties(initial));
Review Comment:
[openai-gpt-6-astra] Please also use this defaults-free migration path when
preparing an explicit provider save. Removing the sync-time save alone does not
persist the migrated names on the next user save:
`StandardConnectorRepository.buildMergedWorkingConfiguration()` reloads the raw
provider document and merges the submitted step without invoking migration. On
this head, saving another step therefore retains the legacy key. After
incorporating #11776, filtering that raw document first would instead discard
the legacy key before its value can be renamed.
Suggested order: load the provider snapshot -> migrate its properties/steps
-> filter undeclared entries -> overlay declared user edits -> save. Migration
must precede filtering, and user edits must take precedence over migrated
values. Keep framework-added required defaults separate from this persisted
representation. Please cover saving an unrelated step after a rename, and an
explicit new-name edit overriding the migrated value.
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/components/connector/TestStandardConnectorNode.java:
##########
@@ -1388,6 +1482,70 @@ public void
testVerifyCanStartAfterInheritingConfigurationMissingRequiredPropert
assertEquals("1",
connectorNode.getActiveFlowContext().getConfigurationContext().getProperty("settings",
"Repeat Count").getValue());
}
+ @Test
+ public void
testMigrateProviderConfigurationExcludesRequiredDefaultsFromPersistedConfiguration()
throws FlowUpdateException {
+ final DefaultValueConnector connector = new DefaultValueConnector() {
+ @Override
+ public void migrateProperties(final ConnectorPropertyConfiguration
configuration) {
+ configuration.forStep("settings").renameProperty("Legacy
Greeting", "Greeting");
+ }
+ };
+ final StandardConnectorNode connectorNode =
createConnectorNode(connector);
+
+ final VersionedConnectorValueReference greetingReference = new
VersionedConnectorValueReference();
+ greetingReference.setValueType("STRING_LITERAL");
+ greetingReference.setValue("Welcome");
+ final VersionedConnectorValueReference obsoleteReference = new
VersionedConnectorValueReference();
+ obsoleteReference.setValueType("STRING_LITERAL");
+ obsoleteReference.setValue("obsolete");
+
+ final VersionedConfigurationStep persistedStep = new
VersionedConfigurationStep();
+ persistedStep.setName("settings");
+ persistedStep.setProperties(Map.of(
+ "Legacy Greeting", greetingReference,
+ "Obsolete Property", obsoleteReference));
+
+ final List<VersionedConfigurationStep> migrated =
connectorNode.migrateConfiguration(List.of(persistedStep));
+
+ assertEquals(1, migrated.size());
+ assertEquals(Set.of("Greeting"),
migrated.getFirst().getProperties().keySet());
+ assertEquals("Welcome",
migrated.getFirst().getProperties().get("Greeting").getValue());
+
+ connectorNode.replaceWorkingConfiguration(List.of(persistedStep));
Review Comment:
[openai-gpt-6-astra] This covers the normalized working values, but please
also exercise the provider reload -> prepareForUpdate -> apply/validation path
with an immutable required-with-default property. The positive case is active
`STANDARD` plus an omitted working property defaulting to `STANDARD`; the
negative case is active `LEGACY` plus the same omission, which must still be
rejected as a real change. Backfilling must not mean retaining any previous
active value or bypassing immutability.
Please include repeated reloads of the older provider document, an entirely
absent newly declared step with required defaults, and the
optional-default/dependency guard through the provider replacement path. The
focused tests in #11788 can inform that coverage without requiring both
implementations to be merged.
--
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]
