Skip to content

#989: allow expressions in template variable definitions - #2282

Open
Paras14 wants to merge 18 commits into
devonfw:mainfrom
Paras14:feature/989-expression-functions
Open

#989: allow expressions in template variable definitions#2282
Paras14 wants to merge 18 commits into
devonfw:mainfrom
Paras14:feature/989-expression-functions

Conversation

@Paras14

@Paras14 Paras14 commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

This PR fixes #989

Implements #989 and supersedes the $[ask:...] / $[secret:...] syntax of #2179.

Closes #2165.

Implemented changes

  • Added new package com.devonfw.tools.ide.expression with 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 the IdeContext, variable lookup and persistence.
  • Added function @path(«path»[, unix|native]) that normalises a path, by default replacing backslashes with slashes.
  • Added functions @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.
  • Added functions @if-windows, @if-mac, @if-linux and @if-unix that insert their argument if the OS matches.
  • Added IdeContext.askForSecret(String, String) analogous to askForInput. AbstractIdeContext implements the prompt loop and the batch mode contract and delegates reading to the new protected readSecretLine(), which IdeContextConsole overrides with Console.readPassword().
  • Values entered for a workspace template are persisted to conf/ide.properties so the question is only asked the first time. A value that could not be asked for in batch mode is not persisted.
  • Arguments are resolved through the regular variable resolution, so a String argument may itself contain variables (e.g. @path('$[IDE_HOME]/software/node')).
  • Text that does not call a registered function is left untouched.

Testing instructions

Masked input needs a real console, so run this from a normal terminal and not from the IDE.

  1. Build the branch and dump the classpath, from the repository root:
mvn -pl cli -am install -DskipTests
mvn -pl cli dependency:build-classpath "-Dmdep.outputFile=cp.txt"
$CP = (Get-Content cli\cp.txt -Raw).Trim()
$CLASSES = "C:\projects\IDEasy\workspaces\main\IDEasy\cli\target\classes"

(make sure you add your path correctly for the cli target classes: "\cli\target\classes")

  1. In a test project create IDEasy\settings\workspace\update\ai-test.properties:
ai.backend.url=@ask-variable('MY_URL')
ai.api.key=@ask-secret('MY_TOKEN', 'Enter your API key (from the portal):')
ai.node.path=@path('$[IDE_HOME]/software/node/node', native)
ai.other=@ask-secret('MY_OPTIONAL', 'Password (may be empty):', '')

Make sure MY_URL, MY_TOKEN and MY_OPTIONAL are not yet defined in IDEasy\conf\ide.properties.

  1. From the project directory, run the local build:
java -cp "$CLASSES;$CP" com.devonfw.tools.ide.cli.Ideasy update
  1. You are asked three times. MY_URL is echoed while typing, MY_TOKEN and MY_OPTIONAL are not. The question for MY_TOKEN is shown as given, including the parenthesis. Press enter without typing anything for MY_OPTIONAL.

  2. IDEasy\workspaces\main\ai-test.properties contains the three entered values and ai.node.path with backslashes. IDEasy\conf\ide.properties contains MY_URL, MY_TOKEN and MY_OPTIONAL.

  3. Run again. There is no prompt and the file keeps the same values.

  4. Remove MY_URL, MY_TOKEN and MY_OPTIONAL from IDEasy\conf\ide.properties and run again with --batch. There is no prompt, the workspace merge fails with CliAbortException: Aborted by end-user. and nothing is added to IDEasy\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.

  • When running mvn clean test locally all tests pass and build is successful
  • PR title is of the form #«issue-id»: «brief summary» (e.g. #921: fixed setup.bat). If no issue ID exists, title only.
  • PR top-level comment summarizes what has been done and contains link to addressed issue(s)
  • PR and issue(s) have suitable labels
  • Issue is set to In Progress and assigned to you or there is no issue (might happen for very small PRs)
  • You followed all coding conventions
  • You have added the issue implemented by your PR in CHANGELOG.adoc unless issue is labeled
    with internal
  • You have formulated clear instructions on how to test your contribution under "Testing instructions"

@github-project-automation github-project-automation Bot moved this to 🆕 New in IDEasy board Aug 6, 2026
@Paras14 Paras14 self-assigned this Aug 6, 2026
@Paras14 Paras14 moved this from 🆕 New to Team Review in IDEasy board Aug 6, 2026
@Paras14 Paras14 added enhancement New feature or request configuration should be configurable or configuration change settings ide-settings repo and replated processes and features merger workspace template merger (XML, JSON, properties) ready-to-implement labels Aug 6, 2026
@coveralls

coveralls commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Coverage Report for CI Build 33180486580

Coverage increased (+0.2%) to 73.816%

Details

  • Coverage increased (+0.2%) from the base build.
  • Patch coverage: No coverable lines changed in this PR.
  • 229 coverage regressions across 9 files.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

229 previously-covered lines in 9 files lost coverage.

File Lines Losing Coverage Coverage
com/devonfw/tools/ide/context/AbstractIdeContext.java 173 71.88%
com/devonfw/tools/ide/environment/AbstractEnvironmentVariables.java 18 82.66%
com/devonfw/tools/ide/context/IdeContext.java 17 76.92%
com/devonfw/tools/ide/context/IdeContextConsole.java 7 0.0%
com/devonfw/tools/ide/merge/FileMerger.java 7 86.96%
com/devonfw/tools/ide/environment/EnvironmentVariablesMap.java 3 81.82%
com/devonfw/tools/ide/environment/SortedProperties.java 2 53.49%
com/devonfw/tools/ide/tool/ide/IdeToolCommandlet.java 1 87.42%
com/devonfw/tools/ide/version/VersionSegment.java 1 91.34%

Coverage Stats

