Fix #13191: add -Dmaven.maven3Personality hint to FATAL message for wrong parent relativePath

[gnodet]

Summary

Fixes #13191 — improves the FATAL error message when an explicit <relativePath> resolves to a different G:A than the declared parent.

Background

Maven 4 correctly raises a [FATAL] when a POM has an explicit <relativePath> pointing at the wrong artifact — this is a configuration error. However, the error message was unhelpful: it didn't tell users why this might happen or what to do about it.

A common cause is maven-shade-plugin generating dependency-reduced-pom.xml with a <relativePath> that is invalid in the generated context (see apache/maven-shade-plugin#813). Users hit a FATAL with no guidance.

Change

Only the FATAL message (the else branch in mismatchRelativePathAndGA) is updated. No logic change.

The new message adds:

• a hint that build tools generating POMs with incorrect <relativePath> (e.g. maven-shade-plugin) should be upgraded
• --maven3-personality as an explicit temporary workaround (it already demoted this to WARNING — now users are told about it)

The right fix

The shade plugin fix is in apache/maven-shade-plugin#843: emit <relativePath/> (empty) in generated POMs. Once that ships, the --maven3-personality workaround won't be needed.

Test

ParentCycleDetectionTest#testWrongRelativePathInGeneratedPomIsFatalWithMaven3Hint — verifies the FATAL is raised and the message contains the --maven3-personality hint.

[gnodet-bot]

Review — fix/13191-shade-relativepath

Fix logic: correct and well-scoped. The selfReference detection in mismatchRelativePathAndGA() is correct: comparing the resolved file's G:A against the child's own G:A. Both call sites receive this pair as parameters, so the check is hermetic regardless of whether G comes from getGroupId() (null-safe fallback) or is explicitly declared. The severity downgrade from FATAL → WARNING with repository fallback is the right behaviour, matching Maven 3.

One gap worth addressing before undrafting:

Call site 1 (doReadFileModel) has no test coverage. The PR claims both call sites are covered by the fix, but the test only exercises the ParentResolutionFrame.advance() path (call site 2). Call site 1 — in doReadFileModel — is only reached when the parent's G/A/V are partially missing (e.g. <version>${revision}</version>) AND an explicit <relativePath> is present. The shade-plugin scenario doesn't trigger it (full G:A:V specified), so the test misses it.

A second test case like this would close that gap:

// dependency-reduced-pom.xml with version-expression parent (triggers doReadFileModel path)
Files.writeString(reducedPom, """
    <project xmlns="http://maven.apache.org/POM/4.0.0">
        <modelVersion>4.0.0</modelVersion>
        <parent>
            <groupId>org.apache.sling</groupId>
            <artifactId>sling-bundle-parent</artifactId>
            <version>${revision}</version>
            <relativePath>pom.xml</relativePath>
        </parent>
        <groupId>org.apache.sling</groupId>
        <artifactId>org.apache.sling.models.impl</artifactId>
        <version>${revision}</version>
        <packaging>jar</packaging>
    </project>
    """);

If this path is unreachable in practice (e.g. ${revision} can't appear in a shade-generated POM), a comment explaining why call site 1 is safe without a dedicated test would be sufficient.

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

[gnodet-bot]

Review — fix/13191-shade-relativepath (re-review, commit 0347e1c)

This is a clean redesign from the draft I previously reviewed. The old version downgraded FATAL → WARNING for self-referential cases; this version keeps FATAL and only improves the message. That's the right call — the FATAL is correct behaviour, the problem was user confusion, and a better message addresses that without compromising the integrity check.

Previous gap (doReadFileModel test coverage) is resolved by the redesign. The prior concern was that a selfReference detection only had test coverage for one of two call sites. Since this PR no longer adds selfReference logic, both call sites (doReadFileModel ~L1898 and ParentResolutionFrame.advance() ~L2308) call the unchanged mismatchRelativePathAndGA() and both get the new message automatically. No asymmetry.

Test is correct. testWrongRelativePathInGeneratedPomIsFatalWithMaven3Hint accurately reproduces the shade scenario: dependency-reduced-pom.xml with <relativePath>pom.xml</relativePath> where pom.xml belongs to a different G:A. ApiRunner.createSession() creates a default session without maven3Personality, so maven3Mode = false, the FATAL branch fires, and the three contains assertions ("relativePath", "pom.xml", "maven3-personality") all match the actual message. ✅

One minor note: The test is in ParentCycleDetectionTest which is slightly misnamed for this scenario, but it already contains other parent-resolution edge cases (wrong relativePath, version constants) so it's the right home. No action needed.

Ready to undraft.

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

[gnodet-bot]

Re-review — fix/13191-shade-relativepath (SHA 0347e1cb)

This is a complete direction change from the prior approach (downgrade FATAL→WARNING with self-reference detection). The new approach keeps FATAL and improves the message — that's a valid design choice. One actionable issue before merging.

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

[gnodet-bot]

Re-review — fix/13191-shade-relativepath (SHA f38ccf5d)

The prior CHANGES_REQUESTED finding (--maven3-personality is not a valid CLI flag) was addressed — that was the right move. Two related issues remain.

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

[gnodet-bot]

All three findings from the previous CHANGES_REQUESTED (f38ccf5) are fixed in 172f34b:

1. ✅ Production message now reads -Dmaven.maven3Personality=true (with =true) — Boolean.parseBoolean("") returning false is no longer a trap.
2. ✅ Test Javadoc updated to {@code -Dmaven.maven3Personality=true}.
3. ✅ assertTrue failure message uses -Dmaven.maven3Personality=true hint.

Logic verified: in mismatchRelativePathAndGA, defaultPath=false (explicit <relativePath>pom.xml</relativePath>) + maven3Mode=false (not set in test session) → warn=false → FATAL fires with the new hint. Test assertion contains("maven.maven3Personality") matches the actual runtime message. The workaround claim is accurate: maven3Personality=true demotes to WARNING and allows repository fallback.

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

[gnodet-bot]

Re-review after spotless formatting commit (9895c86).

Pure mechanical reformat: assertTrue(hasFatalWithHint, ...) split to two-argument vertical form to satisfy spotless. Zero logic change.

All prior findings remain addressed. APPROVE.

---

generated by an AI agent, Hermès

[gnodet-bot]

Re-review — fix/13191-shade-relativepath (SHA 9895c861)

Spotless formatting commit only — assertTrue(hasFatalWithHint, "...") reformatted to the two-line form. Zero semantic change.

All findings from prior CHANGES_REQUESTED reviews (f38ccf5, 0347e1c) were confirmed fixed in 172f34b. Nothing new introduced by this commit. Ready to merge.

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

[gnodet-bot]

Re-review — fix/13191-shade-relativepath (SHA 8bf66263)

This commit reverts the previous =true suffix on -Dmaven.maven3Personality. That is correct.

The prior bot review at 172f34ba flagged the absence of =true as a bug, but that was a false positive. Apache Commons CLI's getOptionProperties("D") assigns "true" as the value for a bare -Dfoo flag (no = separator), per the documented behaviour: "If there are an odd number of values the last value is assumed to be a boolean flag and the value is true." So Boolean.parseBoolean("true") fires correctly. The existing IT MavenITgh2576ItrNotHonoredTest already uses bare -Dmaven.maven3Personality confirming this is idiomatic in the codebase.

Test assertion contains("maven.maven3Personality") still holds — the production message contains -Dmaven.maven3Personality which is a superset of the checked substring. ✅

All prior findings are resolved. Ready to merge.

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