[MNG-8765] Pre-interpolate plugin configuration before type conversion

[gnodet]

Summary

• Pre-interpolates PlexusConfiguration values using the expression evaluator in DefaultMavenPluginManager before passing them to the ComponentConfigurator, ensuring ${...} property references are fully resolved before type converters (like UriConverter) process the values
• Fixes the case where properties set dynamically at runtime (e.g., by Groovy/GMaven scripts via project.properties.setProperty()) are not available during model interpolation and reach type converters unresolved, causing URISyntaxException for URI-typed parameters
• Adds integration tests covering URI property interpolation with POM properties, inherited parent properties, and CLI-passed properties

Regression found in CloudStack (gnodet/maven4-testing#34733) where a URI parameter with ${cs.version} set by a Groovy script at runtime caused URISyntaxException.

Test plan

• Unit tests pass (580 tests in impl/maven-core)
• New IT MavenITmng8765UriPropertyInterpolationTest passes:
  • POM-defined properties in URI parameters (parent + child modules)
  • CLI-passed properties (-D) in URI parameters
• Existing plugin config ITs pass (MNG-3827, MNG-3864, MNG-0828, IT-0009)
• Full CI build

🤖 Generated with Claude Code

[gnodet]

The pre-interpolation approach is sound in principle, but the implementation has a fundamental issue that renders the fix ineffective:

1. Interpolated values are silently discarded (high severity)

interpolateConfiguration stores resolved values in child XmlPlexusConfiguration wrappers obtained via getChildren(), but both the Plexus ObjectWithFieldsConverter and Maven's EnhancedConfigurationConverter access children via getChild(int), which creates new wrappers from the parent's unmodified immutable XmlNode tree — all interpolated values are lost.

Trace:

• getChildren() populates a childrenCache with wrappers
• setValue() replaces the wrapper's internal xmlNode but does NOT update the parent's xmlNode.children() (XmlNode is immutable)
• getChild(int) does NOT use the cache — it creates a fresh wrapper from xmlNode.children().get(i), which still holds the original uninterpolated child

Suggested approach: Reconstruct the XmlNode tree bottom-up with interpolated values and wrap it in a fresh XmlPlexusConfiguration, rather than mutating transient wrapper objects.

2. Tests don't cover the actual failure scenario (high severity)

The tests use POM-defined properties (test.version=1.2.3) and CLI properties (-Dcli.version=2.0.0), both of which are already resolved during model interpolation — before interpolateConfiguration runs. The tests pass with or without the fix. The actual MNG-8765 scenario (properties set dynamically at runtime via project.properties.setProperty() by Groovy/GMaven scripts) is not tested.

3. Fix only in loadV3Mojo (medium severity)

The interpolateConfiguration call is only added to loadV3Mojo. loadV4Mojo has the same pomConfiguration → populateMojoExecutionFields flow but no pre-interpolation, leaving V4 mojos with URI-typed parameters unprotected.

4. Javadoc inaccuracy (low severity)

The Javadoc says @return a new PlexusConfiguration but the method returns the same object mutated in place.

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]

Review: [MNG-8765] Pre-interpolate plugin configuration before type conversion

The intent of this fix is sound — runtime properties set by scripts (e.g., Groovy calling project.properties.setProperty(...)) need resolution before URI type converters parse the value. However, the implementation has a structural issue that makes it ineffective.

Confirmed findings:

1. [high] Pre-interpolation has no effect on type converters — interpolateConfiguration modifies child wrappers obtained via getChildren(), but the ComponentConfigurator (both standard plexus ObjectWithFieldsConverter and Maven's EnhancedConfigurationConverter) accesses children via getChild(int i), which creates new wrappers from the parent's unchanged immutable XmlNode. Since XmlNode is @Immutable, setValue() on a child wrapper replaces only that wrapper's xmlNode reference — the parent's xmlNode.children() list is never updated. The interpolated values are invisible to type converters.

   A possible fix: build a new XmlNode tree with interpolated values (using XmlNode.newBuilder() recursively) and wrap the result in a fresh XmlPlexusConfiguration, or fix XmlPlexusConfiguration.getChild(int i) to consult the childrenCache when populated.

2. [medium] Missing from loadV4Mojo — The pre-interpolation is only added to loadV3Mojo but not to loadV4Mojo (line 598), which has the identical structure. If V4 mojos use URI-typed parameters with runtime properties, they would have the same issue.

3. [medium] Integration tests don't cover the actual scenario — Both test methods use properties that model interpolation already resolves: ${test.version} (POM property) and ${cli.version} (system property from -D). By the time loadV3Mojo runs, these values are already resolved in the DOM. The actual MNG-8765 scenario — a property set at runtime via project.properties.setProperty() by a prior plugin — is not covered.

4. [low] Silent exception swallowing — Empty catch (ExpressionEvaluationException e) blocks. A logger.debug() call would aid troubleshooting.

---

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 pre-interpolation approach is sound in principle, but the implementation has a structural issue that may render the fix ineffective:

High severity:

1. Interpolated values invisible to downstream type converters: interpolateConfiguration() modifies child wrappers obtained via getChildren(), which populates childrenCache. However, the downstream Plexus configurators (BasicComponentConfigurator and EnhancedConfigurationConverter) access children via getChild(int i), which in XmlPlexusConfiguration (line 165-171) creates new wrappers directly from xmlNode.children().get(i) — completely bypassing childrenCache. Since XmlNode is immutable and setValue() only replaces a wrapper's internal xmlNode reference (not the parent's child list), all interpolated values are invisible to type converters.

   Additionally, the existing expression evaluation path in AbstractBasicConverter.fromExpression() already resolves ${...} expressions via PluginParameterExpressionEvaluator before type conversion, and UriConverter.fromString() then converts the resolved string. This may make pre-interpolation redundant even if it worked.

2. Tests don't cover the actual MNG-8765 scenario: Both test methods use properties already resolved during model interpolation — ${test.version} (POM <properties>) and ${cli.version} (-D CLI). These values are fully resolved before loadV3Mojo runs, so the tests pass with or without the interpolateConfiguration call. The actual scenario (properties set dynamically at runtime via project.properties.setProperty()) is not tested.

Medium severity:

3. V4 mojos unprotected: interpolateConfiguration is only added to loadV3Mojo. The loadV4Mojo method has the identical flow but no pre-interpolation.

Low severity:

4. Javadoc says "@return a new PlexusConfiguration" but the method mutates and returns the same object.
5. Two catch (ExpressionEvaluationException e) blocks silently swallow exceptions — consider adding logger.debug() for troubleshooting.

Suggested approach: either reconstruct the XmlNode tree bottom-up with interpolated values using XmlNode.newBuilder(), or fix XmlPlexusConfiguration.getChild(int i) to consult childrenCache, or move interpolation to a point where values are consumed directly.

---

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