[MNG-8655] Add a settings parser SPI

[goutamadwant]

Add a SettingsParser SPI so extensions can read settings in an alternative syntax (e.g. YAML, properties) without replacing DefaultSettingsBuilder.

Each Source selects one parser via supports(). Strict parse failures are retried leniently with the strict warning preserved. Multiple matching parsers produce a structured fatal BuilderProblem. RuntimeException from supports() is caught, recorded as a fatal problem with provider name and source location, and selection stops for that source. XML fallback through XmlSettingsParser remains the default.

Interpolation, decryption, validation, project-settings restrictions, and merging remain in DefaultSettingsBuilder unchanged. The SPI accepts file-backed and in-memory sources.

Settings discovery and extension loading are unchanged. A parser must already be available to the container reading settings; an extension cannot parse the bootstrap settings needed to resolve itself.

New files:

• api/maven-api-spi: SettingsParser SPI interface, SettingsParserException
• impl/maven-impl: XmlSettingsParser (wraps existing XML logic), updated DefaultSettingsBuilder
• Unit tests: DefaultSettingsParserTest (parser selection, diagnostics, fallback), SettingsParserExceptionTest
• Core IT: MavenITmng8655SettingsParserTest — extension-classpath parsing, XML fallback, bootstrap limitation, core-realm parser

Closes #10436.

[gnodet-bot]

Overall this is a clean design — the SPI contract is well-specified, the retry semantics are preserved from the original code, and the test coverage is comprehensive. A few issues need addressing before merge.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

⚠️ No-arg and cause-only constructors silently allow null message

SettingsParserException() and SettingsParserException(Throwable cause) both delegate to this(null, null) / this(null, cause), setting message = null. In DefaultSettingsBuilder.readSettings(), the warning and fatal problem messages include e.getMessage() directly — so a parser that throws new SettingsParserException(cause) (no message) will produce a problem like "Non-parseable settings settings.yaml: null", which is useless for diagnostics.

The @param on SettingsParserException(Throwable cause) should either:

1. Require a non-null message (match the 2-arg constructor pattern), or
2. At least document that message may be null, so callers of getMessage() know to guard.

Prefer option 1 — make it impossible to create a message-less SettingsParserException from a cause:

Suggested change

	public SettingsParserException(String message, Throwable cause) {
	this(message != null ? message : (cause != null ? cause.getMessage() : "unknown error"), -1, -1, cause);
	}

(And drop the no-arg constructor or at least give it a default message.)

[gnodet-bot]

⚠️ Original strict-parse exception is silently discarded if lenient parse also fails

When the strict parse throws SettingsParserException e, the inner catch retries with strict=false (line 174). If that retry also throws a SettingsParserException, it escapes the inner catch block and is caught by the outer catch on line 183 as a FATAL error. Two consequences:

1. The WARNING problem for the original strict failure is never recorded (line 175-181 are never reached).
2. The outer FATAL records the lenient-parse exception, losing the original strict one entirely — no cause chain linking them.

The old code had the same structure, so this is not a regression, but the new code is a good opportunity to fix it. The simplest fix is to record the strict-parse warning first, before retrying:

