[MNG-8425] Fix mvnenc init saving invalid master source configuration

[Hiteshsai007]

[MNG-8425] Fix mvnenc init saving invalid master source configuration

Fixes #10202

Description

This PR fixes MNG-8425 (#10202) where mvn --enc init generates an invalid settings-security4.xml, causing mvn --enc encrypt to fail with "Maven Encryption is not configured."

Root Cause

When the encryption wizard prompts the user to customize an editable value, the template is presented as env:$VARIABLE_NAME. The user is expected to replace only the $VARIABLE_NAME portion (e.g., typing MVN_PASSWORD), but the Init goal saves the raw user input directly — resulting in <value>MVN_PASSWORD</value> instead of the correct <value>env:MVN_PASSWORD</value>.

The MasterDispatcher in plexus-sec-dispatcher then fails validation because no MasterSource recognizes a config string without its required prefix (env:, sys-property:, etc.), producing the error: Configured Source configuration not handled.

Fix

In Init.java, after the user provides their input for an editable field, the code now:

1. Extracts the prefix from the template (everything before the $ placeholder).
2. Checks if the user's input already includes the prefix.
3. Prepends the prefix automatically if missing.

This ensures the generated settings-security4.xml always contains well-formed source values like env:MVN_PASSWORD.

Testing

• All 468 tests in maven-cli pass.
• Manually verified that mvn --enc init now writes the correct env: prefix and mvn --enc encrypt succeeds afterward.

---

• Your pull request should address just one issue, without pulling in other changes.

• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.

• Each commit in the pull request should have a meaningful subject line and body.

• Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied.

• Run mvn verify to make sure basic checks pass.

• You have run the Core IT successfully.

• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004

[elharo]

needs test

[Hiteshsai007]

@elharo Thanks for the review!

I have addressed your feedback by adding comprehensive unit tests for the configuration saving logic.

To accomplish this, I:

1. Refactored Init.java by extracting the prefix-prepending logic into a testable, package-private static method (applyTemplatePrefix).
2. Added a new InitTest.java suite that covers all edge cases, including:
   • Missing prefixes: prepending env: when the user only types MVN_PASSWORD.
   • Duplicate prefixes: preventing double-prefixing if the user manually types env:MVN_PASSWORD.
   • Other prefix types: ensuring it correctly handles sys-property:, file:, etc.
   • Edge conditions: properly handling empty user inputs, missing templates, and templates without placeholders.
3. Verified the changes locally with a full mvn clean verify on the maven-cli module to ensure all tests, Spotless formatting, and Checkstyle rules pass successfully.

Let me know if there's anything else needed before we can merge this!

[elharo]

This should be testable through the existing API or maybe an integration test. This approach is a red flag. I would start over by reproducing the issue in a failing test, and then implement a fix.

[Hiteshsai007]

@elharo Thank you for the feedback. That is a completely fair point—extracting an internal static method purely to make it testable was definitely a smell.

I've pushed a new commit that starts over and follows your suggested approach:

1. Reverted the applyTemplatePrefix extraction and put the logic back inline where it originally belonged inside Init.java's execution loop.
2. Wrote a new integration-style test (InitTest.java) that tests through the existing Init.doExecute() API.
3. To test the interactive CLI logic without exposing internal state, I used Mockito's MockedConstruction to mock ConsolePrompt and simulate the sequence of user inputs that caused the original bug.
4. Verified that the test successfully fails by reproducing the issue (resulting in a saved value without the env: prefix), and then applied the inline fix to make it pass.

All maven-cli tests, spotless, and checkstyle checks are passing locally. Let me know if this aligns better with the project's testing patterns!

[gnodet]

Thanks for tackling MNG-8425 — the root-cause analysis is spot-on and the fix targets the right place. The prefix-extraction approach is correct for all known MasterSource types. A few things to address before this can be merged:

Missing edge-case test coverage

The single test covers only the happy path (user omits the prefix). Please add tests for:

• User already includes the prefix (e.g., types env:MY_VAR) — verifies no double-prepending
• Different prefix type (e.g., template system-property:$systemProperty) — verifies it works beyond env:
• Empty prefix (e.g., template $VAR) — verifies user input is used as-is

Inline fully-qualified class names in test

Several types are referenced by FQCN inline rather than imported at the top of the file (e.g., java.nio.file.Paths, java.util.ArrayList, org.jline.utils.AttributedStyle, java.io.PrintWriter, org.jline.consoleui.prompt.ConfirmResult, org.jline.consoleui.elements.ConfirmChoice). Other tests in impl/maven-cli use proper imports — please follow the same convention.

See the inline comments for the remaining items.

Note: this review does not replace specialized AI review tools (CodeRabbit, Sourcery) or static analyzers (SonarCloud).

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

[gnodet]

PromptResultItemIF is structurally a functional interface (single abstract method getResult(), one default method). The anonymous inner class can be replaced with a lambda, which also eliminates the need for the finalResult intermediate variable:

Suggested change

	final String finalResult = userInput;
	dispatcherConfigResult.put(editable.getKey(), new PromptResultItemIF() {
	@Override
	public String getResult() {
	return finalResult;
	}
	});
	String result = userInput;
	dispatcherConfigResult.put(editable.getKey(), () -> result);

Same simplification applies to the createResult() helper in the test file.

[gnodet]

This template.contains("$") guard is always true here — the editables list (line 127) is already filtered to entries whose result contains $, and template is assigned from editable.getValue().getResult(). The redundant check adds unnecessary nesting. Consider removing it:

Suggested change

	if (template.contains("$")) {
	String prefix = template.substring(0, template.indexOf('$'));
	if (!prefix.isEmpty() && !userInput.startsWith(prefix)) {
	userInput = prefix + userInput;
	}
	}
	String prefix = template.substring(0, template.indexOf('$'));
	if (!prefix.isEmpty() && !userInput.startsWith(prefix)) {
	userInput = prefix + userInput;
	}

[gnodet]

The createResult() helper at the bottom of this file also uses an anonymous inner class where a lambda would suffice:

private PromptResultItemIF createResult(String value) {
    return () -> value;
}

[Hiteshsai007]

Thanks for the thorough review! I've gone ahead and addressed all the feedback.

Here is a summary of the changes made in the latest commit:

1. Missing edge-case test coverage:
   I rewrote the test using JUnit 5's @ParameterizedTest and @CsvSource to effectively cover all the requested edge-cases without duplicating code. The test now validates:

   • User already including the prefix (e.g., env:MY_VAR) — verifies no double-prepending.
   • Different prefix types (e.g., system-property:my_prop) — verifies the logic handles arbitrary types beyond env:.
   • Empty prefixes ($VAR) — verifies user input is safely used as-is.

2. Inline fully-qualified class names in test:
   I removed the inline FQCNs (such as java.nio.file.Paths, java.util.ArrayList, org.jline.consoleui.prompt.ConfirmResult, etc.) and correctly imported them at the top of InitTest.java to align with the conventions of the other tests in impl/maven-cli.

3. Replaced anonymous inner classes with lambdas:
   Since PromptResultItemIF is structurally a functional interface, I simplified the code by replacing the anonymous inner classes with lambda expressions (() -> result) in both Init.java and the createResult helper in InitTest.java.

4. Removed redundant template.contains("$") guard:
   I removed the redundant if (template.contains("$")) check inside Init.java. The editables list is indeed already strictly filtered by e.getValue().getResult().contains("$") earlier in the pipeline, so removing the guard safely eliminates unnecessary nesting.

Let me know if there's anything else you'd like me to adjust!

[Copilot]

Pull request overview

This PR fixes the Maven encryption wizard (mvn --enc init) so that editable master-source configuration values persist with their required source prefix (e.g., env:), preventing generation of invalid settings-security4.xml that would later cause mvn --enc encrypt to report “Maven Encryption is not configured.”

Changes:

• Update Init to automatically prepend the template-derived prefix (text before $) to the user’s edited value when missing.
• Add a parameterized unit test covering prefixed and already-prefixed inputs (and a no-prefix template case).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnenc/goals/Init.java	Prepends the template prefix (before $) to editable user input when absent, ensuring stored master-source values remain valid.
impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnenc/goals/InitTest.java	Adds parameterized coverage validating the prefix-prepend behavior for multiple template/input combinations.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[gnodet]

All review comments have been addressed — nice work!

• Redundant template.contains("$") check removed ✅
• Anonymous inner classes replaced with lambdas ✅
• Inline FQCNs converted to proper imports ✅
• Parameterized test now covers: prefix omitted, prefix already present, different prefix type (system-property:), and empty prefix ($VAR) ✅

The fix is clean and well-tested. Thanks for the thorough follow-up.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.