fix: interpolate properties in module/subproject path before filesystem resolution

[waterWang]

Problem

Maven 4.0.0-rc6 does not interpolate properties in <module> (or <subproject>) paths of aggregator POMs. For example:

<properties>
    <version-discriminator>-v14</version-discriminator>
</properties>
<modules>
    <module>./../module/pom${version-discriminator}.xml</module>
</modules>

Maven 3 correctly interpolates ${version-discriminator} to produce ./../module/pom-v14.xml, but Maven 4 fails with:

[ERROR] Child subproject .../pom${version-discriminator}.xml of ... does not exist

Root Cause

In DefaultModelBuilder.loadFilePom(), the subproject/module path strings are resolved against the filesystem before model-wide property interpolation runs. The path ${version-discriminator} is never resolved, so Maven 4 treats it as a literal directory name.

Fix

Interpolate each subproject path string against the model, user, and system properties before resolving it against the filesystem. This is a targeted interpolation of the path only — the full model interpolation still runs later in the build pipeline, unchanged.

Testing

• Maven 3 behavior: module paths with ${property} references are correctly interpolated
• This fix restores that behavior in Maven 4 by interpolating the path before the filesystem lookup
• Non-interpolated paths (the common case) pass through unchanged

Fixes #12729

[gnodet]

The fix correctly identifies the root cause — Maven 4 doesn't interpolate properties in module/subproject paths before filesystem resolution, breaking ${property}-based module paths that worked in Maven 3. The approach (targeted early interpolation) is sound.

However, two issues should be addressed:

Property lookup precedence is inverted

The callback checks activated.getProperties() (model/POM properties) first, then falls back to request.getUserProperties(). Maven convention is that user properties (-Dprop=value) override POM-defined properties.

Compare with the established patterns in the same file:

• interpolateModel() (~line 2448): Interpolator.chain(userProps, modelProps, systemProps) — user properties first
• getPropertiesWithProfiles() (~lines 798-799): putAll(request.getUserProperties()) called last so user properties override everything

With the current code, if a user passes -Dversion-discriminator=-v15, the POM-defined value would still be used instead of the user override.

Suggested fix — swap the order:

subproject = interpolator.interpolate(
        subproject,
        Interpolator.chain(
                request.getUserProperties()::get,
                activated.getProperties()::get,
                request.getSystemProperties()::get));

Missing test

No test accompanies this regression fix. The issue (#12729) provides a reproducible project structure that could be adapted into an integration test under its/core-it-suite/ to prevent future regression.

Minor nits

• The comment references MNG-XXXX — should be #12729
• The if (interpolated != null) guard is redundant since subproject is guaranteed non-null at that point

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

Claude Code on behalf of gnodet

[gnodet]

The fix correctly identifies the root cause (module paths need interpolation before filesystem resolution), but several issues need to be addressed.

Findings:

1. [high] Property lookup precedence is inverted — The code checks activated.getProperties().get(key) (model) before request.getUserProperties().get(key), but Maven convention is user → model → system. Compare with interpolateModel() at line 2398 in the same file which uses Interpolator.chain(userProps::get, modelProps::get, systemProps::get). With the current code, a user passing -Dprop=value would be silently ignored if the POM also defines that property.

2. [medium] Use Interpolator.chain() instead of manual lambda — The manual key -> { ... } lambda duplicates what Interpolator.chain() already provides. The same file uses Interpolator.chain() at line 2398 for the same purpose. Suggested replacement:

   subproject = interpolator.interpolate(
       subproject,
       Interpolator.chain(
           request.getUserProperties()::get,
           activated.getProperties()::get,
           request.getSystemProperties()::get));

3. [medium] No regression test — This is a fix for issue #12729 (Maven 3 behavior broken in Maven 4). A regression test should accompany this fix to prevent recurrence.

4. [low] Placeholder issue reference — Comment says MNG-XXXX which should be replaced with the actual issue reference #12729.

5. [low] Redundant null guard — The if (interpolated != null) check is unnecessary since interpolator.interpolate() returns non-null for non-null input, consistent with how interpolateModel() uses the result directly without a null check.

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

Claude Code on behalf of Guillaume Nodet

[gnodet]

The conceptual fix is sound — module/subproject paths are resolved against the filesystem before full model interpolation runs, so early targeted interpolation is the right approach. However, the prior CHANGES_REQUESTED feedback (Aug 17) remains unaddressed:

High severity:

1. Property lookup precedence is inverted: The code checks activated.getProperties() (model/profile properties) before request.getUserProperties(). Maven convention is user > model > system. Compare with interpolateModel() at line 2364 which uses Interpolator.chain() with user properties first. With the current code, -Dprop=value on the command line would be silently ignored if the POM also defines that property.

Medium severity:

2. Manual lambda duplicates Interpolator.chain() utility: The same file uses Interpolator.chain(map1::get, map2::get, map3::get) at line 2364 for the identical pattern. Using the existing utility would be more consistent and handle precedence correctly.

3. No regression test: Issue #12729 provides a reproducible project structure that could be adapted into an integration test.

Low severity:

4. Comment references MNG-XXXX instead of the actual issue #12729.
5. The if (interpolated != null) guard is redundant — interpolate() never returns null for non-null input.

---

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

Claude Code on behalf of Guillaume Nodet

[gnodet]

All five findings from prior reviews have been addressed in the latest commit:

#	Finding	Status
1	Property lookup precedence inverted (high)	✅ Resolved — now uses Interpolator.chain(userProps, modelProps, systemProps), matching interpolateModel()
2	Use Interpolator.chain() instead of manual lambda (medium)	✅ Resolved — manual lambda replaced with idiomatic chain() call
3	No regression test (medium)	✅ Resolved — MavenITgh12729ModulePathPropertyInterpolationTest added with two test methods: POM-property interpolation and user-property-override
4	Placeholder MNG-XXXX (low)	✅ Resolved — comment now reads // #12729:
5	Redundant null guard (low)	✅ Resolved — removed; interpolate() result assigned directly

The user-property-override test is a particularly good addition as it directly validates the corrected precedence chain. Clean, focused diff (17 lines removed, 10 added in source + 130 lines of test).

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

Claude Code on behalf of Guillaume Nodet