gnodet-bot commented on code in PR #13537:
URL: https://github.com/apache/maven/pull/13537#discussion_r4278884047
##########
impl/maven-core/src/test/java/org/apache/maven/settings/SettingsUtilsTest.java:
##########
@@ -155,4 +155,44 @@ 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());
+ }
Review Comment:
⚠️ **Test does not validate the `sourceDominant` fix**
This test uses mirrors with different IDs (`"user-mirror"` vs
`"maven-default-http-blocker"`). With distinct IDs, `MergingList` adds both
entries to the result regardless of the `sourceDominant` flag — ordering is
determined by insertion order (dominant first), not by `sourceDominant`
semantics. The test passes even without the `false` → `true` change in
`SettingsUtilsV4.merge()`.
To actually validate the fix, add a test with **same-ID mirrors** in both
dominant and recessive settings, and assert that the dominant entry wins:
```java
Mirror dominantMirror =
Mirror.newBuilder().id("shared-mirror").url("http://dominant.example.com/";).mirrorOf("*").build();
Mirror recessiveMirror =
Mirror.newBuilder().id("shared-mirror").url("http://recessive.example.com/";).mirrorOf("*").build();
// After merge: dominant URL must win
assertEquals("http://dominant.example.com/";,
merged.getMirrors().get(0).getUrl());
```
##########
impl/maven-core/src/test/java/org/apache/maven/settings/SettingsUtilsTest.java:
##########
@@ -155,4 +155,44 @@ 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 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:
💬 Trailing blank line after the class closing brace.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSettingsBuilder.java:
##########
@@ -74,6 +75,26 @@ public class DefaultSettingsBuilder implements
SettingsBuilder {
protected KeyComputer<Repository> getRepositoryKey() {
return IdentifiableBase::getId;
}
+
+ @Override
+ protected KeyComputer<Mirror> getMirrorKey() {
+ return IdentifiableBase::getId;
+ }
+
+ @Override
+ protected KeyComputer<Server> getServerKey() {
+ return IdentifiableBase::getId;
+ }
+
+ @Override
+ protected KeyComputer<Proxy> getProxyKey() {
+ return IdentifiableBase::getId;
+ }
+
+ @Override
+ protected KeyComputer<Profile> getProfileKey() {
+ return IdentifiableBase::getId;
+ }
Review Comment:
🔶 **Redundant after the `merger.vm` template change**
These four overrides (`getMirrorKey`, `getServerKey`, `getProxyKey`,
`getProfileKey`) return `IdentifiableBase::getId` — the same value the
generated base class will now produce via the template change. They're no-ops.
Choose one approach: keep the explicit overrides here (and revert the
`merger.vm` change), or keep the template change (and remove these overrides).
The current state does both.
##########
src/mdo/merger.vm:
##########
@@ -208,7 +208,11 @@ public class ${className} {
#foreach ( $class in $model.allClasses )
#if ( $class.name != "InputSource" && $class.name != "InputLocation" )
protected KeyComputer<${class.name}> get${class.name}Key() {
+ #if ( $class.superClass == "IdentifiableBase" )
Review Comment:
⚠️ **Design concern — template scope too broad**
This change makes `IdentifiableBase::getId` the default key for **all**
generated mergers whose model class extends `IdentifiableBase`. While only
`settings.mdo` has such classes today, the template change creates forward
coupling: any future model that introduces an `IdentifiableBase` subclass will
silently get ID-based dedup, which may or may not be the right behavior.
Consider one of:
- Remove the template change and rely solely on the explicit overrides in
`DefaultSettingsBuilder` (more explicit, zero forward coupling)
- Keep the template change and remove the redundant explicit overrides in
`DefaultSettingsBuilder`
The PR currently does both, which is confusing and makes the intent unclear.
--
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]
