gnodet-bot commented on code in PR #13607:
URL: https://github.com/apache/maven/pull/13607#discussion_r4306436906
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Constants.java:
##########
@@ -198,6 +198,14 @@ public final class Constants {
public static final String MAVEN_SETTINGS_SECURITY_FILE_NAME =
"settings-security4.xml";
+ /**
+ * When set to {@code "true"}, settings parsing errors are fatal instead
of warnings.
+ *
Review Comment:
💡 **Wrong `@since` version — should be `4.1.0`.** (re-raise — not yet
addressed)
`master` is building `4.1.0-SNAPSHOT`. All recently-added constants in this
file use `@since 4.1.0`. This constant is new functionality not present on
`4.0.x`.
```suggestion
* @since 4.1.0
```
##########
impl/maven-impl/src/test/java/org/apache/maven/impl/DefaultSettingsBuilderFactoryTest.java:
##########
@@ -189,6 +207,52 @@ void testRelativeLocalRepositoryIsResolvedToAbsolute() {
assertTrue(repoPath.isAbsolute(), "Relative local repository should be
resolved to absolute");
}
+ @Test
+ void testProxyNonStringFieldInterpolation() {
+ Map<String, String> userProperties = Map.of(
+ "test.proxy.active", "true",
+ "test.proxy.port", "9090",
+ "test.proxy.nonProxyHosts", "localhost|*.example.org");
+
+ Settings settings =
+ execute("settings-proxy-interpolation",
userProperties).getEffectiveSettings();
+
+ List<Proxy> proxies = settings.getProxies();
+ assertEquals(1, proxies.size());
+
+ Proxy proxy = proxies.get(0);
+ assertEquals("test-proxy", proxy.getId());
+ assertTrue(proxy.isActive(), "proxy.active should be interpolated to
true");
+ assertEquals(9090, proxy.getPort(), "proxy.port should be interpolated
to 9090");
Review Comment:
⚠️ **Pre-fix validation: test passes WITHOUT the fix — not yet addressed.**
(re-raise)
Verified dynamically: `testProxyNonStringFieldInterpolation` passes with the
master version of `DefaultSettingsBuilder.java` (no `interpolateSource()`, no
pre-parse XML interpolation applied). Root cause: Maven 4's model stores
`proxy.portString` and `proxy.activeString` as `String` fields, so the existing
post-parse `SettingsTransformer`-based interpolation already resolves
`${test.proxy.port}` and `${test.proxy.active}` before they're converted to
`int`/`boolean`.
The test needs to be rewritten to exercise a scenario that is actually
broken without the fix. If the fix genuinely targets a real regression,
identify a field type or parsing path where the post-parse transformer cannot
reach (e.g. a field that is NOT String-backed in the model). If no such case
exists, the `interpolateSource()` approach may be solving a problem that
doesn't exist in Maven 4's model — and the PR description should be updated to
clarify the actual motivation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
