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 84d69df701 Issue #8495 : Optionally resolve OS environment variables
as Hop variables (#8638)
84d69df701 is described below
commit 84d69df701dcc02ac28ad3f2493965924e06cccb
Author: vbhanuchander-lang <[email protected]>
AuthorDate: Mon Oct 19 03:29:02 2026 -0400
Issue #8495 : Optionally resolve OS environment variables as Hop variables
(#8638)
* Issue #8495 : Optionally resolve OS environment variables as Hop variables
Variables.initializeFrom seeds the variable space from
System.getProperties()
and the described variables in hop-config.json, but never from
System.getenv(). An exported environment variable is therefore not
resolvable,
so a project Home folder of ${PROJECT_HOME_PATH} stays literal even though
ProjectConfig does call variables.resolve() on it.
HOP_IMPORT_ENVIRONMENT_VARIABLES, off by default, makes the environment
part of
the variable space. It is read through HopResolvedSettings so the flag
itself
can be set from the environment, which is what a container deployment has to
hand.
Off by default because the whole environment becomes visible as variables
when
it is on, which is the bleed-over Matt raised on the issue. Turning it on
is a
deliberate act by whoever runs Hop.
The environment is seeded before everything else, so it has the lowest
precedence: a name also set with -D, in hop-config.json, by a parent or by
injection keeps the value it resolves to today, and the environment only
supplies names nothing else defines. No existing configuration changes
behaviour, whether the flag is on or off.
That ordering also matches HopResolvedSettings, which already resolves
hop-config then environment then -D, and whose javadoc notes that the
variable
space is not yet aligned with it.
Three tests: the environment stays out by default, ${NAME} resolves an
exported
variable when the flag is on, and -D still wins over the environment. The
second
fails without this change.
* Issue #8495 : Let an exported HOP_IMPORT_ENVIRONMENT_VARIABLES win over
the seeded default
HopEnvironment copies the hop-config value of every described variable onto
the system properties, so HopResolvedSettings.resolveString always saw the
default N in the JVM layer and an exported flag was never read. A system
property now only counts as a -D override when it differs from the
hop-config
value. The decision takes the three layers as arguments so it can be tested
without changing the process environment.
---
core/src/main/java/org/apache/hop/core/Const.java | 14 +++
.../org/apache/hop/core/variables/Variables.java | 47 ++++++++
.../apache/hop/core/variables/VariablesTest.java | 128 +++++++++++++++++++++
3 files changed, 189 insertions(+)
diff --git a/core/src/main/java/org/apache/hop/core/Const.java
b/core/src/main/java/org/apache/hop/core/Const.java
index 79ace80d08..0aeadf8c8f 100644
--- a/core/src/main/java/org/apache/hop/core/Const.java
+++ b/core/src/main/java/org/apache/hop/core/Const.java
@@ -828,6 +828,20 @@ public class Const {
"Set this variable to 'Y' to disable standard Hop logging to the
console. (stdout)")
public static final String HOP_DISABLE_CONSOLE_LOGGING =
"HOP_DISABLE_CONSOLE_LOGGING";
+ /**
+ * Set this variable to Y to make the operating system environment available
as Hop variables, so
+ * that ${NAME} resolves an exported environment variable. Off by default,
because the whole
+ * environment becomes visible as variables when it is on. Environment
variables have the lowest
+ * precedence: anything set with -D, in hop-config.json, or by a parent
keeps its current value.
+ */
+ @Variable(
+ value = "N",
+ description =
+ "Set this variable to 'Y' to make operating system environment
variables available as Hop"
+ + " variables, so that ${NAME} resolves an exported environment
variable. They have"
+ + " the lowest precedence: anything set with -D or in
hop-config.json wins.")
+ public static final String HOP_IMPORT_ENVIRONMENT_VARIABLES =
"HOP_IMPORT_ENVIRONMENT_VARIABLES";
+
/** The name of the variable containing an alternative default number format
*/
@Variable(
description = "The name of the variable containing an alternative
default number format")
diff --git a/core/src/main/java/org/apache/hop/core/variables/Variables.java
b/core/src/main/java/org/apache/hop/core/variables/Variables.java
index 3690ff9e9a..0afaa39d17 100644
--- a/core/src/main/java/org/apache/hop/core/variables/Variables.java
+++ b/core/src/main/java/org/apache/hop/core/variables/Variables.java
@@ -26,6 +26,7 @@ import java.util.Set;
import org.apache.commons.lang3.StringUtils;
import org.apache.hop.core.Const;
import org.apache.hop.core.config.HopConfig;
+import org.apache.hop.core.config.HopResolvedSettings;
import org.apache.hop.core.exception.HopException;
import org.apache.hop.core.exception.HopRuntimeException;
import org.apache.hop.core.exception.HopValueException;
@@ -112,6 +113,15 @@ public class Variables implements IVariables {
public void initializeFrom(IVariables parent) {
this.parent = parent;
+ // The operating system environment, only when explicitly enabled. It is
seeded before
+ // everything else so that it has the lowest precedence: a name that is
also set with -D, in
+ // hop-config.json, by a parent or by injection keeps the value it
resolves to today, and the
+ // environment only supplies names nothing else defines.
+ //
+ if (isEnvironmentImported()) {
+ getProperties().putAll(System.getenv());
+ }
+
// Clone the system properties to avoid ConcurrentModificationException
while iterating
// and then add all of them to properties variable.
//
@@ -135,6 +145,43 @@ public class Variables implements IVariables {
initialized = true;
}
+ /**
+ * Whether the operating system environment should be made available as Hop
variables, read from
+ * the three places the flag can be set.
+ */
+ private static boolean isEnvironmentImported() {
+ String name = Const.HOP_IMPORT_ENVIRONMENT_VARIABLES;
+ return isEnvironmentImported(
+ HopConfig.readStringVariable(name, null), System.getenv(name),
System.getProperty(name));
+ }
+
+ /**
+ * Decides whether the environment is imported, following the hop-config,
environment, {@code -D}
+ * precedence of {@link HopResolvedSettings} so that the flag itself can be
exported, which is
+ * what a container deployment has to hand.
+ *
+ * <p>{@link HopResolvedSettings#resolveString} cannot be used directly:
once {@code
+ * HopEnvironment} has started it copies the hop-config value (the
annotation default {@code N}
+ * unless configured) onto the system properties, so the JVM layer would
always win and an
+ * exported flag would never be seen. A system property therefore only
counts as a {@code -D}
+ * override when it differs from the hop-config value. An explicit {@code
-D} that repeats the
+ * hop-config value cannot be told apart from that copy, so an exported
value wins over it.
+ *
+ * @param configValue the value in hop-config.json, or null when it is not
configured
+ * @param environmentValue the exported environment variable, or null when
it is not set
+ * @param systemPropertyValue the system property, or null when it is not set
+ */
+ static boolean isEnvironmentImported(
+ String configValue, String environmentValue, String systemPropertyValue)
{
+ String value = configValue;
+ if (systemPropertyValue != null &&
!systemPropertyValue.equals(Const.NVL(configValue, ""))) {
+ value = systemPropertyValue;
+ } else if (environmentValue != null) {
+ value = environmentValue;
+ }
+ return "Y".equalsIgnoreCase(value);
+ }
+
@Override
public String[] getVariableNames() {
Set<String> keySet = properties.keySet();
diff --git
a/core/src/test/java/org/apache/hop/core/variables/VariablesTest.java
b/core/src/test/java/org/apache/hop/core/variables/VariablesTest.java
index 42b0ce9952..65cb66207c 100644
--- a/core/src/test/java/org/apache/hop/core/variables/VariablesTest.java
+++ b/core/src/test/java/org/apache/hop/core/variables/VariablesTest.java
@@ -19,8 +19,10 @@ package org.apache.hop.core.variables;
import static org.junit.jupiter.api.Assertions.assertArrayEquals;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assumptions.assumeTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.doAnswer;
@@ -30,15 +32,21 @@ import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import java.util.ArrayList;
+import java.util.HashSet;
import java.util.List;
import java.util.Map;
+import java.util.Set;
import java.util.concurrent.Callable;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.hop.core.Const;
+import org.apache.hop.core.config.HopConfig;
import org.apache.hop.core.exception.HopValueException;
import org.apache.hop.core.row.RowMeta;
import org.apache.hop.core.row.value.ValueMetaString;
import org.apache.hop.metadata.api.IHopMetadataProvider;
+import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Test;
import org.mockito.invocation.InvocationOnMock;
import org.mockito.stubbing.Answer;
@@ -196,4 +204,124 @@ class VariablesTest {
assertEquals(provider, child.findExecutionMetadataProvider());
assertNull(new Variables().findExecutionMetadataProvider());
}
+
+ /**
+ * An environment entry that nothing else defines, so that what it resolves
to is unambiguous.
+ * Names are sorted so the same entry is chosen on every run.
+ */
+ private static Map.Entry<String, String>
environmentEntryNotDefinedElsewhere() {
+ Set<String> definedNames = new
HashSet<>(System.getProperties().stringPropertyNames());
+ for (DescribedVariable describedVariable :
HopConfig.getInstance().getDescribedVariables()) {
+ definedNames.add(describedVariable.getName());
+ }
+ return System.getenv().entrySet().stream()
+ .filter(entry -> !definedNames.contains(entry.getKey()))
+ .filter(entry -> StringUtils.isNotEmpty(entry.getValue()))
+ .min(Map.Entry.comparingByKey())
+ .orElse(null);
+ }
+
+ /** The flag must not already be exported, or the process environment
decides these tests. */
+ private static void assumeFlagNotExported() {
+ assumeTrue(
+ System.getenv(Const.HOP_IMPORT_ENVIRONMENT_VARIABLES) == null,
+ Const.HOP_IMPORT_ENVIRONMENT_VARIABLES + " is exported in this
environment");
+ }
+
+ @AfterEach
+ void clearEnvironmentImportFlag() {
+ System.clearProperty(Const.HOP_IMPORT_ENVIRONMENT_VARIABLES);
+ }
+
+ /** The operating system environment stays out of the variable space unless
it is asked for. */
+ @Test
+ void environmentIsNotImportedByDefault() {
+ assumeFlagNotExported();
+ Map.Entry<String, String> entry = environmentEntryNotDefinedElsewhere();
+ assumeTrue(entry != null, "no usable environment variable to test with");
+
+ Variables variables = new Variables();
+ variables.initializeFrom(null);
+
+ assertNull(variables.getVariable(entry.getKey()));
+ }
+
+ /** With the flag on, ${NAME} resolves an exported environment variable
(#8495). */
+ @Test
+ void environmentIsImportedWhenEnabled() {
+ assumeFlagNotExported();
+ Map.Entry<String, String> entry = environmentEntryNotDefinedElsewhere();
+ assumeTrue(entry != null, "no usable environment variable to test with");
+ System.setProperty(Const.HOP_IMPORT_ENVIRONMENT_VARIABLES, "Y");
+
+ Variables variables = new Variables();
+ variables.initializeFrom(null);
+
+ assertEquals(entry.getValue(), variables.getVariable(entry.getKey()));
+ assertEquals(entry.getValue(), variables.resolve("${" + entry.getKey() +
"}"));
+ }
+
+ /**
+ * The environment has the lowest precedence, so a name that is also set
with -D keeps the value
+ * it resolves to today.
+ */
+ @Test
+ void systemPropertiesWinOverTheEnvironment() {
+ assumeFlagNotExported();
+ Map.Entry<String, String> entry = environmentEntryNotDefinedElsewhere();
+ assumeTrue(entry != null, "no usable environment variable to test with");
+ assumeTrue(!"overridden-by-minus-D".equals(entry.getValue()));
+ System.setProperty(Const.HOP_IMPORT_ENVIRONMENT_VARIABLES, "Y");
+ System.setProperty(entry.getKey(), "overridden-by-minus-D");
+ try {
+ Variables variables = new Variables();
+ variables.initializeFrom(null);
+
+ assertEquals("overridden-by-minus-D",
variables.getVariable(entry.getKey()));
+ } finally {
+ System.clearProperty(entry.getKey());
+ }
+ }
+
+ /** Nothing set anywhere: off. */
+ @Test
+ void environmentImportIsOffWhenNothingIsSet() {
+ assertFalse(Variables.isEnvironmentImported(null, null, null));
+ }
+
+ /**
+ * The container case after HopEnvironment has started: it copied the
default N from hop-config
+ * onto the system properties, and the flag is exported as Y. The export
must win (#8638).
+ */
+ @Test
+ void exportedFlagWinsOverTheSeededDefault() {
+ assertTrue(Variables.isEnvironmentImported("N", "Y", "N"));
+ }
+
+ /** Before HopEnvironment has started nothing is seeded, and an exported
flag is enough. */
+ @Test
+ void exportedFlagIsEnoughBeforeHopEnvironmentStarts() {
+ assertTrue(Variables.isEnvironmentImported(null, "Y", null));
+ }
+
+ /** HopEnvironment seeds an empty string when hop-config has the name
without a value. */
+ @Test
+ void exportedFlagWinsOverASeededEmptyValue() {
+ assertTrue(Variables.isEnvironmentImported(null, "Y", ""));
+ }
+
+ /** The environment overrides hop-config, as in HopResolvedSettings. */
+ @Test
+ void exportedFlagOverridesHopConfig() {
+ assertFalse(Variables.isEnvironmentImported("Y", "N", "Y"));
+ assertTrue(Variables.isEnvironmentImported("Y", null, "Y"));
+ }
+
+ /** A -D that differs from hop-config is an explicit override and wins over
the environment. */
+ @Test
+ void minusDWinsOverTheEnvironment() {
+ assertTrue(Variables.isEnvironmentImported("N", "N", "Y"));
+ assertFalse(Variables.isEnvironmentImported("Y", "Y", "N"));
+ assertTrue(Variables.isEnvironmentImported(null, null, "Y"));
+ }
}
