From c2003ea02076f8e90518699b5dfe8d03759755c2 Mon Sep 17 00:00:00 2001 From: Hans Van Akelyen Date: Fri, 24 Jul 2026 16:43:28 +0200 Subject: [PATCH] Run Options variables in pipeline do not appear anymore, fixes #2481 --- .../org/apache/hop/core/util/StringUtil.java | 24 ++- .../apache/hop/core/util/StringUtilTest.java | 35 +++++ .../hop/core/reflection/StringSearcher.java | 6 + .../org/apache/hop/pipeline/PipelineMeta.java | 4 +- .../org/apache/hop/workflow/WorkflowMeta.java | 4 +- .../core/reflection/StringSearcherTest.java | 145 +++++++++++------- 6 files changed, 159 insertions(+), 59 deletions(-) diff --git a/core/src/main/java/org/apache/hop/core/util/StringUtil.java b/core/src/main/java/org/apache/hop/core/util/StringUtil.java index 3688528bc44..0668ec52c58 100644 --- a/core/src/main/java/org/apache/hop/core/util/StringUtil.java +++ b/core/src/main/java/org/apache/hop/core/util/StringUtil.java @@ -369,9 +369,31 @@ public static boolean isSystemVariable(String aString) { public static void getUsedVariables( String aString, List list, boolean includeSystemVariables) { + // Include variable resolvers by default. This is what the encryption code relies on: a value + // like #{vault:hop/data/some-db:password} must be recognized as "contains variables" so it is + // not encrypted (see #7293). + getUsedVariables(aString, list, includeSystemVariables, true); + } + + /** + * Collect the variables used in the given string. + * + * @param aString the string to scan + * @param list the list to add the used variable names to + * @param includeSystemVariables whether to include already-set system variables + * @param includeResolvers whether to also collect variable resolver references (the {@code + * #{name:arguments}} syntax). These are not plain, user-settable variables, so callers that + * build a list of variables to present to the user (e.g. the run options dialog) should pass + * {@code false} to avoid polluting the list with resolver names and secret paths. Real {@code + * ${...}} variables nested inside resolver arguments are still collected by the UNIX scan. + */ + public static void getUsedVariables( + String aString, List list, boolean includeSystemVariables, boolean includeResolvers) { getUsedVariables(aString, UNIX_OPEN, UNIX_CLOSE, list, includeSystemVariables); getUsedVariables(aString, WINDOWS_OPEN, WINDOWS_CLOSE, list, includeSystemVariables); - getUsedVariables(aString, RESOLVER_OPEN, RESOLVER_CLOSE, list, includeSystemVariables); + if (includeResolvers) { + getUsedVariables(aString, RESOLVER_OPEN, RESOLVER_CLOSE, list, includeSystemVariables); + } } public static String generateRandomString( diff --git a/core/src/test/java/org/apache/hop/core/util/StringUtilTest.java b/core/src/test/java/org/apache/hop/core/util/StringUtilTest.java index fa4222d476e..41013a8a238 100644 --- a/core/src/test/java/org/apache/hop/core/util/StringUtilTest.java +++ b/core/src/test/java/org/apache/hop/core/util/StringUtilTest.java @@ -23,7 +23,9 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; +import java.util.ArrayList; import java.util.HashMap; +import java.util.List; import java.util.Map; import org.apache.hop.core.exception.HopRuntimeException; import org.junit.jupiter.api.Test; @@ -220,4 +222,37 @@ void testTrimEnd_None() { assertEquals("", StringUtil.trimEnd("/", '/')); assertEquals("", StringUtil.trimEnd("///", '/')); } + + /** By default (and for the encryption path) resolvers count as "used variables". */ + @Test + void testGetUsedVariablesIncludesResolversByDefault() { + List list = new ArrayList<>(); + StringUtil.getUsedVariables("#{vault:hop/data/some-db:password}", list, true); + assertTrue( + list.contains("vault:hop/data/some-db:password"), + "resolver reference should be collected by default"); + } + + /** + * The run options variable list excludes resolvers: a {@code #{...}} reference must not show up + * as a settable variable, but a real {@code ${...}} variable nested in the resolver arguments + * still must. + */ + @Test + void testGetUsedVariablesExcludesResolvers() { + List list = new ArrayList<>(); + StringUtil.getUsedVariables("#{vault:${env}/data/some-db:password}", list, false, false); + assertFalse( + list.stream().anyMatch(v -> v.startsWith("vault:")), + "resolver reference should not be collected when includeResolvers=false"); + assertTrue(list.contains("env"), "nested ${...} variable should still be collected"); + } + + @Test + void testGetUsedVariablesStillCollectsPlainVariables() { + List list = new ArrayList<>(); + StringUtil.getUsedVariables("${VARIABLE_1} and %%VARIABLE_2%%", list, false, false); + assertTrue(list.contains("VARIABLE_1")); + assertTrue(list.contains("VARIABLE_2")); + } } diff --git a/engine/src/main/java/org/apache/hop/core/reflection/StringSearcher.java b/engine/src/main/java/org/apache/hop/core/reflection/StringSearcher.java index f3592298246..2d6c0118414 100644 --- a/engine/src/main/java/org/apache/hop/core/reflection/StringSearcher.java +++ b/engine/src/main/java/org/apache/hop/core/reflection/StringSearcher.java @@ -176,6 +176,12 @@ private static void stringSearchInObject( for (int j = 0; j < objectArray.length; j++) { findMetaData(((Object[]) obj)[j], level + 1, stringList, parentObject, grandParentObject); } + } else if (obj instanceof Iterable iterable) { + for (Object element : iterable) { + if (element != null) { + stringSearchInObject(element, level, stringList, parentObject, grandParentObject, field); + } + } } else { findMetaData(obj, level + 1, stringList, parentObject, grandParentObject); } 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 b45e0aace9e..b53a7c7db2c 100644 --- a/engine/src/main/java/org/apache/hop/pipeline/PipelineMeta.java +++ b/engine/src/main/java/org/apache/hop/pipeline/PipelineMeta.java @@ -2915,8 +2915,10 @@ public List getUsedVariables() { List varList = new ArrayList<>(); // Look around in the strings, see what we find... + // Exclude variable resolvers (#{...}): their content is a resolver name plus a secret path, not + // a user-settable variable, so it shouldn't end up in the run options variable list. for (StringSearchResult result : stringList) { - StringUtil.getUsedVariables(result.getString(), varList, false); + StringUtil.getUsedVariables(result.getString(), varList, false, false); } return varList; 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 be0d4c550fb..05f2fc36a8c 100644 --- a/engine/src/main/java/org/apache/hop/workflow/WorkflowMeta.java +++ b/engine/src/main/java/org/apache/hop/workflow/WorkflowMeta.java @@ -1366,8 +1366,10 @@ public List getUsedVariables() { List varList = new ArrayList<>(); // Look around in the strings, see what we find... + // Exclude variable resolvers (#{...}): their content is a resolver name plus a secret path, not + // a user-settable variable, so it shouldn't end up in the run options variable list. for (StringSearchResult result : stringList) { - StringUtil.getUsedVariables(result.getString(), varList, false); + StringUtil.getUsedVariables(result.getString(), varList, false, false); } return varList; diff --git a/engine/src/test/java/org/apache/hop/core/reflection/StringSearcherTest.java b/engine/src/test/java/org/apache/hop/core/reflection/StringSearcherTest.java index 387e904c629..af3889c3ee5 100644 --- a/engine/src/test/java/org/apache/hop/core/reflection/StringSearcherTest.java +++ b/engine/src/test/java/org/apache/hop/core/reflection/StringSearcherTest.java @@ -16,68 +16,101 @@ */ package org.apache.hop.core.reflection; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.List; import org.apache.hop.junit.rules.RestoreHopEngineEnvironmentExtension; +import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; -// import org.apache.hop.pipeline.transforms.filterrows.FilterRowsMeta; - @ExtendWith(RestoreHopEngineEnvironmentExtension.class) public class StringSearcherTest { - // TODO: Move Test - /* @Test - public void testSearchConditionCase() { - String dummyTransformName = "Output"; - DummyMeta dummyMeta = new DummyMeta(); - String dummyTransformPid = PluginRegistry.getInstance().getPluginId( TransformPluginType.class, dummyMeta ); - TransformMeta dummyTransform = new TransformMeta( dummyTransformPid, dummyTransformname, dummyMeta ); - - List stringList = new ArrayList(); - StringSearcher.findMetaData( dummyTransform, 0, stringList, dummyMeta, 0 ); - - int checkCount = 0; - String aResult = null; - // Check that it found a couple of fields and emits the values properly - for ( int i = 0; i < stringList.size(); i++ ) { - aResult = stringList.get( i ).toString(); - if ( aResult.endsWith( "Dummy (transformId)" ) ) { - checkCount++; - } else if ( aResult.endsWith( "Output (name)" ) ) { - checkCount++; - } - if ( checkCount == 2 ) { - break; - } + /** A leaf metadata object holding a variable reference, e.g. a GetVariables field definition. */ + public static class Field { + private String variableString; + + public Field(String variableString) { + this.variableString = variableString; + } + + public String getVariableString() { + return variableString; + } + } + + /** Metadata shape used before the @HopMetadataProperty migration: an array of objects. */ + public static class ArrayHolder { + private Field[] fields; + + public Field[] getFields() { + return fields; } - assertEquals( 2, checkCount ); - - FilterRowsMeta filterRowsMeta = new FilterRowsMeta(); - Condition condition = new Condition(); - condition.setNegated( false ); - condition.setLeftValuename( "wibble_t" ); - condition.setRightValuename( "wobble_s" ); - condition.setFunction( org.apache.hop.core.Condition.FUNC_EQUAL ); - filterRowsMeta.setDefault(); - filterRowsMeta.setCondition( condition ); - - String filterRowsPluginPid = PluginRegistry.getInstance().getPluginId( TransformPluginType.class, filterRowsMeta ); - TransformMeta filterRowsTransform = new TransformMeta( filterRowsPluginPid, "Filter Rows", filterRowsMeta ); - - stringList.clear(); - StringSearcher.findMetaData( filterRowsTransform, 0, stringList, filterRowsMeta, 0 ); - - checkCount = 0; - for ( int i = 0; i < stringList.size(); i++ ) { - aResult = stringList.get( i ).toString(); - if ( aResult.endsWith( "FilterRows (transformId)" ) ) { - checkCount++; - } else if ( aResult.endsWith( "Filter Rows (name)" ) ) { - checkCount++; - } - if ( checkCount == 2 ) { - break; - } + } + + /** Metadata shape used after the migration: a List of objects (issue #2481). */ + public static class ListHolder { + private List fields; + + public List getFields() { + return fields; } - assertEquals( 2, checkCount ); - }*/ + } + + /** A List of plain strings must also have its elements discovered. */ + public static class StringListHolder { + private List values; + + public List getValues() { + return values; + } + } + + private static boolean containsString(List results, String expected) { + return results.stream().anyMatch(r -> expected.equals(r.getString())); + } + + @Test + public void testFindMetaDataScansObjectArray() { + ArrayHolder holder = new ArrayHolder(); + holder.fields = new Field[] {new Field("${VARIABLE_1}"), new Field("${VARIABLE_2}")}; + + List results = new ArrayList<>(); + StringSearcher.findMetaData(holder, 1, results, holder, holder); + + assertTrue(containsString(results, "${VARIABLE_1}"), "array element 1 not found"); + assertTrue(containsString(results, "${VARIABLE_2}"), "array element 2 not found"); + } + + /** + * Regression test for #2481: variables + * nested in List-based metadata (the @HopMetadataProperty model most transforms migrated to) were + * no longer discovered, so Run Options no longer pre-populated them. + */ + @Test + public void testFindMetaDataScansObjectList() { + ListHolder holder = new ListHolder(); + holder.fields = new ArrayList<>(); + holder.fields.add(new Field("${VARIABLE_1}")); + holder.fields.add(new Field("${VARIABLE_2}")); + + List results = new ArrayList<>(); + StringSearcher.findMetaData(holder, 1, results, holder, holder); + + assertTrue(containsString(results, "${VARIABLE_1}"), "list element 1 not found"); + assertTrue(containsString(results, "${VARIABLE_2}"), "list element 2 not found"); + } + + @Test + public void testFindMetaDataScansStringList() { + StringListHolder holder = new StringListHolder(); + holder.values = new ArrayList<>(); + holder.values.add("${VARIABLE_1}"); + + List results = new ArrayList<>(); + StringSearcher.findMetaData(holder, 1, results, holder, holder); + + assertTrue(containsString(results, "${VARIABLE_1}"), "string list element not found"); + } }