pvillard31 commented on code in PR #11399: URL: https://github.com/apache/nifi/pull/11399#discussion_r3641109712
########## nifi-connector-mock-bundle/nifi-connector-mock/src/main/java/org/apache/nifi/mock/connector/migration/MockConnectorPropertyConfiguration.java: ########## @@ -0,0 +1,354 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.nifi.mock.connector.migration; + +import org.apache.nifi.components.connector.ConnectorValueReference; +import org.apache.nifi.components.connector.StringLiteralValue; +import org.apache.nifi.migration.ConnectorPropertyConfiguration; +import org.apache.nifi.migration.ConnectorStepPropertyConfiguration; + +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; + +/** + * Test-only {@link ConnectorPropertyConfiguration} implementation for exercising a Connector's + * {@link org.apache.nifi.components.connector.Connector#migrateProperties(ConnectorPropertyConfiguration) migrateProperties} + * method in unit tests. Tracks per-step property renames, removals, and additions, plus step-level renames, + * removals, and additions. Callers can drive the mock with either a plain string-literal shorthand map or a + * fully typed {@link ConnectorValueReference} map and then inspect the outcome via + * {@link #toMigrationResult()}, {@link #getStepNames()}, and {@link #forStep(String)}. + */ +public class MockConnectorPropertyConfiguration implements ConnectorPropertyConfiguration { + + private final Map<String, Map<String, ConnectorValueReference>> stepProperties = new HashMap<>(); + private final Set<String> initialStepNames; + + private final Map<String, Map<String, String>> propertiesRenamed = new HashMap<>(); + private final Map<String, Set<String>> propertiesRemoved = new HashMap<>(); + private final Map<String, Set<String>> propertiesUpdated = new HashMap<>(); + + private final Map<String, String> renamedSteps = new HashMap<>(); + private final Set<String> removedSteps = new HashSet<>(); + private final Set<String> addedSteps = new HashSet<>(); + + private MockConnectorPropertyConfiguration(final Map<String, ? extends Map<String, ConnectorValueReference>> initialProperties) { + if (initialProperties != null) { + for (final Map.Entry<String, ? extends Map<String, ConnectorValueReference>> entry : initialProperties.entrySet()) { + final Map<String, ConnectorValueReference> copy = new HashMap<>(); + if (entry.getValue() != null) { + copy.putAll(entry.getValue()); + } + stepProperties.put(entry.getKey(), copy); + } + } + this.initialStepNames = Collections.unmodifiableSet(new HashSet<>(stepProperties.keySet())); + } + + /** + * Creates a {@link MockConnectorPropertyConfiguration} pre-populated with typed {@link ConnectorValueReference} + * values for each step. + * + * @param initialProperties per-step property maps of {@link ConnectorValueReference} values + * @return a new configuration + */ + public static MockConnectorPropertyConfiguration fromValueReferences(final Map<String, ? extends Map<String, ConnectorValueReference>> initialProperties) { + return new MockConnectorPropertyConfiguration(initialProperties); + } + + /** + * Creates a {@link MockConnectorPropertyConfiguration} pre-populated with plain string-literal values for each + * step. Each supplied string is wrapped in a {@link StringLiteralValue}. + * + * @param initialProperties per-step property maps of raw string values + * @return a new configuration + */ + public static MockConnectorPropertyConfiguration fromStringLiterals(final Map<String, ? extends Map<String, String>> initialProperties) { + final Map<String, Map<String, ConnectorValueReference>> converted = new HashMap<>(); + if (initialProperties != null) { + for (final Map.Entry<String, ? extends Map<String, String>> entry : initialProperties.entrySet()) { + final Map<String, ConnectorValueReference> stepMap = new HashMap<>(); + if (entry.getValue() != null) { + for (final Map.Entry<String, String> propertyEntry : entry.getValue().entrySet()) { + stepMap.put(propertyEntry.getKey(), new StringLiteralValue(propertyEntry.getValue())); + } + } + converted.put(entry.getKey(), stepMap); + } + } + return new MockConnectorPropertyConfiguration(converted); + } + + @Override + public Set<String> getStepNames() { + return Collections.unmodifiableSet(stepProperties.keySet()); + } + + @Override + public boolean hasStep(final String stepName) { + return stepProperties.containsKey(stepName); + } + + @Override + public boolean renameStep(final String oldStepName, final String newStepName) { + if (!stepProperties.containsKey(oldStepName)) { + return false; + } + if (Objects.equals(oldStepName, newStepName)) { + return false; + } + if (stepProperties.containsKey(newStepName)) { + throw new IllegalStateException("Cannot rename step [" + oldStepName + "] to [" + newStepName + + "] because a step with the new name already exists"); + } + + final Map<String, ConnectorValueReference> existing = stepProperties.remove(oldStepName); + stepProperties.put(newStepName, existing); + renamedSteps.put(oldStepName, newStepName); + + final Map<String, String> renames = propertiesRenamed.remove(oldStepName); + if (renames != null) { + propertiesRenamed.put(newStepName, renames); + } + final Set<String> removed = propertiesRemoved.remove(oldStepName); + if (removed != null) { + propertiesRemoved.put(newStepName, removed); + } + final Set<String> updated = propertiesUpdated.remove(oldStepName); + if (updated != null) { + propertiesUpdated.put(newStepName, updated); + } + return true; + } + + @Override + public boolean removeStep(final String stepName) { + if (!stepProperties.containsKey(stepName)) { + return false; + } + stepProperties.remove(stepName); + removedSteps.add(stepName); + propertiesRenamed.remove(stepName); + propertiesRemoved.remove(stepName); + propertiesUpdated.remove(stepName); + return true; + } + + @Override + public ConnectorStepPropertyConfiguration forStep(final String stepName) { + return new MockStepPropertyConfiguration(stepName); + } + + /** + * @return a summary of every mutation observed during migration, suitable for assertions + */ + public MigrationResult toMigrationResult() { + return new MigrationResult( + Collections.unmodifiableMap(new HashMap<>(renamedSteps)), + Collections.unmodifiableSet(new HashSet<>(removedSteps)), + Collections.unmodifiableSet(new HashSet<>(addedSteps)), + deepCopy(propertiesRenamed), + deepCopySets(propertiesRemoved), + deepCopySets(propertiesUpdated) + ); + } + + private static Map<String, Map<String, String>> deepCopy(final Map<String, Map<String, String>> source) { + final Map<String, Map<String, String>> copy = new HashMap<>(); + for (final Map.Entry<String, Map<String, String>> entry : source.entrySet()) { + copy.put(entry.getKey(), Collections.unmodifiableMap(new HashMap<>(entry.getValue()))); + } + return Collections.unmodifiableMap(copy); + } + + private static Map<String, Set<String>> deepCopySets(final Map<String, Set<String>> source) { + final Map<String, Set<String>> copy = new HashMap<>(); + for (final Map.Entry<String, Set<String>> entry : source.entrySet()) { + copy.put(entry.getKey(), Collections.unmodifiableSet(new HashSet<>(entry.getValue()))); + } + return Collections.unmodifiableMap(copy); + } + + private Map<String, ConnectorValueReference> getOrCreateStepMap(final String stepName) { + return stepProperties.computeIfAbsent(stepName, key -> { + if (!initialStepNames.contains(key)) { + addedSteps.add(key); + } + return new HashMap<>(); + }); + } + + private void trackRenamed(final String stepName, final String oldName, final String newName) { + propertiesRenamed.computeIfAbsent(stepName, key -> new HashMap<>()).put(oldName, newName); + } + + private void trackRemoved(final String stepName, final String propertyName) { + propertiesRemoved.computeIfAbsent(stepName, key -> new HashSet<>()).add(propertyName); + } + + private void trackUpdated(final String stepName, final String propertyName) { + propertiesUpdated.computeIfAbsent(stepName, key -> new HashSet<>()).add(propertyName); + } + + /** + * A summary of every step and property mutation observed during a call to + * {@link org.apache.nifi.components.connector.Connector#migrateProperties(ConnectorPropertyConfiguration)}. + * + * @param renamedSteps old step name to new step name for every renamed step + * @param removedSteps step names removed during migration + * @param addedSteps step names introduced during migration + * @param propertiesRenamed per-step map from old property name to new property name + * @param propertiesRemoved per-step set of property names removed + * @param propertiesUpdated per-step set of property names added or overwritten (including via setValueReference) + */ + public record MigrationResult( + Map<String, String> renamedSteps, + Set<String> removedSteps, + Set<String> addedSteps, + Map<String, Map<String, String>> propertiesRenamed, + Map<String, Set<String>> propertiesRemoved, + Map<String, Set<String>> propertiesUpdated + ) { + } + + private final class MockStepPropertyConfiguration implements ConnectorStepPropertyConfiguration { + private final String stepName; + + private MockStepPropertyConfiguration(final String stepName) { + this.stepName = stepName; + } + + @Override + public String getStepName() { + return stepName; + } + + @Override + public boolean renameProperty(final String propertyName, final String newName) { + trackRenamed(stepName, propertyName, newName); Review Comment: Should trackRenamed only run after we confirm the property exists and the name actually changes, so a no-op rename does not show up in MigrationResult.propertiesRenamed()? ########## nifi-connector-mock-bundle/nifi-connector-mock/src/main/java/org/apache/nifi/mock/connector/migration/MockConnectorPropertyConfiguration.java: ########## @@ -0,0 +1,354 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.nifi.mock.connector.migration; + +import org.apache.nifi.components.connector.ConnectorValueReference; +import org.apache.nifi.components.connector.StringLiteralValue; +import org.apache.nifi.migration.ConnectorPropertyConfiguration; +import org.apache.nifi.migration.ConnectorStepPropertyConfiguration; + +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; + +/** + * Test-only {@link ConnectorPropertyConfiguration} implementation for exercising a Connector's + * {@link org.apache.nifi.components.connector.Connector#migrateProperties(ConnectorPropertyConfiguration) migrateProperties} + * method in unit tests. Tracks per-step property renames, removals, and additions, plus step-level renames, + * removals, and additions. Callers can drive the mock with either a plain string-literal shorthand map or a + * fully typed {@link ConnectorValueReference} map and then inspect the outcome via + * {@link #toMigrationResult()}, {@link #getStepNames()}, and {@link #forStep(String)}. + */ +public class MockConnectorPropertyConfiguration implements ConnectorPropertyConfiguration { + + private final Map<String, Map<String, ConnectorValueReference>> stepProperties = new HashMap<>(); + private final Set<String> initialStepNames; + + private final Map<String, Map<String, String>> propertiesRenamed = new HashMap<>(); + private final Map<String, Set<String>> propertiesRemoved = new HashMap<>(); + private final Map<String, Set<String>> propertiesUpdated = new HashMap<>(); + + private final Map<String, String> renamedSteps = new HashMap<>(); + private final Set<String> removedSteps = new HashSet<>(); + private final Set<String> addedSteps = new HashSet<>(); + + private MockConnectorPropertyConfiguration(final Map<String, ? extends Map<String, ConnectorValueReference>> initialProperties) { + if (initialProperties != null) { + for (final Map.Entry<String, ? extends Map<String, ConnectorValueReference>> entry : initialProperties.entrySet()) { + final Map<String, ConnectorValueReference> copy = new HashMap<>(); + if (entry.getValue() != null) { + copy.putAll(entry.getValue()); + } + stepProperties.put(entry.getKey(), copy); + } + } + this.initialStepNames = Collections.unmodifiableSet(new HashSet<>(stepProperties.keySet())); + } + + /** + * Creates a {@link MockConnectorPropertyConfiguration} pre-populated with typed {@link ConnectorValueReference} + * values for each step. + * + * @param initialProperties per-step property maps of {@link ConnectorValueReference} values + * @return a new configuration + */ + public static MockConnectorPropertyConfiguration fromValueReferences(final Map<String, ? extends Map<String, ConnectorValueReference>> initialProperties) { + return new MockConnectorPropertyConfiguration(initialProperties); + } + + /** + * Creates a {@link MockConnectorPropertyConfiguration} pre-populated with plain string-literal values for each + * step. Each supplied string is wrapped in a {@link StringLiteralValue}. + * + * @param initialProperties per-step property maps of raw string values + * @return a new configuration + */ + public static MockConnectorPropertyConfiguration fromStringLiterals(final Map<String, ? extends Map<String, String>> initialProperties) { + final Map<String, Map<String, ConnectorValueReference>> converted = new HashMap<>(); + if (initialProperties != null) { + for (final Map.Entry<String, ? extends Map<String, String>> entry : initialProperties.entrySet()) { + final Map<String, ConnectorValueReference> stepMap = new HashMap<>(); + if (entry.getValue() != null) { + for (final Map.Entry<String, String> propertyEntry : entry.getValue().entrySet()) { + stepMap.put(propertyEntry.getKey(), new StringLiteralValue(propertyEntry.getValue())); + } + } + converted.put(entry.getKey(), stepMap); + } + } + return new MockConnectorPropertyConfiguration(converted); + } + + @Override + public Set<String> getStepNames() { + return Collections.unmodifiableSet(stepProperties.keySet()); + } + + @Override + public boolean hasStep(final String stepName) { + return stepProperties.containsKey(stepName); + } + + @Override + public boolean renameStep(final String oldStepName, final String newStepName) { + if (!stepProperties.containsKey(oldStepName)) { + return false; + } + if (Objects.equals(oldStepName, newStepName)) { + return false; + } + if (stepProperties.containsKey(newStepName)) { + throw new IllegalStateException("Cannot rename step [" + oldStepName + "] to [" + newStepName + + "] because a step with the new name already exists"); + } + + final Map<String, ConnectorValueReference> existing = stepProperties.remove(oldStepName); + stepProperties.put(newStepName, existing); + renamedSteps.put(oldStepName, newStepName); + + final Map<String, String> renames = propertiesRenamed.remove(oldStepName); + if (renames != null) { + propertiesRenamed.put(newStepName, renames); + } + final Set<String> removed = propertiesRemoved.remove(oldStepName); + if (removed != null) { + propertiesRemoved.put(newStepName, removed); + } + final Set<String> updated = propertiesUpdated.remove(oldStepName); + if (updated != null) { + propertiesUpdated.put(newStepName, updated); + } + return true; + } + + @Override + public boolean removeStep(final String stepName) { + if (!stepProperties.containsKey(stepName)) { + return false; + } + stepProperties.remove(stepName); + removedSteps.add(stepName); + propertiesRenamed.remove(stepName); + propertiesRemoved.remove(stepName); + propertiesUpdated.remove(stepName); + return true; + } + + @Override + public ConnectorStepPropertyConfiguration forStep(final String stepName) { + return new MockStepPropertyConfiguration(stepName); + } + + /** + * @return a summary of every mutation observed during migration, suitable for assertions + */ + public MigrationResult toMigrationResult() { + return new MigrationResult( + Collections.unmodifiableMap(new HashMap<>(renamedSteps)), + Collections.unmodifiableSet(new HashSet<>(removedSteps)), + Collections.unmodifiableSet(new HashSet<>(addedSteps)), + deepCopy(propertiesRenamed), + deepCopySets(propertiesRemoved), + deepCopySets(propertiesUpdated) + ); + } + + private static Map<String, Map<String, String>> deepCopy(final Map<String, Map<String, String>> source) { + final Map<String, Map<String, String>> copy = new HashMap<>(); + for (final Map.Entry<String, Map<String, String>> entry : source.entrySet()) { + copy.put(entry.getKey(), Collections.unmodifiableMap(new HashMap<>(entry.getValue()))); + } + return Collections.unmodifiableMap(copy); + } + + private static Map<String, Set<String>> deepCopySets(final Map<String, Set<String>> source) { + final Map<String, Set<String>> copy = new HashMap<>(); + for (final Map.Entry<String, Set<String>> entry : source.entrySet()) { + copy.put(entry.getKey(), Collections.unmodifiableSet(new HashSet<>(entry.getValue()))); + } + return Collections.unmodifiableMap(copy); + } + + private Map<String, ConnectorValueReference> getOrCreateStepMap(final String stepName) { + return stepProperties.computeIfAbsent(stepName, key -> { + if (!initialStepNames.contains(key)) { + addedSteps.add(key); + } + return new HashMap<>(); + }); + } + + private void trackRenamed(final String stepName, final String oldName, final String newName) { + propertiesRenamed.computeIfAbsent(stepName, key -> new HashMap<>()).put(oldName, newName); + } + + private void trackRemoved(final String stepName, final String propertyName) { + propertiesRemoved.computeIfAbsent(stepName, key -> new HashSet<>()).add(propertyName); + } + + private void trackUpdated(final String stepName, final String propertyName) { + propertiesUpdated.computeIfAbsent(stepName, key -> new HashSet<>()).add(propertyName); + } + + /** + * A summary of every step and property mutation observed during a call to + * {@link org.apache.nifi.components.connector.Connector#migrateProperties(ConnectorPropertyConfiguration)}. + * + * @param renamedSteps old step name to new step name for every renamed step + * @param removedSteps step names removed during migration + * @param addedSteps step names introduced during migration + * @param propertiesRenamed per-step map from old property name to new property name + * @param propertiesRemoved per-step set of property names removed + * @param propertiesUpdated per-step set of property names added or overwritten (including via setValueReference) + */ + public record MigrationResult( + Map<String, String> renamedSteps, + Set<String> removedSteps, + Set<String> addedSteps, + Map<String, Map<String, String>> propertiesRenamed, + Map<String, Set<String>> propertiesRemoved, + Map<String, Set<String>> propertiesUpdated + ) { + } + + private final class MockStepPropertyConfiguration implements ConnectorStepPropertyConfiguration { + private final String stepName; + + private MockStepPropertyConfiguration(final String stepName) { + this.stepName = stepName; + } + + @Override + public String getStepName() { + return stepName; + } + + @Override + public boolean renameProperty(final String propertyName, final String newName) { + trackRenamed(stepName, propertyName, newName); + final Map<String, ConnectorValueReference> properties = stepProperties.get(stepName); + if (properties == null || !properties.containsKey(propertyName)) { + return false; + } + if (Objects.equals(propertyName, newName)) { + return false; + } + final ConnectorValueReference existing = properties.remove(propertyName); + properties.put(newName, existing); + return true; + } + + @Override + public boolean removeProperty(final String propertyName) { + trackRemoved(stepName, propertyName); Review Comment: Should trackRemoved only run when the property actually existed and was removed, to match the real StandardConnectorStepPropertyConfiguration behavior? ########## nifi-connector-mock-bundle/nifi-connector-mock/src/main/java/org/apache/nifi/mock/connector/migration/MockConnectorPropertyConfiguration.java: ########## @@ -0,0 +1,354 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.nifi.mock.connector.migration; + +import org.apache.nifi.components.connector.ConnectorValueReference; +import org.apache.nifi.components.connector.StringLiteralValue; +import org.apache.nifi.migration.ConnectorPropertyConfiguration; +import org.apache.nifi.migration.ConnectorStepPropertyConfiguration; + +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; + +/** + * Test-only {@link ConnectorPropertyConfiguration} implementation for exercising a Connector's + * {@link org.apache.nifi.components.connector.Connector#migrateProperties(ConnectorPropertyConfiguration) migrateProperties} + * method in unit tests. Tracks per-step property renames, removals, and additions, plus step-level renames, + * removals, and additions. Callers can drive the mock with either a plain string-literal shorthand map or a + * fully typed {@link ConnectorValueReference} map and then inspect the outcome via + * {@link #toMigrationResult()}, {@link #getStepNames()}, and {@link #forStep(String)}. + */ +public class MockConnectorPropertyConfiguration implements ConnectorPropertyConfiguration { Review Comment: Would it help to add a small unit test for this class, given it is a new public test utility with no direct test coverage in this PR? ########## nifi-system-tests/nifi-system-test-suite/src/test/java/org/apache/nifi/tests/system/connectors/ConnectorPropertyMigrationIT.java: ########## @@ -0,0 +1,262 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.nifi.tests.system.connectors; + +import org.apache.nifi.tests.system.NiFiSystemIT; +import org.apache.nifi.toolkit.client.NiFiClientException; +import org.apache.nifi.web.api.dto.ConfigurationStepConfigurationDTO; +import org.apache.nifi.web.api.dto.ConnectorConfigurationDTO; +import org.apache.nifi.web.api.dto.ConnectorValueReferenceDTO; +import org.apache.nifi.web.api.dto.PropertyGroupConfigurationDTO; +import org.apache.nifi.web.api.entity.ConnectorEntity; +import org.apache.nifi.web.api.entity.ParameterProviderEntity; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import java.io.File; +import java.io.IOException; +import java.nio.file.FileVisitResult; +import java.nio.file.FileVisitor; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.attribute.BasicFileAttributes; +import java.util.List; +import java.util.Map; +import java.util.regex.Pattern; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * End-to-end system test for {@code Connector.migrateProperties}. A pre-migration + * {@code MigratePropertiesConnector} fixture in {@code nifi-system-test-extensions-nar} is created and configured + * through the REST API using legacy step / property names. NiFi is then stopped, the system-test extensions NARs are + * swapped for {@code nifi-alternate-config-extensions-nar} (which contains a same-simple-typed-name fixture that + * implements {@code migrateProperties}), and NiFi is restarted. The framework's {@code StandardConnectorRepository} + * sync path invokes {@code StandardConnectorNode.inheritConfiguration}, which drives + * {@code Connector.migrateProperties}. The test asserts that the persisted active and working configurations both + * reflect the migrated step and property names / values. + */ +public class ConnectorPropertyMigrationIT extends NiFiSystemIT { + + private static final String CONNECTOR_TYPE_SIMPLE_NAME = "MigratePropertiesConnector"; + + private static final String LEGACY_STEP_NAME = "Legacy Step"; + private static final String LEGACY_BROKER_URL_NAME = "legacy-broker-url"; + private static final String LEGACY_CREDENTIALS_NAME = "legacy-credentials"; + private static final String LEGACY_OBSOLETE_NAME = "legacy-obsolete"; + + private static final String MIGRATED_STEP_NAME = "Kafka Connection"; + private static final String BROKER_URL_NAME = "Broker URL"; + private static final String CREDENTIALS_NAME = "Credentials"; + private static final String CLIENT_ID_NAME = "Client Id"; + + private static final String BROKER_URL_VALUE = "kafka-broker:9092"; + private static final String OBSOLETE_VALUE = "gone"; + private static final String CLIENT_ID_MIGRATED_VALUE = "auto-migrated"; + + private static final String SECRET_NAME = "supersecret"; + private static final String FULLY_QUALIFIED_SECRET_NAME = "PropertiesParameterProvider.Parameters.supersecret"; + private static final String STRING_LITERAL_TYPE = "STRING_LITERAL"; + private static final String SECRET_REFERENCE_TYPE = "SECRET_REFERENCE"; + + @Override + protected boolean isAllowFactoryReuse() { + return false; + } + + @Override + protected boolean isDestroyEnvironmentAfterEachTest() { + return true; + } + + @AfterEach + public void restoreNars() { + // Stop the NiFi instance, ensure that the nifi-system-test-extensions-nar and nifi-alternate-config-extensions + // bundles are back where the next test expects them, and restart so the next test sees the default layout. + getNiFiInstance().stop(); + switchNarsBack(); + getNiFiInstance().start(true); + } + + @Test + public void testConnectorPropertyMigration() throws NiFiClientException, IOException, InterruptedException { + // Stand up a Parameter Provider with a single secret so that the legacy secret property can be configured with + // a SECRET_REFERENCE that survives the migration. + final ParameterProviderEntity paramProvider = getClientUtil().createParameterProvider("PropertiesParameterProvider"); + getClientUtil().updateParameterProviderProperties(paramProvider, Map.of("parameters", SECRET_NAME + "=" + SECRET_NAME)); + + // Create the pre-migration connector and configure the legacy step with a string literal, a secret reference, + // and an obsolete string literal that will be removed by migration. + final ConnectorEntity connector = getClientUtil().createConnector(CONNECTOR_TYPE_SIMPLE_NAME); + assertNotNull(connector); + + final ConnectorValueReferenceDTO brokerUrlRef = createStringLiteralReference(BROKER_URL_VALUE); + final ConnectorValueReferenceDTO credentialsRef = getClientUtil().createSecretValueReference(paramProvider.getId(), + SECRET_NAME, FULLY_QUALIFIED_SECRET_NAME); + final ConnectorValueReferenceDTO obsoleteRef = createStringLiteralReference(OBSOLETE_VALUE); + final Map<String, ConnectorValueReferenceDTO> legacyProperties = Map.of( + LEGACY_BROKER_URL_NAME, brokerUrlRef, + LEGACY_CREDENTIALS_NAME, credentialsRef, + LEGACY_OBSOLETE_NAME, obsoleteRef + ); + getClientUtil().configureConnectorWithReferences(connector.getId(), LEGACY_STEP_NAME, legacyProperties); + getClientUtil().applyConnectorUpdate(connector); + + // Sanity check: pre-restart state still has the legacy schema before we swap NARs. + final ConnectorEntity beforeRestart = getNifiClient().getConnectorClient().getConnector(connector.getId()); + final Map<String, ConnectorValueReferenceDTO> legacyActive = propertyValues(beforeRestart.getComponent().getActiveConfiguration(), LEGACY_STEP_NAME); + assertEquals(BROKER_URL_VALUE, legacyActive.get(LEGACY_BROKER_URL_NAME).getValue()); + assertEquals(SECRET_REFERENCE_TYPE, legacyActive.get(LEGACY_CREDENTIALS_NAME).getValueType()); + assertEquals(OBSOLETE_VALUE, legacyActive.get(LEGACY_OBSOLETE_NAME).getValue()); + + // Swap out the system-test-extensions NARs for the alternate-config NAR that defines the same simple-typed + // MigratePropertiesConnector with the migrated schema and a migrateProperties implementation. + getNiFiInstance().stop(); + switchOutNars(); + getNiFiInstance().start(true); + + // On restart, StandardConnectorRepository.syncConnector -> StandardConnectorNode.inheritConfiguration invokes + // Connector.migrateProperties for both the active and working configurations. Assert that each context now + // reflects the renamed step, renamed / removed / added properties, and that typed value references survive. + final ConnectorEntity afterRestart = getNifiClient().getConnectorClient().getConnector(connector.getId()); + assertMigratedConfiguration(afterRestart.getComponent().getActiveConfiguration(), "active"); + assertMigratedConfiguration(afterRestart.getComponent().getWorkingConfiguration(), "working"); + + getClientUtil().waitForValidConnector(connector.getId()); + } + + private void assertMigratedConfiguration(final ConnectorConfigurationDTO config, final String label) { + final List<ConfigurationStepConfigurationDTO> steps = config.getConfigurationStepConfigurations(); + assertNotNull(steps, label + " configuration is missing steps"); + assertTrue(steps.stream().noneMatch(step -> LEGACY_STEP_NAME.equals(step.getConfigurationStepName())), + label + " configuration still contains legacy step"); + + final Map<String, ConnectorValueReferenceDTO> migrated = propertyValues(config, MIGRATED_STEP_NAME); + + final ConnectorValueReferenceDTO brokerUrl = migrated.get(BROKER_URL_NAME); + assertNotNull(brokerUrl, label + " configuration is missing " + BROKER_URL_NAME); + assertEquals(STRING_LITERAL_TYPE, brokerUrl.getValueType()); + assertEquals(BROKER_URL_VALUE, brokerUrl.getValue()); + + final ConnectorValueReferenceDTO credentials = migrated.get(CREDENTIALS_NAME); + assertNotNull(credentials, label + " configuration is missing " + CREDENTIALS_NAME); + assertEquals(SECRET_REFERENCE_TYPE, credentials.getValueType()); + assertEquals(SECRET_NAME, credentials.getSecretName()); + + final ConnectorValueReferenceDTO clientId = migrated.get(CLIENT_ID_NAME); + assertNotNull(clientId, label + " configuration is missing " + CLIENT_ID_NAME); + assertEquals(STRING_LITERAL_TYPE, clientId.getValueType()); + assertEquals(CLIENT_ID_MIGRATED_VALUE, clientId.getValue()); + + assertFalse(migrated.containsKey(LEGACY_OBSOLETE_NAME), + label + " configuration still contains obsolete legacy property"); + assertFalse(migrated.containsKey(LEGACY_BROKER_URL_NAME), + label + " configuration still contains legacy broker URL name"); + assertFalse(migrated.containsKey(LEGACY_CREDENTIALS_NAME), + label + " configuration still contains legacy credentials name"); + } + + private Map<String, ConnectorValueReferenceDTO> propertyValues(final ConnectorConfigurationDTO config, final String stepName) { + final ConfigurationStepConfigurationDTO step = config.getConfigurationStepConfigurations().stream() + .filter(s -> stepName.equals(s.getConfigurationStepName())) + .findFirst() + .orElseThrow(() -> new AssertionError("Configuration step not found: " + stepName)); + + final List<PropertyGroupConfigurationDTO> groups = step.getPropertyGroupConfigurations(); + assertNotNull(groups, "No property groups on step " + stepName); + assertFalse(groups.isEmpty(), "No property groups on step " + stepName); + return groups.getFirst().getPropertyValues(); + } + + private ConnectorValueReferenceDTO createStringLiteralReference(final String value) { + final ConnectorValueReferenceDTO valueRef = new ConnectorValueReferenceDTO(); + valueRef.setValueType(STRING_LITERAL_TYPE); + valueRef.setValue(value); + return valueRef; + } + + private void switchOutNars() throws IOException { Review Comment: Since PropertyMigrationIT already has an identical switchOutNars, moveNars, and findFile helper set, could this be extracted into a shared base class instead of duplicated here? ########## nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/components/connector/StandardConnectorNode.java: ########## @@ -378,41 +380,71 @@ public void inheritConfiguration(final List<VersionedConfigurationStep> activeCo final Bundle flowContextBundle) throws FlowUpdateException { logger.debug("Inheriting configuration for {}", this); - final MutableConnectorConfigurationContext configurationContext = createConfigurationContext(activeConfig); + + // Give the Connector a chance to evolve its persisted property/step names before we build the runtime + // configuration contexts. Active and working configs are migrated independently because they can diverge. + final Map<String, Map<String, ConnectorValueReference>> migratedActiveProperties = migrateProperties(activeConfig); + final Map<String, Map<String, ConnectorValueReference>> migratedWorkingProperties = migrateProperties(workingConfig); + + final MutableConnectorConfigurationContext activeSeedContext = createConfigurationContext(migratedActiveProperties); final FrameworkFlowContext inheritContext = flowContextFactory.createWorkingFlowContext(identifier, - connectorDetails.getComponentLog(), configurationContext, flowContextBundle); + connectorDetails.getComponentLog(), activeSeedContext, flowContextBundle); - // Apply the update for the active config + // Apply the active configuration. This restores activeFlowContext to migratedActiveProperties and internally + // rebuilds workingFlowContext aliased to activeFlowContext's configuration; we discard that alias below and + // construct an independent working context so active and working do not share configuration state when the + // two lists actually diverge. applyUpdate(inheritContext); - // Configure the working config but do not apply - for (final VersionedConfigurationStep step : workingConfig) { - final StepConfiguration stepConfig = createStepConfiguration(step); - setConfiguration(step.getName(), stepConfig, true); + // Tear down the working context that applyUpdate created aliased to active, and rebuild it around an + // independent configuration seeded from migratedWorkingProperties. Then fire onConfigurationStepConfigured + // for every step so renamed steps trigger the flow-builder callback under their new name and any + // value-derived flow state (resolved asset paths, secret values, etc.) is populated against the fresh + // working context. + destroyWorkingContext(); Review Comment: Could the PR description mention that inheritConfiguration now rebuilds the working context independently from active rather than layering working config onto an active-derived clone, since this affects every Connector on restart, not only ones that implement migrateProperties? -- 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]