Suggested change

	} catch (SettingsParserException e) {
	settings = parser.parse(settingsSource, Map.of(SettingsParser.STRICT, false));
	problems.reportProblem(new DefaultBuilderProblem(
	settingsSource.getLocation(),
	e.getLineNumber(),
	e.getColumnNumber(),
	e,
	e.getMessage(),
	BuilderProblem.Severity.WARNING));
	} catch (SettingsParserException e) {
	problems.reportProblem(new DefaultBuilderProblem(
	settingsSource.getLocation(),
	e.getLineNumber(),
	e.getColumnNumber(),
	e,
	e.getMessage(),
	BuilderProblem.Severity.WARNING));
	settings = parser.parse(settingsSource, Map.of(SettingsParser.STRICT, false));

With this ordering, if the lenient parse succeeds, the WARNING is recorded. If the lenient parse throws, the WARNING is still recorded before the FATAL, giving the user both data points.

[gnodet-bot]

test

[gnodet-bot]

test comment

[gnodet-bot]

test multi-line

[gnodet-bot]

test retry comment

[gnodet-bot]

test outer catch 190

[gnodet-bot]

test selectParser comment at 183-190

[gnodet-bot]

test exception

[gnodet-bot]

test null message

[gnodet-bot]

Three findings from the previous review are still unaddressed, plus one new issue. None are catastrophic but together they leave diagnostic gaps that will hurt real users.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

⚠️ [NOT ADDRESSED] Duplicate Javadoc block on STRICT

The field still has two consecutive Javadoc comments — lines 48–50 (Boolean parsing option...) and lines 51–54 (Option that can be specified...). Only the second block is shown by IDE tooling; the first is silently discarded by the Javadoc compiler. Drop the first block:

Suggested change

	String STRICT = "strict";
	/**
	* Option that can be specified in the options map. The value should be a {@code Boolean};
	* when {@code true} or absent, unknown input is rejected.
	*/
	String STRICT = "strict";

[goutamadwant]

Fixed. Removed the duplicate block. STRICT now has a single Javadoc explaining the Boolean option and default strict behavior.

[gnodet-bot]

⚠️ [NOT ADDRESSED] Null-message constructors produce useless diagnostics

SettingsParserException() (line 42) and SettingsParserException(Throwable cause) (line 60) both set message to null. In DefaultSettingsBuilder.readSettings(), e.getMessage() is used directly in the warning (line 180) and fatal (line 189) problem messages — so a parser that throws new SettingsParserException(cause) produces:

Non-parseable settings settings.yaml: null

That is useless for diagnostics. Fall back to the cause's message:

Suggested change

	public SettingsParserException(String message, Throwable cause) {
	this(message, -1, -1, cause);
	}
	public SettingsParserException(String message, int lineNumber, int columnNumber, Throwable cause) {
	super(message, cause);
	this.lineNumber = lineNumber;
	this.columnNumber = columnNumber;
	}
	public SettingsParserException(Throwable cause) {
	this(null, cause);
	}
	public SettingsParserException(String message, Throwable cause) {
	this(message != null ? message : (cause != null ? cause.getMessage() : "unknown error"), -1, -1, cause);
	}
	public SettingsParserException(Throwable cause) {
	this(cause != null ? cause.getMessage() : "unknown error", cause);
	}

[goutamadwant]

Fixed. Message fallback now lives in the full constructor, covering every overload. Explicit messages are preserved; otherwise the cause's message is used, with a default when that is also null. Added constructor and builder-diagnostic tests, including preservation of causes and line/column information.

[gnodet-bot]

⚠️ [NOT ADDRESSED] WARNING is lost when lenient retry also fails

Current flow: strict parse throws SettingsParserException e (line 173) → lenient retry is attempted (line 174) → if the retry also throws, it escapes the inner catch, hits the outer catch (SettingsParserException) at line 183 as a FATAL, and reportProblem(WARNING, e) on lines 175–181 is never reached. The user gets a FATAL with the retry's exception, losing the original strict-parse error and all its location info.

Record the WARNING before retrying so it survives either outcome:

Suggested change

	} catch (SettingsParserException e) {
	settings = parser.parse(settingsSource, Map.of(SettingsParser.STRICT, false));
	problems.reportProblem(new DefaultBuilderProblem(
	settingsSource.getLocation(),
	e.getLineNumber(),
	e.getColumnNumber(),
	e,
	e.getMessage(),
	BuilderProblem.Severity.WARNING));
	} catch (SettingsParserException e) {
	problems.reportProblem(new DefaultBuilderProblem(
	settingsSource.getLocation(),
	e.getLineNumber(),
	e.getColumnNumber(),
	e,
	e.getMessage(),
	BuilderProblem.Severity.WARNING));
	settings = parser.parse(settingsSource, Map.of(SettingsParser.STRICT, false));

[goutamadwant]

Fixed. The strict warning is now recorded before the lenient retry. Added regressions for retries ending in either a parse error or IOException, verifying that both the original warning and subsequent fatal problem are retained. Updated the SPI documentation accordingly.

[gnodet-bot]

⚠️ [NEW] selectParser conflict error misformatted as "Non-parseable settings"

selectParser() is called at line 170, inside the outer try. When multiple parsers match, it throws SettingsParserException("Multiple settings parsers support this source: ..."), which propagates here and produces:

Non-parseable settings settings.properties: Multiple settings parsers support this source: first, second

That message is wrong — the source was never parsed; the error is a configuration conflict between registered parsers. A user seeing "Non-parseable settings" will look for a syntax error in their file, not a duplicated extension registration.

The test at conflictingParsersAreReportedBeforeParsing only asserts contains("Multiple settings parsers...") so it misses this misleading prefix.

Cleanest fix: have selectParser throw a RuntimeException or dedicated non-SettingsParserException type that bypasses this catch entirely. Alternatively, move the selectParser() call outside of the try block.

[goutamadwant]

Fixed. Parser conflicts now report a fatal builder problem directly, without the misleading “Non-parseable settings” prefix. This preserves the existing SettingsBuilderException handling. Strengthened the tests to check the complete diagnostic, source location, sorted provider names, and that neither parser runs.

[gnodet-bot]

All four findings from the previous review are addressed:

• ✅ Duplicate Javadoc on STRICT — single clean block, aligned with ModelParser.
• ✅ Null-message constructors — messageOrDefault() guarantees a non-null message; SettingsParserExceptionTest covers all constructor paths.
• ✅ WARNING lost when lenient retry fails — reportProblem(WARNING, e) now fires before the lenient parse() call. If that retry throws, the WARNING is already recorded. strictWarningSurvivesFailedLenientParsing and strictWarningSurvivesUnreadableLenientInput pin this.
• ✅ selectParser conflict misformatted as "Non-parseable settings" — conflict is now reported as a structured FATAL via reportProblem and selectParser returns null, bypassing the outer catch entirely. conflictingParsersAreReportedBeforeParsing covers it.

One new concern.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

💡 supports() RuntimeException leaks as a raw stack trace

If a custom SettingsParser.supports() throws a RuntimeException (e.g. an NPE from accessing source.getLocation() before the source is ready), it propagates unchecked out of selectParser, exits readSettings through neither the SettingsParserException nor IOException branches, and surfaces to the caller as a raw exception — not a structured FATAL problem.

For a user with a buggy extension, this means a raw stack trace instead of a Maven diagnostic message. ModelParser has the same gap, so this is consistent with the existing SPI contract, but since SettingsParser.supports() is being introduced fresh, it's worth documenting or guarding:

Suggested change

	.filter(entry -> entry.getValue().supports(source))
	.filter(entry -> {
	try {
	return entry.getValue().supports(source);
	} catch (RuntimeException e) {
	problems.reportProblem(new DefaultBuilderProblem(
	source.getLocation(), -1, -1, e,
	"Settings parser '" + entry.getKey() + "' failed during supports() check: " + e.getMessage(),
	BuilderProblem.Severity.FATAL));
	return false;
	}
	})

Alternatively, document explicitly in SettingsParser.supports() Javadoc that the method must not throw — as-is, the contract is silent on this.

[goutamadwant]

Fixed. Runtime exceptions from supports() now produce a fatal settings-builder problem identifying the provider and source, with the original exception retained. Selection stops immediately for that source, preventing further provider checks, parsing, or XML fallback. Added named and unnamed provider regressions, including failure after an earlier match, and documented the behavior. Full mvn -Prun-its verify passed on Java 17.

[gnodet-bot]

All previous findings are addressed, and the new commit correctly resolves the last open concern.

• ✅ supports() RuntimeException guard — selectParser now catches RuntimeException, records a FATAL DefaultBuilderProblem (with provider name, source location, and the original cause), and returns null immediately. readSettings then returns Settings.newInstance(), and build() converts the FATAL to a SettingsBuilderException via hasErrorProblems(). End-to-end flow is correct.
• ✅ Javadoc on supports() — the added sentence accurately describes the runtime behavior: "A runtime exception from this check is reported as a fatal selection error; Maven does not try another parser or the XML fallback for that source."
• ✅ Test coverage — four test methods cover named/unnamed × broken-first/matching-first. verifyNoInteractions(unvisited, xmlFactory) confirms the loop stops early AND the XML fallback is bypassed. verify(source, never()).openStream() confirms no stream is consumed.

One nit in the shared test helper:

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

💡 Nit: vacuous assertion when matchingFirst=false

When matchingFirst is false, matching is never added to the parsers map, so verify(matching, never()).parse(any(), any()) passes trivially — Mockito never saw an interaction to record. The assertion says nothing in that branch.

If the intent is to prove that stopping on the broken provider also prevents any previously-seen match from being parsed, the meaningful assertion is only valid when matchingFirst=true. For the false case, a more useful assertion would be that the broken provider's supports() was actually called:

Suggested change

	verify(matching, never()).parse(any(), any());
	verify(broken).supports(source);
	if (matchingFirst) {
	verify(matching, never()).parse(any(), any());
	}
	verify(broken, never()).parse(any(), any());

(Or just leave the verify(matching, never()).parse(...) call guarded by if (matchingFirst) — it is harmless as-is but misleading.)

[goutamadwant]

Fixed. The assertion for the previously matched parser is now guarded by matchingFirst. The broken provider's supports() invocation is still verified in every case.

[goutamadwant]

@gnodet Fixed. The settings parser ITs now use an isolated user home, so the Mimir core extension installed by CI cannot trigger an unintended bootstrap settings read in the classpath-parser scenario. The negative test still declares its project extension explicitly.

[gnodet-bot]

The nit from the previous review is correctly fixed:

• ✅ matchingFirst guard — verify(matching, never()).parse(any(), any()) is now inside if (matchingFirst). verify(broken).supports(source) runs unconditionally in both branches. No vacuous assertion.
• ✅ IT isolation — All three tests (extensionClassPathParserAndXmlFallback, declaredParserCannotReadItsOwnBootstrapSettings, coreRealmParserReadsBootstrapSettings) now set an isolated user home via setUserHomeDirectory. The second verifier in extensionClassPathParserAndXmlFallback reuses the same directory.resolve("home") path already created by the first call — intentional and correct.

The 2026-09-16 commit touches only test files (7 lines). No production code was changed. Nothing else to raise.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet]

