Merge pull request #2963 from nadment/1989

Cleanup XML of action Set Variables #1989

plugins/actions/setvariables/src/main/java/org/apache/hop/workflow/actions/setvariables/ActionSetVariablesDialog.java

@@ -17,6 +17,8 @@
 
 package org.apache.hop.workflow.actions.setvariables;
 
+import java.util.ArrayList;
+import java.util.List;
 import org.apache.hop.core.Const;
 import org.apache.hop.core.util.Utils;
 import org.apache.hop.core.variables.IVariables;
@@ -33,11 +35,11 @@
 import org.apache.hop.workflow.WorkflowMeta;
 import org.apache.hop.workflow.action.IAction;
 import org.apache.hop.workflow.action.IActionDialog;
+import org.apache.hop.workflow.actions.setvariables.ActionSetVariables.VariableDefinition;
+import org.apache.hop.workflow.actions.setvariables.ActionSetVariables.VariableType;
 import org.eclipse.swt.SWT;
 import org.eclipse.swt.custom.CCombo;
 import org.eclipse.swt.events.ModifyListener;
-import org.eclipse.swt.events.SelectionAdapter;
-import org.eclipse.swt.events.SelectionEvent;
 import org.eclipse.swt.layout.FormAttachment;
 import org.eclipse.swt.layout.FormData;
 import org.eclipse.swt.layout.FormLayout;
