Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ This file documents all notable changes to https://github.com/devonfw/IDEasy[IDE

Release with new features and bugfixes:

* https://github.com/devonfw/IDEasy/issues/989[#989]: Allow expressions in template variable definitions

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix (not about this line itself) - the feature is undocumented outside the code.

documentation/configurator.adoc:39-42 is where the workspace template syntax lives today ("Variables in the form $[<variable-name>] get resolved..."). The audience for @path / @ask-variable / @ask-secret / @if-windows is exactly the settings maintainers reading that page, and right now the only description of the syntax is the issue and this PR body. Please add a section there covering:

  • the @<function-name>(<args>) syntax and the quoting rules,
  • one example per function (the nodejs.xml case from allow expressions in template variable definitions #989 is the perfect motivating example),
  • that unknown @name(...) is passed through untouched, so @media etc. are safe,
  • where @ask-* values are stored, and what that means for secrets (see my comment on AbstractEnvironmentVariables.java:414),
  • that expressions belong in workspace templates - putting an @ask-* call into a value in ide.properties means every variable resolution, including ide env, will try to prompt.

The CHANGELOG line itself is correct and under the right milestone.


The full list of changes for this release can be found in https://github.com/devonfw/IDEasy/milestone/49?closed=1[milestone 2026.08.002].

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1104,6 +1104,31 @@ public String askForInput(String message, String defaultValue) {
}
}

@Override
public String askForSecret(String message, String defaultValue) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix - this is askForInput(String, String) (lines 1083-1105) copied verbatim; the only difference in 24 lines is readSecretLine() instead of readLine() on line 1121. Duplicated logic like this drifts: the next fix to the batch-mode / force-mode / default-value contract will land in one copy only, and the two prompts will start behaving differently in ways nobody notices.

Please extract the shared loop and delegate, e.g.:

@Override
public String askForInput(String message, String defaultValue) {

  return ask(message, defaultValue, false);
}

@Override
public String askForSecret(String message, String defaultValue) {

  return ask(message, defaultValue, true);
}

private String ask(String message, String defaultValue, boolean secret) {

  while (true) {
    // ... existing body, with:
    String input = secret ? readSecretLine() : readLine().trim();
  }
}

See documentation/contributing/coding-conventions.adoc - duplicated code either moves up or stays where it was.


while (true) {
if (!message.isBlank()) {
IdeLogLevel.INTERACTION.log(LOG, message);
}
if (isBatchMode()) {
if (isForceMode()) {
return defaultValue;
} else {
throw new CliAbortException();
}
}
String input = readSecretLine().trim();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor - trim() is inherited from the askForInput copy, but it is wrong for a secret: leading or trailing whitespace can be part of a password, and more practically, a token pasted with a stray space is silently altered so the user gets an authentication failure with no hint why.

Suggested change
String input = readSecretLine().trim();
String input = readSecretLine();

(the isEmpty() check below still does the right thing for a plain Enter).

if (!input.isEmpty()) {
return input;
} else {
if (defaultValue != null) {
return defaultValue;
}
}
}
}

