exceptionfactory commented on code in PR #11571:
URL: https://github.com/apache/nifi/pull/11571#discussion_r3856283356
##########
nifi-registry/nifi-registry-core/nifi-registry-flow-diff/src/test/java/org/apache/nifi/registry/flow/diff/RebaseEngineTest.java:
##########
@@ -159,10 +162,167 @@ void testUnsupportedDifferenceTypeNoHandler() {
final RebaseAnalysis.ClassifiedDifference classified =
analysis.getClassifiedLocalChanges().get(0);
assertEquals(RebaseClassification.UNSUPPORTED,
classified.getClassification());
- assertEquals(RebaseConflictCode.NO_HANDLER,
classified.getConflictCode());
+ assertEquals(RebaseConflictCode.UNSUPPORTED_COMPONENT_TYPE,
classified.getConflictCode());
assertNull(analysis.getMergedSnapshot());
}
+ @Test
+ void
testAnalyzeScenario1PreservesAddedControllerServiceAndProcessorReference() {
+ final VersionedControllerService versionNService =
createControllerService("service-x", "Service X", "root");
+ final VersionedControllerService localAddedService =
createControllerService("service-y", "Service Y", "root");
+
+ final VersionedProcessor versionNProcessor =
createProcessorWithProperty("proc-a", "ProcessorA", "controller.service",
"service-x");
+ final VersionedProcessor localProcessor =
createProcessorWithProperty("proc-a", "ProcessorA", "controller.service",
"service-y");
Review Comment:
The string values in this method and others could use promotion to static
final variables
##########
nifi-system-tests/nifi-system-test-suite/src/test/java/org/apache/nifi/tests/system/registry/RebaseVersionIT.java:
##########
@@ -396,6 +401,98 @@ public void
testRebasePreservesLocalModificationsAgainstTargetVersion() throws N
"Expected the preserved local change to be reported as a local
modification after rebase, but none were found");
}
+ @Test
+ public void
testRebasePreservesLocallyAddedControllerServiceReferencedByProcessor() throws
NiFiClientException, IOException, InterruptedException {
+ final FlowRegistryClientEntity clientEntity = registerClient();
+ final NiFiClientUtil util = getClientUtil();
+
+ final ProcessGroupEntity originalGroup =
util.createProcessGroup("Original", "root");
+ final ControllerServiceEntity serviceX =
util.createControllerService("FakeControllerService1", originalGroup.getId());
+ final ProcessorEntity fakeProcessor =
util.createProcessor("FakeProcessor", originalGroup.getId());
+ util.updateProcessorProperties(fakeProcessor, Map.of("Fake Service",
serviceX.getId()));
Review Comment:
It looks like `Fake Service` and some other values in this method should be
declared once and reused
##########
nifi-registry/nifi-registry-core/nifi-registry-flow-diff/src/main/java/org/apache/nifi/registry/flow/diff/ComponentAddedRebaseHandler.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.registry.flow.diff;
+
+import org.apache.nifi.flow.VersionedComponent;
+import org.apache.nifi.flow.VersionedControllerService;
+import org.apache.nifi.flow.VersionedProcessGroup;
+
+import java.util.HashSet;
+import java.util.Set;
+
+public class ComponentAddedRebaseHandler implements RebaseHandler {
+
+ @Override
+ public DifferenceType getSupportedType() {
+ return DifferenceType.COMPONENT_ADDED;
+ }
+
+ @Override
+ public RebaseAnalysis.ClassifiedDifference classify(final FlowDifference
localDifference, final Set<FlowDifference> upstreamDifferences,
+ final
VersionedProcessGroup targetSnapshot) {
+ final VersionedComponent addedComponent =
localDifference.getComponentB();
+ if (!(addedComponent instanceof VersionedControllerService
controllerService)) {
+ final String componentType = addedComponent == null ? "null" :
addedComponent.getClass().getSimpleName();
+ return
RebaseAnalysis.ClassifiedDifference.unsupported(localDifference,
RebaseConflictCode.UNSUPPORTED_COMPONENT_TYPE,
+ "Local component addition type %s is not supported for
rebase".formatted(componentType));
+ }
+
+ final String parentGroupIdentifier =
controllerService.getGroupIdentifier();
+ if (parentGroupIdentifier == null) {
+ return
RebaseAnalysis.ClassifiedDifference.unsupported(localDifference,
RebaseConflictCode.COMPONENT_NOT_FOUND,
+ "Controller service %s does not specify a parent process
group".formatted(controllerService.getIdentifier()));
+ }
+
+ final VersionedProcessGroup parentGroup =
resolveParentGroup(targetSnapshot, parentGroupIdentifier, upstreamDifferences);
+ if (parentGroup == null) {
+ return
RebaseAnalysis.ClassifiedDifference.unsupported(localDifference,
RebaseConflictCode.COMPONENT_NOT_FOUND,
+ "Parent process group %s for controller service %s not
found in target snapshot"
Review Comment:
Recommend capitalizing `Process Group` and `Controller Service` here an
elsewhere as needed
##########
nifi-registry/nifi-registry-core/nifi-registry-flow-diff/src/main/java/org/apache/nifi/registry/flow/diff/ComponentAddedRebaseHandler.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.registry.flow.diff;
+
+import org.apache.nifi.flow.VersionedComponent;
+import org.apache.nifi.flow.VersionedControllerService;
+import org.apache.nifi.flow.VersionedProcessGroup;
+
+import java.util.HashSet;
+import java.util.Set;
+
+public class ComponentAddedRebaseHandler implements RebaseHandler {
+
+ @Override
+ public DifferenceType getSupportedType() {
+ return DifferenceType.COMPONENT_ADDED;
+ }
+
+ @Override
+ public RebaseAnalysis.ClassifiedDifference classify(final FlowDifference
localDifference, final Set<FlowDifference> upstreamDifferences,
+ final
VersionedProcessGroup targetSnapshot) {
+ final VersionedComponent addedComponent =
localDifference.getComponentB();
+ if (!(addedComponent instanceof VersionedControllerService
controllerService)) {
+ final String componentType = addedComponent == null ? "null" :
addedComponent.getClass().getSimpleName();
Review Comment:
Is the use of `"null"` as a string for `componenType` intentional? That
seems like an odd default value. If it should be used, recommend declaring it
statically as `NULL_COMPONENT_TYPE`
##########
nifi-registry/nifi-registry-core/nifi-registry-flow-diff/src/test/java/org/apache/nifi/registry/flow/diff/ComponentAddedRebaseHandlerTest.java:
##########
@@ -0,0 +1,284 @@
+/*
+ * 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.registry.flow.diff;
+
+import org.apache.nifi.flow.Bundle;
+import org.apache.nifi.flow.ScheduledState;
+import org.apache.nifi.flow.VersionedControllerService;
+import org.apache.nifi.flow.VersionedProcessGroup;
+import org.apache.nifi.flow.VersionedProcessor;
+import org.apache.nifi.flow.VersionedPropertyDescriptor;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+class ComponentAddedRebaseHandlerTest {
+
+ private static final String ROOT = "root";
+ private static final String CHILD = "child";
+ private static final String PROCESSOR_ID = "processor-a";
+ private static final String EXISTING_SERVICE_ID = "service-x";
+ private static final String ADDED_SERVICE_ID = "service-y";
+ private static final String SERVICE_REFERENCE_PROPERTY =
"delegate.service";
+
+ private ComponentAddedRebaseHandler handler;
+
+ @BeforeEach
+ void setup() {
+ handler = new ComponentAddedRebaseHandler();
+ }
+
+ @Test
+ void testClassifyReferencedControllerServiceAdditionIsCompatible() {
+ final VersionedControllerService addedService =
createControllerService(ADDED_SERVICE_ID, ROOT);
+ addedService.setProperties(Map.of(SERVICE_REFERENCE_PROPERTY,
EXISTING_SERVICE_ID));
+ addedService.setPropertyDescriptors(Map.of(SERVICE_REFERENCE_PROPERTY,
createDescriptor(SERVICE_REFERENCE_PROPERTY, false, false)));
+
+ final FlowDifference localDifference = new
StandardFlowDifference(DifferenceType.COMPONENT_ADDED, null, addedService,
+ null, addedService, "Referenced controller service added
locally");
+
+ final VersionedProcessGroup targetSnapshot =
createTargetSnapshotWithExistingService(EXISTING_SERVICE_ID, ROOT);
+
+ final RebaseAnalysis.ClassifiedDifference result =
handler.classify(localDifference, Collections.emptySet(), targetSnapshot);
+
+ assertEquals(RebaseClassification.COMPATIBLE,
result.getClassification());
+ }
+
+ @Test
+ void testClassifyUnreferencedControllerServiceAdditionIsCompatible() {
+ final VersionedControllerService addedService =
createControllerService(ADDED_SERVICE_ID, ROOT);
+ final FlowDifference localDifference = new
StandardFlowDifference(DifferenceType.COMPONENT_ADDED, null, addedService,
+ null, addedService, "Unreferenced controller service added
locally");
+
+ final VersionedProcessGroup targetSnapshot =
createTargetSnapshotWithExistingService(EXISTING_SERVICE_ID, ROOT);
+
+ final RebaseAnalysis.ClassifiedDifference result =
handler.classify(localDifference, Collections.emptySet(), targetSnapshot);
+
+ assertEquals(RebaseClassification.COMPATIBLE,
result.getClassification());
+ }
+
+ @Test
+ void testClassifyNonControllerServiceAdditionIsUnsupported() {
+ final VersionedProcessor processor = new VersionedProcessor();
+ processor.setIdentifier(PROCESSOR_ID);
+
+ final FlowDifference localDifference = new
StandardFlowDifference(DifferenceType.COMPONENT_ADDED, null, processor,
+ null, processor, "Processor added locally");
+
+ final VersionedProcessGroup targetSnapshot = createRootGroup();
+
+ final RebaseAnalysis.ClassifiedDifference result =
handler.classify(localDifference, Collections.emptySet(), targetSnapshot);
+
+ assertEquals(RebaseClassification.UNSUPPORTED,
result.getClassification());
+ assertEquals(RebaseConflictCode.UNSUPPORTED_COMPONENT_TYPE,
result.getConflictCode());
+ }
+
+ @Test
+ void testClassifyNullParentIsUnsupported() {
+ final VersionedControllerService addedService =
createControllerService(ADDED_SERVICE_ID, null);
+ final FlowDifference localDifference = new
StandardFlowDifference(DifferenceType.COMPONENT_ADDED, null, addedService,
+ null, addedService, "Controller service added without parent");
+
+ final RebaseAnalysis.ClassifiedDifference result =
handler.classify(localDifference, Collections.emptySet(), createRootGroup());
+
+ assertEquals(RebaseClassification.UNSUPPORTED,
result.getClassification());
+ assertEquals(RebaseConflictCode.COMPONENT_NOT_FOUND,
result.getConflictCode());
+ }
+
+ @Test
+ void testClassifyMissingParentIsUnsupported() {
+ final VersionedControllerService addedService =
createControllerService(ADDED_SERVICE_ID, CHILD);
+ final FlowDifference localDifference = new
StandardFlowDifference(DifferenceType.COMPONENT_ADDED, null, addedService,
+ null, addedService, "Controller service added to missing
parent");
+ final VersionedProcessGroup removedParent = createChildGroup(CHILD);
+ final Set<FlowDifference> upstreamDifferences = Set.of(new
StandardFlowDifference(DifferenceType.COMPONENT_REMOVED, removedParent, null,
+ removedParent, null, "Parent removed upstream"));
+
+ final RebaseAnalysis.ClassifiedDifference result =
handler.classify(localDifference, upstreamDifferences, createRootGroup());
+
+ assertEquals(RebaseClassification.UNSUPPORTED,
result.getClassification());
+ assertEquals(RebaseConflictCode.COMPONENT_NOT_FOUND,
result.getConflictCode());
+ }
+
+ @Test
+ void testClassifySameIdentifierTargetCollisionIsConflicting() {
+ final VersionedControllerService addedService =
createControllerService(ADDED_SERVICE_ID, ROOT);
+ final FlowDifference localDifference = new
StandardFlowDifference(DifferenceType.COMPONENT_ADDED, null, addedService,
+ null, addedService, "Controller service added with colliding
identifier");
+
+ final VersionedProcessGroup childGroup = new VersionedProcessGroup();
+ childGroup.setIdentifier(CHILD);
+
+ final VersionedProcessor collidingProcessor = new VersionedProcessor();
+ collidingProcessor.setIdentifier(ADDED_SERVICE_ID);
+ childGroup.getProcessors().add(collidingProcessor);
+
+ final VersionedProcessGroup targetSnapshot = createRootGroup();
+ targetSnapshot.getProcessGroups().add(childGroup);
+
+ final RebaseAnalysis.ClassifiedDifference result =
handler.classify(localDifference, Collections.emptySet(), targetSnapshot);
+
+ assertEquals(RebaseClassification.CONFLICTING,
result.getClassification());
+ assertEquals(RebaseConflictCode.COMPONENT_IDENTIFIER_COLLISION,
result.getConflictCode());
+ }
+
+ @Test
+ void
testApplyAddsControllerServiceToRootPreservingIdentityAndConfiguration() {
+ final VersionedControllerService addedService =
createControllerService(ADDED_SERVICE_ID, ROOT);
+ final FlowDifference localDifference = new
StandardFlowDifference(DifferenceType.COMPONENT_ADDED, null, addedService,
+ null, addedService, "Controller service added to root");
+
+ final VersionedProcessGroup mergedFlow = new VersionedProcessGroup();
+ mergedFlow.setIdentifier(ROOT);
+
+ handler.apply(localDifference, mergedFlow);
+
+ assertNotNull(mergedFlow.getControllerServices());
+ assertEquals(1, mergedFlow.getControllerServices().size());
+
+ final VersionedControllerService insertedService =
mergedFlow.getControllerServices().iterator().next();
+ assertSame(addedService, insertedService);
+ assertEquals(ADDED_SERVICE_ID, insertedService.getIdentifier());
+ assertEquals(ROOT, insertedService.getGroupIdentifier());
+ assertEquals("Local Controller Service", insertedService.getName());
+ assertEquals("org.apache.nifi.services.LocalControllerService",
insertedService.getType());
+ assertEquals("group", insertedService.getBundle().getGroup());
+ assertEquals("artifact", insertedService.getBundle().getArtifact());
+ assertEquals("1.0.0", insertedService.getBundle().getVersion());
Review Comment:
These values should be declared statically and reused
--
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]