Coverage Status
Relevant Lines: 18582
Covered Lines: 14334
Line Coverage: 77.14%
Relevant Branches: 8274
Covered Branches: 5490
Branch Coverage: 66.35%
Branches in Coverage %: Yes
Coverage Strength: 3.29 hits per line

💛 - Coveralls

Paras14 and others added 3 commits August 12, 2026 11:58
Updated changelog for version 2026.08.002, including new features and bugfixes.

@maybeec maybeec left a comment

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.

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

  1. @ask-secret values are persisted and logged in clear text - AbstractEnvironmentVariables.java:414
  2. @path(..., native) hand-rolls separator replacement instead of WindowsPathSyntax - PathFunction.java:51
  3. isPersistent() is hard-coded to true, 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:198
  4. askForSecret is a verbatim copy of askForInput - AbstractIdeContext.java:1108
  5. DirectoryMergerExpressionTest depends on Hashtable iteration order - DirectoryMergerExpressionTest.java:34
  6. The masking itself has no automated coverage, and the Windows git-bash path needs a manual test pass - IdeContextConsole.java:65
  7. The new expression syntax is not documented anywhere - CHANGELOG.adoc:9
  8. A template authoring error aborts ide update with a raw IllegalArgumentException - ExpressionParser.java:89

Minor

  1. Secrets are trim()ed - AbstractIdeContext.java:1121
  2. Function results are re-scanned by the variable resolver - AbstractEnvironmentVariables.java:214
  3. AbstractIdeContextTest.TEST_RESOURCES already exists - DirectoryMergerExpressionTest.java:36
  4. 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).

Comment thread cli/src/main/java/com/devonfw/tools/ide/expression/function/PathFunction.java Outdated
Comment thread cli/src/test/java/com/devonfw/tools/ide/expression/ExpressionParserTest.java Outdated
Comment thread cli/src/main/java/com/devonfw/tools/ide/expression/ExpressionParser.java Outdated
Comment thread cli/src/main/java/com/devonfw/tools/ide/context/AbstractIdeContext.java Outdated
Comment thread cli/src/test/java/com/devonfw/tools/ide/merge/DirectoryMergerExpressionTest.java Outdated
@QuangAnhLe

QuangAnhLe commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

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
can't (quoted commas, quoted parens, nesting), foreign @... syntax is passed through untouched, and the test suite is thorough (33 tests: parser unit tests, integration through
EnvironmentVariables, and a real DirectoryMerger run). All pass. Commit messages are clean. The core logic is correct.

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:
Set variable 'AI_API_KEY=sk-TOPSECRET' in ...conf\ide.properties
That log line comes from PropertiesMerger (pre-existing), but it's newly reachable now that a secret can be the value being set. The value also lands in conf/ide.properties in cleartext — which
is the intended persistence, so that's fine. But the console/log echo of the secret is a real exposure for the "secret" case the ticket is trying to protect. Worth confirming intent; if
secrets are meant to stay off the log, this path (and any other value-logging) should be masked for @ask-secret results.

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
a settings template. The ticket is explicit:

"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
resolution is a settings vs. workspace template. Impact is low in practice (settings are instantiated once, so the "asked only once" benefit rarely matters), but the behavior contradicts the
spec and the doc. Either implement the settings/workspace distinction or soften the Javadoc/commit claim. Note the unit test testSettingsTemplateDoesNotPersist only exercises a hand-rolled
TestExpressionContext with persistent=false — it doesn't prove the real AbstractEnvironmentVariables path honors the setting, so this gap is masked in tests.

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
by resolveWithSyntax, but embedded @... expressions inside an already-defined variable's value would not be, since apply's return is never fed back through the expression parser. This is an
asymmetry (a plain $[FOO] whose value contains @path(...) behaves the same — also not re-resolved — so it's consistent with existing variable semantics, but worth knowing). Edge case, low
priority; flagging for completeness.

4. Minor

  • readSecretLine fallback is unmasked: when System.console() is null (piped input, running from the IDE), IdeContextConsole.readSecretLine() logs a warning and reads via the plaintext Scanner.
    Acceptable given the constraints, and it correctly warns, but it's worth ensuring the manual-testing doc emphasizes "run from a normal terminal" (it does — good).
  • No way to escape a quote inside a quoted argument. Backslash is deliberately not an escape char (correct, tested), so a literal ' inside a single-quoted arg is unrepresentable. Fine for KISS;
    just a documented limitation.
  • EXTRA_CAPACITY = 8 is duplicated in both ExpressionParser (line 31) and AbstractEnvironmentVariables (line 34). Harmless, but could be a shared constant.
  • resolve() double-find: the guard if (!matcher.find()) (line 57) is followed by while (matcher.find(pos)) from pos=0, so the first match is found twice. Correct but slightly wasteful.

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.

@Paras14 Paras14 moved this from Team Review to 👀 In review in IDEasy board Aug 25, 2026
@gitguardian

gitguardian Bot commented Aug 25, 2026

Copy link
Copy Markdown

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

Since your pull request originates from a forked repository, GitGuardian is not able to associate the secrets uncovered with secret incidents on your GitGuardian dashboard.
Skipping this check run and merging your pull request will create secret incidents on your GitGuardian dashboard.

🔎 Detected hardcoded secret in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
- - Generic High Entropy Secret 3b2dd8c cli/src/test/java/com/devonfw/tools/ide/expression/ExpressionParserTest.java View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secret safely. Learn here the best practices.
  3. Revoke and rotate this secret.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

configuration should be configurable or configuration change enhancement New feature or request merger workspace template merger (XML, JSON, properties) ready-to-implement settings ide-settings repo and replated processes and features

Projects

Status: 👀 In review

Development

Successfully merging this pull request may close these issues.

prompt user for template variables at apply time allow expressions in template variable definitions

4 participants