This is an automated email from the ASF dual-hosted git repository.
hansva pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/hop.git
The following commit(s) were added to refs/heads/main by this push:
new 6bafdeca53 harden incorrect hops and cleanup pipeline/workflow
correctly, fixes #8128 (#8129)
6bafdeca53 is described below
commit 6bafdeca53d71003e8b2eaf1f287728254b1e515
Author: Hans Van Akelyen <[email protected]>
AuthorDate: Fri Aug 28 10:46:37 2026 +0200
harden incorrect hops and cleanup pipeline/workflow correctly, fixes #8128
(#8129)
---
.../java/org/apache/hop/pipeline/PipelineMeta.java | 66 ++++
.../java/org/apache/hop/workflow/WorkflowMeta.java | 44 ++-
.../pipeline/messages/messages_en_US.properties | 2 +
.../workflow/messages/messages_en_US.properties | 1 +
.../org/apache/hop/pipeline/PipelineMetaTest.java | 136 ++++++++
.../org/apache/hop/workflow/WorkflowMetaTest.java | 90 +++++
.../HopGuiWorkflowGraphDanglingHopTest.java | 369 +++++++++++++++++++++
.../delegates/HopGuiPipelineTransformDelegate.java | 4 +-
.../delegates/HopGuiWorkflowActionDelegate.java | 4 +-
9 files changed, 710 insertions(+), 6 deletions(-)
diff --git a/engine/src/main/java/org/apache/hop/pipeline/PipelineMeta.java
b/engine/src/main/java/org/apache/hop/pipeline/PipelineMeta.java
index fbf8da6984..7db7e7d582 100644
--- a/engine/src/main/java/org/apache/hop/pipeline/PipelineMeta.java
+++ b/engine/src/main/java/org/apache/hop/pipeline/PipelineMeta.java
@@ -431,11 +431,37 @@ public class PipelineMeta extends AbstractMeta
removeMissingPipeline(missingTransform);
}
+ // Nothing may keep pointing at a transform that is no longer in the
pipeline: a hop or an
+ // error handling entry that outlives its transform is written back to the
file as a reference
+ // to a transform that isn't there.
+ //
+ removeReferencesTo(removeTransform);
+
changedTransforms = true;
setChanged();
clearCaches();
}
+ /** Drops the hops attached to a transform and the error handling aimed at
it. */
+ private void removeReferencesTo(TransformMeta removedTransform) {
+ for (int h = hops.size() - 1; h >= 0; h--) {
+ PipelineHopMeta hop = hops.get(h);
+ if (removedTransform.equals(hop.getFromTransform())
+ || removedTransform.equals(hop.getToTransform())) {
+ hops.remove(h);
+ changedHops = true;
+ }
+ }
+
+ for (TransformMeta transformMeta : transforms) {
+ TransformErrorMeta errorMeta = transformMeta.getTransformErrorMeta();
+ if (errorMeta != null &&
removedTransform.equals(errorMeta.getTargetTransform())) {
+ // The error rows have nowhere to go anymore, so the whole entry goes
with the target.
+ transformMeta.setTransformErrorMeta(null);
+ }
+ }
+ }
+
/**
* Removes a hop from the pipeline on a certain location (i.e. the specified
index). Also marks
* that the pipeline's hops have changed.
@@ -1691,9 +1717,49 @@ public class PipelineMeta extends AbstractMeta
addMissingPipeline(missing);
}
}
+ dropReferencesToTransformsNotInTheFile();
syncTransformErrorHandlingWithHops();
}
+ /**
+ * A hop or an error handling entry naming a transform that the file does
not contain - a name
+ * left behind by a rename or by a transform that was deleted elsewhere - is
resolved to null
+ * while de-serializing. Half of a hop is of no use to anyone: it is not
drawn, it is not
+ * executed, and saving the pipeline again writes it back with one end
missing. So it is dropped
+ * here, and the user is told about it.
+ */
+ private void dropReferencesToTransformsNotInTheFile() {
+ for (int i = hops.size() - 1; i >= 0; i--) {
+ PipelineHopMeta hop = hops.get(i);
+ TransformMeta from = hop.getFromTransform();
+ TransformMeta to = hop.getToTransform();
+ if (from == null || to == null) {
+ hops.remove(i);
+ changedHops = true;
+ TransformMeta known = from == null ? to : from;
+ LogChannel.GENERAL.logError(
+ BaseMessages.getString(
+ PKG,
+ "PipelineMeta.Log.RemovedHopToUnknownTransform",
+ known == null ? "?" : known.getName(),
+ Const.NVL(filename, getName())));
+ }
+ }
+
+ for (TransformMeta transformMeta : transforms) {
+ TransformErrorMeta errorMeta = transformMeta.getTransformErrorMeta();
+ if (errorMeta != null && errorMeta.getTargetTransform() == null) {
+ transformMeta.setTransformErrorMeta(null);
+ LogChannel.GENERAL.logError(
+ BaseMessages.getString(
+ PKG,
+ "PipelineMeta.Log.RemovedErrorHandlingToUnknownTransform",
+ transformMeta.getName(),
+ Const.NVL(filename, getName())));
+ }
+ }
+ }
+
/**
* Align {@link TransformErrorMeta} with the enabled state of the hop to the
error target.
* Pipelines saved before error hops were flagged in hop metadata can have
error handling marked
diff --git a/engine/src/main/java/org/apache/hop/workflow/WorkflowMeta.java
b/engine/src/main/java/org/apache/hop/workflow/WorkflowMeta.java
index afb4826b24..e9d8ae417b 100644
--- a/engine/src/main/java/org/apache/hop/workflow/WorkflowMeta.java
+++ b/engine/src/main/java/org/apache/hop/workflow/WorkflowMeta.java
@@ -485,6 +485,31 @@ public class WorkflowMeta extends AbstractMeta
addMissingAction(missingAction);
}
}
+ dropHopsToActionsNotInTheFile();
+ }
+
+ /**
+ * A hop naming an action that the file does not contain - a name left
behind by a rename or by an
+ * action that was deleted elsewhere - is resolved to null while
de-serializing. Half of a hop is
+ * of no use to anyone: it is not drawn, it is not executed, and saving the
workflow again writes
+ * it back with one end missing. So it is dropped here, and the user is told
about it.
+ */
+ private void dropHopsToActionsNotInTheFile() {
+ for (int i = workflowHops.size() - 1; i >= 0; i--) {
+ WorkflowHopMeta hop = workflowHops.get(i);
+ ActionMeta from = hop.getFromAction();
+ ActionMeta to = hop.getToAction();
+ if (from == null || to == null) {
+ workflowHops.remove(i);
+ ActionMeta known = from == null ? to : from;
+ LogChannel.GENERAL.logError(
+ BaseMessages.getString(
+ PKG,
+ "WorkflowMeta.Log.RemovedHopToUnknownAction",
+ known == null ? "?" : known.getName(),
+ Const.NVL(filename, getName())));
+ }
+ }
}
/**
@@ -610,10 +635,25 @@ public class WorkflowMeta extends AbstractMeta
if (deleted.getAction() instanceof MissingAction missingAction) {
removeMissingAction(missingAction);
}
+ // No hop may keep pointing at an action that is no longer in the
workflow: a hop that
+ // outlives its action is written back to the file as a reference to an
action that isn't
+ // there.
+ //
+ removeHopsAttachedTo(deleted);
}
setChanged();
}
+ /** Drops every hop that starts or ends at an action. */
+ private void removeHopsAttachedTo(ActionMeta action) {
+ for (int i = workflowHops.size() - 1; i >= 0; i--) {
+ WorkflowHopMeta hop = workflowHops.get(i);
+ if (action.equals(hop.getFromAction()) ||
action.equals(hop.getToAction())) {
+ workflowHops.remove(i);
+ }
+ }
+ }
+
/**
* Removes the workflow hop.
*
@@ -796,7 +836,7 @@ public class WorkflowMeta extends AbstractMeta
for (WorkflowHopMeta hop : workflowHops) {
// Look at all the hops
- if (hop.isEnabled() && hop.getToAction().equals(to)) {
+ if (hop.isEnabled() && to.equals(hop.getToAction())) {
count++;
}
}
@@ -817,7 +857,7 @@ public class WorkflowMeta extends AbstractMeta
for (WorkflowHopMeta hop : workflowHops) {
// Look at all the hops
- if (hop.isEnabled() && hop.getToAction().equals(to)) {
+ if (hop.isEnabled() && to.equals(hop.getToAction())) {
if (count == nr) {
return hop.getFromAction();
}
diff --git
a/engine/src/main/resources/org/apache/hop/pipeline/messages/messages_en_US.properties
b/engine/src/main/resources/org/apache/hop/pipeline/messages/messages_en_US.properties
index 42fb37bcd0..5a9e8a86d4 100644
---
a/engine/src/main/resources/org/apache/hop/pipeline/messages/messages_en_US.properties
+++
b/engine/src/main/resources/org/apache/hop/pipeline/messages/messages_en_US.properties
@@ -106,6 +106,8 @@ PipelineMeta.Log.LookingAtConnection=Looking at connection
\#
PipelineMeta.Log.NumberOfHopsReaded=nr of hops read \:
PipelineMeta.Log.NumberOfTransformReaded=nr of transforms read \:
PipelineMeta.Log.ReadingTransforms=Reading
+PipelineMeta.Log.RemovedErrorHandlingToUnknownTransform=Removed the error
handling of transform ''{0}'' in ''{1}''\: it sends its error rows to a
transform that is not in the file.
+PipelineMeta.Log.RemovedHopToUnknownTransform=Removed a hop of transform
''{0}'' in ''{1}''\: the transform at the other end is not in the file. Check
the <order> section of the file to see which name it referred to.
PipelineMeta.Log.TimeExecutionTransformSort=Natural sort of transforms
executed in {0} ms ({1} time previous transforms calculated)
PipelineMeta.Log.WeHaveHops=We have
PipelineMeta.MissingPluginsFoundWhileLoadingPipeline.Exception=Missing plugins
found while loading a pipeline
diff --git
a/engine/src/main/resources/org/apache/hop/workflow/messages/messages_en_US.properties
b/engine/src/main/resources/org/apache/hop/workflow/messages/messages_en_US.properties
index 021dfdbc70..506aa5e331 100644
---
a/engine/src/main/resources/org/apache/hop/workflow/messages/messages_en_US.properties
+++
b/engine/src/main/resources/org/apache/hop/workflow/messages/messages_en_US.properties
@@ -64,6 +64,7 @@ WorkflowMeta.Exception.AnErrorOccuredReadingWorkflow=There
was an error reading
WorkflowMeta.Exception.ErrorReadingFromXMLFile=Error reading/validating
information from XML file\:
WorkflowMeta.Exception.UnableToLoadWorkflowFromXMLFile=Unable to load the
workflow from XML file [
WorkflowMeta.Exception.UnableToLoadWorkflowFromXMLNode=Unable to load workflow
info from XML node
+WorkflowMeta.Log.RemovedHopToUnknownAction=Removed a hop of action ''{0}'' in
''{1}''\: the action at the other end is not in the file. Check the <hops>
section of the file to see which name it referred to.
WorkflowMeta.Monitor.GettingSQLForActionCopy=Getting SQL statements for action
copy [
WorkflowMeta.Monitor.GettingSQLNeededForThisWorkflow=Getting the SQL needed
for this workflow...
WorkflowMeta.Monitor.VerifyingAction.Title=Verifying action [{0}]
diff --git a/engine/src/test/java/org/apache/hop/pipeline/PipelineMetaTest.java
b/engine/src/test/java/org/apache/hop/pipeline/PipelineMetaTest.java
index c5024c15a7..c152c4389a 100644
--- a/engine/src/test/java/org/apache/hop/pipeline/PipelineMetaTest.java
+++ b/engine/src/test/java/org/apache/hop/pipeline/PipelineMetaTest.java
@@ -862,4 +862,140 @@ class PipelineMetaTest {
assertEquals("100",
copy.getTransformErrorMetas().get(0).getSourceTransform().getName());
assertNull(copy.getTransformErrorMetas().get(0).getTargetTransform());
}
+
+ /**
+ * Issue #8128: deleting a transform used to leave the error handling of the
transform feeding it
+ * pointing at a transform that is no longer in the pipeline, which was then
written back to the
+ * file as a reference to a transform that isn't there.
+ */
+ @Test
+ void removingATransformDropsItsHopsAndTheErrorHandlingAimedAtIt() throws
Exception {
+ TransformMeta source = new TransformMeta("REST client", new FakeMeta());
+ TransformMeta errorTarget = new TransformMeta("Dummy (do nothing)", new
FakeMeta());
+ pipelineMeta.addTransform(source);
+ pipelineMeta.addTransform(errorTarget);
+ pipelineMeta.addPipelineHop(new PipelineHopMeta(source, errorTarget));
+
+ TransformErrorMeta errorMeta = new TransformErrorMeta(source, errorTarget);
+ errorMeta.setEnabled(true);
+ source.setTransformErrorMeta(errorMeta);
+
+ pipelineMeta.removeTransform(pipelineMeta.indexOfTransform(errorTarget));
+
+ assertEquals(0, pipelineMeta.nrPipelineHops(), "the hop to the deleted
transform must be gone");
+ assertNull(
+ source.getTransformErrorMeta(),
+ "the error rows have nowhere to go, so the error handling must be gone
too");
+ assertFalse(
+ pipelineMeta.getXml(new Variables()).contains("Dummy (do nothing)"),
+ "the deleted transform must not be named anywhere in the saved
pipeline");
+ }
+
+ /**
+ * A hop naming a transform that the file does not contain resolves to null,
so it is dropped
+ * rather than kept as half a hop that is neither drawn, executed, nor saved
in one piece.
+ */
+ @Test
+ void loadingDropsAHopThatNamesATransformNotInTheFile() throws Exception {
+ String xml =
+ "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
+ + "<pipeline>\n"
+ + " <transform><type>Dummy</type><name>Gen</name></transform>\n"
+ + " <transform><type>Dummy</type><name>Keep</name></transform>\n"
+ + " <order>\n"
+ + "
<hop><from>Gen</from><to>Keep</to><enabled>Y</enabled></hop>\n"
+ + " <hop><from>Gen</from><to>Renamed
away</to><enabled>Y</enabled></hop>\n"
+ + " </order>\n"
+ + "</pipeline>";
+
+ PipelineMeta loaded = new PipelineMeta();
+ loaded.loadXml(
+ XmlHandler.loadXmlString(xml, PipelineMeta.XML_TAG),
+ null,
+ metadataProvider,
+ new Variables());
+
+ assertEquals(1, loaded.nrPipelineHops(), "only the hop between two known
transforms survives");
+ assertEquals("Gen", loaded.getPipelineHop(0).getFromTransform().getName());
+ assertEquals("Keep", loaded.getPipelineHop(0).getToTransform().getName());
+ assertFalse(
+ loaded.getXml(new Variables()).contains("Renamed away"),
+ "saving must not write the unresolvable reference back");
+ }
+
+ /** Issue #8128: the error handling of the reported pipeline pointed at a
deleted transform. */
+ @Test
+ void loadingDropsErrorHandlingThatNamesATransformNotInTheFile() throws
Exception {
+ String xml =
+ "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
+ + "<pipeline>\n"
+ + " <transform><type>Dummy</type><name>REST
client</name></transform>\n"
+ + " <transform_error_handling>\n"
+ + " <error>\n"
+ + " <source_transform>REST client</source_transform>\n"
+ + " <target_transform>Dummy (do nothing)</target_transform>\n"
+ + " <is_enabled>Y</is_enabled>\n"
+ + " </error>\n"
+ + " </transform_error_handling>\n"
+ + "</pipeline>";
+
+ PipelineMeta loaded = new PipelineMeta();
+ loaded.loadXml(
+ XmlHandler.loadXmlString(xml, PipelineMeta.XML_TAG),
+ null,
+ metadataProvider,
+ new Variables());
+
+ assertTrue(loaded.getTransformErrorMetas().isEmpty(), "the error handling
has no target left");
+ assertFalse(loaded.findTransform("REST client").isDoingErrorHandling());
+ assertFalse(
+ loaded.getXml(new Variables()).contains("Dummy (do nothing)"),
+ "saving must not write the unresolvable reference back");
+ }
+
+ /**
+ * A disabled hop is a hop the user switched off, not a broken one: dropping
unresolvable
+ * references may not touch it, nor the error handling that runs over it.
+ */
+ @Test
+ void loadingKeepsDisabledHopsAndTheirErrorHandling() throws Exception {
+ String xml =
+ "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
+ + "<pipeline>\n"
+ + " <transform><type>Dummy</type><name>Gen</name></transform>\n"
+ + " <transform><type>Dummy</type><name>Good</name></transform>\n"
+ + "
<transform><type>Dummy</type><name>Errors</name></transform>\n"
+ + " <order>\n"
+ + "
<hop><from>Gen</from><to>Good</to><enabled>N</enabled></hop>\n"
+ + "
<hop><from>Gen</from><to>Errors</to><enabled>N</enabled></hop>\n"
+ + " </order>\n"
+ + " <transform_error_handling>\n"
+ + " <error>\n"
+ + " <source_transform>Gen</source_transform>\n"
+ + " <target_transform>Errors</target_transform>\n"
+ + " <is_enabled>Y</is_enabled>\n"
+ + " </error>\n"
+ + " </transform_error_handling>\n"
+ + "</pipeline>";
+
+ PipelineMeta loaded = new PipelineMeta();
+ loaded.loadXml(
+ XmlHandler.loadXmlString(xml, PipelineMeta.XML_TAG),
+ null,
+ metadataProvider,
+ new Variables());
+
+ assertEquals(2, loaded.nrPipelineHops(), "both disabled hops must survive
the load");
+ assertFalse(loaded.getPipelineHop(0).isEnabled());
+ assertFalse(loaded.getPipelineHop(1).isEnabled());
+ assertEquals(
+ 1,
+ loaded.getTransformErrorMetas().size(),
+ "the error handling still has its target, so it stays");
+ assertEquals(2, countOf(loaded.getXml(new Variables()),
"<enabled>N</enabled>"));
+ }
+
+ private static int countOf(String xml, String fragment) {
+ return xml.split(fragment, -1).length - 1;
+ }
}
diff --git a/engine/src/test/java/org/apache/hop/workflow/WorkflowMetaTest.java
b/engine/src/test/java/org/apache/hop/workflow/WorkflowMetaTest.java
index d9d9a9e22f..c1f4009c20 100644
--- a/engine/src/test/java/org/apache/hop/workflow/WorkflowMetaTest.java
+++ b/engine/src/test/java/org/apache/hop/workflow/WorkflowMetaTest.java
@@ -34,6 +34,7 @@ import org.apache.hop.core.annotations.Action;
import org.apache.hop.core.exception.HopException;
import org.apache.hop.core.exception.HopPluginException;
import org.apache.hop.core.listeners.IContentChangedListener;
+import org.apache.hop.core.logging.HopLogStore;
import org.apache.hop.core.plugins.ActionPluginType;
import org.apache.hop.core.plugins.PluginRegistry;
import org.apache.hop.core.variables.IVariables;
@@ -441,4 +442,93 @@ class WorkflowMetaTest {
assertEquals(copy2, copy23.getFrom());
assertEquals(copy3, copy23.getTo());
}
+
+ /**
+ * A hop naming an action that the file does not contain - the state {@code
+ * main-0012-fuzzymatch.hwf} was in before <a
href="https://github.com/apache/hop/pull/7989";>
+ * #7989</a> - resolves to null, so it is dropped rather than kept as half a
hop that is neither
+ * drawn, executed, nor saved in one piece.
+ */
+ @Test
+ void loadingDropsAHopThatNamesAnActionNotInTheFile() throws Exception {
+ String xml =
+ "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
+ + "<workflow>\n"
+ + " <name>fuzzymatch</name>\n"
+ + " <actions>\n"
+ + "
<action><name>Start</name><type>ActionFake</type></action>\n"
+ + " <action><name>Run Fuzzy match
tests</name><type>ActionFake</type></action>\n"
+ + " </actions>\n"
+ + " <hops>\n"
+ + " <hop><from>Start</from><to>Run Group By
tests</to><enabled>Y</enabled></hop>\n"
+ + " </hops>\n"
+ + "</workflow>";
+
+ // Dropping the hop is logged, and logging needs a log store to write to.
+ HopLogStore.init();
+
+ WorkflowMeta loaded = new WorkflowMeta();
+ loaded.loadXml(
+ XmlHandler.loadXmlString(xml, WorkflowMeta.XML_TAG),
+ null,
+ new MemoryMetadataProvider(),
+ variables);
+
+ assertEquals(2, loaded.nrActions());
+ assertEquals(0, loaded.nrWorkflowHops(), "the hop points at an action that
is not in the file");
+ assertFalse(
+ loaded.getXml(variables).contains("Run Group By tests"),
+ "saving must not write the unresolvable reference back");
+ }
+
+ /** Deleting an action takes the hops attached to it along, whoever does the
deleting. */
+ @Test
+ void removingAnActionDropsTheHopsAttachedToIt() {
+ ActionMeta first = new ActionMeta(new ActionDummy());
+ first.setName("first");
+ ActionMeta second = new ActionMeta(new ActionDummy());
+ second.setName("second");
+ ActionMeta third = new ActionMeta(new ActionDummy());
+ third.setName("third");
+ workflowMeta.addAction(first);
+ workflowMeta.addAction(second);
+ workflowMeta.addAction(third);
+ workflowMeta.addWorkflowHop(new WorkflowHopMeta(first, second));
+ workflowMeta.addWorkflowHop(new WorkflowHopMeta(second, third));
+
+ workflowMeta.removeAction(workflowMeta.indexOfAction(second));
+
+ assertEquals(0, workflowMeta.nrWorkflowHops(), "both hops were attached to
the deleted action");
+ }
+
+ /**
+ * A disabled hop is a hop the user switched off, not a broken one: dropping
unresolvable
+ * references may not touch it.
+ */
+ @Test
+ void loadingKeepsDisabledHops() throws Exception {
+ String xml =
+ "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
+ + "<workflow>\n"
+ + " <name>disabled</name>\n"
+ + " <actions>\n"
+ + "
<action><name>Start</name><type>ActionFake</type></action>\n"
+ + "
<action><name>Second</name><type>ActionFake</type></action>\n"
+ + " </actions>\n"
+ + " <hops>\n"
+ + "
<hop><from>Start</from><to>Second</to><enabled>N</enabled></hop>\n"
+ + " </hops>\n"
+ + "</workflow>";
+
+ WorkflowMeta loaded = new WorkflowMeta();
+ loaded.loadXml(
+ XmlHandler.loadXmlString(xml, WorkflowMeta.XML_TAG),
+ null,
+ new MemoryMetadataProvider(),
+ variables);
+
+ assertEquals(1, loaded.nrWorkflowHops(), "the disabled hop must survive
the load");
+ assertFalse(loaded.getWorkflowHop(0).isEnabled());
+ assertTrue(loaded.getXml(variables).contains("<enabled>N</enabled>"));
+ }
}
diff --git
a/rcp/src/test/java/org/apache/hop/ui/hopgui/file/workflow/HopGuiWorkflowGraphDanglingHopTest.java
b/rcp/src/test/java/org/apache/hop/ui/hopgui/file/workflow/HopGuiWorkflowGraphDanglingHopTest.java
new file mode 100644
index 0000000000..49801e3515
--- /dev/null
+++
b/rcp/src/test/java/org/apache/hop/ui/hopgui/file/workflow/HopGuiWorkflowGraphDanglingHopTest.java
@@ -0,0 +1,369 @@
+/*
+ * 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.hop.ui.hopgui.file.workflow;
+
+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
+
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicReference;
+import org.apache.hop.core.exception.HopException;
+import org.apache.hop.core.gui.AreaOwner;
+import org.apache.hop.core.gui.Point;
+import org.apache.hop.core.variables.Variables;
+import org.apache.hop.metadata.serializer.memory.MemoryMetadataProvider;
+import org.apache.hop.ui.core.PropsUi;
+import org.apache.hop.ui.hopgui.HopGuiEnvironment;
+import org.apache.hop.ui.hopgui.file.GraphCanvasTestBase;
+import org.apache.hop.ui.hopgui.file.IHopFileTypeHandler;
+import org.apache.hop.ui.hopgui.perspective.explorer.ExplorerPerspective;
+import org.apache.hop.workflow.WorkflowHopMeta;
+import org.apache.hop.workflow.WorkflowMeta;
+import org.apache.hop.workflow.action.ActionMeta;
+import org.eclipse.swt.SWT;
+import org.eclipse.swt.layout.GridLayout;
+import org.eclipse.swt.widgets.Canvas;
+import org.eclipse.swt.widgets.Shell;
+import org.eclipse.swtbot.swt.finder.SWTBot;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+/**
+ * Opening a workflow whose hop names an action that is not in the file - a
stale name left behind
+ * by a rename or by deleting the action elsewhere - and working on the canvas
afterwards.
+ *
+ * <p>The XML de-serializer resolves an unresolvable reference to null rather
than failing the whole
+ * file, so such a hop reaches the workflow with one end missing. Half a hop
is of no use to anyone:
+ * it is not drawn, it is not executed, saving the file writes it back without
its target, and the
+ * loop check of the next hop the user draws walks straight into it. So the
workflow drops it while
+ * loading, which is what these tests hold it to.
+ *
+ * <p>This is the state {@code
integration-tests/transforms/main-0012-fuzzymatch.hwf} was in before
+ * <a href="https://github.com/apache/hop/pull/7989";>#7989</a>, and the
workflow side of <a
+ * href="https://github.com/apache/hop/issues/8128";>issue #8128</a>.
+ */
+@Tag("uitest")
+class HopGuiWorkflowGraphDanglingHopTest extends GraphCanvasTestBase {
+
+ private static final String START_ACTION = "Start";
+ private static final String SOURCE_ACTION = "Run Fuzzy match tests";
+ private static final String TARGET_ACTION = "Verify results";
+
+ /** The name the hop in the file points at. No action carries it. */
+ private static final String MISSING_ACTION = "Run Group By tests";
+
+ private static final Point START_LOCATION = new Point(60, 60);
+ private static final Point SOURCE_LOCATION = new Point(280, 60);
+ private static final Point TARGET_LOCATION = new Point(500, 60);
+
+ @TempDir private Path folder;
+
+ @BeforeAll
+ static void registerGuiPlugins() throws HopException {
+ HopGuiEnvironment.init();
+ }
+
+ /** Every hop that survives the load has to be a hop the user can see, run
and save. */
+ @Test
+ void openingTheFileLeavesNoHalfBuiltHop() throws Exception {
+ WorkflowMeta workflowMeta = openWorkflowFile();
+
+ List<String> halfBuilt = new ArrayList<>();
+ for (WorkflowHopMeta hop : workflowMeta.getWorkflowHops()) {
+ if (hop.getFromAction() == null || hop.getToAction() == null) {
+ halfBuilt.add(hop.toString());
+ }
+ }
+
+ assertTrue(
+ halfBuilt.isEmpty(),
+ "the hop names an action that is not in the file, so it must not be
kept half built: "
+ + halfBuilt);
+ }
+
+ /** Opening the file and saving it again may not quietly rewrite the hop
without its target. */
+ @Test
+ void savingTheOpenedFileDoesNotWriteAHopWithoutATarget() throws Exception {
+ WorkflowMeta workflowMeta = openWorkflowFile();
+
+ String xml = workflowMeta.getXml(new Variables());
+ for (String hop : hopsOf(xml)) {
+ assertTrue(
+ hop.contains("<from>") && hop.contains("<to>"),
+ "saving wrote a hop that has lost an end, corrupting the file
further: " + hop);
+ }
+ }
+
+ /** The canvas has to paint the workflow the user just opened. */
+ @Test
+ void theCanvasPaintsTheOpenedWorkflow() throws Exception {
+ onCanvas(
+ (bot, graph, workflowMeta, spots) ->
+ assertAll(
+ () -> assertNotNull(spots.source, "the source action was never
painted"),
+ () -> assertNotNull(spots.target, "the target action was never
painted"),
+ () -> assertNoFailures()));
+ }
+
+ /**
+ * Drawing a hop on the workflow that was just opened. The loop check runs
over every hop of the
+ * workflow, so a half-built one would break a gesture that has nothing to
do with it.
+ */
+ @Test
+ void drawingANewHopWorksAfterOpeningTheFile() throws Exception {
+ onCanvas(
+ (bot, graph, workflowMeta, spots) -> {
+ ActionMeta source = workflowMeta.findAction(SOURCE_ACTION);
+ ActionMeta target = workflowMeta.findAction(TARGET_ACTION);
+
+ // Shift-drag from the source onto the target, exactly as in the hop
creation suite.
+ fire(spots.canvas, SWT.MouseDown, spots.scale, spots.source, 1,
SWT.SHIFT);
+ fire(
+ spots.canvas,
+ SWT.MouseMove,
+ spots.scale,
+ midpoint(spots.source, spots.target),
+ 0,
+ SWT.SHIFT | SWT.BUTTON1);
+ fire(spots.canvas, SWT.MouseMove, spots.scale, spots.target, 0,
SWT.SHIFT | SWT.BUTTON1);
+ String popup = releaseAndCatchDialog(bot, spots, spots.target,
SWT.SHIFT | SWT.BUTTON1);
+
+ WorkflowHopMeta created = awaitHop(bot, workflowMeta, source,
target);
+
+ assertAll(
+ () -> assertNoFailures(),
+ () -> assertNull(popup, "no dialog may open here, but one did: "
+ popup),
+ () ->
+ assertNotNull(
+ created,
+ "the gesture should have created the hop " + source + "
→ " + target));
+ });
+ }
+
+ /**
+ * Waits for the hop the gesture asked for: the graph finishes a gesture
through the event loop,
+ * so the hop is not there the instant the mouse button comes back up.
+ */
+ private WorkflowHopMeta awaitHop(
+ SWTBot bot, WorkflowMeta workflowMeta, ActionMeta from, ActionMeta to) {
+ for (int attempt = 0; attempt < 40; attempt++) {
+ WorkflowHopMeta hop = onUi(() -> workflowMeta.findWorkflowHop(from, to));
+ if (hop != null) {
+ return hop;
+ }
+ bot.sleep(50);
+ }
+ return null;
+ }
+
+ /**
+ * Releases the mouse button and reports the title of the dialog that
opened, or null when none
+ * did. Any dialog is closed again so the canvas event loop is handed back -
a release is
+ * dispatched asynchronously because a dialog runs an event loop of its own.
+ */
+ private String releaseAndCatchDialog(SWTBot bot, Spots spots, Point at, int
stateMask) {
+ Set<Shell> before = openShells();
+ fireAsync(spots.canvas, SWT.MouseUp, spots.scale, at, 1, stateMask);
+ Shell popup = awaitNewShell(bot, before);
+ String title = titleOf(popup);
+ closeShell(bot, popup);
+ return title;
+ }
+
+ // ------------------------------------------------------------------
assertions
+
+ private void assertNoFailures() {
+ assertTrue(swallowed.isEmpty(), "the canvas must not throw, but got " +
swallowed);
+ }
+
+ // ------------------------------------------------------------------ the
file
+
+ /** Loads the workflow from disk the way the Hop GUI does when the user
opens the file. */
+ private WorkflowMeta openWorkflowFile() throws Exception {
+ Path file = folder.resolve("dangling-hop.hwf");
+ Files.write(file, workflowXml().getBytes(StandardCharsets.UTF_8));
+ return new WorkflowMeta(
+ new Variables(), file.toAbsolutePath().toString(), new
MemoryMetadataProvider());
+ }
+
+ /** Start and two actions, with the only hop pointing at a name no action
carries. */
+ private static String workflowXml() {
+ return "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
+ + "<workflow>\n"
+ + " <name>dangling-hop</name>\n"
+ + " <actions>\n"
+ + startAction()
+ + dummyAction(SOURCE_ACTION, SOURCE_LOCATION)
+ + dummyAction(TARGET_ACTION, TARGET_LOCATION)
+ + " </actions>\n"
+ + " <hops>\n"
+ + " <hop>\n"
+ + " <from>"
+ + START_ACTION
+ + "</from>\n"
+ + " <to>"
+ + MISSING_ACTION
+ + "</to>\n"
+ + " <enabled>Y</enabled>\n"
+ + " <evaluation>Y</evaluation>\n"
+ + " <unconditional>Y</unconditional>\n"
+ + " </hop>\n"
+ + " </hops>\n"
+ + "</workflow>\n";
+ }
+
+ private static String startAction() {
+ return " <action>\n"
+ + " <name>"
+ + START_ACTION
+ + "</name>\n"
+ + " <type>SPECIAL</type>\n"
+ + " <start>Y</start>\n"
+ + " <repeat>N</repeat>\n"
+ + " <schedulerType>0</schedulerType>\n"
+ + " <intervalSeconds>0</intervalSeconds>\n"
+ + " <intervalMinutes>60</intervalMinutes>\n"
+ + " <hour>12</hour>\n"
+ + " <minutes>0</minutes>\n"
+ + " <weekDay>1</weekDay>\n"
+ + " <DayOfMonth>1</DayOfMonth>\n"
+ + " <parallel>N</parallel>\n"
+ + " <xloc>"
+ + START_LOCATION.x
+ + "</xloc>\n"
+ + " <yloc>"
+ + START_LOCATION.y
+ + "</yloc>\n"
+ + " <attributes/>\n"
+ + " </action>\n";
+ }
+
+ private static String dummyAction(String name, Point location) {
+ return " <action>\n"
+ + " <name>"
+ + name
+ + "</name>\n"
+ + " <type>DUMMY</type>\n"
+ + " <parallel>N</parallel>\n"
+ + " <xloc>"
+ + location.x
+ + "</xloc>\n"
+ + " <yloc>"
+ + location.y
+ + "</yloc>\n"
+ + " <attributes/>\n"
+ + " </action>\n";
+ }
+
+ /** The {@code <hop>} elements of a serialized workflow, one string each. */
+ private static List<String> hopsOf(String xml) {
+ List<String> hops = new ArrayList<>();
+ int from = xml.indexOf("<hop>");
+ while (from >= 0) {
+ int to = xml.indexOf("</hop>", from);
+ if (to < 0) {
+ fail("unbalanced <hop> element in the serialized workflow");
+ }
+ hops.add(xml.substring(from, to + "</hop>".length()));
+ from = xml.indexOf("<hop>", to);
+ }
+ return hops;
+ }
+
+ // ------------------------------------------------------------------ scene
+
+ /** What a test needs to aim at: the canvas, its scale and the interesting
graph coordinates. */
+ private record Spots(Canvas canvas, double scale, Point source, Point
target) {}
+
+ @FunctionalInterface
+ private interface CanvasTest {
+ void run(SWTBot bot, HopGuiWorkflowGraph graph, WorkflowMeta workflowMeta,
Spots spots);
+ }
+
+ private void onCanvas(CanvasTest test) throws Exception {
+ WorkflowMeta workflowMeta = openWorkflowFile();
+ AtomicReference<HopGuiWorkflowGraph> graphRef = new AtomicReference<>();
+
+ withScene(
+ shell -> {
+ shell.setSize(1000, 700);
+ shell.setLayout(new GridLayout(1, false));
+ PropsUi.getInstance().setUseDoubleClickOnCanvas(false);
+
+ graphRef.set(
+ new HopGuiWorkflowGraph(
+ shell,
+ hopGui(),
+ new TreelessExplorerPerspective(),
+ workflowMeta,
+ new HopWorkflowFileType<>()));
+ attachKeyboardShortcuts(shell);
+ },
+ bot -> {
+ HopGuiWorkflowGraph graph = graphRef.get();
+ test.run(bot, graph, workflowMeta, aim(bot, graph, workflowMeta));
+ });
+ }
+
+ /**
+ * The perspective of a test never built its file tree - that happens when
the real application
+ * shell opens - so the tree update a file-backed graph asks for on every
refresh has to be a
+ * no-op here. The graph itself is the real one.
+ */
+ private static class TreelessExplorerPerspective extends ExplorerPerspective
{
+ @Override
+ public void updateTreeItem(IHopFileTypeHandler fileTypeHandler) {
+ // the tree this would walk is only built by the application shell
+ }
+ }
+
+ private Spots aim(SWTBot bot, HopGuiWorkflowGraph graph, WorkflowMeta
workflowMeta) {
+ Canvas canvas = onUi(graph::getCanvas);
+ double scale = canvasToGraphScale(graph);
+ AreaLookup lookup = graph::getVisibleAreaOwner;
+
+ Point source =
+ awaitIcon(
+ bot,
+ lookup,
+ AreaOwner.AreaType.ACTION_ICON,
+ workflowMeta.findAction(SOURCE_ACTION),
+ SOURCE_LOCATION);
+ Point target =
+ awaitIcon(
+ bot,
+ lookup,
+ AreaOwner.AreaType.ACTION_ICON,
+ workflowMeta.findAction(TARGET_ACTION),
+ TARGET_LOCATION);
+
+ assertEquals(3, workflowMeta.nrActions(), "the file holds Start and two
actions");
+ return new Spots(canvas, scale, source, target);
+ }
+}
diff --git
a/ui/src/main/java/org/apache/hop/ui/hopgui/file/pipeline/delegates/HopGuiPipelineTransformDelegate.java
b/ui/src/main/java/org/apache/hop/ui/hopgui/file/pipeline/delegates/HopGuiPipelineTransformDelegate.java
index 34cfb87777..e1a5ee83ec 100644
---
a/ui/src/main/java/org/apache/hop/ui/hopgui/file/pipeline/delegates/HopGuiPipelineTransformDelegate.java
+++
b/ui/src/main/java/org/apache/hop/ui/hopgui/file/pipeline/delegates/HopGuiPipelineTransformDelegate.java
@@ -746,8 +746,8 @@ public class HopGuiPipelineTransformDelegate {
for (int i = pipelineMeta.nrPipelineHops() - 1; i >= 0; i--) {
PipelineHopMeta hi = pipelineMeta.getPipelineHop(i);
for (int j = 0; j < transforms.size() && hopIndex < hopIndexes.length;
j++) {
- if (hi.getFromTransform().equals(transforms.get(j))
- || hi.getToTransform().equals(transforms.get(j))) {
+ if (transforms.get(j).equals(hi.getFromTransform())
+ || transforms.get(j).equals(hi.getToTransform())) {
int idx = pipelineMeta.indexOfPipelineHop(hi);
pipelineHops.add((PipelineHopMeta) hi.clone());
hopIndexes[hopIndex] = idx;
diff --git
a/ui/src/main/java/org/apache/hop/ui/hopgui/file/workflow/delegates/HopGuiWorkflowActionDelegate.java
b/ui/src/main/java/org/apache/hop/ui/hopgui/file/workflow/delegates/HopGuiWorkflowActionDelegate.java
index 3d854252a4..1979eb1e37 100644
---
a/ui/src/main/java/org/apache/hop/ui/hopgui/file/workflow/delegates/HopGuiWorkflowActionDelegate.java
+++
b/ui/src/main/java/org/apache/hop/ui/hopgui/file/workflow/delegates/HopGuiWorkflowActionDelegate.java
@@ -337,7 +337,7 @@ public class HopGuiWorkflowActionDelegate {
for (int i = workflow.nrWorkflowHops() - 1; i >= 0; i--) {
WorkflowHopMeta hi = workflow.getWorkflowHop(i);
for (int j = 0; j < actions.size() && hopIndex < hopIndexes.length; j++)
{
- if (hi.getFromAction().equals(actions.get(j)) ||
hi.getToAction().equals(actions.get(j))) {
+ if (actions.get(j).equals(hi.getFromAction()) ||
actions.get(j).equals(hi.getToAction())) {
int idx = workflow.indexOfWorkflowHop(hi);
workflowHops.add((WorkflowHopMeta) hi.clone());
hopIndexes[hopIndex] = idx;
@@ -367,7 +367,7 @@ public class HopGuiWorkflowActionDelegate {
public void deleteAction(WorkflowMeta workflowMeta, ActionMeta action) {
for (int i = workflowMeta.nrWorkflowHops() - 1; i >= 0; i--) {
WorkflowHopMeta hi = workflowMeta.getWorkflowHop(i);
- if (hi.getFromAction().equals(action) ||
hi.getToAction().equals(action)) {
+ if (action.equals(hi.getFromAction()) ||
action.equals(hi.getToAction())) {
int idx = workflowMeta.indexOfWorkflowHop(hi);
hopGui.undoDelegate.addUndoDelete(
workflowMeta, new WorkflowHopMeta[] {(WorkflowHopMeta)
hi.clone()}, new int[] {idx});