selectParser can be made more compact by:

1. Extracting the repeated entry.getKey() != null ? entry.getKey() : "<unnamed>" into a small parserName helper
2. Building the message in a local variable before reportProblem, so the 6-arg constructor fits on one line
3. Moving matches.add(entry) inside the try, eliminating the boolean supported temp var

Spotless-verified (compiled and reformatted cleanly).

Suggested change

	@Nullable
	private SettingsParser selectParser(Source source, ProblemCollector<BuilderProblem> problems) {
	List<Map.Entry<String, SettingsParser>> matches = new ArrayList<>();
	for (Map.Entry<String, SettingsParser> entry : settingsParsers.entrySet()) {
	boolean supported;
	try {
	supported = entry.getValue().supports(source);
	} catch (RuntimeException e) {
	problems.reportProblem(new DefaultBuilderProblem(
	source.getLocation(),
	-1,
	-1,
	e,
	"Settings parser '" + (entry.getKey() != null ? entry.getKey() : "<unnamed>")
	+ "' failed to determine support for this source",
	BuilderProblem.Severity.FATAL));
	return null;
	}
	if (supported) {
	matches.add(entry);
	}
	}
	if (matches.size() > 1) {
	problems.reportProblem(new DefaultBuilderProblem(
	source.getLocation(),
	-1,
	-1,
	null,
	"Multiple settings parsers support this source: "
	+ String.join(
	", ",
	matches.stream()
	.map(entry -> entry.getKey() != null ? entry.getKey() : "<unnamed>")
	.sorted()
	.toList()),
	BuilderProblem.Severity.FATAL));
	return null;
	}
	return matches.isEmpty() ? xmlSettingsParser : matches.get(0).getValue();
	}
	@Nullable
	private SettingsParser selectParser(Source source, ProblemCollector<BuilderProblem> problems) {
	List<Map.Entry<String, SettingsParser>> matches = new ArrayList<>();
	for (Map.Entry<String, SettingsParser> entry : settingsParsers.entrySet()) {
	try {
	if (entry.getValue().supports(source)) {
	matches.add(entry);
	}
	} catch (RuntimeException e) {
	String msg = "Settings parser '" + parserName(entry.getKey())
	+ "' failed to determine support for this source";
	problems.reportProblem(
	new DefaultBuilderProblem(source.getLocation(), -1, -1, e, msg, BuilderProblem.Severity.FATAL));
	return null;
	}
	}
	if (matches.size() > 1) {
	String parsers =
	matches.stream().map(e -> parserName(e.getKey())).sorted().collect(Collectors.joining(", "));
	String msg = "Multiple settings parsers support this source: " + parsers;
	problems.reportProblem(
	new DefaultBuilderProblem(source.getLocation(), -1, -1, null, msg, BuilderProblem.Severity.FATAL));
	return null;
	}
	return matches.isEmpty() ? xmlSettingsParser : matches.get(0).getValue();
	}
	private static String parserName(@Nullable String key) {
	return key != null ? key : "<unnamed>";
	}

Also needs import java.util.stream.Collectors; added to the imports.

[gnodet-bot]

The selectParser refactor from the previous review is correctly applied:

• ✅ parserName() helper — extracts the repeated key != null ? key : "<unnamed>" into a reusable private method. Used in both the RuntimeException catch and the multi-match conflict path.
• ✅ matches.add() moved inside try — eliminates the boolean supported temp variable. Cleaner control flow with no behavioral change.
• ✅ Collectors.joining(, ) replaces String.join(…, .toList()) — more idiomatic streaming; produces identical output.
• ✅ import java.util.stream.Collectors — added as requested. The previously present import java.util.stream.Collectors in gnodet's suggestion block is present in the commit.

Single-file change, purely cosmetic/readability. No production behaviour changed, no test changes needed.

This review was generated by an AI agent, Hermès on behalf of @gnodet.