@@ -170,7 +172,7 @@ public IAction open() {
     fdFileVariableType.top = new FormAttachment(wFilename, margin);
     fdFileVariableType.right = new FormAttachment(100, 0);
     wFileVariableType.setLayoutData(fdFileVariableType);
-    wFileVariableType.setItems(ActionSetVariables.getVariableTypeDescriptions());
+    wFileVariableType.setItems(VariableType.getDescriptions());
 
     FormData fdgFilename = new FormData();
     fdgFilename.left = new FormAttachment(0, margin);
@@ -206,13 +208,7 @@ public IAction open() {
     fdVarSubs.top = new FormAttachment(wlVarSubs, 0, SWT.CENTER);
     fdVarSubs.right = new FormAttachment(100, 0);
     wVarSubs.setLayoutData(fdVarSubs);
-    wVarSubs.addSelectionListener(
-        new SelectionAdapter() {
-          @Override
-          public void widgetSelected(SelectionEvent e) {
-            action.setChanged();
-          }
-        });
+    wVarSubs.addListener(SWT.Selection, e -> action.setChanged());
 
     FormData fdgSettings = new FormData();
     fdgSettings.left = new FormAttachment(0, margin);
@@ -232,10 +228,7 @@ public void widgetSelected(SelectionEvent e) {
     fdlFields.top = new FormAttachment(gSettings, margin);
     wlFields.setLayoutData(fdlFields);
 
-    int rows =
-        action.variableName == null
-            ? 1
-            : (action.variableName.length == 0 ? 0 : action.variableName.length);
+    int rows = action.getVariableDefinitions().size();
     final int FieldsRows = rows;
 
     ColumnInfo[] colinf = {
@@ -250,7 +243,7 @@ public void widgetSelected(SelectionEvent e) {
       new ColumnInfo(
           BaseMessages.getString(PKG, "ActionSetVariables.Fields.Column.VariableType"),
           ColumnInfo.COLUMN_TYPE_CCOMBO,
-          ActionSetVariables.getVariableTypeDescriptions(),
+          VariableType.getDescriptions(),
           false),
     };
     colinf[0].setUsingVariables(true);
@@ -285,22 +278,17 @@ public void getData() {
     wName.setText(Const.nullToEmpty(action.getName()));
 
     wFilename.setText(Const.NVL(action.getFilename(), ""));
-    wFileVariableType.setText(
-        ActionSetVariables.getVariableTypeDescription(action.getFileVariableType()));
+    wFileVariableType.setText(action.getFileVariableType().getDescription());
 
     wVarSubs.setSelection(action.isReplaceVars());
 
-    if (action.variableName != null) {
-      for (int i = 0; i < action.variableName.length; i++) {
-        TableItem ti = wFields.table.getItem(i);
-        if (action.variableName[i] != null) {
-          ti.setText(1, action.variableName[i]);
-        }
-        if (action.getVariableValue()[i] != null) {
-          ti.setText(2, action.getVariableValue()[i]);
-        }
-
-        ti.setText(3, ActionSetVariables.getVariableTypeDescription(action.getVariableType()[i]));
+    if (action.getVariableDefinitions() != null) {
+      int i = 0;
+      for (VariableDefinition definition : action.getVariableDefinitions()) {
+        TableItem item = wFields.table.getItem(i++);
+        item.setText(1, Const.nullToEmpty(definition.getName()));
+        item.setText(2, Const.nullToEmpty(definition.getValue()));
+        item.setText(3, definition.getType().getDescription());
       }
       wFields.setRowNums();
       wFields.optWidth(true);
@@ -324,37 +312,23 @@ private void ok() {
       mb.open();
       return;
     }
-    action.setName(wName.getText());
 
+    action.setName(wName.getText());
     action.setFilename(wFilename.getText());
-    action.setFileVariableType(ActionSetVariables.getVariableType(wFileVariableType.getText()));
+    action.setFileVariableType(VariableType.lookupDescription(wFileVariableType.getText()));
     action.setReplaceVars(wVarSubs.getSelection());
 
     int nrItems = wFields.nrNonEmpty();
-    int nr = 0;
-    for (int i = 0; i < nrItems; i++) {
-      String arg = wFields.getNonEmpty(i).getText(1);
-      if (arg != null && arg.length() != 0) {
-        nr++;
-      }
-    }
-    action.variableName = new String[nr];
-    action.variableValue = new String[nr];
-    action.variableType = new int[nr];
-
-    nr = 0;
+    List<VariableDefinition> list = new ArrayList<>();
     for (int i = 0; i < nrItems; i++) {
-      String varname = wFields.getNonEmpty(i).getText(1);
-      String varvalue = wFields.getNonEmpty(i).getText(2);
-      String vartype = wFields.getNonEmpty(i).getText(3);
-
-      if (varname != null && varname.length() != 0) {
-        action.variableName[nr] = varname;
-        action.variableValue[nr] = varvalue;
-        action.variableType[nr] = ActionSetVariables.getVariableType(vartype);
-        nr++;
+      String name = wFields.getNonEmpty(i).getText(1);
+      if (name != null && name.length() != 0) {
+        String value = wFields.getNonEmpty(i).getText(2);
+        VariableType scope = VariableType.lookupDescription(wFields.getNonEmpty(i).getText(3));
+        list.add(new VariableDefinition(name, value, scope));
       }
     }
+    action.setVariableDefinitions(list);
 
     dispose();
   }

plugins/actions/setvariables/src/main/resources/org/apache/hop/workflow/actions/setvariables/messages/messages_en_US.properties

@@ -34,7 +34,6 @@ ActionSetVariables.UnExcpectedError=Unexpected error\! {0}
 ActionSetVariables.Title=Set variables
 ActionSetVariables.Fields.Column.VariableType=Variable scope type
 ActionSetVariables.VariableType.CurrentWorkflow=Valid in the current workflow
-ActionSetVariables.Meta.UnableLoadXML=Unable to load action of type ''Set variables'' from XML node.Exception \: {0}
 ActionSetVariables.Fields.Column.Value=Value
 ActionSetVariables.VariableType.ParentWorkflow=Valid in the parent workflow
 ActionSetVariables.Settings.Label=Settings

plugins/actions/setvariables/src/main/resources/org/apache/hop/workflow/actions/setvariables/messages/messages_es_AR.properties

@@ -32,7 +32,6 @@ ActionSetVariables.UnExcpectedError=Error inesperado\: {0}
 ActionSetVariables.Title=Establecer Variables
 ActionSetVariables.Fields.Column.VariableType=Alcance de la variable
 ActionSetVariables.VariableType.CurrentWorkflow=V\u00E1lido en el presente trabajo
-ActionSetVariables.Meta.UnableLoadXML=Imposible cargar la entrada de trabajo del tipo "Establecer Variables" desde el nodo XML. Excepci\u00F3n\: {0}
 ActionSetVariables.Fields.Column.Value=Valor
 ActionSetVariables.VariableType.ParentWorkflow=V\u00E1lido en el trabajo padre
 ActionSetVariables.Settings.Label=Ajustes

plugins/actions/setvariables/src/main/resources/org/apache/hop/workflow/actions/setvariables/messages/messages_fr_FR.properties

@@ -34,7 +34,6 @@ ActionSetVariables.UnExcpectedError=Erreur inattendue\! {0}
 ActionSetVariables.Title=Affectation variables
 ActionSetVariables.Fields.Column.VariableType=Port\u00E9e variable
 ActionSetVariables.VariableType.CurrentWorkflow=Valide dans le workflow courant
-ActionSetVariables.Meta.UnableLoadXML=Impossible de charger depuis le fichier XML, l''action de type ''Affectation variables''. Exception\:{0}
 ActionSetVariables.Fields.Column.Value=Valeur
 ActionSetVariables.VariableType.ParentWorkflow=Valide dans le workflow parent
 ActionSetVariables.Settings.Label=Param\u00E8tres

plugins/actions/setvariables/src/main/resources/org/apache/hop/workflow/actions/setvariables/messages/messages_it_IT.properties

@@ -36,7 +36,6 @@ ActionSetVariables.UnExcpectedError=Errore inatteso\! {0}
 ActionSetVariables.Title=Imposta variabili
 ActionSetVariables.Fields.Column.VariableType=Tipo di scope per la variabile
 ActionSetVariables.VariableType.CurrentWorkflow=Valido nel workflow corrente
-ActionSetVariables.Meta.UnableLoadXML=Impossibile caricare la action di tipo ''Imposta variabili'' dal nodo XML. Eccezione\: {0}
 ActionSetVariables.Error.UnableReadPropertiesFile=Errore durante la lettura del file di propriet\u00E0 ''{0}''\: 
 ActionSetVariables.Fields.Column.Value=Valore
 ActionSetVariables.VariableType.ParentWorkflow=Valido nel workflow padre

plugins/actions/setvariables/src/main/resources/org/apache/hop/workflow/actions/setvariables/messages/messages_ja_JP.properties

@@ -34,7 +34,6 @@ ActionSetVariables.UnExcpectedError=\u4e88\u671f\u305b\u306c\u30a8\u30e9\u30fc {
 ActionSetVariables.Title=\u5909\u6570\u3092\u8a2d\u5b9a
 ActionSetVariables.Fields.Column.VariableType=\u5909\u6570\u306e\u30b9\u30b3\u30fc\u30d7
 ActionSetVariables.VariableType.CurrentWorkflow=\u6709\u52b9\u306a\u73fe\u5728\u306e\u30b8\u30e7\u30d6
-ActionSetVariables.Meta.UnableLoadXML=Unable to load action of type ''Set variables'' from XML node.Exception \: {0}
 ActionSetVariables.Fields.Column.Value=\u5024
 ActionSetVariables.VariableType.ParentWorkflow=\u6709\u52b9\u306a\u89aa\u30b8\u30e7\u30d6
 ActionSetVariables.Settings.Label=\u8a2d\u5b9a

plugins/actions/setvariables/src/main/resources/org/apache/hop/workflow/actions/setvariables/messages/messages_ko_KR.properties

@@ -32,7 +32,6 @@ ActionSetVariables.UnExcpectedError=\uC54C \uC218 \uC5C6\uB294 \uC624\uB958\! {0
 ActionSetVariables.Title=Set variables
 ActionSetVariables.Fields.Column.VariableType=\uBCC0\uC218 \uBC94\uC704 \uC885\uB958
 ActionSetVariables.VariableType.CurrentWorkflow=Valid in the current workflow
-ActionSetVariables.Meta.UnableLoadXML=XML \uB178\uB4DC\uC5D0\uC11C Workflow \uC5D4\uD2B8\uB9AC \uD615\uC2DD 'Set variables''\uC744 \uB85C\uB4DC\uD560 \uC218 \uC5C6\uC2B5\uB2C8\uB2E4. \uC608\uC678\:{1} 
 ActionSetVariables.Fields.Column.Value=\uAC12
 ActionSetVariables.VariableType.ParentWorkflow=Valid in the parent workflow
 ActionSetVariables.Settings.Label=\uC124\uC815

plugins/actions/setvariables/src/main/resources/org/apache/hop/workflow/actions/setvariables/messages/messages_zh_CN.properties

@@ -27,7 +27,6 @@ ActionSetVariables.FileVariableType.Label=\u53D8\u91CF\u6709\u6548\u8303\u56F4
 ActionSetVariables.Filename.Label=\u5C5E\u6027\u6587\u4EF6\u540D
 ActionSetVariables.FilenameGroup.Label=\u5C5E\u6027\u6587\u4EF6
 ActionSetVariables.Log.SetVariableToValue=Set variable {0} to value [{1}]
-ActionSetVariables.Meta.UnableLoadXML=Unable to load action of type ''Set variables'' from XML node.Exception \: {0}
 ActionSetVariables.Name=\u8BBE\u7F6E\u53D8\u91CF
 ActionSetVariables.Name.Default=\u8BBE\u7F6E\u53D8\u91CF
 ActionSetVariables.Name.Label=Action \u540D\u79F0\:

plugins/actions/setvariables/src/test/java/org/apache/hop/workflow/actions/setvariables/WorkflowEntrySetVariablesTest.java → plugins/actions/setvariables/src/test/java/org/apache/hop/workflow/actions/setvariables/ActionSetVariablesTest.java

@@ -17,6 +17,11 @@
 
 package org.apache.hop.workflow.actions.setvariables;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.mock;
+
 import java.io.ByteArrayInputStream;
 import java.io.File;
 import java.io.FileNotFoundException;
@@ -26,14 +31,19 @@
 import java.nio.charset.StandardCharsets;
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.ParserConfigurationException;
+import org.apache.hop.core.HopClientEnvironment;
 import org.apache.hop.core.Result;
 import org.apache.hop.core.logging.HopLogStore;
 import org.apache.hop.core.variables.Variables;
 import org.apache.hop.core.xml.XmlHandler;
 import org.apache.hop.core.xml.XmlParserFactoryProducer;
 import org.apache.hop.metadata.api.IHopMetadataProvider;
+import org.apache.hop.metadata.serializer.memory.MemoryMetadataProvider;
 import org.apache.hop.workflow.WorkflowMeta;
 import org.apache.hop.workflow.action.ActionMeta;
+import org.apache.hop.workflow.action.ActionSerializationTestUtil;
+import org.apache.hop.workflow.actions.setvariables.ActionSetVariables.VariableDefinition;
+import org.apache.hop.workflow.actions.setvariables.ActionSetVariables.VariableType;
 import org.apache.hop.workflow.engine.IWorkflowEngine;
 import org.apache.hop.workflow.engines.local.LocalWorkflowEngine;
 import org.junit.After;
@@ -46,12 +56,7 @@
 import org.w3c.dom.Node;
 import org.xml.sax.SAXException;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertTrue;
-import static org.mockito.Mockito.mock;
-
-public class WorkflowEntrySetVariablesTest {
+public class ActionSetVariablesTest {
   private IWorkflowEngine<WorkflowMeta> workflow;
   private ActionSetVariables action;
 
@@ -75,6 +80,41 @@ public void setUp() throws Exception {
   @After
   public void tearDown() throws Exception {}
 
+  @Test
+  public void testSerialization() throws Exception {
+    HopClientEnvironment.init();
+    MemoryMetadataProvider provider = new MemoryMetadataProvider();
+
+    ActionSetVariables action =
+        ActionSerializationTestUtil.testSerialization(
+            "/set-variables-action.xml", ActionSetVariables.class, provider);
+
+    // Properties file variables
+    assertEquals(VariableType.CURRENT_WORKFLOW, action.getFileVariableType());
+    assertEquals("filename.txt", action.getFilename());
+
+    // Settings
+    assertTrue(action.isReplaceVars());
+
+    // Static variables
+    VariableDefinition definition = action.getVariableDefinitions().get(0);
+    assertEquals("VAR_CURRENT", definition.getName());
+    assertEquals("current", definition.getValue());
+    assertEquals(VariableType.CURRENT_WORKFLOW, definition.getType());
+    definition = action.getVariableDefinitions().get(1);
+    assertEquals("VAR_PARENT", definition.getName());
+    assertEquals("parent", definition.getValue());
+    assertEquals(VariableType.PARENT_WORKFLOW, definition.getType());
+    definition = action.getVariableDefinitions().get(2);
+    assertEquals("VAR_ROOT", definition.getName());
+    assertEquals("root", definition.getValue());
+    assertEquals(VariableType.ROOT_WORKFLOW, definition.getType());
+    definition = action.getVariableDefinitions().get(3);
+    assertEquals("VAR_JVM", definition.getName());
+    assertEquals("jvm", definition.getValue());
+    assertEquals(VariableType.JVM, definition.getType());
+  }
+
   @Test
   public void testASCIIText() throws Exception {
     // properties file with native2ascii
@@ -129,7 +169,7 @@ public void testInputStreamClosed() throws Exception {
   public void testParentJobVariablesExecutingFilePropertiesThatChangesVariablesAndParameters()
       throws Exception {
     action.setReplaceVars(true);
-    action.setFileVariableType(1);
+    action.setFileVariableType(VariableType.CURRENT_WORKFLOW);
 
     IWorkflowEngine<WorkflowMeta> parentWorkflow = action.getParentWorkflow();
 
@@ -186,7 +226,7 @@ public void testJobEntrySetVariablesExecute_VARIABLE_TYPE_JVM_NullVariable() thr
   }
 
   @Test
-  public void testJobEntrySetVariablesExecute_VARIABLE_TYPE_CURRENT_WORKFLOW_NullVariable()
+  public void testSetVariablesExecute_VARIABLE_TYPE_CURRENT_WORKFLOW_NullVariable()
       throws Exception {
     IHopMetadataProvider metadataProvider = mock(IHopMetadataProvider.class);
     action.loadXml(
@@ -197,7 +237,7 @@ public void testJobEntrySetVariablesExecute_VARIABLE_TYPE_CURRENT_WORKFLOW_NullV
   }
 
   @Test
-  public void testJobEntrySetVariablesExecute_VARIABLE_TYPE_JVM_VariableNotNull() throws Exception {
+  public void testSetVariablesExecute_VARIABLE_TYPE_JVM_VariableNotNull() throws Exception {
     IHopMetadataProvider metadataProvider = mock(IHopMetadataProvider.class);
     action.loadXml(
         getEntryNode("variableNotNull", "someValue", "JVM"), metadataProvider, new Variables());
@@ -208,7 +248,7 @@ public void testJobEntrySetVariablesExecute_VARIABLE_TYPE_JVM_VariableNotNull()
   }
 
   @Test
-  public void testJobEntrySetVariablesExecute_VARIABLE_TYPE_CURRENT_WORKFLOW_VariableNotNull()
+  public void testSetVariablesExecute_VARIABLE_TYPE_CURRENT_WORKFLOW_VariableNotNull()
       throws Exception {
     IHopMetadataProvider metadataProvider = mock(IHopMetadataProvider.class);
     action.loadXml(