gnodet-bot commented on code in PR #13537:
URL: https://github.com/apache/maven/pull/13537#discussion_r4301693404
##########
impl/maven-core/src/test/java/org/apache/maven/settings/SettingsUtilsTest.java:
##########
@@ -155,4 +155,66 @@ void testRoundTripProfiles() {
p.getPluginRepositories().size(),
clone.getPluginRepositories().size());
// TODO deep compare the lists
}
+
+ @Test
+ void testDominantMirrorsPrecedeRecessiveMirrors() {
+ org.apache.maven.api.settings.Mirror dominantMirror =
org.apache.maven.api.settings.Mirror.newBuilder()
+ .id("user-mirror")
+ .mirrorOf("insecure-repo")
+ .url("http://insecure.example.com/";)
+ .build();
+ org.apache.maven.api.settings.Mirror recessiveMirror =
org.apache.maven.api.settings.Mirror.newBuilder()
+ .id("maven-default-http-blocker")
+ .mirrorOf("external:http:*")
+ .url("http://0.0.0.0/";)
+ .blocked(true)
+ .build();
+
+ Settings dominant =
Settings.newBuilder().mirrors(List.of(dominantMirror)).build();
+ Settings recessive =
Settings.newBuilder().mirrors(List.of(recessiveMirror)).build();
+
+ Settings merged = SettingsUtilsV4.merge(dominant, recessive);
+
+ List<org.apache.maven.api.settings.Mirror> mirrors =
merged.getMirrors();
+ assertNotNull(mirrors);
+ assertEquals(2, mirrors.size());
+ assertEquals("user-mirror", mirrors.get(0).getId());
+ assertEquals("maven-default-http-blocker", mirrors.get(1).getId());
+ }
+
+ @Test
+ void testDominantMirrorWinsOnConflict() {
+ org.apache.maven.api.settings.Mirror dominantMirror =
org.apache.maven.api.settings.Mirror.newBuilder()
+ .id("shared-mirror")
+ .url("http://dominant.example.com/";)
+ .build();
+ org.apache.maven.api.settings.Mirror recessiveMirror =
org.apache.maven.api.settings.Mirror.newBuilder()
+ .id("shared-mirror")
+ .url("http://recessive.example.com/";)
+ .build();
+
+ Settings dominant =
Settings.newBuilder().mirrors(List.of(dominantMirror)).build();
+ Settings recessive =
Settings.newBuilder().mirrors(List.of(recessiveMirror)).build();
+
+ Settings merged = SettingsUtilsV4.merge(dominant, recessive);
+
+ List<org.apache.maven.api.settings.Mirror> mirrors =
merged.getMirrors();
+ assertNotNull(mirrors);
+ assertEquals(1, mirrors.size());
+ assertEquals("shared-mirror", mirrors.get(0).getId());
+ assertEquals("http://dominant.example.com/";, mirrors.get(0).getUrl());
+ }
+
+ @Test
+ void testDominantSettingsValuesPrecedeRecessive() {
+ Settings dominant = Settings.newBuilder()
+ .localRepository("/dominant/path")
+ .build();
+ Settings recessive = Settings.newBuilder()
+ .localRepository("/recessive/path")
+ .build();
+
+ Settings merged = SettingsUtilsV4.merge(dominant, recessive);
+ assertEquals("/dominant/path", merged.getLocalRepository());
+ }
Review Comment:
💡 **Nit:** `testDominantSettingsValuesPrecedeRecessive` only covers the
dominant-wins case (both sides set, dominant wins). Consider adding a second
assertion for the recessive-fallback case (dominant has no `localRepository`,
recessive fills the gap) — the mirror-ordering change could theoretically
affect scalar fallback behavior too.
```suggestion
}
@Test
void testRecessiveSettingsValuesFillGaps() {
Settings dominant = Settings.newBuilder().build();
Settings recessive = Settings.newBuilder()
.localRepository("/recessive/path")
.build();
Settings merged = SettingsUtilsV4.merge(dominant, recessive);
assertEquals("/recessive/path", merged.getLocalRepository());
}
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSettingsBuilder.java:
##########
@@ -69,13 +70,35 @@ public class DefaultSettingsBuilder implements
SettingsBuilder {
private final DefaultSettingsValidator settingsValidator = new
DefaultSettingsValidator();
- private final SettingsMerger settingsMerger = new SettingsMerger() {
+ static final SettingsMerger SETTINGS_MERGER = new SettingsMerger() {
Review Comment:
💡 **Nit:** `SETTINGS_MERGER` was promoted from `private final` (instance) to
`static final` (package-private) so `SettingsUtilsV4` can reuse the same
configured instance. A brief comment here would help future readers understand
the coupling:
```suggestion
// Package-private so SettingsUtilsV4 can reuse the same configured
merger instance.
static final SettingsMerger SETTINGS_MERGER = new SettingsMerger() {
```
--
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]
