gnodet-bot commented on code in PR #13607:
URL: https://github.com/apache/maven/pull/13607#discussion_r4306306069
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSettingsBuilder.java:
##########
@@ -248,6 +267,58 @@ private Server serverAlias(Server server, String id) {
return Server.newBuilder(server,
true).id(id).aliases(List.of()).build();
}
+ /**
+ * Wraps the source so that when {@code openStream()} is called, the raw
XML is read,
+ * interpolated as a string (resolving non-String fields like {@code
<port>${test.port}</port>}
+ * before Modello converts them to int/boolean), and returned as a new
stream.
+ */
+ private Source interpolateSource(Source settingsSource,
SettingsBuilderRequest request) {
+ UnaryOperator<String> src;
+ if (request.getInterpolationSource().isPresent()) {
+ src = request.getInterpolationSource().get();
+ } else {
+ Map<String, String> userProperties =
request.getSession().getUserProperties();
+ Map<String, String> systemProperties =
request.getSession().getSystemProperties();
+ if (userProperties == null && systemProperties == null) {
+ return settingsSource;
+ }
+ src = Interpolator.chain(
+ userProperties != null ? userProperties::get : s -> null,
+ systemProperties != null ? systemProperties::get : s ->
null);
+ }
+
+ return new Source() {
+ @Override
+ public Path getPath() {
+ return settingsSource.getPath();
+ }
+
+ @Override
+ public InputStream openStream() throws IOException {
+ String rawXml;
+ try (InputStream in = settingsSource.openStream()) {
+ byte[] bytes = in.readAllBytes();
+ if (bytes == null) {
Review Comment:
🐛 **Dead code — `InputStream.readAllBytes()` never returns `null`.**
Per the Java API contract, `InputStream.readAllBytes()` either returns a
(possibly empty) `byte[]` or throws `IOException`. It never returns `null`. The
null check at line 301 is unreachable dead code and adds confusion.
```suggestion
byte[] bytes = in.readAllBytes();
rawXml = new String(bytes, StandardCharsets.UTF_8);
```
##########
impl/maven-impl/src/test/java/org/apache/maven/impl/DefaultSettingsBuilderFactoryTest.java:
##########
@@ -189,6 +205,41 @@ 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");
+ assertEquals("localhost|*.example.org", proxy.getNonProxyHosts());
+ assertEquals("http", proxy.getProtocol());
+ assertEquals("proxy.example.org", proxy.getHost());
+ }
+
+ @Test
+ void testStrictParsingReportsError() {
+ // Create settings with an invalid XML element to trigger strict
parsing failure
+ // The settings-simple.xml uses 1.0.0 namespace which is valid but
let's test
+ // that strictParsing=true makes errors fatal
+ Map<String, String> systemProperties =
Map.of("maven.settings.strictParsing", "true");
+
+ // Even with strict parsing, valid settings should still parse fine
+ Settings settings =
+ execute("settings-simple", Map.of(),
systemProperties).getEffectiveSettings();
+ assertNotNull(settings);
Review Comment:
⚠️ **Tautology — test only exercises the happy path.**
The test name `testStrictParsingReportsError` implies it verifies that
strict parsing causes invalid XML to fail. But the body only feeds a valid
settings file (`settings-simple`) with `strictParsing=true` and asserts
`assertNotNull`.
This test adds no coverage of the failure path. To be meaningful it should
feed intentionally malformed settings (e.g. an unrecognised element) and assert
that parsing throws or returns an error when `strictParsing=true`.
```suggestion
void testStrictParsingReportsError() {
// Valid settings should still parse fine even with strict mode
Map<String, String> systemProperties =
Map.of("maven.settings.strictParsing", "true");
Settings settings =
execute("settings-simple", Map.of(),
systemProperties).getEffectiveSettings();
assertNotNull(settings);
// Invalid settings should fail with strict mode
// TODO: add a test resource with an unknown element and assert it
fails
}
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSettingsBuilder.java:
##########
@@ -248,6 +267,58 @@ private Server serverAlias(Server server, String id) {
return Server.newBuilder(server,
true).id(id).aliases(List.of()).build();
}
+ /**
+ * Wraps the source so that when {@code openStream()} is called, the raw
XML is read,
+ * interpolated as a string (resolving non-String fields like {@code
<port>${test.port}</port>}
+ * before Modello converts them to int/boolean), and returned as a new
stream.
+ */
+ private Source interpolateSource(Source settingsSource,
SettingsBuilderRequest request) {
+ UnaryOperator<String> src;
+ if (request.getInterpolationSource().isPresent()) {
+ src = request.getInterpolationSource().get();
+ } else {
+ Map<String, String> userProperties =
request.getSession().getUserProperties();
+ Map<String, String> systemProperties =
request.getSession().getSystemProperties();
+ if (userProperties == null && systemProperties == null) {
Review Comment:
💡 **Inconsistent null-guarding between `interpolateSource()` and
`interpolate()`.**
`interpolateSource()` guards against `userProperties == null &&
systemProperties == null` (lines 282–283) before constructing the
`Interpolator.chain(...)`. The existing `interpolate()` method (lines 353–363)
does no such guarding — it passes `userProperties::get` and
`systemProperties::get` directly to `Interpolator.chain()`.
Either both methods should handle nulls the same way, or neither should (if
`getUserProperties()`/`getSystemProperties()` are also `@Nonnull`). The
inconsistency is confusing and likely indicates a copy-paste artifact. Align
with `interpolate()` and remove the null guards if the API contracts guarantee
non-null returns.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSettingsBuilder.java:
##########
@@ -166,24 +169,40 @@ private Settings readSettings(
return Settings.newInstance();
}
+ // Determine if strict parsing mode is requested
+ Map<String, String> effectiveProperties =
request.getSession().getEffectiveProperties();
+ boolean strictParsing = effectiveProperties != null
Review Comment:
💡 **Misleading null-guard — `Session.getEffectiveProperties()` is
`@Nonnull`.**
Both `Session.getEffectiveProperties()` (in `Session.java`, line 101) and
`ProtoSession.getEffectiveProperties()` (line 64) are annotated `@Nonnull`. The
defensive `effectiveProperties != null` check implies the return value could be
null, which it cannot by contract.
Compare with the existing `interpolate()` method (line 353–363), which calls
`getUserProperties()` and `getSystemProperties()` directly without
null-checking them.
```suggestion
boolean strictParsing = Boolean.parseBoolean(
request.getSession().getEffectiveProperties().get(Constants.MAVEN_SETTINGS_STRICT_PARSING));
```
--
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]