@Override
public <O> O question(O[] options, String question, Object... args) {

Expand Down Expand Up @@ -1171,6 +1196,15 @@ private static String computeOptionKey(String option) {
*/
protected abstract String readLine();

/**
* @return the secret input from the end-user (e.g. read from the console without echoing it). The default implementation simply delegates to
* {@link #readLine()} so that sub-classes without a secure console (e.g. in tests) work out of the box.
*/
protected String readSecretLine() {

return readLine();
}

private static <O> void addMapping(Map<String, O> mapping, String key, O option) {

O duplicate = mapping.put(key, option);
Expand Down
20 changes: 20 additions & 0 deletions cli/src/main/java/com/devonfw/tools/ide/context/IdeContext.java
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,26 @@ default String askForInput(String message) {
return askForInput(message, null);
}

/**
* Asks the user for a single secret input (e.g. a password or API token). Unlike {@link #askForInput(String, String)} the input is not echoed to the console
* if a secure console is available.
*
* @param message The information message to display.
* @param defaultValue The default value to return when no input is provided or {@code null} to keep asking until the user entered a non empty value.
* @return The secret input from the user, or the default value if no input is provided.
*/
String askForSecret(String message, String defaultValue);

/**
* Asks the user for a single secret input (e.g. a password or API token).
*
* @param message The information message to display.
* @return The secret input from the user.
*/
default String askForSecret(String message) {
return askForSecret(message, null);
}

/**
* @param question the question to ask.
* @param args arguments for filling the templates
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,21 @@ protected String readLine() {
}
}

@Override
protected String readSecretLine() {

if (this.scanner == null) {
char[] password = System.console().readPassword();
if (password == null) {
return "";
}
return new String(password);
} else {
LOG.warn("System console not available - secret input will be visible while typing.");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix - two things about the masking, both about the part CI cannot see.

  1. No automated coverage. AbstractIdeTestContext overrides readLine() only (AbstractIdeTestContext.java:155) and does not override readSecretLine(), so every @ask-secret test in this PR flows through the echoing readLine() - and AbstractIdeTestContext.readLine even logs the answer at INTERACTION level. If someone changed AskFunction.ask to call askForInput for secrets too, every test here would stay green. Please override readSecretLine() in AbstractIdeTestContext (recording that it was used, or serving from a separate answer list) and assert it in at least one @ask-secret test.

  2. This warn branch is the common case on Windows, not an exotic one: under mintty (git-bash) System.console() returns null, so the constructor takes the scanner fallback (IdeContextConsole.java:36-42) and the secret is echoed. That is the right behaviour given the constraints and the warning is good, but it means the feature needs a manual pass on Windows cmd/PowerShell, Windows git-bash, Linux and macOS before merge - green CI proves nothing about masking. Your testing instructions already say "run this from a normal terminal"; please state the result per OS in the PR when you have it.

return this.scanner.nextLine();
}
}

@Override
public IdeProgressBar newProgressBar(String title, long size, String unitName, long unitSize) {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
import org.slf4j.event.Level;

import com.devonfw.tools.ide.context.IdeContext;
import com.devonfw.tools.ide.expression.ExpressionContext;
import com.devonfw.tools.ide.expression.ExpressionFunctionManager;
import com.devonfw.tools.ide.expression.ExpressionParser;
import com.devonfw.tools.ide.variable.IdeVariables;
import com.devonfw.tools.ide.variable.VariableDefinition;
import com.devonfw.tools.ide.variable.VariableSyntax;
Expand All @@ -34,6 +37,8 @@ public abstract class AbstractEnvironmentVariables implements EnvironmentVariabl

private static final int MAX_RECURSION = 9;

private static final ExpressionParser EXPRESSION_PARSER = new ExpressionParser(ExpressionFunctionManager.get());

/**
* @see #getParent()
*/
Expand Down Expand Up @@ -206,14 +211,16 @@ private String resolveRecursive(String value, Object source, int recursion, Abst
}
recursion++;

String value2 = EXPRESSION_PARSER.resolve(value, new EnvironmentExpressionContext(source, recursion, resolvedVars, context));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor - running the expression parser before resolveWithSyntax means the function results are then fed back through variable resolution. Since arguments are already resolved explicitly by ExpressionParser.parseArgument via context.resolve(...), that second pass buys nothing and can only do harm: a value the user typed that happens to contain $[ (or ${ with legacySupport on) is reinterpreted as a variable reference and logs an "Undefined variable" warning containing the value.

Not blocking - just noting that resolving expressions after the variable pass would be equivalent for every case in your tests and would keep function output opaque. If you keep the current order, a short comment here explaining why would help the next reader.

Also: value2 reads as a scratch name. withExpressions or expressionsResolved would say what it is (coding-conventions.adoc, Naming).


String resolved;
if (context.syntax == null) {
resolved = resolveWithSyntax(value, source, recursion, resolvedVars, context, VariableSyntax.SQUARE);
resolved = resolveWithSyntax(value2, source, recursion, resolvedVars, context, VariableSyntax.SQUARE);
if (context.legacySupport) {
resolved = resolveWithSyntax(resolved, source, recursion, resolvedVars, context, VariableSyntax.CURLY);
}
} else {
resolved = resolveWithSyntax(value, source, recursion, resolvedVars, context, context.syntax);
resolved = resolveWithSyntax(value2, source, recursion, resolvedVars, context, context.syntax);
}
return resolved;
}
Expand Down Expand Up @@ -357,6 +364,65 @@ public String toString() {
return getSource().toString();
}

/**
* Implementation of {@link ExpressionContext} that connects an {@link com.devonfw.tools.ide.expression.ExpressionFunction} with this
* {@link EnvironmentVariables} hierarchy.
*/
private final class EnvironmentExpressionContext implements ExpressionContext {

private final Object src;

private final int recursion;

private final AbstractEnvironmentVariables resolvedVars;

private final ResolveContext context;

private EnvironmentExpressionContext(Object src, int recursion, AbstractEnvironmentVariables resolvedVars, ResolveContext context) {

super();
this.src = src;
this.recursion = recursion;
this.resolvedVars = resolvedVars;
this.context = context;
}

@Override
public IdeContext getIdeContext() {

return AbstractEnvironmentVariables.this.context;
}

@Override
public String resolve(String value) {

return this.resolvedVars.resolveRecursive(value, this.src, this.recursion, this.resolvedVars, this.context);
}

@Override
public String getVariable(String name) {

return this.resolvedVars.getValue(name, false);
}

@Override
public void setVariable(String name, String value) {

EnvironmentVariables conf = getByType(EnvironmentVariablesType.CONF);
if (conf instanceof EnvironmentVariablesPropertiesFile propertiesFile) {
propertiesFile.set(name, value);
propertiesFile.save();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix - the value the user just typed behind a masked prompt is written and logged in clear text.

The mechanism, all in existing code this now feeds:

  • EnvironmentVariablesPropertiesFile.set(String, String, boolean) logs LOG.debug("Set variable '{}={}' in {}", name, value, this.propertiesFilePath) (EnvironmentVariablesPropertiesFile.java:346), so ide -d update prints the API token on the console.
  • Once persisted, EnvironmentVariablesMap.getFlat logs LOG.trace("{}: Variable {}={}", getSource(), name, value) (EnvironmentVariablesMap.java:41) on every later read.
  • EnvironmentCommandlet.doRun calls collectVariables() (all variables, not only the exported ones) and prints them, so plain ide env dumps AI_API_KEY=sk-....

So @ask-secret currently differs from @ask-variable only in how the value is entered, not in how it is stored or shown afterwards. That is the part that will surprise users: masked input implies the value stays secret.

Minimum I would like to see here: register the entered secret in the privacy map so PrivacyUtil masks it in log output (AbstractIdeContext.initializePrivacyMap at AbstractIdeContext.java:1060 is the existing hook), and document explicitly in the docs that @ask-secret stores the value unencrypted in conf/ide.properties. Encryption itself is fine as the follow-up story the issue asks for.

} else {
LOG.warn("Cannot persist variable {} since no configuration file is available.", name);
}
}

@Override
public boolean isPersistent() {
return true;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should-fix - this is the only production implementation of ExpressionContext, and it returns a constant true. That means the requirement from #989 - "For settings templates this should not happen since these templates are only instantiated once" - is not implemented: every @ask-* call persists, no matter where it came from.

The abstraction cannot decide it at this point either, because resolveRecursive has no idea whether its caller is PropertiesMerger on a workspace template or something else. It needs to be threaded in from the caller, e.g. as a field on the ResolveContext record set by EnvironmentVariables.resolve(...).

Two honest options:

  1. Wire it up: add the flag to ResolveContext and let the merger pass it, then isPersistent() returns it.
  2. Drop it: remove isPersistent() from ExpressionContext and always persist, plus a comment saying why. You invoked KISS in the issue yourself, and an interface method that no production caller can ever make false is dead weight that reads as if the feature exists.

Either is fine with me, but the current middle ground is the worst of the three because the API and the test both suggest the behaviour is there.

}
}

/**
* Simple record for the immutable arguments of recursive resolve methods.
*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package com.devonfw.tools.ide.expression;

import com.devonfw.tools.ide.context.IdeContext;

/**
* Interface for the context available to an {@link ExpressionFunction} while an expression is evaluated.
*/
public interface ExpressionContext {

/**
* @return the {@link IdeContext}.
*/
IdeContext getIdeContext();

/**
* Resolves variables in the given value. Used to resolve arguments of a function that may themselves contain
* variables or nested expressions (e.g. {@code @path('$[IDE_HOME]/software/node')}).
*
* @param value the value to resolve.
* @return the given value with variables and nested expressions resolved.
*/
String resolve(String value);

/**
* @param name the name of the variable.
* @return the value of the variable or {@code null} if not defined in any level of the hierarchy.
*/
String getVariable(String name);

/**
* Persists the given variable to the user local {@code conf/ide.properties} so the user is not asked again.
* <p>
* Only has an effect if {@link #isPersistent()} returns {@code true}.
*
* @param name the name of the variable.
* @param value the value to persist.
*/
void setVariable(String name, String value);

/**
* @return {@code true} if values acquired from the user should be {@link #setVariable(String, String) persisted}.
* This is the case for workspace templates that are re-applied on every {@code ide update}. For settings
* templates that are only instantiated once, this is {@code false}.
*/
boolean isPersistent();

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package com.devonfw.tools.ide.expression;

import java.util.List;

/**
* Interface for a function that can be used in an expression of a template variable definition.
* <p>
* The syntax of an expression is {@code @«function-name»([«arg»[,«arg»]*])}. Implementations are registered in the
* {@link ExpressionFunctionManager}.
*
* @see ExpressionFunctionManager
*/
public interface ExpressionFunction {

/**
* @return the name of this function as used in the expression syntax (e.g. "path" for {@code @path(...)}). Has to match
* {@code [a-z][a-z0-9-]*}.
*/
String getName();

/**
* @return the minimum number of arguments required by this function.
*/
int getMinArgs();

/**
* @return the maximum number of arguments supported by this function or {@code -1} for an unlimited number.
*/
int getMaxArgs();

/**
* @param args the {@link List} of arguments. Already trimmed, unquoted and with variables resolved. Guaranteed to
* satisfy {@link #getMinArgs()} and {@link #getMaxArgs()}.
* @param context the {@link ExpressionContext}.
* @return the result of this function that will replace the entire expression.
*/
String apply(List<String> args, ExpressionContext context);

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
package com.devonfw.tools.ide.expression;

import java.util.HashMap;
import java.util.Map;

import com.devonfw.tools.ide.expression.function.AskFunction;
import com.devonfw.tools.ide.expression.function.IfOsFunction;
import com.devonfw.tools.ide.expression.function.PathFunction;

/**
* Manager where all {@link ExpressionFunction}s are registered so they can be looked up by
* {@link #getFunction(String) name} while an expression is resolved.
* <p>
* With new IDEasy releases additional functions can simply be registered here.
*/
public class ExpressionFunctionManager {

private static final ExpressionFunctionManager DEFAULT = createDefault();

private final Map<String, ExpressionFunction> functions;

/**
* The constructor.
*/
public ExpressionFunctionManager() {

super();
this.functions = new HashMap<>();
}

/**
* @param function the {@link ExpressionFunction} to register.
*/
public void register(ExpressionFunction function) {

ExpressionFunction duplicate = this.functions.put(function.getName(), function);
if (duplicate != null) {
throw new IllegalStateException("Duplicate expression function @" + function.getName());
}
}

/**
* @param name the {@link ExpressionFunction#getName() name} of the requested function.
* @return the {@link ExpressionFunction} or {@code null} if no function is registered for the given name. A
* {@code null} result is not an error: the expression is then left untouched.
*/
public ExpressionFunction getFunction(String name) {

return this.functions.get(name);
}

/**
* @return the default instance with all standard functions registered.
*/
public static ExpressionFunctionManager get() {

return DEFAULT;
}

private static ExpressionFunctionManager createDefault() {

ExpressionFunctionManager manager = new ExpressionFunctionManager();
manager.register(new PathFunction());
manager.register(AskFunction.ofVariable());
manager.register(AskFunction.ofSecret());
for (IfOsFunction function : IfOsFunction.all()) {
manager.register(function);
}
return manager;
}

}
Loading