exceptionfactory commented on code in PR #10844: URL: https://github.com/apache/nifi/pull/10844#discussion_r2760840450
########## nifi-system-tests/nifi-system-test-suite/src/test/java/org/apache/nifi/tests/system/registry/ParameterContextPreservationIT.java: ########## @@ -0,0 +1,203 @@ +/* + * 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.registry; + +import org.apache.nifi.tests.system.NiFiClientUtil; +import org.apache.nifi.tests.system.NiFiSystemIT; +import org.apache.nifi.toolkit.client.NiFiClientException; +import org.apache.nifi.web.api.dto.ProcessGroupDTO; +import org.apache.nifi.web.api.dto.VersionControlInformationDTO; +import org.apache.nifi.web.api.dto.flow.FlowDTO; +import org.apache.nifi.web.api.entity.FlowRegistryClientEntity; +import org.apache.nifi.web.api.entity.ParameterContextEntity; +import org.apache.nifi.web.api.entity.ProcessGroupEntity; +import org.apache.nifi.web.api.entity.ProcessGroupFlowEntity; +import org.apache.nifi.web.api.entity.ProcessorEntity; +import org.apache.nifi.web.api.entity.VersionControlInformationEntity; +import org.junit.jupiter.api.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +/** + * System test to verify that parameter context bindings are preserved during versioned flow upgrades + * when new process groups are added. + * + * This test reproduces a bug where: + * 1. v1: Process Group A with Parameter Context P, containing only a Processor X using param1 + * 2. v2: Process Group A with Parameter Context P, now containing a NEW Process Group B also attached to P + * 3. When checking out v1 twice with "do not keep parameter context": + * - First checkout creates A1 with Parameter Context P + * - Second checkout creates A2 with Parameter Context P (1) since P already exists + * 4. When upgrading A2 from v1 to v2, the newly added Process Group B incorrectly gets + * bound to P instead of P (1) + * + * The expectation is that the new Process Group B should be bound to P (1), the same + * parameter context that its parent A2 uses. + */ +public class ParameterContextPreservationIT extends NiFiSystemIT { + private static final Logger logger = LoggerFactory.getLogger(ParameterContextPreservationIT.class); + public static final String TEST_FLOWS_BUCKET = "test-flows"; + + @Test + public void testNewProcessGroupUsesCorrectParameterContextDuringUpgrade() throws NiFiClientException, IOException, InterruptedException { Review Comment: ```suggestion class ParameterContextPreservationIT extends NiFiSystemIT { private static final Logger logger = LoggerFactory.getLogger(ParameterContextPreservationIT.class); public static final String TEST_FLOWS_BUCKET = "test-flows"; @Test void testNewProcessGroupUsesCorrectParameterContextDuringUpgrade() throws NiFiClientException, IOException, InterruptedException { ``` ########## nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/flow/synchronization/StandardVersionedComponentSynchronizer.java: ########## @@ -2173,6 +2187,61 @@ private void updateParameterContext(final ProcessGroup group, final VersionedPro } } + /** + * Finds a parameter context from the parent group hierarchy that corresponds to the given versioned + * parameter context name. This is used to ensure that when a new child process group is added during + * a flow version upgrade, it uses the same parameter context as its parent (if they both reference + * the same parameter context in the versioned flow), rather than looking up by name globally which + * could result in using a different parameter context with the same base name. + * + * @param group the process group being updated + * @param versionedParameterContextName the name of the parameter context in the versioned flow + * @return the matching parent parameter context, or null if not found + */ + private ParameterContext findMatchingParentParameterContext(final ProcessGroup group, final String versionedParameterContextName) { + ProcessGroup parent = group.getParent(); + while (parent != null) { + final ParameterContext parentContext = parent.getParameterContext(); + if (parentContext != null) { + // Check if the parent's context corresponds to the same versioned parameter context name. + // The parent's context name might be the exact name or have a suffix like " (1)", " (2)", etc. + // if it was created during an import with REPLACE strategy. + final String parentContextName = parentContext.getName(); + if (parentContextName.equals(versionedParameterContextName) + || isParameterContextNameWithSuffix(parentContextName, versionedParameterContextName)) { + return parentContext; + } + } + parent = parent.getParent(); + } + return null; + } + + /** + * Checks if the given parameter context name is the versioned name with a numeric suffix. + * For example, "P (1)" or "P (2)" would match versioned name "P". + * + * @param contextName the actual parameter context name + * @param versionedName the parameter context name from the versioned flow + * @return true if contextName is versionedName with a suffix like " (n)" + */ + private boolean isParameterContextNameWithSuffix(final String contextName, final String versionedName) { + if (!contextName.startsWith(versionedName + " (")) { Review Comment: It looks like the logic in this method could be replaced with a regular expression pattern. ########## nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/flow/synchronization/StandardVersionedComponentSynchronizer.java: ########## @@ -2152,16 +2152,30 @@ private void updateParameterContext(final ProcessGroup group, final VersionedPro createMissingParameterProvider(versionedParameterContext, versionedParameterContext.getParameterProvider(), parameterProviderReferences, componentIdGenerator); if (currentParamContext == null) { // Create a new Parameter Context based on the parameters provided - final ParameterContext contextByName = getParameterContextByName(versionedParameterContext.getName()); final ParameterContext selectedParameterContext; - if (contextByName == null) { - final String parameterContextId = componentIdGenerator.generateUuid(versionedParameterContext.getName(), - versionedParameterContext.getName(), versionedParameterContext.getName()); - selectedParameterContext = createParameterContext(versionedParameterContext, parameterContextId, versionedParameterContexts, - parameterProviderReferences, componentIdGenerator); - } else { - selectedParameterContext = contextByName; + + // First, check if the parent group has a parameter context that corresponds to the same + // versioned parameter context. If so, we should use the parent's context to maintain + // consistency within this flow instance. This is important during flow version upgrades + // where new child process groups are added - they should use the same parameter context + // as their parent, not a different one that happens to match by name. + final ParameterContext parentParameterContext = findMatchingParentParameterContext(group, versionedParameterContext.getName()); + if (parentParameterContext != null) { + selectedParameterContext = parentParameterContext; + // Ensure the parent's context has all the parameters from the versioned context addMissingConfiguration(versionedParameterContext, selectedParameterContext, versionedParameterContexts, parameterProviderReferences, componentIdGenerator); + } else { Review Comment: I recommend adjusting the logic so that the first block is `if (parentParameterContext == null) { ... } else {` ########## nifi-system-tests/nifi-system-test-suite/src/test/java/org/apache/nifi/tests/system/registry/ParameterContextPreservationIT.java: ########## @@ -0,0 +1,203 @@ +/* + * 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.registry; + +import org.apache.nifi.tests.system.NiFiClientUtil; +import org.apache.nifi.tests.system.NiFiSystemIT; +import org.apache.nifi.toolkit.client.NiFiClientException; +import org.apache.nifi.web.api.dto.ProcessGroupDTO; +import org.apache.nifi.web.api.dto.VersionControlInformationDTO; +import org.apache.nifi.web.api.dto.flow.FlowDTO; +import org.apache.nifi.web.api.entity.FlowRegistryClientEntity; +import org.apache.nifi.web.api.entity.ParameterContextEntity; +import org.apache.nifi.web.api.entity.ProcessGroupEntity; +import org.apache.nifi.web.api.entity.ProcessGroupFlowEntity; +import org.apache.nifi.web.api.entity.ProcessorEntity; +import org.apache.nifi.web.api.entity.VersionControlInformationEntity; +import org.junit.jupiter.api.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +/** + * System test to verify that parameter context bindings are preserved during versioned flow upgrades + * when new process groups are added. + * + * This test reproduces a bug where: + * 1. v1: Process Group A with Parameter Context P, containing only a Processor X using param1 + * 2. v2: Process Group A with Parameter Context P, now containing a NEW Process Group B also attached to P + * 3. When checking out v1 twice with "do not keep parameter context": + * - First checkout creates A1 with Parameter Context P + * - Second checkout creates A2 with Parameter Context P (1) since P already exists + * 4. When upgrading A2 from v1 to v2, the newly added Process Group B incorrectly gets + * bound to P instead of P (1) + * + * The expectation is that the new Process Group B should be bound to P (1), the same + * parameter context that its parent A2 uses. + */ +public class ParameterContextPreservationIT extends NiFiSystemIT { + private static final Logger logger = LoggerFactory.getLogger(ParameterContextPreservationIT.class); + public static final String TEST_FLOWS_BUCKET = "test-flows"; + + @Test + public void testNewProcessGroupUsesCorrectParameterContextDuringUpgrade() throws NiFiClientException, IOException, InterruptedException { + final FlowRegistryClientEntity clientEntity = registerClient(); + final NiFiClientUtil util = getClientUtil(); + + // Step 1: Create Parameter Context "P" with param1 + final String paramContextName = "P"; + final Map<String, String> params = new HashMap<>(); + params.put("param1", "value1"); + final ParameterContextEntity paramContextP = util.createParameterContext(paramContextName, params); + + // Step 2: Create v1 - Process Group A with just a Processor X using param1 + final ProcessGroupEntity groupA = util.createProcessGroup("A", "root"); Review Comment: There are a number of string names and relationship values that could be changed to static final member variables and reused for consistency in the test method. ########## nifi-system-tests/nifi-system-test-suite/src/test/java/org/apache/nifi/tests/system/registry/ParameterContextPreservationIT.java: ########## @@ -0,0 +1,203 @@ +/* + * 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.registry; + +import org.apache.nifi.tests.system.NiFiClientUtil; +import org.apache.nifi.tests.system.NiFiSystemIT; +import org.apache.nifi.toolkit.client.NiFiClientException; +import org.apache.nifi.web.api.dto.ProcessGroupDTO; +import org.apache.nifi.web.api.dto.VersionControlInformationDTO; +import org.apache.nifi.web.api.dto.flow.FlowDTO; +import org.apache.nifi.web.api.entity.FlowRegistryClientEntity; +import org.apache.nifi.web.api.entity.ParameterContextEntity; +import org.apache.nifi.web.api.entity.ProcessGroupEntity; +import org.apache.nifi.web.api.entity.ProcessGroupFlowEntity; +import org.apache.nifi.web.api.entity.ProcessorEntity; +import org.apache.nifi.web.api.entity.VersionControlInformationEntity; +import org.junit.jupiter.api.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +/** + * System test to verify that parameter context bindings are preserved during versioned flow upgrades + * when new process groups are added. + * + * This test reproduces a bug where: + * 1. v1: Process Group A with Parameter Context P, containing only a Processor X using param1 + * 2. v2: Process Group A with Parameter Context P, now containing a NEW Process Group B also attached to P + * 3. When checking out v1 twice with "do not keep parameter context": + * - First checkout creates A1 with Parameter Context P + * - Second checkout creates A2 with Parameter Context P (1) since P already exists + * 4. When upgrading A2 from v1 to v2, the newly added Process Group B incorrectly gets + * bound to P instead of P (1) + * + * The expectation is that the new Process Group B should be bound to P (1), the same + * parameter context that its parent A2 uses. + */ +public class ParameterContextPreservationIT extends NiFiSystemIT { + private static final Logger logger = LoggerFactory.getLogger(ParameterContextPreservationIT.class); + public static final String TEST_FLOWS_BUCKET = "test-flows"; + + @Test + public void testNewProcessGroupUsesCorrectParameterContextDuringUpgrade() throws NiFiClientException, IOException, InterruptedException { + final FlowRegistryClientEntity clientEntity = registerClient(); + final NiFiClientUtil util = getClientUtil(); + + // Step 1: Create Parameter Context "P" with param1 + final String paramContextName = "P"; + final Map<String, String> params = new HashMap<>(); + params.put("param1", "value1"); Review Comment: This can be simplified with `Map.of()` ########## nifi-system-tests/nifi-system-test-suite/src/test/java/org/apache/nifi/tests/system/registry/ParameterContextPreservationIT.java: ########## @@ -0,0 +1,203 @@ +/* + * 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.registry; + +import org.apache.nifi.tests.system.NiFiClientUtil; +import org.apache.nifi.tests.system.NiFiSystemIT; +import org.apache.nifi.toolkit.client.NiFiClientException; +import org.apache.nifi.web.api.dto.ProcessGroupDTO; +import org.apache.nifi.web.api.dto.VersionControlInformationDTO; +import org.apache.nifi.web.api.dto.flow.FlowDTO; +import org.apache.nifi.web.api.entity.FlowRegistryClientEntity; +import org.apache.nifi.web.api.entity.ParameterContextEntity; +import org.apache.nifi.web.api.entity.ProcessGroupEntity; +import org.apache.nifi.web.api.entity.ProcessGroupFlowEntity; +import org.apache.nifi.web.api.entity.ProcessorEntity; +import org.apache.nifi.web.api.entity.VersionControlInformationEntity; +import org.junit.jupiter.api.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +/** + * System test to verify that parameter context bindings are preserved during versioned flow upgrades + * when new process groups are added. + * + * This test reproduces a bug where: + * 1. v1: Process Group A with Parameter Context P, containing only a Processor X using param1 + * 2. v2: Process Group A with Parameter Context P, now containing a NEW Process Group B also attached to P + * 3. When checking out v1 twice with "do not keep parameter context": + * - First checkout creates A1 with Parameter Context P + * - Second checkout creates A2 with Parameter Context P (1) since P already exists + * 4. When upgrading A2 from v1 to v2, the newly added Process Group B incorrectly gets + * bound to P instead of P (1) + * + * The expectation is that the new Process Group B should be bound to P (1), the same + * parameter context that its parent A2 uses. + */ +public class ParameterContextPreservationIT extends NiFiSystemIT { + private static final Logger logger = LoggerFactory.getLogger(ParameterContextPreservationIT.class); + public static final String TEST_FLOWS_BUCKET = "test-flows"; + + @Test + public void testNewProcessGroupUsesCorrectParameterContextDuringUpgrade() throws NiFiClientException, IOException, InterruptedException { + final FlowRegistryClientEntity clientEntity = registerClient(); + final NiFiClientUtil util = getClientUtil(); + + // Step 1: Create Parameter Context "P" with param1 + final String paramContextName = "P"; + final Map<String, String> params = new HashMap<>(); + params.put("param1", "value1"); + final ParameterContextEntity paramContextP = util.createParameterContext(paramContextName, params); + + // Step 2: Create v1 - Process Group A with just a Processor X using param1 + final ProcessGroupEntity groupA = util.createProcessGroup("A", "root"); + util.setParameterContext(groupA.getId(), paramContextP); + + final ProcessorEntity processorX = util.createProcessor("GenerateFlowFile", groupA.getId()); + util.updateProcessorProperties(processorX, Collections.singletonMap("Text", "#{param1}")); + util.setAutoTerminatedRelationships(processorX, "success"); + + // Save as v1 + final VersionControlInformationEntity vciV1 = util.startVersionControl(groupA, clientEntity, TEST_FLOWS_BUCKET, "FlowWithParameterContext"); + final String flowId = vciV1.getVersionControlInformation().getFlowId(); + logger.info("Saved v1: Process Group A with Processor X"); Review Comment: I recommend removing the log statements from the test method, the integration test logs for `nifi-app.log` generally capture changes, and otherwise these additional logs are not a common approach in the system tests. -- 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]
