-
Notifications
You must be signed in to change notification settings - Fork 87
#989: allow expressions in template variable definitions #2282
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
8762376
a41eace
d1945db
b315917
c01ce8c
53b84ea
39cbef5
0af5ed9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1104,6 +1104,31 @@ public String askForInput(String message, String defaultValue) { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| @Override | ||||||
| public String askForSecret(String message, String defaultValue) { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should-fix - this is 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 |
||||||
|
|
||||||
| while (true) { | ||||||
| if (!message.isBlank()) { | ||||||
| IdeLogLevel.INTERACTION.log(LOG, message); | ||||||
| } | ||||||
| if (isBatchMode()) { | ||||||
| if (isForceMode()) { | ||||||
| return defaultValue; | ||||||
| } else { | ||||||
| throw new CliAbortException(); | ||||||
| } | ||||||
| } | ||||||
| String input = readSecretLine().trim(); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor -
Suggested change
(the |
||||||
| if (!input.isEmpty()) { | ||||||
| return input; | ||||||
| } else { | ||||||
| if (defaultValue != null) { | ||||||
| return defaultValue; | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| @Override | ||||||
| public <O> O question(O[] options, String question, Object... args) { | ||||||
|
|
||||||
|
|
@@ -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); | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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."); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
|
||
| return this.scanner.nextLine(); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public IdeProgressBar newProgressBar(String title, long size, String unitName, long unitSize) { | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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() | ||
| */ | ||
|
|
@@ -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)); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor - running the expression parser before 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: |
||
|
|
||
| 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; | ||
| } | ||
|
|
@@ -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(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
So Minimum I would like to see here: register the entered secret in the privacy map so |
||
| } else { | ||
| LOG.warn("Cannot persist variable {} since no configuration file is available.", name); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isPersistent() { | ||
| return true; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should-fix - this is the only production implementation of The abstraction cannot decide it at this point either, because Two honest options:
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. | ||
| * | ||
|
|
||
| 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; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
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-42is where the workspace template syntax lives today ("Variables in the form$[<variable-name>]get resolved..."). The audience for@path/@ask-variable/@ask-secret/@if-windowsis 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:@<function-name>(<args>)syntax and the quoting rules,nodejs.xmlcase from allow expressions in template variable definitions #989 is the perfect motivating example),@name(...)is passed through untouched, so@mediaetc. are safe,@ask-*values are stored, and what that means for secrets (see my comment onAbstractEnvironmentVariables.java:414),@ask-*call into a value inide.propertiesmeans every variable resolution, includingide env, will try to prompt.The CHANGELOG line itself is correct and under the right milestone.