#989: allow expressions in template variable definitions - #2282
Conversation
Paras14
added
enhancement
…/989-expression-functions
Coverage Report for CI Build 31964006856Warning No base build found for commit Coverage: 73.059%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
Updated changelog for version 2026.08.002, including new features and bugfixes.
…ion-functions # Conflicts: # CHANGELOG.adoc
maybeec
left a comment
maybeec
left a comment
There was a problem hiding this comment.
Thanks for this PR, and thanks in particular for the parser itself. Locating a call with a RegEx and then scanning the argument list by hand is exactly the right call: findClosingParenthesis / parseArguments handle quoted commas, quoted parenthesis and nesting correctly, and the parametrized testForeignExpressionIsUntouched covering @media, @include, @Override and "@angular/core" is precisely the test I would have asked for. Passing unknown functions through untouched is the single most important property of this feature and you got it right and covered it. The JavaDoc on the new types is also good.
No blockers, and nothing below needs a redesign. The two I care most about are the plaintext handling of @ask-secret values and @path(..., native) not using WindowsPathSyntax.
Should-fix
@ask-secretvalues are persisted and logged in clear text -AbstractEnvironmentVariables.java:414@path(..., native)hand-rolls separator replacement instead ofWindowsPathSyntax-PathFunction.java:51isPersistent()is hard-coded totrue, so the "settings templates must not persist" requirement of #989 is not actually implemented -AbstractEnvironmentVariables.java:422, and its test only exercises a test double -ExpressionParserTest.java:198askForSecretis a verbatim copy ofaskForInput-AbstractIdeContext.java:1108DirectoryMergerExpressionTestdepends onHashtableiteration order -DirectoryMergerExpressionTest.java:34- The masking itself has no automated coverage, and the Windows git-bash path needs a manual test pass -
IdeContextConsole.java:65 - The new expression syntax is not documented anywhere -
CHANGELOG.adoc:9 - A template authoring error aborts
ide updatewith a rawIllegalArgumentException-ExpressionParser.java:89
Minor
- Secrets are
trim()ed -AbstractIdeContext.java:1121 - Function results are re-scanned by the variable resolver -
AbstractEnvironmentVariables.java:214 AbstractIdeContextTest.TEST_RESOURCESalready exists -DirectoryMergerExpressionTest.java:36- Redundant initial
matcher.find()-ExpressionParser.java:57
Scope against #989
| Requirement | Status |
|---|---|
@<function-name>([<arg>[,<arg>]*]) syntax, args always String, comma separated, trimmed, quoted with ' or " |
met |
| Args may themselves contain variables | met |
| Manual argument scanning instead of a pure RegEx | met, and better than the RegEx sketched in the issue |
@path with 1st arg path, optional 2nd arg unix (default) / native |
partial - native does not use WindowsPathSyntax (see 2) |
@ask-variable / @ask-secret, defined variable returned without asking, empty 1st arg always asks, default question text, 3rd arg default value |
met |
Values from a workspace template persisted to conf/ide.properties |
met |
| "For settings templates this should not happen since these templates are only instantiated once" | missing (see 3) |
@if-windows / @if-mac / @if-linux / @if-unix |
met |
ExpressionFunction interface + ExpressionFunctionManager registry so new releases can register more functions |
met |
Also: the issue invites a follow-up story for the maven settings.xml password prompting/encryption (@ask-maven-secret or a resolve flag telling the function it is resolving settings.xml). Worth creating it and linking it here so the plaintext-storage topic from finding 1 has an owner.
CI / DoD
All checks green, CLA signed, branch is up-to-date with main, CHANGELOG entry present under the correct milestone, PR title follows #989: .... One nit: the commit Update CHANGELOG for version 2026.08.002 does not follow the #<issue-id>: <summary> commit format (see documentation/contributing/commit.adoc).
| EnvironmentVariables conf = getByType(EnvironmentVariablesType.CONF); | ||
| if (conf instanceof EnvironmentVariablesPropertiesFile propertiesFile) { | ||
| propertiesFile.set(name, value); | ||
| propertiesFile.save(); |
There was a problem hiding this comment.
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)logsLOG.debug("Set variable '{}={}' in {}", name, value, this.propertiesFilePath)(EnvironmentVariablesPropertiesFile.java:346), soide -d updateprints the API token on the console.- Once persisted,
EnvironmentVariablesMap.getFlatlogsLOG.trace("{}: Variable {}={}", getSource(), name, value)(EnvironmentVariablesMap.java:41) on every later read. EnvironmentCommandlet.doRuncallscollectVariables()(all variables, not only the exported ones) and prints them, so plainide envdumpsAI_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.
| return path.replace('\\', '/'); | ||
| } else if (MODE_NATIVE.equals(mode)) { | ||
| if (context.getIdeContext().getSystemInfo().isWindows()) { | ||
| return path.replace('/', '\\'); |
There was a problem hiding this comment.
Should-fix - please use WindowsPathSyntax here rather than a character replace. The issue asks for it by name ("on Windows use WindowsPathSyntax.WINDOWS with backslashes, etc.") and the class exists exactly for this: WindowsPathSyntax.normalize(String) detects the drive letter in either syntax and rewrites root and separators.
Why it matters mechanically: IDEasy explicitly tolerates MSYS-style paths on Windows. WindowsPathSyntax.MSYS.getDrive accepts /d/..., AbstractIdeContext.initializePrivacyMap (AbstractIdeContext.java:1066-1067) registers both the D:\... and the /d/... form of the same path, and EnvironmentVariablesMap.getFlat (EnvironmentVariablesMap.java:42-48) normalizes variable values through pathSyntax.normalize(value). So a value in MSYS form can reach @path, and replace('/', '\\') turns /d/projects/foo into \d\projects\foo - the drive letter is silently lost and the resulting path is broken, which is the exact class of bug #989 was filed to remove.
} else if (MODE_NATIVE.equals(mode)) {
if (context.getIdeContext().getSystemInfo().isWindows()) {
return WindowsPathSyntax.WINDOWS.normalize(path);
}
return path.replace('\\', '/');
}(needs import com.devonfw.tools.ide.os.WindowsPathSyntax;). A test with IDE_HOME set to /d/projects/my-project alongside the existing testPathNativeOnWindows would lock this in.
The MODE_UNIX branch above is fine as-is - the issue defines that one as plain backslash-to-slash.
|
|
||
| @Override | ||
| public boolean isPersistent() { | ||
| return true; |
There was a problem hiding this comment.
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:
- Wire it up: add the flag to
ResolveContextand let the merger pass it, thenisPersistent()returns it. - Drop it: remove
isPersistent()fromExpressionContextand always persist, plus a comment saying why. You invoked KISS in the issue yourself, and an interface method that no production caller can ever makefalseis 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.
| IdeTestContext context = newContext(PROJECT_BASIC); | ||
| context.setAnswers("value"); | ||
| TestExpressionContext expressionContext = new TestExpressionContext(context); | ||
| expressionContext.persistent = false; |
There was a problem hiding this comment.
Should-fix - paired with AbstractEnvironmentVariables.java:422: this test sets a field on TestExpressionContext, then asserts that TestExpressionContext.setVariable was not called. It validates the test double, not any IDEasy production code - no production code path can ever produce isPersistent() == false today.
Per documentation/contributing/junit-testing.adoc a test has to exercise the project's own logic. Once the flag is threaded through ResolveContext this test should assert on the real EnvironmentExpressionContext (i.e. that nothing landed in conf/ide.properties), the way DirectoryMergerExpressionTest already does for the positive case. If you take the "drop isPersistent()" route instead, this test should go with it.
| } | ||
|
|
||
| @Override | ||
| public String askForSecret(String message, String defaultValue) { |
There was a problem hiding this comment.
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.
| int min = function.getMinArgs(); | ||
| int max = function.getMaxArgs(); | ||
| if ((size < min) || ((max >= 0) && (size > max))) { | ||
| throw new IllegalArgumentException( |
There was a problem hiding this comment.
Should-fix - a wrong argument count is a configuration mistake by the settings maintainer, but this raw IllegalArgumentException propagates out of variable resolution and aborts ide update with a stack trace.
That is inconsistent with how the very same resolution step handles the sibling case: an undefined variable is reported with LOG.atLevel(logLevel).log("Undefined variable {} in '{}'", var, src) and resolution continues (AbstractEnvironmentVariables.java:239-250). The rationale there applies here too - a broken template line should not block a user from getting their IDE started.
Please either log a warning naming the file (src) and leave the expression untouched, or throw a CliException with a message pointing at the template - CliException is what IDEasy uses for expected, user-facing failures, and it gets rendered without a stack trace. The same applies to the IllegalArgumentException in AskFunction.apply (empty variable name without a question) and the one in PathFunction.apply (invalid mode): in all three the end user gets a technical stacktrace for someone else's typo.
One detail worth keeping either way: value is interpolated into the message, so for @ask-secret an already-resolved secret could end up in the exception text. Prefer the source/template reference over the raw value.
| throw new CliAbortException(); | ||
| } | ||
| } | ||
| String input = readSecretLine().trim(); |
There was a problem hiding this comment.
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.
| String input = readSecretLine().trim(); | |
| String input = readSecretLine(); |
(the isEmpty() check below still does the right thing for a plain Enter).
| } | ||
| recursion++; | ||
|
|
||
| String value2 = EXPRESSION_PARSER.resolve(value, new EnvironmentExpressionContext(source, recursion, resolvedVars, context)); |
There was a problem hiding this comment.
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).
| // and therefore does not preserve the order of the lines in the template file. | ||
| context.setAnswers("sk-TOPSECRET", "http://llama.local"); | ||
| DirectoryMerger merger = context.getWorkspaceMerger(); | ||
| Path templates = Path.of("src/test/resources/templates-expression"); |
There was a problem hiding this comment.
Minor - AbstractIdeContextTest (which this test extends) already defines protected static final Path TEST_RESOURCES = Path.of("src/test/resources") at AbstractIdeContextTest.java:36.
| Path templates = Path.of("src/test/resources/templates-expression"); | |
| Path templates = TEST_RESOURCES.resolve("templates-expression"); |
| return null; | ||
| } | ||
| Matcher matcher = FUNCTION_START.matcher(value); | ||
| if (!matcher.find()) { |
There was a problem hiding this comment.
Minor - this find() and the find(pos) on line 62 with pos == 0 search the same region twice. The early return is a nice fast path for the overwhelmingly common "no @ call in this value" case (and this runs on every single variable resolution, so it is worth keeping), but it can reuse the result:
if (!matcher.find()) {
return value;
}
StringBuilder sb = new StringBuilder(value.length() + EXTRA_CAPACITY);
int pos = 0;
do {
...
} while (matcher.find(pos));which mirrors the do { ... } while (matcher.find()) shape already used in AbstractEnvironmentVariables.resolveWithSyntax.
|
Thanks for the PR. Most of my review points align with those of @maybeec mentioned earlier, so you can simply refer to my summary below; I won't be reviewing each section individually. Please feel free to contact me if you have any questions. Review: #989 — expression functions for template variables Overall: solid, well-tested implementation. The architecture matches the ticket's design (interface + manager + parser + per-OS/path/ask functions), the parser correctly handles what a regex Below are findings, most-severe first. None block merge; the first two are worth a decision. 1. Secret values flow into logs in plaintext (masking only applies to keystrokes) @ask-secret masks input while typing, but the returned value is then treated as an ordinary variable value. The integration test's own output shows it: 2. isPersistent() is hardcoded true — settings templates persist too (spec deviation) AbstractEnvironmentVariables.EnvironmentExpressionContext.isPersistent() (line 421–423) always returns true, so @ask-variable/@ask-secret persist to conf/ide.properties even when resolved from "For settings templates this should not happen since these templates are only instantiated once." The Javadoc on ExpressionContext.isPersistent() (line 41–44) even documents the intended distinction, but the implementation doesn't make it — it has no way to tell whether the current 3. A defined variable's value returned by @ask-variable is not re-resolved for nested expressions In AskFunction.apply, the defined-variable fast path returns context.getVariable(...) directly (line 71–73). That value is appended into value2 and then $[...] variables are resolved once more 4. Minor
Suggested before merge Items 1 and 2 are the only ones needing a judgment call — decide whether secrets should be masked in logs and whether the settings-vs-workspace persistence distinction is in scope now or a follow-up. Everything else is optional polish. The implementation and tests otherwise meet the Definition of Done for the core story. |
4 participants
This PR fixes #989
Implements #989 and supersedes the
$[ask:...]/$[secret:...]syntax of #2179.Closes #2165.
Implemented changes
com.devonfw.tools.ide.expressionwith the expression syntax@«function-name»([«arg»[,«arg»]*])resolved during variable resolution.ExpressionFunction- interface implemented by every function.ExpressionFunctionManager- registry to look up functions by name so further functions can be registered with new IDEasy releases.ExpressionParser- locates function calls and parses their arguments. A RegEx is only used to locate the start of a call, the argument list is scanned manually since a RegEx cannot express a balanced list of arguments containing quoted commas, quoted parenthesis or nested calls.ExpressionContext- gives a function access to theIdeContext, variable lookup and persistence.@path(«path»[, unix|native])that normalises a path, by default replacing backslashes with slashes.@ask-variable(«name»[, «question»[, «default»]])and@ask-secret(...)with masked input. An already defined variable is returned without asking, an empty 1st argument always asks, and an empty string as 3rd argument permits empty input.@if-windows,@if-mac,@if-linuxand@if-unixthat insert their argument if the OS matches.IdeContext.askForSecret(String, String)analogous toaskForInput.AbstractIdeContextimplements the prompt loop and the batch mode contract and delegates reading to the new protectedreadSecretLine(), whichIdeContextConsoleoverrides withConsole.readPassword().conf/ide.propertiesso the question is only asked the first time. A value that could not be asked for in batch mode is not persisted.@path('$[IDE_HOME]/software/node')).Testing instructions
Masked input needs a real console, so run this from a normal terminal and not from the IDE.
(make sure you add your path correctly for the cli target classes: "\cli\target\classes")
IDEasy\settings\workspace\update\ai-test.properties:Make sure
MY_URL,MY_TOKENandMY_OPTIONALare not yet defined inIDEasy\conf\ide.properties.You are asked three times.
MY_URLis echoed while typing,MY_TOKENandMY_OPTIONALare not. The question forMY_TOKENis shown as given, including the parenthesis. Press enter without typing anything forMY_OPTIONAL.IDEasy\workspaces\main\ai-test.propertiescontains the three entered values andai.node.pathwith backslashes.IDEasy\conf\ide.propertiescontainsMY_URL,MY_TOKENandMY_OPTIONAL.Run again. There is no prompt and the file keeps the same values.
Remove
MY_URL,MY_TOKENandMY_OPTIONALfromIDEasy\conf\ide.propertiesand run again with--batch. There is no prompt, the workspace merge fails withCliAbortException: Aborted by end-user.and nothing is added toIDEasy\conf\ide.properties, as an undefined variable cannot be asked for in batch mode.Checklist for this PR
Make sure everything is checked before merging this PR. For further info please also see
our DoD.
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»(e.g.#921: fixed setup.bat). If no issue ID exists, title only.In Progressand assigned to you or there is no issue (might happen for very small PRs)with
internal