gnodet-bot commented on code in PR #13607:
URL: https://github.com/apache/maven/pull/13607#discussion_r4306376409
##########
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`.**
`master` is building `4.1.0-SNAPSHOT` and other recently-added constants in
this file already 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() {
Review Comment:
⚠️ **Pre-fix validation: test passes WITHOUT the fix — it does not exercise
the corrected code path.**
Dynamically verified: `testProxyNonStringFieldInterpolation` passes on
`master` without any of the PR changes applied.
The Maven 4 model stores `proxy.port` and `proxy.active` as String-backed
fields (`portString` / `activeString`). The existing post-parse `interpolate()`
method already visits both via
`SettingsTransformer.transformProxy_PortString()` and
`transformProxy_ActiveString()`. So `${test.proxy.port}` resolves to `"9090"`
even without the new pre-parse `interpolateSource()` step, and `getPort()` →
`Integer.parseInt("9090")` succeeds regardless.
The pre-parse approach adds value for scenarios the existing interpolation
cannot handle — for example when interpolation should happen before schema
validation, or when a field's raw text must be resolved before the parser
context is available. To make this test meaningful, it should demonstrate a
case that the post-parse path cannot handle. One option: temporarily disable
the post-parse `interpolate()` call in the test and assert the raw proxy fields
still resolve correctly. Another: test that a field that fails schema
validation when uninterpolated (if any exist) is handled correctly by the
pre-parse step.
As written, the test is a green test before and after the fix — it gives
false confidence that the pre-parse path is necessary.
--
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]
